Improving the decoder pipeline.

remoting/client/plugin/pepper_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/client/plugin/pepper_view.h"
 
+#include <functional>
+
 #include "base/message_loop.h"
 #include "base/string_util.h"
+#include "base/synchronization/waitable_event.h"
 #include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/graphics_2d.h"
 #include "ppapi/cpp/image_data.h"
 #include "ppapi/cpp/point.h"
 #include "ppapi/cpp/rect.h"
 #include "ppapi/cpp/size.h"
 #include "remoting/base/util.h"
 #include "remoting/client/chromoting_stats.h"
 #include "remoting/client/client_context.h"
+#include "remoting/client/frame_producer.h"
 #include "remoting/client/plugin/chromoting_instance.h"
 #include "remoting/client/plugin/pepper_util.h"
 
+using base::Passed;
+
 namespace remoting {
 
 namespace {
 
+// The maximum number of image buffers to be allocated at any point of time.
+const size_t kMaxPendingBuffersCount = 2;
+
 ChromotingScriptableObject::ConnectionError ConvertConnectionError(
     protocol::ConnectionToHost::Error error) {
   switch (error) {
     case protocol::ConnectionToHost::OK:
       return ChromotingScriptableObject::ERROR_NONE;
     case protocol::ConnectionToHost::HOST_IS_OFFLINE:
       return ChromotingScriptableObject::ERROR_HOST_IS_OFFLINE;
     case protocol::ConnectionToHost::SESSION_REJECTED:
       return ChromotingScriptableObject::ERROR_SESSION_REJECTED;
     case protocol::ConnectionToHost::INCOMPATIBLE_PROTOCOL:
       return ChromotingScriptableObject::ERROR_INCOMPATIBLE_PROTOCOL;
     case protocol::ConnectionToHost::NETWORK_FAILURE:
       return ChromotingScriptableObject::ERROR_NETWORK_FAILURE;
   }
   DLOG(FATAL) << "Unknown error code" << error;
   return  ChromotingScriptableObject::ERROR_NONE;
 }
 
 }  // namespace
 
-PepperView::PepperView(ChromotingInstance* instance, ClientContext* context)
+PepperView::PepperView(ChromotingInstance* instance,
+                       ClientContext* context,
+                       FrameProducer* producer)
   : instance_(instance),
     context_(context),
-    flush_blocked_(false),
-    is_static_fill_(false),
+    producer_(producer),
+    merge_buffer_(NULL),
+    merge_clip_area_(SkIRect::MakeEmpty()),
+    view_size_(SkISize::Make(0, 0)),
+    clip_area_(SkIRect::MakeEmpty()),
+    screen_size_(SkISize::Make(0, 0)),
     static_fill_color_(0),
+    flush_pending_(false),
+    producer_disabled_(true),
+    solid_fill_requested_(false),
     ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 PepperView::~PepperView() {
+  DCHECK(merge_buffer_ == NULL);
+  DCHECK(buffers_.empty());
 }
 
 bool PepperView::Initialize() {
   return true;
 }
 
 void PepperView::TearDown() {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
+  // The producer should now return any pending buffers.

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

This comment doesn't seem to describe the line that follows it?

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

Done.

+  producer_disabled_ = true;
+
+  // At this point ReturnBuffer() tasks scheduled by the producer will not be
+  // delivered, so instead we free all the buffers once the producer's queu is
+  // empty.
+  base::WaitableEvent done_event(true, false);
+  producer_->DrainQueue(base::Bind(&base::WaitableEvent::Signal,
+                                   base::Unretained(&done_event)));
+  done_event.Wait();

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

Hmmm.  It's a real shame to have to block here...

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

Yes. The same approach is used when tearing down the network connection, so I
figured when don't have a better choice.

+
+  merge_buffer_ = NULL;
+  while (!buffers_.empty()) {
+    FreeBuffer(buffers_.front());
+  }
+
   weak_factory_.InvalidateWeakPtrs();
 }
 
 void PepperView::Paint() {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
-  if (is_static_fill_) {
+  if (producer_disabled_ && !clip_area_.isEmpty()) {

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

We should just strip out the static-fill code, I think...

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

Probably, but not in this CL. :-)

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

Unless it simplifies the CL...?

[alexeypa (please no reviews), 2012/02/23 17:10:33]

OK.

     VLOG(1) << "Static filling " << static_fill_color_;
-    pp::ImageData image(instance_, pp::ImageData::GetNativeImageDataFormat(),
-                        pp::Size(graphics2d_.size().width(),
-                                 graphics2d_.size().height()),
-                        false);
-    if (image.is_null()) {
-      LOG(ERROR) << "Unable to allocate image of size: "
-                 << graphics2d_.size().width() << " x "
-                 << graphics2d_.size().height();
-      return;
+    pp::ImageData* buffer = AllocateBuffer();
+    if (buffer) {
+      solid_fill_requested_ = false;
+      for (int y = 0; y < buffer->size().height(); y++) {
+        for (int x = 0; x < buffer->size().width(); x++) {
+          *buffer->GetAddr32(pp::Point(x, y)) = static_fill_color_;
+        }
+      }
+
+      SkRegion region;
+      region.op(clip_area_, SkRegion::kUnion_Op);
+      FlushBuffer(clip_area_, buffer, region);
     }
-
-    for (int y = 0; y < image.size().height(); y++) {
-      for (int x = 0; x < image.size().width(); x++) {
-        *image.GetAddr32(pp::Point(x, y)) = static_fill_color_;
-      }
-    }
-
-    // For ReplaceContents, make sure the image size matches the device context
-    // size!  Otherwise, this will just silently do nothing.
-    graphics2d_.ReplaceContents(&image);
-    FlushGraphics(base::Time::Now());
   } else {
     // TODO(ajwong): We need to keep a backing store image of the host screen
     // that has the data here which can be redrawn.
     return;
   }
 }
 
-void PepperView::SetHostSize(const SkISize& host_size) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
-
-  if (host_size_ == host_size)
-    return;
-
-  host_size_ = host_size;
-
-  // Submit an update of desktop size to Javascript.
-  instance_->GetScriptableObject()->SetDesktopSize(
-      host_size.width(), host_size.height());
-}
-
-void PepperView::PaintFrame(media::VideoFrame* frame, const SkRegion& region) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
-
-  SetHostSize(SkISize::Make(frame->width(), frame->height()));
-
-  if (!backing_store_.get() || backing_store_->is_null()) {
-    LOG(ERROR) << "Backing store is not available.";
-    return;
-  }
-
-  base::Time start_time = base::Time::Now();
-
-  // Copy updated regions to the backing store and then paint the regions.
-  bool changes_made = false;
-  for (SkRegion::Iterator i(region); !i.done(); i.next())
-    changes_made |= PaintRect(frame, i.rect());
-
-  if (changes_made)
-    FlushGraphics(start_time);
-}
-
-bool PepperView::PaintRect(media::VideoFrame* frame, const SkIRect& r) {
-  const uint8* frame_data = frame->data(media::VideoFrame::kRGBPlane);
-  const int kFrameStride = frame->stride(media::VideoFrame::kRGBPlane);
-  const int kBytesPerPixel = GetBytesPerPixel(media::VideoFrame::RGB32);
-
-  pp::Size backing_store_size = backing_store_->size();
-  SkIRect rect(r);
-  if (!rect.intersect(SkIRect::MakeWH(backing_store_size.width(),
-                                      backing_store_size.height()))) {
-    return false;
-  }
-
-  const uint8* in =
-      frame_data +
-      kFrameStride * rect.fTop +   // Y offset.
-      kBytesPerPixel * rect.fLeft;  // X offset.
-  uint8* out =
-      reinterpret_cast<uint8*>(backing_store_->data()) +
-      backing_store_->stride() * rect.fTop +  // Y offset.
-      kBytesPerPixel * rect.fLeft;  // X offset.
-
-  // TODO(hclam): We really should eliminate this memory copy.
-  for (int j = 0; j < rect.height(); ++j) {
-    memcpy(out, in, rect.width() * kBytesPerPixel);
-    in += kFrameStride;
-    out += backing_store_->stride();
-  }
-
-  // Pepper Graphics 2D has a strange and badly documented API that the
-  // point here is the offset from the source rect. Why?
-  graphics2d_.PaintImageData(
-      *backing_store_.get(),
-      pp::Point(0, 0),
-      pp::Rect(rect.fLeft, rect.fTop, rect.width(), rect.height()));
-  return true;
-}
-
-void PepperView::BlankRect(pp::ImageData& image_data, const pp::Rect& rect) {
-  const int kBytesPerPixel = GetBytesPerPixel(media::VideoFrame::RGB32);
-  for (int y = rect.y(); y < rect.bottom(); y++) {
-    uint8* to = reinterpret_cast<uint8*>(image_data.data()) +
-        (y * image_data.stride()) + (rect.x() * kBytesPerPixel);
-    memset(to, 0xff, rect.width() * kBytesPerPixel);
-  }
-}
-
-void PepperView::FlushGraphics(base::Time paint_start) {
-  scoped_ptr<base::Closure> task(
-      new base::Closure(
-          base::Bind(&PepperView::OnPaintDone, weak_factory_.GetWeakPtr(),
-                     paint_start)));
-
-  // Flag needs to be set here in order to get a proper error code for Flush().
-  // Otherwise Flush() will always return PP_OK_COMPLETIONPENDING and the error
-  // would be hidden.
-  //
-  // Note that we can also handle this by providing an actual callback which
-  // takes the result code. Right now everything goes to the task that doesn't
-  // result value.
-  pp::CompletionCallback pp_callback(&CompletionCallbackClosureAdapter,
-                                     task.get(),
-                                     PP_COMPLETIONCALLBACK_FLAG_OPTIONAL);
-  int error = graphics2d_.Flush(pp_callback);
-
-  // There is already a flush in progress so set this flag to true so that we
-  // can flush again later.
-  // |paint_start| is then discarded but this is fine because we're not aiming
-  // for precise measurement of timing, otherwise we need to keep a list of
-  // queued start time(s).
-  if (error == PP_ERROR_INPROGRESS)
-    flush_blocked_ = true;
-  else
-    flush_blocked_ = false;
-
-  // If Flush() returns asynchronously then release the task.
-  if (error == PP_OK_COMPLETIONPENDING)
-    ignore_result(task.release());
-}
-
 void PepperView::SetSolidFill(uint32 color) {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
-  is_static_fill_ = true;
+  solid_fill_requested_ = true;
   static_fill_color_ = color;
 
+  // The producer should not draw anything while the solid fill is enabled.
+  producer_disabled_ = true;
+  producer_->DrainQueue(base::Closure());
   Paint();
 }
 
 void PepperView::UnsetSolidFill() {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
-  is_static_fill_ = false;
+  solid_fill_requested_ = false;
+
+  // Now the producer may draw.
+  producer_disabled_ = false;
+  InitiateDrawing();
 }
 
 void PepperView::SetConnectionState(protocol::ConnectionToHost::State state,
                                     protocol::ConnectionToHost::Error error) {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
   // TODO(hclam): Re-consider the way we communicate with Javascript.
   ChromotingScriptableObject* scriptable_obj = instance_->GetScriptableObject();
   switch (state) {
     case protocol::ConnectionToHost::CONNECTING:
@@ skipping 19 matching lines @@
 
     case protocol::ConnectionToHost::FAILED:
       SetSolidFill(kFailedColor);
       scriptable_obj->SetConnectionStatus(
           ChromotingScriptableObject::STATUS_FAILED,
           ConvertConnectionError(error));
       break;
   }
 }
 
