Android: Move scheduling logic to CompositorImpl

content/browser/renderer_host/compositor_impl_android.cc

Index: content/browser/renderer_host/compositor_impl_android.cc
diff --git a/content/browser/renderer_host/compositor_impl_android.cc b/content/browser/renderer_host/compositor_impl_android.cc
index 2e8e8825d31f4c3f7804e9d1fc078e0362ac75a6..dacf72fd7641a51fc47dd682ca6886c2e04db00b 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -238,7 +238,14 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       window_(NULL),
       surface_id_(0),
       client_(client),
-      root_window_(root_window) {
+      root_window_(root_window),
+      did_post_swapbuffers_(false),
+      ignore_schedule_composite_(false),
+      needs_composite_(false),
+      should_composite_on_vsync_(false),
+      did_composite_outside_vsync_(false),
+      pending_swapbuffers_(0U),
+      weak_factory_(this) {
   DCHECK(client);
   DCHECK(root_window);
   ImageTransportFactoryAndroid::AddObserver(this);
@@ -252,9 +259,64 @@ CompositorImpl::~CompositorImpl() {
   SetSurface(NULL);
 }
 
-void CompositorImpl::Composite() {
-  if (host_)
-    host_->Composite(gfx::FrameTime::Now());
+void CompositorImpl::PostComposite(base::TimeDelta delay) {
+  base::MessageLoop::current()->PostDelayedTask(

[Sami, 2014/05/09 13:21:14]

Is there a chance we could post this task several times?

[no sievers, 2014/05/09 22:53:44]

ScheduleComposite() has an early-out if needs_composite_ is already true.
So only in some more rare cases when we are throttling, we'd post multiple
tasks, but we should still not end up actually compositing too often.

+      FROM_HERE,
+      base::Bind(&CompositorImpl::Composite, weak_factory_.GetWeakPtr(), false),
+      delay);
+}
+
+void CompositorImpl::Composite(bool is_vsync) {
+  if (!host_)
+    return;
+
+  if (!needs_composite_)
+    return;
+
+  if (!is_vsync && should_composite_on_vsync_) {
+    TRACE_EVENT0("compositor", "CompositorImpl_DeferCompositeToVSync");
+    root_window_->RequestVSyncUpdate();
+    return;
+  }
+
+  // Don't Composite immediately more than once in between vsync ticks.

[Sami, 2014/05/09 13:21:14]

Maybe add something like this as a safeguard: DCHECK(!(is_vsync &&
did_composite_outside_vsync));

[no sievers, 2014/05/09 22:53:44]

Actually obsolete now with your other comment addressed
(did_composite_outside_vsync_ -> did_composite_this_frame_).
Now both can happen when we transition from one way of scheduling to the other.

+  if (did_composite_outside_vsync_) {
+    TRACE_EVENT0("compositor", "CompositorImpl_ThrottleComposite");
+    PostComposite(vsync_period_);

[Sami, 2014/05/09 13:21:14]

Just thinking out loud here: we generally composite immediately if there's a new
frame from the renderer. We can't tell in advance when that happens. If that
happens twice per frame, then it seems like there's something wrong with
BeginFrame scheduling and we should be hitting this case regularly.

[no sievers, 2014/05/09 22:53:44]

Yea I was wondering too and logged when it happens, but fortunately I don't see
it happen too often (probably glow effect and things can cause it).

+    return;
+  }
+
+  const unsigned int kMaxSwapBuffers = 2U;
+  DCHECK_LE(pending_swapbuffers_, kMaxSwapBuffers);
+  if (pending_swapbuffers_ == kMaxSwapBuffers) {
+    TRACE_EVENT0("compositor", "CompositorImpl_SwapLimit");
+    if (should_composite_on_vsync_)
+      root_window_->RequestVSyncUpdate();
+    else
+      PostComposite(vsync_period_);

[Sami, 2014/05/09 13:21:14]

Should we instead trigger the composite as soon as the swap limit clears?
Otherwise this can add some latency if the GPU is backed up.

[no sievers, 2014/05/09 22:53:44]

I was actually wondering if this is better for latency, i.e. to skip a frame
instead of keeping up as much backpressure as possible. wdyt?

Again, seems like with a deadline this would be way easier :/

+    return;
+  }
+
+  // Reset state before Layout+Composite since that might create more
+  // requests to Composite that we need to respect.
+  needs_composite_ = false;
+  should_composite_on_vsync_ = false;
+
+  // Ignore ScheduleComposite() from layer tree changes during Layout.
+  ignore_schedule_composite_ = true;
+  client_->Layout();
+  ignore_schedule_composite_ = false;
+
+  did_post_swapbuffers_ = false;
+  host_->Composite(gfx::FrameTime::Now());

[Sami, 2014/05/09 13:21:14]

This should probably be derived from the most recent vsync time we have.

[no sievers, 2014/05/09 22:53:44]

Hmm if we composite on vsync, now is the vsync time.
Otherwise would we really want to use vsync time? AURA seems to be using now()
also.

+  if (did_post_swapbuffers_)
+    pending_swapbuffers_++;
+
+  if (!is_vsync) {
+    // Need to track vsync to avoid compositing more than once per frame.
+    root_window_->RequestVSyncUpdate();
+    did_composite_outside_vsync_ = true;
+  }
 }
 
 void CompositorImpl::SetRootLayer(scoped_refptr<cc::Layer> root_layer) {
@@ -316,6 +378,10 @@ void CompositorImpl::SetVisible(bool visible) {
     host_.reset();
     client_->UIResourcesAreInvalid();
   } else if (!host_) {
+    needs_composite_ = false;
+    did_composite_outside_vsync_ = false;
+    should_composite_on_vsync_ = false;
+    pending_swapbuffers_ = 0;
     cc::LayerTreeSettings settings;
     settings.refresh_rate = 60.0;
     settings.impl_side_painting = false;
@@ -374,6 +440,15 @@ bool CompositorImpl::CompositeAndReadback(void *pixels, const gfx::Rect& rect) {
     return false;
 }
 
+void CompositorImpl::SetNeedsComposite() {
+  if (!host_.get())
+    return;
+
+  needs_composite_ = true;
+  should_composite_on_vsync_ = true;

[Sami, 2014/05/09 13:21:14]

Doesn't this have the effect of re-scheduling any pending (immediate) composite
operation to the next vsync? Not sure if that happens in practice, but maybe we
should early out here if needs_composite_ == true.

[no sievers, 2014/05/09 22:53:44]

Done.

+  root_window_->RequestVSyncUpdate();
+}
+
 cc::UIResourceId CompositorImpl::GenerateUIResourceFromUIResourceBitmap(
     const cc::UIResourceBitmap& bitmap,
     bool is_transient) {
@@ -468,6 +543,13 @@ CreateGpuProcessViewContext(
                                                 NULL));
 }
 
+void CompositorImpl::Layout() {
+  // TODO: If we get this callback from the SingleThreadProxy, we need
+  // to stop calling it ourselves in Composite() below.

[Sami, 2014/05/09 13:21:14]

Composite() is above, not below :)

[no sievers, 2014/05/09 22:53:44]

Done :)

+  NOTREACHED();
+  client_->Layout();
+}
+
 scoped_ptr<cc::OutputSurface> CompositorImpl::CreateOutputSurface(
     bool fallback) {
   blink::WebGraphicsContext3D::Attributes attrs;
@@ -493,12 +575,12 @@ void CompositorImpl::OnLostResources() {
   client_->DidLoseResources();
 }
 
-void CompositorImpl::DidCompleteSwapBuffers() {
-  client_->OnSwapBuffersCompleted();
-}
-
 void CompositorImpl::ScheduleComposite() {
-  client_->ScheduleComposite();
+  if (needs_composite_ || ignore_schedule_composite_)
+    return;
+
+  needs_composite_ = true;
+  PostComposite(base::TimeDelta());
 }
 
 void CompositorImpl::ScheduleAnimation() {
@@ -507,12 +589,19 @@ void CompositorImpl::ScheduleAnimation() {
 
 void CompositorImpl::DidPostSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidPostSwapBuffers");
-  client_->OnSwapBuffersPosted();
+  did_post_swapbuffers_ = true;
+}
+
+void CompositorImpl::DidCompleteSwapBuffers() {
+  TRACE_EVENT0("compositor", "CompositorImpl::DidCompleteSwapBuffers");
+  DCHECK_GT(pending_swapbuffers_, 0U);
+  client_->OnSwapBuffersCompleted(pending_swapbuffers_--);
 }
 
 void CompositorImpl::DidAbortSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidAbortSwapBuffers");
-  client_->OnSwapBuffersCompleted();
+  DCHECK_GT(pending_swapbuffers_, 0U);
+  client_->OnSwapBuffersCompleted(pending_swapbuffers_--);
 }
 
 void CompositorImpl::DidCommit() {
@@ -523,4 +612,13 @@ void CompositorImpl::AttachLayerForReadback(scoped_refptr<cc::Layer> layer) {
   root_layer_->AddChild(layer);
 }
 
+void CompositorImpl::OnVSync(base::TimeTicks frame_time,
+                             base::TimeDelta vsync_period) {
+  vsync_period_ = vsync_period;
+  did_composite_outside_vsync_ = false;
+
+  if (should_composite_on_vsync_)
+    Composite(true);
+}
+
 }  // namespace content