Browser side changes for software compositing

content/browser/renderer_host/render_widget_host_view_aura.cc

Index: content/browser/renderer_host/render_widget_host_view_aura.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_aura.cc b/content/browser/renderer_host/render_widget_host_view_aura.cc
index 913e63c8f155e91b96a1204ee16cdd70141567d1..fe39416b70ce3738086d8cf4bd9f7d786e35bb9c 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -918,7 +918,7 @@ bool RenderWidgetHostViewAura::HasFocus() const {
 }
 
 bool RenderWidgetHostViewAura::IsSurfaceAvailableForCopy() const {
-  return current_surface_ || !!host_->GetBackingStore(false);
+  return current_surface_ || current_dib_ || !!host_->GetBackingStore(false);
 }
 
 void RenderWidgetHostViewAura::Show() {
@@ -1323,16 +1323,10 @@ void RenderWidgetHostViewAura::SwapBuffersCompleted(
     const BufferPresentedCallback& ack_callback,
     const scoped_refptr<ui::Texture>& texture_to_return) {
   ui::Compositor* compositor = GetCompositor();
-  if (!compositor) {
+  if (!compositor)
     ack_callback.Run(false, texture_to_return);
-  } else {
-    // Add sending an ACK to the list of things to do OnCompositingDidCommit
-    can_lock_compositor_ = NO_PENDING_COMMIT;
-    on_compositing_did_commit_callbacks_.push_back(
-        base::Bind(ack_callback, false, texture_to_return));
-    if (!compositor->HasObserver(this))
-      compositor->AddObserver(this);
-  }
+  else
+    AddOnCommitCallback(base::Bind(ack_callback, false, texture_to_return));
 }
 
 #if defined(OS_WIN)
@@ -1400,12 +1394,9 @@ void RenderWidgetHostViewAura::SwapDelegatedFrame(
   if (!compositor) {
     SendDelegatedFrameAck();
   } else {
-    can_lock_compositor_ = NO_PENDING_COMMIT;
-    on_compositing_did_commit_callbacks_.push_back(
+    AddOnCommitCallback(
         base::Bind(&RenderWidgetHostViewAura::SendDelegatedFrameAck,
-                   base::Unretained(this)));
-    if (!compositor->HasObserver(this))
-      compositor->AddObserver(this);
+                   AsWeakPtr()));
   }
 }
 
@@ -1416,6 +1407,59 @@ void RenderWidgetHostViewAura::SendDelegatedFrameAck() {
       host_->GetRoutingID(), host_->GetProcess()->GetID(), ack);
 }
 
+void RenderWidgetHostViewAura::SwapSoftwareFrame(
+    scoped_ptr<cc::SoftwareFrameData> frame_data,
+    float frame_device_scale_factor) {
+  const gfx::Size& frame_size = frame_data->size;
+  const gfx::Rect& damage_rect = frame_data->damage_rect;
+  const TransportDIB::Id& dib_id = frame_data->dib_id;
+
+#if defined(OS_WIN)
+  TransportDIB::Handle my_handle = TransportDIB::DefaultHandleValue();
+  ::DuplicateHandle(host_->GetProcess()->GetHandle(), dib_id.handle,
+                    ::GetCurrentProcess(), &my_handle,
+                    0, FALSE, DUPLICATE_SAME_ACCESS);
+  current_dib_.reset(TransportDIB::Map(my_handle));
+#else
+  current_dib_.reset(TransportDIB::Map(dib_id.handle));
+#endif

[piman, 2013/04/01 22:25:19]

You need to check the new dib's size (in bytes) against the frame_size (in
pixels), because it's untrusted data. Below in OnPaint you will access the
shared memory based on the frame_size, but you need to check that it's safe (if
it's not, you can discard the frame or whatever, but the renderer shouldn't be
allowed to crash - or worse - the browser).

[slavi, 2013/04/03 00:29:11]

Done.

