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..ce2de3604e8a915fff76c894fead0a711d7c63f8 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,10 +244,10 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       did_post_swapbuffers_(false),
       ignore_schedule_composite_(false),
       needs_composite_(false),
-      should_composite_on_vsync_(false),
-      did_composite_this_frame_(false),
-      pending_swapbuffers_(0U),
-      weak_factory_(this) {
+      needs_animate_(false),
+      will_composite_immediately_(false),
+      composite_on_vsync_trigger_(DO_NOT_COMPOSITE),
+      pending_swapbuffers_(0U) {
   DCHECK(client);
   DCHECK(root_window);
   ImageTransportFactoryAndroid::AddObserver(this);
@@ -259,57 +261,100 @@ CompositorImpl::~CompositorImpl() {
   SetSurface(NULL);
 }
 
-void CompositorImpl::PostComposite(base::TimeDelta delay) {
-  base::MessageLoop::current()->PostDelayedTask(
-      FROM_HERE,
-      base::Bind(&CompositorImpl::Composite,
-                 weak_factory_.GetWeakPtr(),
-                 COMPOSITE_IMMEDIATELY),
-      delay);
-}
-
-void CompositorImpl::Composite(CompositingTrigger trigger) {
-  if (!host_)
-    return;
+void CompositorImpl::PostComposite(CompositingTrigger trigger) {
+  DCHECK(needs_composite_);
+  DCHECK(trigger == COMPOSITE_IMMEDIATELY || trigger == COMPOSITE_EVENTUALLY);
 
-  if (!needs_composite_)
+  if (will_composite_immediately_ ||
+      (trigger == COMPOSITE_EVENTUALLY && WillComposite())) {
+    // We will already composite soon enough.
+    DCHECK(WillComposite());
     return;
+  }
 
-  if (trigger != COMPOSITE_ON_VSYNC && should_composite_on_vsync_) {
-    TRACE_EVENT0("compositor", "CompositorImpl_DeferCompositeToVSync");
-    root_window_->RequestVSyncUpdate();
+  if (DidCompositeThisFrame()) {
+    DCHECK(!WillCompositeThisFrame());
+    if (!WillComposite()) {
+      if (composite_on_vsync_trigger_ != COMPOSITE_IMMEDIATELY) {
+        composite_on_vsync_trigger_ = trigger;
+        root_window_->RequestVSyncUpdate();
+      }
+      DCHECK(WillComposite());

[brianderson, 2014/05/23 22:34:25]

The value of WillComposite() will only change if composite_on_vsync_trigger !=
COMPOSITE_IMMEDIATELY. Should the if statement above be a DCHECK instead?

[no sievers, 2014/05/27 18:20:45]

Thanks, I think line 277 is actually wrong because what it's supposed to do is
schedule on vsync with a request to schedule immediately taking priority.
However 'if (!WillComposite)' breaks that, so I removed that check.

+    }
     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_);
-    return;
+  base::TimeDelta delay;
+  if (trigger == COMPOSITE_IMMEDIATELY) {
+    will_composite_immediately_ = true;
+  } else {
+    DCHECK(!WillComposite());
+    const base::TimeDelta estimated_composite_time = vsync_period_ / 4;
+    const base::TimeTicks now = base::TimeTicks::Now();
+
+    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) {
+        // It's too late, we will reschedule composite as needed on the next
+        // vsync.

[no sievers, 2014/05/23 18:27:22]

You think we should do this for COMPOSITE_IMMEDIATELY also until we can throttle
the renderer better?

[brianderson, 2014/05/23 22:34:25]

No, I think we should just composite the Renderer swap ASAP  on Android if the
Browser hasn't already composited this frame. If there are perf issues, we
should fix them with better throttling or gpu scheduling.

+        composite_on_vsync_trigger_ = COMPOSITE_EVENTUALLY;
+        root_window_->RequestVSyncUpdate();
+        DCHECK(WillComposite());
+        return;
+      }
+
+      delay = next_composite - now;
+    }
   }
+  TRACE_EVENT2("cc", "CompositorImpl::PostComposite",
+               "trigger", trigger,
+               "delay", delay.InMillisecondsF());
+
+  if (current_composite_task_)
+    current_composite_task_->Cancel();
+
+  // Unretained because we cancel the task on shutdown.
+  current_composite_task_.reset(new base::CancelableClosure(
+      base::Bind(&CompositorImpl::Composite, base::Unretained(this), trigger)));
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, current_composite_task_->callback(), delay);
+}
+
+void CompositorImpl::Composite(CompositingTrigger trigger) {
+  DCHECK(host_);
+  DCHECK(trigger == COMPOSITE_IMMEDIATELY || trigger == COMPOSITE_EVENTUALLY);
+  DCHECK(needs_composite_);
+  DCHECK(!DidCompositeThisFrame());
+  DCHECK(composite_on_vsync_trigger_ == DO_NOT_COMPOSITE);
+
+  if (trigger == COMPOSITE_IMMEDIATELY)
+    will_composite_immediately_ = false;
 
-  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_);
     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.
+  // Only allow compositing once per vsync.
+  current_composite_task_->Cancel();
+  DCHECK(DidCompositeThisFrame() && !WillCompositeThisFrame());
+
+  // 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 +362,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) {
@@ -379,13 +421,15 @@ void CompositorImpl::SetSurface(jobject surface) {
 
 void CompositorImpl::SetVisible(bool visible) {
   if (!visible) {
+    if (WillComposite())
+      CancelComposite();
     ui_resource_map_.clear();
     host_.reset();
     client_->UIResourcesAreInvalid();
   } else if (!host_) {
+    DCHECK(!WillComposite());
     needs_composite_ = false;
-    did_composite_this_frame_ = false;
-    should_composite_on_vsync_ = false;
+    needs_animate_ = false;
     pending_swapbuffers_ = 0;
     cc::LayerTreeSettings settings;
     settings.refresh_rate = 60.0;
@@ -443,14 +487,12 @@ bool CompositorImpl::CompositeAndReadback(void *pixels, const gfx::Rect& rect) {
 }
 
 void CompositorImpl::SetNeedsComposite() {
-  if (!host_.get() || needs_composite_)
+  if (!host_.get())
     return;
+  DCHECK(!needs_composite_ || WillComposite());
 
   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 +622,15 @@ void CompositorImpl::OnLostResources() {
 }
 
 void CompositorImpl::ScheduleComposite() {
-  if (needs_composite_ || ignore_schedule_composite_)
+  DCHECK(!needs_composite_ || WillComposite());
+  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());
+  PostComposite(COMPOSITE_IMMEDIATELY);
 }
 
 void CompositorImpl::ScheduleAnimation() {
@@ -603,13 +645,17 @@ void CompositorImpl::DidPostSwapBuffers() {
 void CompositorImpl::DidCompleteSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidCompleteSwapBuffers");
   DCHECK_GT(pending_swapbuffers_, 0U);
-  client_->OnSwapBuffersCompleted(--pending_swapbuffers_);
+  if (pending_swapbuffers_-- == kMaxSwapBuffers && needs_composite_)
+    PostComposite(COMPOSITE_IMMEDIATELY);
+  client_->OnSwapBuffersCompleted(pending_swapbuffers_);
 }
 
 void CompositorImpl::DidAbortSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidAbortSwapBuffers");
   DCHECK_GT(pending_swapbuffers_, 0U);
-  client_->OnSwapBuffersCompleted(--pending_swapbuffers_);
+  if (pending_swapbuffers_-- == kMaxSwapBuffers && needs_composite_)
+    PostComposite(COMPOSITE_IMMEDIATELY);
+  client_->OnSwapBuffersCompleted(pending_swapbuffers_);
 }
 
 void CompositorImpl::DidCommit() {
@@ -628,10 +674,35 @@ void CompositorImpl::RequestCopyOfOutputOnRootLayer(
 void CompositorImpl::OnVSync(base::TimeTicks frame_time,
                              base::TimeDelta vsync_period) {
   vsync_period_ = vsync_period;
-  did_composite_this_frame_ = false;
+  last_vsync_ = frame_time;
 
-  if (should_composite_on_vsync_)
-    Composite(COMPOSITE_ON_VSYNC);
+  if (WillCompositeThisFrame()) {
+    // We somehow missed the last vsync interval, so reschedule immediately.
+    CancelComposite();
+    composite_on_vsync_trigger_ = COMPOSITE_IMMEDIATELY;
+  } else {
+    current_composite_task_.reset();
+  }
+
+  DCHECK(!DidCompositeThisFrame() && !WillCompositeThisFrame());
+  if (composite_on_vsync_trigger_ != DO_NOT_COMPOSITE) {
+    CompositingTrigger trigger = composite_on_vsync_trigger_;
+    composite_on_vsync_trigger_ = DO_NOT_COMPOSITE;
+    PostComposite(trigger);
+  }
+}
+
+void CompositorImpl::SetNeedsAnimate() {
+  DCHECK(!needs_animate_ || needs_composite_);
+  DCHECK(!needs_composite_ || WillComposite());
+  needs_animate_ = true;
+
+  if (needs_composite_)
+    return;
+
+  TRACE_EVENT0("cc", "CompositorImpl::SetNeedsAnimate");
+  needs_composite_ = true;
+  PostComposite(COMPOSITE_EVENTUALLY);
 }
 
 }  // namespace content