-bool PepperView::SetViewSize(const SkISize& view_size) {
-  if (view_size_ == view_size)
-    return false;
-  view_size_ = view_size;
-
-  pp::Size pp_size = pp::Size(view_size.width(), view_size.height());
-
-  graphics2d_ = pp::Graphics2D(instance_, pp_size, true);
-  if (!instance_->BindGraphics(graphics2d_)) {
-    LOG(ERROR) << "Couldn't bind the device context.";
-    return false;
-  }
-
-  if (view_size.isEmpty())
-    return false;
-
-  // Allocate the backing store to save the desktop image.
-  if ((backing_store_.get() == NULL) ||
-      (backing_store_->size() != pp_size)) {
-    VLOG(1) << "Allocate backing store: "
-            << view_size.width() << " x " << view_size.height();
-    backing_store_.reset(
-        new pp::ImageData(instance_, pp::ImageData::GetNativeImageDataFormat(),
-                          pp_size, false));
-    DCHECK(backing_store_.get() && !backing_store_->is_null())
-        << "Not enough memory for backing store.";
-  }
-  return true;
-}
-
-void PepperView::AllocateFrame(media::VideoFrame::Format format,
-                               const SkISize& size,
-                               scoped_refptr<media::VideoFrame>* frame_out,
-                               const base::Closure& done) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
-
-  *frame_out = media::VideoFrame::CreateFrame(
-      media::VideoFrame::RGB32, size.width(), size.height(),
-      base::TimeDelta(), base::TimeDelta());
-  (*frame_out)->AddRef();
-  done.Run();
-}
-
-void PepperView::ReleaseFrame(media::VideoFrame* frame) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
-
-  if (frame)
-    frame->Release();
-}
-
-void PepperView::OnPartialFrameOutput(media::VideoFrame* frame,
-                                      SkRegion* region,
-                                      const base::Closure& done) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
-
-  // TODO(ajwong): Clean up this API to be async so we don't need to use a
-  // member variable as a hack.
-  PaintFrame(frame, *region);
-  done.Run();
-}
-
-void PepperView::OnPaintDone(base::Time paint_start) {
-  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+bool PepperView::SetView(const SkISize& view_size,
+                         const SkIRect& clip_area) {
+  bool updated = false;

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

Can you give this a more helpful name, e.g. size_or_clip_changed?

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

"Size and Clip" constitute the view, so a called it "view_changed". I also
removed the return value since it is not used any more.

+
+  // Prevent accidental upscaling.

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

nit: It's more that we don't currently support up-scaling.

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

It is more than that. Currently it is the only way to ensure that we will not
upscale by one or two pixels because JS code cannot estimate browser page zoom
exactly.

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

Which wouldn't be a problem if we supported up-scaling. ;)

(And with page zoom enabled we can end up up-scaling by much more than a couple
of pixels.)

[alexeypa (please no reviews), 2012/02/23 17:10:33]

Done as per our discussion.

+  SkISize size = SkISize::Make(
+                     std::min(view_size.width(), screen_size_.width()),
+                     std::min(view_size.height(), screen_size_.height()));
+
+  if (view_size_ != size) {
+    updated = true;
+    view_size_ = size;
+
+    pp::Size pp_size = pp::Size(view_size_.width(), view_size_.height());
+    graphics2d_ = pp::Graphics2D(instance_, pp_size, true);
+    bool result = instance_->BindGraphics(graphics2d_);
+
+    // There is no good way to handle this error currently.
+    DCHECK(result) << "Couldn't bind the device context.";
+  }
+
+  if (clip_area_ != clip_area) {
+    updated = true;
+
+    // YUV to RGB conversion may require even X and Y coordinates for
+    // the top left corner of the clipping area.
+    clip_area_ = AlignRect(clip_area);
+    clip_area_.intersect(SkIRect::MakeSize(view_size_));
+  }
+
+  if (updated) {
+    producer_->SetView(view_size_, clip_area_);
+    InitiateDrawing();
+  }
+
+  return updated;
+}
+
+void PepperView::PaintBuffer(const SkISize& view_size,
+                             const SkIRect& clip_area,
+                             pp::ImageData* buffer,
+                             const SkRegion& region) {
+  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+
+  // The data in the buffer is useless if the scale factor has changed.

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

Not strictly true; we could render what we have and thereby lag behind less
noticably in the case of a gradual resize operation?

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

I added a TODO comment.

+  if (view_size_ != view_size) {
+    FreeBuffer(buffer);
+    InitiateDrawing();
+  } else {
+    FlushBuffer(clip_area, buffer, region);
+  }
+}
+
+void PepperView::ReturnBuffer(pp::ImageData* buffer) {
+  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+
+  // Free the buffer if there is nothing to paint.
+  if (producer_disabled_) {
+    FreeBuffer(buffer);
+    return;
+  }
+
+  // Reuse the buffer if it is large enough, otherwise drop it on the floor
+  // and allocate a new one.
+  if (buffer->size().width() >= clip_area_.width() &&
+      buffer->size().height() >= clip_area_.height()) {
+    producer_->EnqueueBuffer(buffer);
+  } else {
+    FreeBuffer(buffer);
+    InitiateDrawing();
+  }
+}
+
+void PepperView::SetScreenSize(const SkISize& screen_size) {
+  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+
+  if (screen_size_ == screen_size)
+    return;
+
+  screen_size_ = screen_size;
+
+  // Submit an update of desktop size to Javascript.

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

nit: Suggest "Notify JavaScript of the change in size"

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

Done.

+  instance_->GetScriptableObject()->SetDesktopSize(
+      screen_size.width(), screen_size.height());
+}
+
+pp::ImageData* PepperView::AllocateBuffer() {
+  pp::ImageData* buffer = NULL;
+  if (buffers_.size() < kMaxPendingBuffersCount) {
+    pp::Size pp_size = pp::Size(clip_area_.width(), clip_area_.height());
+    buffer =  new pp::ImageData(instance_,
+                                PP_IMAGEDATAFORMAT_BGRA_PREMUL,
+                                pp_size,
+                                false);
+    if (buffer->is_null()) {
+      LOG(WARNING) << "Not enough memory for backing store.";

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

nit: "backing store" -> "frame buffers"

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

Done.

+      delete buffer;
+      buffer = NULL;
+    } else {
+      buffers_.push_back(buffer);
+    }
+  }
+
+  return buffer;
+}
+
+void PepperView::FreeBuffer(pp::ImageData* buffer) {
+  DCHECK(std::find(buffers_.begin(), buffers_.end(), buffer) != buffers_.end());
+
+  buffers_.remove(buffer);
+  delete buffer;
+
+  // Now that we can allocate new buffers retry painting of the static
+  // background.
+  if (solid_fill_requested_)
+    Paint();
+}
+
+void PepperView::InitiateDrawing() {
+  // Do not schedule drawing if there is nothing to paint.
+  if (producer_disabled_)
+    return;
+
+  pp::ImageData* buffer = AllocateBuffer();
+  while (buffer) {
+    producer_->EnqueueBuffer(buffer);
+    buffer = AllocateBuffer();
+  }
+}
+
+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
+    // two buffers simultaneously. If more buffers are allowed this code should
+    // merge all pending changes into a single buffer and then apply it to

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

nit: Suggest "this code should apply all pending changes to the screen"

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

Done.

+    // the screen.
+    DCHECK(merge_buffer_ == NULL);
+
+    merge_clip_area_ = clip_area;
+    merge_buffer_ = buffer;
+    merge_region_ = region;
+    return;
+  }
+
+  // Notify Pepper API about the updated areas and flush pixels to the screen.
+  base::Time start_time = base::Time::Now();

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

