Tighter sync and faster ACK on SwapBuffers/PostSubBuffers

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 453e5d7f0873312705fe0864de8b16a04d16217d..63ac454c1f9b99309decc86131352862354beda2 100644
--- a/content/browser/renderer_host/render_widget_host_view_aura.cc
+++ b/content/browser/renderer_host/render_widget_host_view_aura.cc
@@ -576,9 +576,10 @@ void RenderWidgetHostViewAura::AcceleratedSurfaceBuffersSwapped(
           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);
     }
@@ -620,9 +621,10 @@ void RenderWidgetHostViewAura::AcceleratedSurfacePostSubBuffer(
           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);
     }
@@ -1186,9 +1188,15 @@ void RenderWidgetHostViewAura::OnCompositingStarted(
   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) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -1301,13 +1309,19 @@ bool RenderWidgetHostViewAura::ShouldMoveToCenter() {
       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() {
@@ -1317,7 +1331,7 @@ void RenderWidgetHostViewAura::RemovingFromRootWindow() {
   // 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))