Improving the decoder pipeline.

remoting/base/decoder.h

Index: remoting/base/decoder.h
diff --git a/remoting/base/decoder.h b/remoting/base/decoder.h
index 9efc17e4f4c95005afe274cf58727319409fd2a4..50c4a42be2541bb49fc84be3840532891d0aa0fd 100644
--- a/remoting/base/decoder.h
+++ b/remoting/base/decoder.h
@@ -5,10 +5,12 @@
 #ifndef REMOTING_BASE_DECODER_H_
 #define REMOTING_BASE_DECODER_H_
 
+#include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "media/base/video_frame.h"
 #include "remoting/proto/video.pb.h"
+#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 remoting {
 
@@ -32,20 +34,14 @@ class Decoder {
   Decoder() {}
   virtual ~Decoder() {}
 
-  // Initializes the decoder to draw into the given |frame|.
-  virtual void Initialize(scoped_refptr<media::VideoFrame> frame) = 0;
+  // Initializes the decoder and sets the output dimensions for the decoder.

[Wez, 2012/02/07 01:56:31]

nit: no need for "for the decoder".

[alexeypa (please no reviews), 2012/02/15 23:06:22]

Done.

+  virtual void Initialize(const SkISize& screen_size) = 0;

[Wez, 2012/02/07 01:56:31]

Why do you need |screen_size| here?

[alexeypa (please no reviews), 2012/02/15 23:06:22]

For scaling. It is passed this way because RectangeUpdateDecoder is responsible
for extracting frame dimensions from a packet.

 
   // Feeds more data into the decoder.
   virtual DecodeResult DecodePacket(const VideoPacket* packet) = 0;
 
-  // Returns the region affected by the most recent frame.  Can be called only
-  // after DecodePacket returned DECODE_DONE. Caller keeps ownership of
-  // |region|.
-  virtual void GetUpdatedRegion(SkRegion* region) = 0;
-
-  // Reset the decoder to an uninitialized state. Release all references to
-  // the initialized |frame|.  Initialize() must be called before the decoder
-  // is used again.
+  // Reset the decoder to an uninitialized state. Initialize() must be called
+  // before the decoder is used again.
   virtual void Reset() = 0;
 
   // Returns true if decoder is ready to accept data via DecodePacket.
@@ -53,20 +49,26 @@ class Decoder {
 
   virtual VideoPacketFormat::Encoding Encoding() = 0;
 
-  // Set the output dimensions for the decoder.  If the dimensions are empty
-  // then the source is rendered without scaling.
-  // Output dimensions are ignored if the decoder doesn't support scaling.
-  virtual void SetOutputSize(const SkISize& size) {}
-
-  // Set the clipping rectangle to the decoder. Decoder should respect this and
-  // only output changes in this rectangle. The new clipping rectangle will be
-  // effective on the next decoded video frame.
-  virtual void SetClipRect(const SkIRect& clip_rect) {}
-
   // Force decoder to output a frame based on the specified |region| of the
   // most recently decoded video frame.  |region| is expressed in video frame
   // rather than output coordinates.
-  virtual void RefreshRegion(const SkRegion& region) {}
+  virtual void UpdateRegion(const SkRegion& region) = 0;

[Wez, 2012/02/07 01:56:31]

This should be called ForceUpdate(), Invalidate(), or something along those
lines, to make its function clearer.

[alexeypa (please no reviews), 2012/02/15 23:06:22]

It is Invalidate() now.

+
+  // Outputs pixels within |region| of the video frame to |image_buffer|
+  // performing necessary conversion and scaling.

[Wez, 2012/02/07 01:56:31]

The routine actually refreshes the portions of the video frame within the
specified |clip_area|, that have changed since the last frame, or explicitly
marked to be refreshed?

[alexeypa (please no reviews), 2012/02/15 23:06:22]

Both.

+  //
+  // The routine produces |output_region| to indicate the updated areas of

[Wez, 2012/02/07 01:56:31]

produces ... -> sets ...

[alexeypa (please no reviews), 2012/02/15 23:06:22]

Done.

+  // |image_buffer|. |output_region| is in output buffer coordinates.
+  //
+  // |image_buffer| is assumed to be large enough to hold entire |clip_area|
+  // (RGBA32). The top left corner of the buffer corresponds to the top left

[Wez, 2012/02/07 01:56:31]

What coordinate scheme does |clip_area| apply to?  Input or output?

[alexeypa (please no reviews), 2012/02/15 23:06:22]

Consumer's. Done.

+  // corner of |clip_area|. |image_stride| specifies the size of a single row
+  // of the buffer in bytes.
+  virtual void Draw(const SkISize& view_size,

[Wez, 2012/02/07 01:56:31]

I think it's cleaner to set the output size explicitly, via a separate call,
then to pass it here.

[alexeypa (please no reviews), 2012/02/15 23:06:22]

This call does not set the output size. |view_size| is used to see if scaling
should be done.

+                    const SkIRect& clip_area,
+                    uint8* image_buffer,
+                    int image_stride,
+                    SkRegion* output_region) = 0;

[Wez, 2012/02/07 01:56:31]

Consider renaming this RefreshFrame(), UpdateFrame(), RenderFrame(), or similar.

[alexeypa (please no reviews), 2012/02/15 23:06:22]

RenderFrame(). Done.

 };
 
 }  // namespace remoting