android: Remove custom browser compositor scheduling logic

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 b6453a6e887128451f8c9600b08034adfdd2e136..d6f47c0acfab16824597f569a6e8eaa58e0b2998 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -142,8 +142,7 @@ class OutputSurfaceWithoutParent : public cc::OutputSurface,
     OutputSurface::OnSwapBuffersComplete();
   }
 
-  void OnUpdateVSyncParameters(base::TimeTicks timebase,
-                               base::TimeDelta interval) override {
+  void OnVSync(base::TimeTicks timebase, base::TimeDelta interval) override {
     CommitVSyncParameters(timebase, interval);
   }
 
@@ -155,6 +154,38 @@ class OutputSurfaceWithoutParent : public cc::OutputSurface,
   scoped_ptr<cc::OverlayCandidateValidator> overlay_candidate_validator_;
 };
 
+class ExternalBeginFrameSource : public cc::BeginFrameSourceBase,
+                                 public CompositorImpl::VSyncObserver {
+ public:
+  ExternalBeginFrameSource(CompositorImpl* compositor)
+      : compositor_(compositor) {
+    compositor_->AddObserver(this);
+  }
+
+  ~ExternalBeginFrameSource() {
+    compositor_->RemoveObserver(this);
+  }
+
+  // cc::BeginFrameSourceBase implementation:
+  void OnNeedsBeginFramesChange(
+      bool needs_begin_frames) override {
+    compositor_->OnNeedsBeginFramesChange(needs_begin_frames);
+  }
+
+  // CompositorImpl::VSyncObserver implementation:
+  void OnVSync(base::TimeTicks frame_time,
+               base::TimeDelta vsync_period) override {
+    if (NeedsBeginFrames()) {

[brianderson, 2015/12/12 01:41:46]

This check shouldn't be needed. The scheduler should be immune to spurious
BeginFrames, and actually wants them in case it suddenly decides it wants
BeginFrames.

[no sievers, 2015/12/17 00:52:32]

Done.

+      CallOnBeginFrame(cc::BeginFrameArgs::Create(
+          BEGINFRAME_FROM_HERE, frame_time, base::TimeTicks::Now(),
+          vsync_period, cc::BeginFrameArgs::NORMAL));
+    }
+  }
+
+ private:
+  CompositorImpl* compositor_;
+};
+
 static bool g_initialized = false;
 
 bool g_use_surface_manager = false;
@@ -237,15 +268,11 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       surface_id_(0),
       client_(client),
       root_window_(root_window),
-      did_post_swapbuffers_(false),
-      ignore_schedule_composite_(false),
-      needs_composite_(false),
       needs_animate_(false),
-      will_composite_immediately_(false),
-      composite_on_vsync_trigger_(DO_NOT_COMPOSITE),
       pending_swapbuffers_(0U),
       num_successive_context_creation_failures_(0),
       output_surface_request_pending_(false),