+
+  previous_damage_.op(RectToSkIRect(damage_rect), SkRegion::kUnion_Op);
+  if (last_swapped_surface_size_ != frame_size) {
+    DLOG_IF(ERROR, damage_rect != gfx::Rect(frame_size))
+        << "Expected full damage rect";
+    last_swapped_surface_size_ = frame_size;
+  }
+
+  gfx::Size frame_size_in_dip = gfx::ToFlooredSize(
+      gfx::ScaleSize(frame_size, 1.0f / frame_device_scale_factor));
+  ui::Compositor* compositor = GetCompositor();
+  if (!compositor || !current_dib_ || ShouldSkipFrame(frame_size_in_dip)) {

[piman, 2013/04/01 22:25:19]

I don't think we can lump together those 3 cases.

For the !compositor case, we definitely want to keep the new dib anyway, but we
can return the *old* dib immediately (the old dib isn't "in flight").

For the ShouldSkipFrame flow, we can decide that we don't want to keep the new
dib around, and return it immediately, but if that's the case we need to keep
the old dib around. However, we can also keep the new dib around and return the
old dib, i.e. ignore the whole ShouldSkipFrame - it's necessary for GL textures
because they're used on another thread, but here it's only used on the main
thread so there's no race. That is, you could remove the ShouldSkipFrame logic
for the software mode.

For the !current_dib_ case, I assume this happens when we can't map for any
reason, between out-of-address-space or bogus (malicious?) data, and I guess
it's ok to drop the old dib, though you could also keep it.

[slavi, 2013/04/03 00:29:11]

I split out the check for a valid dib and dib's size.

But I still think that the ShouldSkipFrame check is necessary to make sure that
the frames that hit the screen are always the right size. And when the browser
is being resized, it waits for a frame with the correct frame to received. WDYT?

[piman, 2013/04/03 18:36:53]

ShouldSkipFrame has no impact on that though. It's there for the case where:
- the old frame is of the wrong size
- the new frame still isn't of the right size (and so we want to unblock the
renderer so that it can produce the one with the right size)
It only waits with a timeout. If we take the new frame, even though it's not the
right size, at least is more up-to-date than the old one, so that if the timeout
expires and we gutter, we have a more up-to-date frame.
Neither the old nor the new frame will be of the right size, but the new one
would be "better".

+    SendSoftwareFrameAck(dib_id);
+    return;
+  }
+
+  window_->SetExternalTexture(NULL);
+  released_front_lock_ = NULL;
+  CheckResizeLocks(frame_size_in_dip);
+  AddOnCommitCallback(
+      base::Bind(&RenderWidgetHostViewAura::SendSoftwareFrameAck,
+                 AsWeakPtr(), dib_id));

[piman, 2013/04/01 22:25:19]

I think here you need to return the old dib ID, not the new one - the browser
may need to draw multiple frames with the same renderer frame.

[slavi, 2013/04/03 00:29:11]

Done.