Gahhh.  We should update these stats gathering points to use TimeTicks(),
really.

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

... in a separate CL.

+
+  for (SkRegion::Iterator i(region); !i.done(); i.next()) {
+    SkIRect rect = i.rect();
+
+    // N.B. |clip_area_| may be different from |clip_area| meaning that
+    // the clipping area has changed since the time the image was drawn.
+    // The parts of the image that fit the new clipping area still need
+    // to be painted.

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

How does this comment relate to the intersect() call below?

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

Well, it explains why the intersect() call below is needed. The call literally
does what the comment say: it figures out what is the portion of of the rect
needs to be painted and skips the rect entirely if it doesn't fit the new
clipping area.

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

The comment refers to |clip_area_| and |clip_area|, but not to |region|, which
is the thing that is (effectively) being intersected with |clip_area_|.  It's
not clear from the comment what the intersect is intended to achieve.  I think
you're just saying that we need to re-clip |region| in case the clip area has
changed since the buffer was queued for painting?

[alexeypa (please no reviews), 2012/02/23 17:10:33]

I've reworded the comment.

+    if (!rect.intersect(clip_area_))
+      continue;
+
+    // Specify the rectangle coordinates relative to the clipping area.
+    rect.offset(- clip_area.left(), - clip_area.top());

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

nit: Not 100% sure, but I think the '-' should not be separated from the terms,
since they're the unary form.

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

Done.

+
+    // Pepper Graphics 2D has a strange and badly documented API that the
+    // point here is the offset from the source rect. Why?

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

It is within our power to fix the Graphics2D comments. ;)

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