+      needs_begin_frames_(false),
       weak_factory_(this) {
   DCHECK(client);
   DCHECK(root_window);
@@ -260,111 +287,6 @@ CompositorImpl::~CompositorImpl() {
   SetSurface(NULL);
 }
 
-void CompositorImpl::PostComposite(CompositingTrigger trigger) {
-  DCHECK(host_->visible());
-  DCHECK(needs_composite_);
-  DCHECK(trigger == COMPOSITE_IMMEDIATELY || trigger == COMPOSITE_EVENTUALLY);
-
-  if (will_composite_immediately_ ||
-      (trigger == COMPOSITE_EVENTUALLY && WillComposite())) {
-    // We will already composite soon enough.
-    DCHECK(WillComposite());
-    return;
-  }
-
-  if (DidCompositeThisFrame()) {
-    DCHECK(!WillCompositeThisFrame());
-    if (composite_on_vsync_trigger_ != COMPOSITE_IMMEDIATELY) {
-      composite_on_vsync_trigger_ = trigger;
-      root_window_->RequestVSyncUpdate();
-    }
-    DCHECK(WillComposite());
-    return;
-  }
-
-  base::TimeDelta delay;
-  if (trigger == COMPOSITE_IMMEDIATELY) {
-    will_composite_immediately_ = true;
-    composite_on_vsync_trigger_ = DO_NOT_COMPOSITE;
-  } 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.
-        composite_on_vsync_trigger_ = COMPOSITE_EVENTUALLY;
-        root_window_->RequestVSyncUpdate();
-        DCHECK(WillComposite());
-        return;
-      }
-
-      delay = next_composite - now;
-    }
-  }
-  TRACE_EVENT2("cc,benchmark", "CompositorImpl::PostComposite",
-               "trigger", trigger,
-               "delay", delay.InMillisecondsF());
-
-  DCHECK(composite_on_vsync_trigger_ == DO_NOT_COMPOSITE);
-  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::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, current_composite_task_->callback(), delay);
-}
-
-void CompositorImpl::Composite(CompositingTrigger trigger) {
-  if (trigger == COMPOSITE_IMMEDIATELY)
-    will_composite_immediately_ = false;
-
-  DCHECK(host_->visible());
-  DCHECK(trigger == COMPOSITE_IMMEDIATELY || trigger == COMPOSITE_EVENTUALLY);
-  DCHECK(needs_composite_);
-  DCHECK(!DidCompositeThisFrame());
-
-  DCHECK_LE(pending_swapbuffers_, kMaxUiSwapBuffers);
-  // Swap Ack accounting is unreliable if the OutputSurface was lost.
-  // In that case still attempt to composite, which will cause creation of a
-  // new OutputSurface and reset pending_swapbuffers_.
-  if (pending_swapbuffers_ == kMaxUiSwapBuffers &&
-      !host_->output_surface_lost()) {
-    TRACE_EVENT0("compositor", "CompositorImpl_SwapLimit");
-    return;
-  }
-
-  // Reset state before Layout+Composite since that might create more
-  // requests to Composite that we need to respect.
-  needs_composite_ = false;
-
-  // Only allow compositing once per vsync.
-  current_composite_task_->Cancel();
-  DCHECK(DidCompositeThisFrame() && !WillComposite());
-
-  const base::TimeTicks frame_time = base::TimeTicks::Now();
-  if (needs_animate_) {
-    base::AutoReset<bool> auto_reset_ignore_schedule(
-        &ignore_schedule_composite_, true);
-    needs_animate_ = false;
-    root_window_->Animate(frame_time);
-  }
-
-  did_post_swapbuffers_ = false;
-  host_->Composite(frame_time);
-  if (did_post_swapbuffers_)
-    pending_swapbuffers_++;
-
-  // Need to track vsync to avoid compositing more than once per frame.
-  root_window_->RequestVSyncUpdate();
-}
-
 ui::UIResourceProvider& CompositorImpl::GetUIResourceProvider() {
   return *this;
 }
