Improving the decoder pipeline.

remoting/client/frame_consumer.h

Index: remoting/client/frame_consumer.h
diff --git a/remoting/client/frame_consumer.h b/remoting/client/frame_consumer.h
index de56d816925bb8aa1522e0b8e5bb0e35bef39e46..ccbdb0d8c6e72c05dc190546b746a0845e686e39 100644
--- a/remoting/client/frame_consumer.h
+++ b/remoting/client/frame_consumer.h
@@ -5,7 +5,13 @@
 #ifndef REMOTING_CLIENT_FRAME_CONSUMER_H_
 #define REMOTING_CLIENT_FRAME_CONSUMER_H_
 
-#include "remoting/base/decoder.h"  // For UpdatedRects
+#include "third_party/skia/include/core/SkRect.h"
+#include "third_party/skia/include/core/SkRegion.h"
+#include "third_party/skia/include/core/SkSize.h"
+
+namespace pp {
+class ImageData;
+} // namespace pp
 
 namespace remoting {
 
@@ -14,40 +20,22 @@ class FrameConsumer {
   FrameConsumer() {}
   virtual ~FrameConsumer() {}
 
-  // Request a frame be allocated from the FrameConsumer.
-  //
-  // If a frame cannot be allocated to fit the format, and |size|
-  // requirements, |frame_out| will be set to NULL.
-  //
-  // An allocated frame will have at least the |size| requested, but
-  // may be bigger. Query the retrun frame for the actual frame size,
-  // stride, etc.
-  //
-  // The AllocateFrame call is asynchronous.  From invocation, until when the
-  // |done| callback is invoked, |frame_out| should be considered to be locked
-  // by the FrameConsumer, must remain a valid pointer, and should not be
-  // examined or modified.  After |done| is called, the |frame_out| will
-  // contain a result of the allocation. If a frame could not be allocated,
-  // |frame_out| will be NULL.
-  //
-  // All frames retrieved via the AllocateFrame call must be released by a
-  // corresponding call ReleaseFrame(scoped_refptr<VideoFrame>* frame_out.
-  virtual void AllocateFrame(media::VideoFrame::Format format,
-                             const SkISize& size,
-                             scoped_refptr<media::VideoFrame>* frame_out,
-                             const base::Closure& done) = 0;
-
-  virtual void ReleaseFrame(media::VideoFrame* frame) = 0;
-
-  // OnPartialFrameOutput() is called every time at least one rectangle of
-  // output is produced.  The |frame| is guaranteed to have valid data for all
-  // of |region|.
-  //
-  // Both |frame| and |region| are guaranteed to be valid until the |done|
-  // callback is invoked.
-  virtual void OnPartialFrameOutput(media::VideoFrame* frame,
-                                    SkRegion* region,
-                                    const base::Closure& done) = 0;
+  // The frame producer calls this method to return a drawing buffer containing
+  // pixels that need to be flushed to the screen. |region| specifies the region
+  // of the frame to be flushed. I.e. the coordinates are relative to the frame,
+  // not to the clipping area.

[Wez, 2012/02/17 23:42:17]

nit: nor to the |buffer| coordinates

[Wez, 2012/02/17 23:42:17]

Can you rephrase this comment to be more in keeping with other Chromium
comments, e.g:

// Accepts a buffer and update region to be painted to the screen.
// ...
void PaintBuffer()

[alexeypa (please no reviews), 2012/02/21 23:00:44]

Done.

+  virtual void PaintBuffer(const SkISize& view_size,
+                           const SkIRect& clip_area,

[Wez, 2012/02/17 23:42:17]

If the top-left of |buffer| is assumed to match |clip_area| then |clip_area|'s
width & height are effectively duplicating the |buffer| dimensions - perhaps
it's cleaner to send just the |clip_area| origin instead?

+                           pp::ImageData* buffer,

[Wez, 2012/02/17 23:42:17]

Hmmm.  We really want a PP_Resource equivalent of scoped_ptr<pp::ImageData>. 
Really, to keep this interface abstract, we should not use pp::IMageData here,
but keep that an implementation detail of PepperView.

[alexeypa (please no reviews), 2012/02/21 23:00:44]

Well, we can introduce a wrapper object that carries a single buffer along with
it's size, position within the frame, scaling factor and updated region. I.e. it
will completely describe a single incremental update of the screen.

This will also answer the previous question about |clip_area|. I'd like to defer
any changed like that till a later CL though. :-)

[Wez, 2012/02/23 00:11:09]

OK!

+                           const SkRegion& region) = 0;
+
+  // The frame producer returns the drawing buffers that could not be used for
+  // drawing because of various reasons (shutdown is in progress, the view area
+  // has changed, etc.) by calling ReturnBuffer().
+  virtual void ReturnBuffer(pp::ImageData* buffer) = 0;

[Wez, 2012/02/17 23:42:17]

scoped_ptr<>?

[alexeypa (please no reviews), 2012/02/21 23:00:44]

No, this pointer does not carry ownership. The consumer keeps list of allocated
buffers (and therefore ownership) and is responsible for freeing those buffers.

The reason for this scheme is how PepperView shutdown works.

+
+  // Set the dimension of the entire host screen.
+  virtual void SetScreenSize(const SkISize& screen_size) = 0;

[Wez, 2012/02/17 23:42:17]

This is the only API with a Chromoting-specific name, but it's function isn't
actually Chromoting-specific - it just informs the FrameConsumer of the original
dimensions of the media.

How about SetSourceSize(), SetOriginalSize() or SetNaturalSize()?

[alexeypa (please no reviews), 2012/02/21 23:00:44]

SetSourceSize(). Done.

 
  private:
   DISALLOW_COPY_AND_ASSIGN(FrameConsumer);