Android: Decouple animate from vsync

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 2deb35849e7b4ead535b74c524c13ef34e961407..8f9c6f6c51fd517004e47a831e58f6a3e912681e 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -57,6 +57,8 @@ class JavaBitmap;
 
 namespace {
 
+const unsigned int kMaxSwapBuffers = 2U;
+
 // Used to override capabilities_.adjust_deadline_for_parent to false
 class OutputSurfaceWithoutParent : public cc::OutputSurface {
  public:
@@ -242,7 +244,9 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       did_post_swapbuffers_(false),
       ignore_schedule_composite_(false),
       needs_composite_(false),
-      should_composite_on_vsync_(false),
+      needs_animate_(false),
+      will_composite_immediately_(false),
+      will_composite_eventually_(false),
       did_composite_this_frame_(false),
       pending_swapbuffers_(0U),
       weak_factory_(this) {
@@ -259,12 +263,43 @@ CompositorImpl::~CompositorImpl() {
   SetSurface(NULL);
 }
 
-void CompositorImpl::PostComposite(base::TimeDelta delay) {
+void CompositorImpl::PostComposite(CompositingTrigger trigger) {
+  base::TimeDelta delay;
+  switch (trigger) {
+    case COMPOSITE_IMMEDIATELY:
+      if (will_composite_immediately_)
+        return;
+      will_composite_immediately_ = true;
+      break;
+    case COMPOSITE_EVENTUALLY: {
+      if (will_composite_eventually_)

[brianderson, 2014/05/21 17:23:00]

Can we also return early here if will_composite_immediately_ is true?

[no sievers, 2014/05/23 02:49:44]

Good catch. Done.

+        return;
+      will_composite_eventually_ = true;
+      const base::TimeDelta estimated_composite_time = vsync_period_ / 4;
+      const base::TimeTicks now = base::TimeTicks::Now();
+      bool skip_frame =
+          did_composite_this_frame_ || pending_swapbuffers_ == kMaxSwapBuffers;
+
+      if (!last_vsync_.is_null() && (now - last_vsync_) < vsync_period_) {
+        base::TimeTicks next_composite =
+            last_vsync_ + vsync_period_ - estimated_composite_time;
+        if (next_composite < now || skip_frame)
+          next_composite += vsync_period_;
+
+        delay = next_composite - now;
+      } else if (skip_frame) {
+        delay = vsync_period_;
+      }
+    }
+    break;
+  }
+  TRACE_EVENT2("cc", "CompositorImpl::PostComposite",
+               "trigger", trigger,
+               "delay", delay.InMillisecondsF());
   base::MessageLoop::current()->PostDelayedTask(

[brianderson, 2014/05/21 17:23:00]

If will_composite_immediately_ just became true and will_composite_eventually_
was previously true, do we need to cancel the previously posted composite?

[no sievers, 2014/05/23 02:49:44]

I think the early-out in Composite() (!needs_composite_) mostly takes care of
that. Also we should never composite twice per frame. 

It's actually pretty bad and noticeable in traces when we over-composite,
because we still have to do a lot of work in the compositor until we notice
there is no visible damage. I saw it initially with my patch when I had the
skip_frame logic and timing off in the code above.

       FROM_HERE,
-      base::Bind(&CompositorImpl::Composite,
-                 weak_factory_.GetWeakPtr(),
-                 COMPOSITE_IMMEDIATELY),
+      base::Bind(
+          &CompositorImpl::Composite, weak_factory_.GetWeakPtr(), trigger),
       delay);
 }
 
@@ -272,44 +307,41 @@ void CompositorImpl::Composite(CompositingTrigger trigger) {
   if (!host_)
     return;
 
+  will_composite_immediately_ &= trigger != COMPOSITE_IMMEDIATELY;
+  will_composite_eventually_ &= trigger != COMPOSITE_EVENTUALLY;

[brianderson, 2014/05/21 17:23:00]

It is hard to decipher the intention here. Is this equivalent?

if (trigger == COMPOSITE_IMMEDIATELY)
  will_composite_immediately_ = false;
if (trigger == COMPOSITE_EVENTUALLY)
  will_composite_eventually_ = false;

[no sievers, 2014/05/23 02:49:44]

Done. You're right, it's equivalent and more readable.

+
   if (!needs_composite_)
     return;
 
-  if (trigger != COMPOSITE_ON_VSYNC && should_composite_on_vsync_) {
-    TRACE_EVENT0("compositor", "CompositorImpl_DeferCompositeToVSync");
-    root_window_->RequestVSyncUpdate();
-    return;
-  }
-
   // Don't Composite more than once in between vsync ticks.
   if (did_composite_this_frame_) {
     TRACE_EVENT0("compositor", "CompositorImpl_ThrottleComposite");
-    if (should_composite_on_vsync_)
-      root_window_->RequestVSyncUpdate();
-    else
-      PostComposite(vsync_period_);
+    PostComposite(COMPOSITE_EVENTUALLY);
     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_);
+    PostComposite(COMPOSITE_EVENTUALLY);

[brianderson, 2014/05/21 17:23:00]

Instead of posting a composite, would it be better to set a flag that
automatically triggers a composite when we get the swap ack?

[no sievers, 2014/05/23 02:49:44]

Ok since Sami suggested this earlier too, will do :)

     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;
+  did_composite_this_frame_ = true;
 
-  // Ignore ScheduleComposite() from layer tree changes during Layout.
+  // Ignore ScheduleComposite() from layer tree changes during layout and
+  // animation updates that will already be reflected in the current frame
+  // we are about to draw.
   ignore_schedule_composite_ = true;
   client_->Layout();
+
+  if (needs_animate_) {
+    needs_animate_ = false;
+    root_window_->Animate(gfx::FrameTime::Now());
+  }
   ignore_schedule_composite_ = false;
 
   did_post_swapbuffers_ = false;
@@ -317,11 +349,8 @@ void CompositorImpl::Composite(CompositingTrigger trigger) {
   if (did_post_swapbuffers_)
     pending_swapbuffers_++;
 
-  if (trigger != COMPOSITE_ON_VSYNC) {
-    // Need to track vsync to avoid compositing more than once per frame.
-    root_window_->RequestVSyncUpdate();
-  }
-  did_composite_this_frame_ = true;
+  // Need to track vsync to avoid compositing more than once per frame.
+  root_window_->RequestVSyncUpdate();
 }
 
 void CompositorImpl::SetRootLayer(scoped_refptr<cc::Layer> root_layer) {
@@ -384,8 +413,10 @@ void CompositorImpl::SetVisible(bool visible) {
     client_->UIResourcesAreInvalid();
   } else if (!host_) {
     needs_composite_ = false;
+    needs_animate_ = false;
     did_composite_this_frame_ = false;
-    should_composite_on_vsync_ = false;
+    will_composite_immediately_ = false;
+    will_composite_eventually_ = false;
     pending_swapbuffers_ = 0;
     cc::LayerTreeSettings settings;
     settings.refresh_rate = 60.0;
@@ -443,14 +474,13 @@ bool CompositorImpl::CompositeAndReadback(void *pixels, const gfx::Rect& rect) {
 }
 
 void CompositorImpl::SetNeedsComposite() {
+  DCHECK(!needs_composite_ || will_composite_immediately_ ||
+         will_composite_eventually_);
   if (!host_.get() || needs_composite_)
     return;
 
   needs_composite_ = true;
-
-  // For explicit requests we try to composite regularly on vsync.
-  should_composite_on_vsync_ = true;
-  root_window_->RequestVSyncUpdate();
+  PostComposite(COMPOSITE_EVENTUALLY);
 }
 
 cc::UIResourceId CompositorImpl::GenerateUIResourceFromUIResourceBitmap(
@@ -580,15 +610,20 @@ void CompositorImpl::OnLostResources() {
 }
 
 void CompositorImpl::ScheduleComposite() {
-  if (needs_composite_ || ignore_schedule_composite_)
+  DCHECK(!needs_composite_ || will_composite_immediately_ ||
+         will_composite_eventually_);
+  if (ignore_schedule_composite_)
     return;
 
   needs_composite_ = true;
-
-  // We currently expect layer tree invalidations at most once per frame
-  // during normal operation and therefore try to composite immediately
-  // to minimize latency.
-  PostComposite(base::TimeDelta());
+  if (did_composite_this_frame_) {
+    PostComposite(COMPOSITE_EVENTUALLY);
+  } else {
+    // We currently expect layer tree invalidations at most once per frame
+    // during normal operation and therefore try to composite immediately
+    // to minimize latency.
+    PostComposite(COMPOSITE_IMMEDIATELY);
+  }
 }
 
 void CompositorImpl::ScheduleAnimation() {
@@ -628,10 +663,22 @@ void CompositorImpl::RequestCopyOfOutputOnRootLayer(
 void CompositorImpl::OnVSync(base::TimeTicks frame_time,
                              base::TimeDelta vsync_period) {
   vsync_period_ = vsync_period;
+  last_vsync_ = frame_time;
   did_composite_this_frame_ = false;

[brianderson, 2014/05/21 17:23:00]

If did_composite_this_frame_ is true at this point due to an "eventually
composite" and there is an "immediate composite" pending, can we trigger the
immediate composite right here?

[no sievers, 2014/05/23 02:49:44]

You mean for when there is a ScheduleComposite(), let's say late frame from the
renderer, happening after a previous deadline where we composited? Originally I
thought it's maybe fine to wait until the next deadline since we cannot
composite twice per vsync anyways. But I guess it might be better to unblock the
renderer sooner, rather than later.

+}
+
+void CompositorImpl::SetNeedsAnimate() {
+  DCHECK(!needs_animate_ || needs_composite_);
+  DCHECK(!needs_composite_ || will_composite_immediately_ ||
+         will_composite_eventually_);
+  needs_animate_ = true;
 
-  if (should_composite_on_vsync_)
-    Composite(COMPOSITE_ON_VSYNC);
+  if (needs_composite_)
+    return;
+
+  TRACE_EVENT0("cc", "CompositorImpl::SetNeedsAnimate");
+  needs_composite_ = true;
+  PostComposite(COMPOSITE_EVENTUALLY);
 }
 
 }  // namespace content