As you can imagine I didn't write this comment. WE could fix the comment and
Graphics2D but let's keep more changes to this code out of this CL.

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

Indeed.  Not sure where my comment on this sprang from...

+    graphics2d_.PaintImageData(
+        *buffer,
+        pp::Point(clip_area.left(), clip_area.top()),
+        pp::Rect(rect.left(), rect.top(), rect.width(), rect.height()));
+  }
+
+  // Flush the updated areas ro the screen.

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

typo: ro -> to

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

Done.

+  scoped_ptr<base::Closure> task(
+      new base::Closure(
+          base::Bind(&PepperView::OnFlushDone, weak_factory_.GetWeakPtr(),
+                     start_time, buffer)));
+
+  // Flag needs to be set here in order to get a proper error code for Flush().
+  // Otherwise Flush() will always return PP_OK_COMPLETIONPENDING and the error
+  // would be hidden.
+  //
+  // Note that we can also handle this by providing an actual callback which
+  // takes the result code. Right now everything goes to the task that doesn't
+  // result value.

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

This is ugly.  We should be able to clean this up reasonably easily, I think?

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

I inherited this code. I'd like to defer any such clean up until a better time.

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

SGTM

+  pp::CompletionCallback pp_callback(&CompletionCallbackClosureAdapter,
+                                     task.get(),
+                                     PP_COMPLETIONCALLBACK_FLAG_OPTIONAL);
+  int error = graphics2d_.Flush(pp_callback);
+
+  // If Flush() returns asynchronously then release the task.
+  flush_pending_ = (error == PP_OK_COMPLETIONPENDING);
+  if (flush_pending_) {
+    ignore_result(task.release());

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

This doesn't look right.  Shouldn't we be release()ing task for
CompletionCallbackClosureAdapter to destroy?  As it stands this code looks like
it'll leak the task.

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

Looks OK to me. If completion is pending, CompletionCallbackClosureAdapter will
be called and the closure will be deleted there. Otherwise scoped_ptr will
delete it.

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

Yes, you're right!

+  } else {
+    instance_->GetStats()->video_paint_ms()->Record(
+        (base::Time::Now() - start_time).InMilliseconds());
+
+    ReturnBuffer(buffer);
+
+    // Resume painting for the buffer that was previoulsy postponed because of
+    // pending flush.
+    if (merge_buffer_ != NULL) {
+      buffer = merge_buffer_;
+      merge_buffer_ = NULL;
+      FlushBuffer(merge_clip_area_, buffer, merge_region_);
+    }
+  }
+}
+
+void PepperView::OnFlushDone(base::Time paint_start,
+                             pp::ImageData* buffer) {
+  DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
+  DCHECK(flush_pending_);
+
   instance_->GetStats()->video_paint_ms()->Record(
       (base::Time::Now() - paint_start).InMilliseconds());
 
-  // If the last flush failed because there was already another one in progress
-  // then we perform the flush now.
-  if (flush_blocked_)
-    FlushGraphics(base::Time::Now());
-  return;
+  flush_pending_ = false;
+  ReturnBuffer(buffer);
+
+  // Resume painting for the buffer that was previoulsy postponed because of
+  // pending flush.
+  if (merge_buffer_ != NULL) {
+    buffer = merge_buffer_;
+    merge_buffer_ = NULL;
+    FlushBuffer(merge_clip_area_, buffer, merge_region_);
+  }
 }
 
 }  // namespace remoting