@@ -425,27 +347,20 @@ void CompositorImpl::SetSurface(jobject surface) {
 
 void CompositorImpl::CreateLayerTreeHost() {
   DCHECK(!host_);
-  DCHECK(!WillCompositeThisFrame());
-
-  // Just in case, since we immediately hide the LTH in this function,
-  // and we do not want to end up with a pending Composite task when the
-  // host is hidden.
-  base::AutoReset<bool> auto_reset_ignore_schedule(&ignore_schedule_composite_,
-                                                   true);
 
   cc::LayerTreeSettings settings;
   settings.renderer_settings.refresh_rate = 60.0;
   settings.renderer_settings.allow_antialiasing = false;
   settings.renderer_settings.highp_threshold_min = 2048;
   settings.use_zero_copy = true;
+  settings.use_external_begin_frame_source = true;
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   settings.initial_debug_state.SetRecordRenderingStats(
       command_line->HasSwitch(cc::switches::kEnableGpuBenchmarking));
   if (command_line->HasSwitch(cc::switches::kDisableCompositorPropertyTrees))
     settings.use_property_trees = false;
-  // TODO(enne): Update this this compositor to use the scheduler.
-  settings.single_thread_proxy_scheduler = false;
+  settings.single_thread_proxy_scheduler = true;
 
   settings.use_compositor_animation_timelines = !command_line->HasSwitch(
       switches::kDisableAndroidCompositorAnimationTimelines);
@@ -457,6 +372,7 @@ void CompositorImpl::CreateLayerTreeHost() {
   params.task_graph_runner = g_task_graph_runner.Pointer();
   params.main_task_runner = base::ThreadTaskRunnerHandle::Get();
   params.settings = &settings;
+  params.external_begin_frame_source.reset(new ExternalBeginFrameSource(this));
   host_ = cc::LayerTreeHost::CreateSingleThreaded(this, &params);
   DCHECK(!host_->visible());
   host_->SetRootLayer(root_layer_);
@@ -472,39 +388,17 @@ void CompositorImpl::SetVisible(bool visible) {
   TRACE_EVENT1("cc", "CompositorImpl::SetVisible", "visible", visible);
   if (!visible) {
     DCHECK(host_->visible());
-    // Look for any layers that were attached to the root for readback
-    // and are waiting for Composite() to happen.
-    bool readback_pending = false;
-    for (size_t i = 0; i < root_layer_->children().size(); ++i) {
-      if (root_layer_->children()[i]->HasCopyRequest()) {
-        readback_pending = true;
-        break;
-      }
-    }
-    if (readback_pending) {

[brianderson, 2015/12/12 01:41:46]

Does this break readbacks at all?

[no sievers, 2015/12/17 00:52:32]

The pending requests will still signal completion (but not success) when layers
are torn down. I'm not so worried about this now, since I only added it
speculatively a while back when there was talk about a use case for doing
readbacks while hiding the activity (which made pending readbacks here likely in
theory), but we actually didn't end up making that change.

-      base::AutoReset<bool> auto_reset_ignore_schedule(
-          &ignore_schedule_composite_, true);
-      host_->Composite(base::TimeTicks::Now());
-    }
-    if (WillComposite())
-      CancelComposite();
     host_->SetVisible(false);
     if (!host_->output_surface_lost())
       host_->ReleaseOutputSurface();
     pending_swapbuffers_ = 0;
-    needs_composite_ = false;
-    composite_on_vsync_trigger_ = DO_NOT_COMPOSITE;
     establish_gpu_channel_timeout_.Stop();
     display_client_.reset();
-    if (current_composite_task_) {
-      current_composite_task_->Cancel();
-      current_composite_task_.reset();
-    }
   } else {
     host_->SetVisible(true);
     if (output_surface_request_pending_)
       RequestNewOutputSurface();
-    SetNeedsComposite();
+    host_->SetNeedsCommit();

[brianderson, 2015/12/12 01:41:46]

Is this needed? I think the impl side requests it if resources have been
discarded or if the output surface is lost.

[no sievers, 2015/12/17 00:52:32]

You're right, not needed. Removed.

   }
 }
 
@@ -535,10 +429,7 @@ void CompositorImpl::SetHasTransparentBackground(bool flag) {
 void CompositorImpl::SetNeedsComposite() {
   if (!host_->visible())
     return;
-  DCHECK(!needs_composite_ || WillComposite());
-
-  needs_composite_ = true;
-  PostComposite(COMPOSITE_IMMEDIATELY);
+  host_->SetNeedsCommit();

[brianderson, 2015/12/12 01:41:46]

Was this why the SetNeedsCommit was needed above? Maybe remove the visibility
check?

[no sievers, 2015/12/17 00:52:32]

This is actually what the browser will call if it wants to draw (let's say for a
tab stack animation or what not). I had to change it to SetNeedsAnimate(), which
is actually the right thing, since SetNeedsCommit() is ignored during
UpdateLTH().
What the caller is trying to say here is 'call me again next frame when I update
my animation (i.e. I want to update layer properties in the next frame instead
of doing any invalidation now)'.

 }
 
 static scoped_ptr<WebGraphicsContext3DCommandBufferImpl>
@@ -572,9 +463,11 @@ CreateGpuProcessViewContext(
 }
 
 void CompositorImpl::UpdateLayerTreeHost() {
-  base::AutoReset<bool> auto_reset_ignore_schedule(&ignore_schedule_composite_,
-                                                   true);
   client_->UpdateLayerTreeHost();
+  if (needs_animate_) {
+    needs_animate_ = false;
+    root_window_->Animate(base::TimeTicks::Now());
+  }
 }
 
 void CompositorImpl::OnGpuChannelEstablished() {
@@ -704,44 +597,15 @@ bool CompositorImpl::SupportsETC1NonPowerOfTwo() const {
   return gpu_capabilities_.texture_format_etc1_npot;
 }
 
-void CompositorImpl::ScheduleComposite() {
-  if (ignore_schedule_composite_ || !host_->visible())
-    return;
-
-  DCHECK(!needs_composite_ || WillComposite());
-  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(COMPOSITE_IMMEDIATELY);
-}
-
-void CompositorImpl::ScheduleAnimation() {
-  needs_animate_ = true;
-
-  if (!host_->visible())
-    return;
-
-  if (needs_composite_) {
-    DCHECK(WillComposite());
-    return;
-  }
-
-  TRACE_EVENT0("cc", "CompositorImpl::ScheduleAnimation");
-  needs_composite_ = true;
-  PostComposite(COMPOSITE_EVENTUALLY);
-}
-
 void CompositorImpl::DidPostSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidPostSwapBuffers");
-  did_post_swapbuffers_ = true;
+  pending_swapbuffers_++;
 }
 
 void CompositorImpl::DidCompleteSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidCompleteSwapBuffers");
   DCHECK_GT(pending_swapbuffers_, 0U);
-  if (pending_swapbuffers_-- == kMaxUiSwapBuffers && needs_composite_)
-    PostComposite(COMPOSITE_IMMEDIATELY);
+  pending_swapbuffers_--;
   client_->OnSwapBuffersCompleted(pending_swapbuffers_);

[brianderson, 2015/12/12 01:41:46]

It doesn't look like any implementers look at pending_swapbuffers_. Can you
delete the pending_swapbuffers_ member variable?

Also, can this be removed all together? I don't understand what needs to be tied
to when a swap occurs. What happens if there's no damage and we don't actually
swap?

[no sievers, 2015/12/17 00:52:32]

Unfortunately this is used in CompositorViewHolder.java, see
onSwapBuffersCompleted() where |pendingSwapBuffersCount == 0| is used to detect
when we go idle here in order to flush pending invalidations for Java Views
(IIRC).

[brianderson, 2015/12/17 06:14:53]

Ok. I see where it's used now and am okay keeping the code as-is for this patch.
It seems like an odd signal to rely on though. I wonder if it makes more sense
to pipe the BeginImplFrameNotExpectedSoon event up (as a content-is-idle signal)
to flush invalidations.

 }
 
@@ -769,28 +633,19 @@ void CompositorImpl::RequestCopyOfOutputOnRootLayer(
 
 void CompositorImpl::OnVSync(base::TimeTicks frame_time,
                              base::TimeDelta vsync_period) {
-  vsync_period_ = vsync_period;
-  last_vsync_ = frame_time;
-
-  if (WillCompositeThisFrame()) {
-    // We somehow missed the last vsync interval, so reschedule for deadline.
-    // We cannot schedule immediately, or will get us out-of-phase with new
-    // renderer frames.
-    CancelComposite();
-    composite_on_vsync_trigger_ = COMPOSITE_EVENTUALLY;
-  } else {
-    current_composite_task_.reset();
-  }
+  FOR_EACH_OBSERVER(VSyncObserver, observer_list_,
+                    OnVSync(frame_time, vsync_period));
+  if (needs_begin_frames_)
+    root_window_->RequestVSyncUpdate();
+}
 
-  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::OnNeedsBeginFramesChange(bool needs_begin_frames) {
+  if (needs_begin_frames_ == needs_begin_frames)
+    return;
 
-  FOR_EACH_OBSERVER(VSyncObserver, observer_list_,
-                    OnUpdateVSyncParameters(frame_time, vsync_period));
+  needs_begin_frames_ = needs_begin_frames;
+  if (needs_begin_frames_)
+    root_window_->RequestVSyncUpdate();
 }
 
 void CompositorImpl::SetNeedsAnimate() {