+
+  if (paint_observer_)
+    paint_observer_->OnUpdateCompositorContent();
+  window_->SchedulePaintInRect(ConvertRectToDIP(this, damage_rect));
+}
+
+void RenderWidgetHostViewAura::SendSoftwareFrameAck(
+    const TransportDIB::Id& id) {
+  cc::CompositorFrameAck ack;
+  ack.last_dib_id = id;
+  RenderWidgetHostImpl::SendSwapCompositorFrameAck(
+      host_->GetRoutingID(), host_->GetProcess()->GetID(), ack);
+}
+
+
 void RenderWidgetHostViewAura::OnSwapCompositorFrame(
     scoped_ptr<cc::CompositorFrame> frame) {
   if (frame->delegated_frame_data) {
@@ -1423,6 +1467,13 @@ void RenderWidgetHostViewAura::OnSwapCompositorFrame(
                        frame->metadata.device_scale_factor);
     return;
   }
+
+  if (frame->software_frame_data) {
+    SwapSoftwareFrame(frame->software_frame_data.Pass(),
+                      frame->metadata.device_scale_factor);
+    return;
+  }
+
   if (!frame->gl_frame_data || frame->gl_frame_data->mailbox.IsZero())
     return;
 
@@ -1452,7 +1503,7 @@ void RenderWidgetHostViewAura::BuffersSwapped(
     const std::string& mailbox_name,
     const BufferPresentedCallback& ack_callback) {
   scoped_refptr<ui::Texture> texture_to_return(current_surface_);
-  const gfx::Rect surface_rect = gfx::Rect(gfx::Point(), size);
+  const gfx::Rect surface_rect = gfx::Rect(size);
   if (!SwapBuffersPrepare(
       surface_rect, surface_rect, mailbox_name, ack_callback)) {
     return;
@@ -1478,7 +1529,7 @@ void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
     int gpu_host_id) {
   scoped_refptr<ui::Texture> previous_texture(current_surface_);
   const gfx::Rect surface_rect =
-      gfx::Rect(gfx::Point(), params_in_pixel.surface_size);
+      gfx::Rect(params_in_pixel.surface_size);
   gfx::Rect damage_rect(params_in_pixel.x,
                         params_in_pixel.y,
                         params_in_pixel.width,
@@ -1552,15 +1603,12 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceRelease() {
     if (compositor) {
       // We need to wait for a commit to clear to guarantee that all we
       // will not issue any more GL referencing the previous surface.
-      can_lock_compositor_ = NO_PENDING_COMMIT;
-      on_compositing_did_commit_callbacks_.push_back(
+      AddOnCommitCallback(
           base::Bind(&RenderWidgetHostViewAura::
                      SetSurfaceNotInUseByCompositor,
                      AsWeakPtr(),
                      current_surface_));  // Hold a ref so the texture will not
                                           // get deleted until after commit.
-      if (!compositor->HasObserver(this))
-        compositor->AddObserver(this);
     }
     current_surface_ = NULL;
     UpdateExternalTexture();
@@ -1943,12 +1991,32 @@ void RenderWidgetHostViewAura::OnCaptureLost() {
 }
 
 void RenderWidgetHostViewAura::OnPaint(gfx::Canvas* canvas) {
-  paint_canvas_ = canvas;
-  BackingStore* backing_store = host_->GetBackingStore(true);
-  paint_canvas_ = NULL;
-  if (backing_store) {
-    static_cast<BackingStoreAura*>(backing_store)->SkiaShowRect(gfx::Point(),
-                                                                canvas);
+  bool is_compositing_active = host_->is_accelerated_compositing_active();
+  bool has_backing_store = !!host_->GetBackingStore(false);
+  if (is_compositing_active && current_dib_) {
+    SkBitmap bitmap;
+    bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                     last_swapped_surface_size_.width(),
+                     last_swapped_surface_size_.height());
+    bitmap.setPixels(current_dib_->memory());
+
+    SkCanvas* sk_canvas = canvas->sk_canvas();
+    for (SkRegion::Iterator it(previous_damage_); !it.done(); it.next()) {
+      const SkIRect& src_rect = it.rect();
+      SkRect dst_rect = SkRect::Make(src_rect);
+      sk_canvas->drawBitmapRect(bitmap, &src_rect, dst_rect, NULL);
+    }
+    previous_damage_.setEmpty();
+
+    if (paint_observer_)
+      paint_observer_->OnPaintComplete();
+  } else if (!is_compositing_active && has_backing_store) {
+    paint_canvas_ = canvas;
+    BackingStoreAura* backing_store = static_cast<BackingStoreAura*>(
+        host_->GetBackingStore(true));
+    paint_canvas_ = NULL;
+    backing_store->SkiaShowRect(gfx::Point(), canvas);
+
     if (paint_observer_)
       paint_observer_->OnPaintComplete();
   } else if (aura::Env::GetInstance()->render_white_bg()) {
@@ -2366,7 +2434,7 @@ void RenderWidgetHostViewAura::OnCompositingDidCommit(
       if ((*it)->GrabDeferredLock())
         can_lock_compositor_ = YES_DID_LOCK;
   }
-  RunCompositingDidCommitCallbacks();
+  RunOnCommitCallbacks();
   locks_pending_commit_.clear();
 }
 
@@ -2465,7 +2533,7 @@ void RenderWidgetHostViewAura::OnLostResources() {
   // Make sure all ImageTransportClients are deleted now that the context those
   // are using is becoming invalid. This sends pending ACKs and needs to happen
   // after calling UpdateExternalTexture() which syncs with the impl thread.
-  RunCompositingDidCommitCallbacks();
+  RunOnCommitCallbacks();
 
   DCHECK(!shared_surface_handle_.is_null());
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
@@ -2621,7 +2689,7 @@ bool RenderWidgetHostViewAura::ShouldMoveToCenter() {
       global_mouse_position_.y() > rect.bottom() - border_y;
 }
 
-void RenderWidgetHostViewAura::RunCompositingDidCommitCallbacks() {
+void RenderWidgetHostViewAura::RunOnCommitCallbacks() {
   for (std::vector<base::Closure>::const_iterator
       it = on_compositing_did_commit_callbacks_.begin();
       it != on_compositing_did_commit_callbacks_.end(); ++it) {
@@ -2630,6 +2698,20 @@ void RenderWidgetHostViewAura::RunCompositingDidCommitCallbacks() {
   on_compositing_did_commit_callbacks_.clear();
 }
 
+void RenderWidgetHostViewAura::AddOnCommitCallback(
+    const base::Closure& callback) {
+  ui::Compositor* compositor = GetCompositor();
+  if (!compositor)

[piman, 2013/04/01 22:25:19]

In all the callsites, this has already been checked, so I would DCHECK instead.

[slavi, 2013/04/03 00:29:11]

Done.

+    return;
+
+  if (!compositor->HasObserver(this))
+    compositor->AddObserver(this);
+
+  can_lock_compositor_ = NO_PENDING_COMMIT;

[piman, 2013/04/01 22:25:19]

I feel somewhat uneasy at moving this here. I understand it saves code, but it's
mostly unrelated to the commit callback.
If you want to leave it here, maybe change the name of the method to be explicit
about this? E.g. AddOnCommitCallbackAndDisableLocks() ?

[slavi, 2013/04/03 00:29:11]

Done.

+  on_compositing_did_commit_callbacks_.push_back(callback);
+  return;

[piman, 2013/04/01 22:25:19]

not needed.

[slavi, 2013/04/03 00:29:11]

Done.

+}
+
 void RenderWidgetHostViewAura::AddedToRootWindow() {
   window_->GetRootWindow()->AddRootWindowObserver(this);
   host_->ParentChanged(GetNativeViewId());
@@ -2649,7 +2731,7 @@ void RenderWidgetHostViewAura::RemovingFromRootWindow() {
   // frame though, because we will reissue a new frame right away without that
   // composited data.
   ui::Compositor* compositor = GetCompositor();
-  RunCompositingDidCommitCallbacks();
+  RunOnCommitCallbacks();
   locks_pending_commit_.clear();
   if (compositor && compositor->HasObserver(this))
     compositor->RemoveObserver(this);