Tighter sync and faster ACK on SwapBuffers/PostSubBuffers

content/browser/renderer_host/render_widget_host_view_aura.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 "content/browser/renderer_host/render_widget_host_view_aura.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 558 matching lines @@
 
     if (!resize_locks_.empty() && !compositor->DrawPending()) {
       // If we are waiting for the resize, fast-track the ACK.
       // However only do so if we're not between the Draw() and the
       // OnCompositingEnded(), because out-of-order execution in the GPU process
       // might corrupt the "front buffer" for the currently issued frame.
       RenderWidgetHostImpl::AcknowledgeSwapBuffers(
           params_in_pixel.route_id, gpu_host_id);
     } else {
       // Add sending an ACK to the list of things to do OnCompositingEnded
-      on_compositing_ended_callbacks_.push_back(
+      on_compositing_will_end_callbacks_.push_back(CallbackAndCompositeNumber(

[piman, 2012/06/04 18:20:39]

A possible gotcha for the future, is that this relies on the fact that we're
never pipelining the composites - i.e. we don't start one until we got the
swapbuffers callback. We should call that out explicitly, because that may not
stay true if we switch to using the CCScheduler / the compositor thread.

On the GPU process side, as soon as we receive the swap buffers ack, we will
touch the ex-front-now-back buffer, possibly resizing it immediately, and
because the callback is sent on another channel than the compositor context, it
will happen out-of-order wrt the rendering commands that may be referring to
that buffer.
Sending the ack on the OnCompositingEnded ensured that the commands that refer
to the new front have been executed, so there is no pending command from the UI
compositor that will refer to the old front. This is now only ensured by the
fact that we don't start a compositing pass until the commands for the previous
pass have been fully executed. We should call that out.

[jonathan.backer, 2012/06/05 13:35:58]

Just to reiterate what I think you are saying: Currently, when the brwoser gets
a swapbuffers callback (triggered by an Echo and Echo_ACK?), we know that all
the GL draw commands referring to previous RWHVA front surface have been issued
by the GPU process (i.e. the order of various client command streams has been
serialized appropriately). All browser composites will refer to the new RWHVA
front surface.

If pipelining the composites, we cannot AcceleratedSurfaceMsg_BuffersSwappedACK
when we receive OnCompositingEnded (the swapbuffers callback) because there may
be another frame currently being issued that refers to the previous RWHVA front
surface (which we don't want the renderer corrupting).

If we can get something similar to the composite frame ID introduced in this CL,
we can handle this appropriately --- figure out what frame ID the commit for the
new RWHVA front buffer happened on and wait for it to clear (via a swapbuffers
callback) before we send AcceleratedSurfaceMsg_BuffersSwappedACK. I don't know
enough of the CC architecture to know how feasible this is.

But just to be 100% clear, we cannot pipeline as ToT stands? This problem is
somewhat independent of my change?
I presume that that callback being sent that you are referring to above is
AcceleratedSurfaceMsg_BuffersSwappedACK. If so, I'm tracking your arguments.
(One channel is owned by a GpuChannelHost and another by a GpuProcessHost and
there is no guaranteed ordering?)
On ToT, there could have been a browser composite in flight (draw commands
issued, just waiting for a swapbuffers callback) when an
GpuHostMsg_AcceleratedSurfaceBuffersSwapped comes in. In this case,
OnCompositingEnded could be called for a frame displaying an old front buffer
(not a new one). Without pipelining, this is sufficient synchronization. But it
is also relying on the fact that we don't start a compositing pass until the
commands for the previous pass have been fully executed.

How about ACKing on OnCompositingEnded, but using the composite ID framework
introduced in this CL? Then we get the desired synchronization (presuming that
pipelining could maintain composite ID's appropriately)?

           base::Bind(&RenderWidgetHostImpl::AcknowledgeSwapBuffers,
-                     params_in_pixel.route_id, gpu_host_id));
+                     params_in_pixel.route_id, gpu_host_id),
+          compositor->last_started_id() + 1));
       if (!compositor->HasObserver(this))
         compositor->AddObserver(this);
     }
   }
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
     const GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params& params_in_pixel,
     int gpu_host_id) {
   current_surface_ = params_in_pixel.surface_handle;
@@ skipping 21 matching lines @@
 
     if (!resize_locks_.empty() && !compositor->DrawPending()) {
       // If we are waiting for the resize, fast-track the ACK.
       // However only do so if we're not between the Draw() and the
       // OnCompositingEnded(), because out-of-order execution in the GPU process
       // might corrupt the "front buffer" for the currently issued frame.
       RenderWidgetHostImpl::AcknowledgePostSubBuffer(
           params_in_pixel.route_id, gpu_host_id);
     } else {
       // Add sending an ACK to the list of things to do OnCompositingEnded
-      on_compositing_ended_callbacks_.push_back(
+      on_compositing_will_end_callbacks_.push_back(CallbackAndCompositeNumber(
           base::Bind(&RenderWidgetHostImpl::AcknowledgePostSubBuffer,
-                     params_in_pixel.route_id, gpu_host_id));
+                     params_in_pixel.route_id, gpu_host_id),
+          compositor->last_started_id() + 1));
       if (!compositor->HasObserver(this))
         compositor->AddObserver(this);
     }
   }
 }
 
 void RenderWidgetHostViewAura::AcceleratedSurfaceSuspend() {
 }
 
 bool RenderWidgetHostViewAura::HasAcceleratedSurface(
@@ skipping 543 matching lines @@
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, ui::CompositorDelegate implementation:
 
 void RenderWidgetHostViewAura::OnCompositingStarted(
     ui::Compositor* compositor) {
   locks_pending_draw_.clear();
 }
 
-void RenderWidgetHostViewAura::OnCompositingEnded(ui::Compositor* compositor) {
-  RunCompositingCallbacks();
-  compositor->RemoveObserver(this);
+void RenderWidgetHostViewAura::OnCompositingWillEnd(
+    ui::Compositor* compositor) {
+  RunCompositingCallbacks(compositor);
+  if (on_compositing_will_end_callbacks_.empty())
+    compositor->RemoveObserver(this);
+}
+
+void RenderWidgetHostViewAura::OnCompositingEnded(
+    ui::Compositor* compositor) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostViewAura, ImageTransportFactoryObserver implementation:
 
 void RenderWidgetHostViewAura::OnLostResources(ui::Compositor* compositor) {
   image_transport_clients_.clear();
   current_surface_ = 0;
   UpdateExternalTexture();
   locks_pending_draw_.clear();
@@ skipping 92 matching lines @@
   gfx::Rect rect = window_->bounds();
   int border_x = rect.width() * kMouseLockBorderPercentage / 100;
   int border_y = rect.height() * kMouseLockBorderPercentage / 100;
 
   return global_mouse_position_.x() < rect.x() + border_x ||
       global_mouse_position_.x() > rect.right() - border_x ||
       global_mouse_position_.y() < rect.y() + border_y ||
       global_mouse_position_.y() > rect.bottom() - border_y;
 }
 
-void RenderWidgetHostViewAura::RunCompositingCallbacks() {
-  for (std::vector< base::Callback<void(void)> >::const_iterator
-      it = on_compositing_ended_callbacks_.begin();
-      it != on_compositing_ended_callbacks_.end(); ++it) {
-    it->Run();
+void RenderWidgetHostViewAura::RunCompositingCallbacks(
+    ui::Compositor* compositor) {
+  std::vector<CallbackAndCompositeNumber>::iterator it =
+      on_compositing_will_end_callbacks_.begin();
+  while (it != on_compositing_will_end_callbacks_.end()) {
+    if (compositor == NULL ||
+        it->second <= compositor->last_will_end_id()) {
+      it->first.Run();
+      it = on_compositing_will_end_callbacks_.erase(it);
+    } else {
+      it++;
+    }
   }
-  on_compositing_ended_callbacks_.clear();
 }
 
 void RenderWidgetHostViewAura::RemovingFromRootWindow() {
   // We are about to disconnect ourselves from the compositor, we need to issue
   // the callbacks now, because we won't get notified when the frame is done.
   // TODO(piman): this might in theory cause a race where the GPU process starts
   // drawing to the buffer we haven't yet displayed. This will only show for 1
   // frame though, because we will reissue a new frame right away without that
   // composited data.
-  RunCompositingCallbacks();
+  RunCompositingCallbacks(NULL);
   locks_pending_draw_.clear();
   ui::Compositor* compositor = GetCompositor();
   if (compositor && compositor->HasObserver(this))
     compositor->RemoveObserver(this);
 }
 
 ui::Compositor* RenderWidgetHostViewAura::GetCompositor() {
   aura::RootWindow* root_window = window_->GetRootWindow();
   return root_window ? root_window->compositor() : NULL;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // RenderWidgetHostView, public:
 
 // static
 RenderWidgetHostView* RenderWidgetHostView::CreateViewForWidget(
     RenderWidgetHost* widget) {
   return new RenderWidgetHostViewAura(widget);
 }
 
 // static
 void content::RenderWidgetHostViewPort::GetDefaultScreenInfo(
     WebKit::WebScreenInfo* results) {
   GetScreenInfoForWindow(results, NULL);
 }