Fix startup race-conditions in new Decoder pipeline:

Fix RectangleUpdateDecoder to cope with rendering requests before it is ready.
Fix PepperView not to InitiateDrawing() until initialized.
Fix DecoderVp8::RenderFrame() to allow being called before a frame is decoded.
Add DCHECKs for screen and view size to DecoderVp8.

Clarify requirements on FrameProducer interface implementation in comments.
Clarify screen and view size requirements on Decoder interface methods.

BUG=116842,116851


Review URL: http://codereview.chromium.org/9624022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@125536 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 48 insertions, 24 deletions

diff --git a/remoting/base/decoder.h b/remoting/base/decoder.h
index dd3d43e..c4cb7fb 100644
--- a/remoting/base/decoder.h
+++ b/remoting/base/decoder.h
@@ -35,6 +35,7 @@ class Decoder {
   virtual ~Decoder() {}
 
   // Initializes the decoder and sets the output dimensions.
+  // |screen size| must not be empty.
   virtual void Initialize(const SkISize& screen_size) = 0;
 
   // Feeds more data into the decoder.
@@ -45,25 +46,24 @@ class Decoder {
 
   virtual VideoPacketFormat::Encoding Encoding() = 0;
 
-  // Forces the decoder to include the specified |region| the next time
-  // RenderFrame() is called. |region| is expressed in output coordinates.
+  // Marks the specified |region| of the view for update the next time
+  // RenderFrame() is called.  |region| is expressed in |view_size| coordinates.
+  // |view_size| must not be empty.
   virtual void Invalidate(const SkISize& view_size,
                           const SkRegion& region) = 0;
 
-  // Copies invalidated pixels of the video frame to |image_buffer|. Both
-  // decoding a packet or Invalidate() call can result in parts of the frame
-  // to be invalidated. Only the pixels within |clip_area| are copied.
-  // Invalidated pixels outside of |clip_area| remain invalidated.
+  // Copies invalidated pixels within |clip_area| to |image_buffer|. Pixels are
+  // invalidated either by new data received in DecodePacket(), or by explicit
+  // calls to Invalidate(). |clip_area| is specified in |view_size| coordinates.
+  // If |view_size| differs from the source size then the copied pixels will be
+  // scaled accordingly. |view_size| cannot be empty.
   //
-  // The routine sets |output_region| to indicate the updated areas of
-  // |image_buffer|. |output_region| is in output buffer coordinates.
+  // |image_buffer|'s origin must correspond to the top-left of |clip_area|,
+  // and the buffer must be large enough to hold |clip_area| RGBA32 pixels.
+  // |image_stride| gives the output buffer's stride in pixels.
   //
-  // |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
-  // corner of |clip_area|. |image_stride| specifies the size of a single row
-  // of the buffer in bytes.
-  //
-  // Both |clip_area| and |output_region| are expressed in output coordinates.
+  // On return, |output_region| contains the updated area, in |view_size|
+  // coordinates.
   virtual void RenderFrame(const SkISize& view_size,
                            const SkIRect& clip_area,
                            uint8* image_buffer,
diff --git a/remoting/base/decoder_vp8.cc b/remoting/base/decoder_vp8.cc
index d4c5807..03d1d0e 100644
--- a/remoting/base/decoder_vp8.cc
+++ b/remoting/base/decoder_vp8.cc
@@ -34,6 +34,8 @@ DecoderVp8::~DecoderVp8() {
 }
 
 void DecoderVp8::Initialize(const SkISize& screen_size) {
+  DCHECK(!screen_size.isEmpty());
+
   screen_size_ = screen_size;
   state_ = kReady;
 }
@@ -107,6 +109,9 @@ VideoPacketFormat::Encoding DecoderVp8::Encoding() {
 
 void DecoderVp8::Invalidate(const SkISize& view_size,
                             const SkRegion& region) {
+  DCHECK_EQ(kReady, state_);
+  DCHECK(!view_size.isEmpty());
+
   for (SkRegion::Iterator i(region); !i.done(); i.next()) {
     SkIRect rect = i.rect();
     rect = ScaleRect(rect, view_size, screen_size_);
@@ -119,6 +124,13 @@ void DecoderVp8::RenderFrame(const SkISize& view_size,
                              uint8* image_buffer,
                              int image_stride,
                              SkRegion* output_region) {
+  DCHECK_EQ(kReady, state_);
+  DCHECK(!view_size.isEmpty());
+
+  // Early-return and do nothing if we haven't yet decoded any frames.
+  if (!last_image_)
+    return;
+
   SkIRect source_clip = SkIRect::MakeWH(last_image_->d_w, last_image_->d_h);
 
   // ScaleYUVToRGB32WithRect doesn't support up-scaling, and our web-app never
diff --git a/remoting/client/frame_producer.h b/remoting/client/frame_producer.h
index 644c270..c7976c5 100644
--- a/remoting/client/frame_producer.h
+++ b/remoting/client/frame_producer.h
@@ -39,6 +39,7 @@ class FrameProducer {
   virtual void RequestReturnBuffers(const base::Closure& done) = 0;
 
   // Notifies the producer of changes to the output view size or clipping area.
+  // Implementations must cope with empty |view_size| or |clip_area|.
   virtual void SetOutputSizeAndClip(const SkISize& view_size,
                                     const SkIRect& clip_area) = 0;
 
diff --git a/remoting/client/plugin/pepper_view.cc b/remoting/client/plugin/pepper_view.cc
index dfa8e847..06eb08e 100644
--- a/remoting/client/plugin/pepper_view.cc
+++ b/remoting/client/plugin/pepper_view.cc
@@ -75,7 +75,7 @@ PepperView::PepperView(ChromotingInstance* instance,
     clip_area_(SkIRect::MakeEmpty()),
     source_size_(SkISize::Make(0, 0)),
     flush_pending_(false),
-    in_teardown_(false),
+    is_initialized_(false),
     ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
@@ -85,16 +85,22 @@ PepperView::~PepperView() {
 }
 
 bool PepperView::Initialize() {
+  DCHECK(!is_initialized_);
+
+  is_initialized_ = true;
+  InitiateDrawing();
   return true;
 }
 
 void PepperView::TearDown() {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+  DCHECK(is_initialized_);
+
+  is_initialized_ = false;
 
   // The producer should now return any pending buffers. At this point, however,
   // ReturnBuffer() tasks scheduled by the producer will not be delivered,
   // so we free all the buffers once the producer's queue is empty.
-  in_teardown_ = true;
   base::WaitableEvent done_event(true, false);
   producer_->RequestReturnBuffers(
       base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event)));
@@ -192,7 +198,7 @@ void PepperView::ReturnBuffer(pp::ImageData* buffer) {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
   // Free the buffer if there is nothing to paint.
-  if (in_teardown_) {
+  if (!is_initialized_) {
     FreeBuffer(buffer);
     return;
   }
@@ -248,8 +254,7 @@ void PepperView::FreeBuffer(pp::ImageData* buffer) {
 }
 
 void PepperView::InitiateDrawing() {
-  // Do not schedule drawing if there is nothing to paint.
-  if (in_teardown_)
+  if (!is_initialized_)
     return;
 
   pp::ImageData* buffer = AllocateBuffer();
@@ -262,7 +267,6 @@ void PepperView::InitiateDrawing() {
 void PepperView::FlushBuffer(const SkIRect& clip_area,
                              pp::ImageData* buffer,
                              const SkRegion& region) {
-
   // Defer drawing if the flush is already in progress.
   if (flush_pending_) {
     // |merge_buffer_| is guaranteed to be free here because we allocate only
diff --git a/remoting/client/plugin/pepper_view.h b/remoting/client/plugin/pepper_view.h
index 391c183..ec1caf9 100644
--- a/remoting/client/plugin/pepper_view.h
+++ b/remoting/client/plugin/pepper_view.h
@@ -111,7 +111,8 @@ class PepperView : public ChromotingView,
   // True if there is already a Flush() pending on the Graphics2D context.
   bool flush_pending_;
 
-  bool in_teardown_;
+  // True after Initialize() has been called, until TearDown().
+  bool is_initialized_;
 
   base::WeakPtrFactory<PepperView> weak_factory_;
 
diff --git a/remoting/client/rectangle_update_decoder.cc b/remoting/client/rectangle_update_decoder.cc
index 6f58564..1a0b059 100644
--- a/remoting/client/rectangle_update_decoder.cc
+++ b/remoting/client/rectangle_update_decoder.cc
@@ -94,7 +94,12 @@ void RectangleUpdateDecoder::DecodePacket(const VideoPacket* packet,
 }
 
 void RectangleUpdateDecoder::DoPaint() {
-  if (buffers_.empty())
+  // If the view size is empty or we have no output buffers ready, return.
+  if (buffers_.empty() || view_size_.isEmpty())
+    return;
+
+  // If no Decoder is initialized, or the host dimensions are empty, return.
+  if (!decoder_.get() || source_size_.isEmpty())
     return;
 
   // Draw the invalidated region to the buffer.
diff --git a/remoting/client/rectangle_update_decoder.h b/remoting/client/rectangle_update_decoder.h
index 0ca7046..c61acb4 100644
--- a/remoting/client/rectangle_update_decoder.h
+++ b/remoting/client/rectangle_update_decoder.h
@@ -40,7 +40,7 @@ class RectangleUpdateDecoder :
   RectangleUpdateDecoder(base::MessageLoopProxy* message_loop,
                          FrameConsumer* consumer);
 
-  // Initializes decoder with the infromation from the protocol config.
+  // Initializes decoder with the information from the protocol config.
   void Initialize(const protocol::SessionConfig& config);
 
   // Decodes the contents of |packet|. DecodePacket may keep a reference to
@@ -48,7 +48,8 @@ class RectangleUpdateDecoder :
   // executed.
   void DecodePacket(const VideoPacket* packet, const base::Closure& done);
 
-  // FrameProducer implementation.
+  // FrameProducer implementation.  These methods may be called before we are
+  // Initialize()d, or we know the source screen size.
   virtual void DrawBuffer(pp::ImageData* buffer) OVERRIDE;
   virtual void InvalidateRegion(const SkRegion& region) OVERRIDE;
   virtual void RequestReturnBuffers(const base::Closure& done) OVERRIDE;