Reland of cc: Remove frame queuing from the scheduler.

cc/scheduler/scheduler.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/scheduler.h"
 
 #include <algorithm>
 
 #include "base/auto_reset.h"
 #include "base/logging.h"
@@ skipping 46 matching lines @@
           SchedulerStateMachine::BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE),
       begin_impl_frame_tracker_(BEGINFRAMETRACKER_FROM_HERE),
       state_machine_(settings),
       inside_process_scheduled_actions_(false),
       inside_action_(SchedulerStateMachine::ACTION_NONE),
       weak_factory_(this) {
   TRACE_EVENT1("cc", "Scheduler::Scheduler", "settings", settings_.AsValue());
   DCHECK(client_);
   DCHECK(!state_machine_.BeginFrameNeeded());
 
-  begin_retro_frame_closure_ =
-      base::Bind(&Scheduler::BeginRetroFrame, weak_factory_.GetWeakPtr());
   begin_impl_frame_deadline_closure_ = base::Bind(
       &Scheduler::OnBeginImplFrameDeadline, weak_factory_.GetWeakPtr());
 
   SetBeginFrameSource(begin_frame_source);
   ProcessScheduledActions();
 }
 
 Scheduler::~Scheduler() {
   SetBeginFrameSource(nullptr);
 }
@@ skipping 114 matching lines @@
   compositor_timing_history_->WillPrepareTiles();
 }
 
 void Scheduler::DidPrepareTiles() {
   compositor_timing_history_->DidPrepareTiles();
   state_machine_.DidPrepareTiles();
 }
 
 void Scheduler::DidLoseOutputSurface() {
   TRACE_EVENT0("cc", "Scheduler::DidLoseOutputSurface");
-  begin_retro_frame_args_.clear();
-  begin_retro_frame_task_.Cancel();
   state_machine_.DidLoseOutputSurface();
   UpdateCompositorTimingHistoryRecordingEnabled();
   ProcessScheduledActions();
 }
 
 void Scheduler::DidCreateAndInitializeOutputSurface() {
   TRACE_EVENT0("cc", "Scheduler::DidCreateAndInitializeOutputSurface");
   DCHECK(!observing_begin_frame_source_);
   DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
   state_machine_.DidCreateAndInitializeOutputSurface();
@@ skipping 16 matching lines @@
 void Scheduler::BeginImplFrameNotExpectedSoon() {
   compositor_timing_history_->BeginImplFrameNotExpectedSoon();
 
   // Tying this to SendBeginMainFrameNotExpectedSoon will have some
   // false negatives, but we want to avoid running long idle tasks when
   // we are actually active.
   client_->SendBeginMainFrameNotExpectedSoon();
 }
 
 void Scheduler::SetupNextBeginFrameIfNeeded() {
-  // Never call SetNeedsBeginFrames if the frame source already has the right
-  // value.
-  if (observing_begin_frame_source_ != state_machine_.BeginFrameNeeded()) {
-    if (state_machine_.BeginFrameNeeded()) {
-      // Call AddObserver as soon as possible.
-      observing_begin_frame_source_ = true;
-      if (begin_frame_source_)
-        begin_frame_source_->AddObserver(this);
-      devtools_instrumentation::NeedsBeginFrameChanged(layer_tree_host_id_,
-                                                       true);
-    } else if (state_machine_.begin_impl_frame_state() ==
-               SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE) {
-      // Call RemoveObserver in between frames only.
-      observing_begin_frame_source_ = false;
-      if (begin_frame_source_)
-        begin_frame_source_->RemoveObserver(this);
-      BeginImplFrameNotExpectedSoon();
-      devtools_instrumentation::NeedsBeginFrameChanged(layer_tree_host_id_,
-                                                       false);
-    }
+  if (state_machine_.begin_impl_frame_state() !=
+      SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE) {
+    return;
   }
 
-  PostBeginRetroFrameIfNeeded();
+  bool needs_begin_frames = state_machine_.BeginFrameNeeded();
+  if (needs_begin_frames && !observing_begin_frame_source_) {
+    observing_begin_frame_source_ = true;
+    if (begin_frame_source_)
+      begin_frame_source_->AddObserver(this);
+    devtools_instrumentation::NeedsBeginFrameChanged(layer_tree_host_id_, true);
+  } else if (!needs_begin_frames && observing_begin_frame_source_) {
+    observing_begin_frame_source_ = false;
+    if (begin_frame_source_)
+      begin_frame_source_->RemoveObserver(this);
+    missed_begin_frame_task_.Cancel();
+    BeginImplFrameNotExpectedSoon();
+    devtools_instrumentation::NeedsBeginFrameChanged(layer_tree_host_id_,
+                                                     false);
+  }
 }
 
 void Scheduler::OnBeginFrameSourcePausedChanged(bool paused) {
   if (state_machine_.begin_frame_source_paused() == paused)
     return;
   TRACE_EVENT_INSTANT1("cc", "Scheduler::SetBeginFrameSourcePaused",
                        TRACE_EVENT_SCOPE_THREAD, "paused", paused);
   state_machine_.SetBeginFrameSourcePaused(paused);
   ProcessScheduledActions();
 }
 
 // BeginFrame is the mechanism that tells us that now is a good time to start
 // making a frame. Usually this means that user input for the frame is complete.
 // If the scheduler is busy, we queue the BeginFrame to be handled later as
 // a BeginRetroFrame.
 bool Scheduler::OnBeginFrameDerivedImpl(const BeginFrameArgs& args) {
   TRACE_EVENT1("cc,benchmark", "Scheduler::BeginFrame", "args", args.AsValue());
 
+  if (!state_machine_.BeginFrameNeeded()) {

[brianderson, 2016/09/19 22:56:44]

This block of code may cause us to skip frames in cases where the renderer
suddenly decides it wants to draw mid frame (for example because of a new input
event or a setInterval driven animation).

Do you think there would be any harm in removing it?

I think the previous logic would queue the BeginFrame up as a retro frame that
could still be used even if it weren't needed immediately.

[sunnyps, 2016/09/19 23:59:04]

This is handled by missed begin frames which are supported by all BFS
implementations now. Also the scheduler can't really get a BeginFrame before it
starts observing the BFS, it used to be the case that the scheduler was the only
observer in the BFS and would receive begin frames even when not needed, but now
the BFS doesn't even know about the scheduler until the scheduler calls
AddObserver.

+    TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginFrameDropped",
+                         TRACE_EVENT_SCOPE_THREAD);
+    return false;
+  }
+
   // Trace this begin frame time through the Chrome stack
   TRACE_EVENT_FLOW_BEGIN0(
       TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"), "BeginFrameArgs",
       args.frame_time.ToInternalValue());
 
   // TODO(brianderson): Adjust deadline in the DisplayScheduler.
   BeginFrameArgs adjusted_args(args);
   adjusted_args.deadline -= EstimatedParentDrawTime();
 
   if (settings_.using_synchronous_renderer_compositor) {
     BeginImplFrameSynchronous(adjusted_args);
     return true;
   }
 
-  // We have just called SetNeedsBeginFrame(true) and the BeginFrameSource has
-  // sent us the last BeginFrame we have missed. As we might not be able to
-  // actually make rendering for this call, handle it like a "retro frame".
-  // TODO(brainderson): Add a test for this functionality ASAP!
-  if (adjusted_args.type == BeginFrameArgs::MISSED) {
-    begin_retro_frame_args_.push_back(adjusted_args);
-    PostBeginRetroFrameIfNeeded();
+  if (inside_process_scheduled_actions_) {
+    // The BFS can send a missed begin frame inside AddObserver. We can't handle
+    // a begin frame inside ProcessScheduledActions so post a task.
+    DCHECK_EQ(adjusted_args.type, BeginFrameArgs::MISSED);
+    DCHECK(missed_begin_frame_task_.IsCancelled());
+    missed_begin_frame_task_.Reset(
+        base::Bind(&Scheduler::BeginImplFrameWithDeadline,
+                   base::Unretained(this), adjusted_args));
+    task_runner_->PostTask(FROM_HERE, missed_begin_frame_task_.callback());
     return true;
   }
 
-  bool should_defer_begin_frame =
-      !begin_retro_frame_args_.empty() ||
-      !begin_retro_frame_task_.IsCancelled() ||
-      !observing_begin_frame_source_ ||
-      (state_machine_.begin_impl_frame_state() !=
-       SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
-
-  if (should_defer_begin_frame) {
-    begin_retro_frame_args_.push_back(adjusted_args);
-    TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginFrame deferred",
-                         TRACE_EVENT_SCOPE_THREAD);
-    // Queuing the frame counts as "using it", so we need to return true.
-  } else {
-    BeginImplFrameWithDeadline(adjusted_args);
-  }
+  BeginImplFrameWithDeadline(adjusted_args);
   return true;
 }
 
 void Scheduler::SetVideoNeedsBeginFrames(bool video_needs_begin_frames) {
   state_machine_.SetVideoNeedsBeginFrames(video_needs_begin_frames);
   ProcessScheduledActions();
 }
 
 void Scheduler::OnDrawForOutputSurface(bool resourceless_software_draw) {
   DCHECK(settings_.using_synchronous_renderer_compositor);
   DCHECK_EQ(state_machine_.begin_impl_frame_state(),
             SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
   DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
 
   state_machine_.SetResourcelessSoftwareDraw(resourceless_software_draw);
   state_machine_.OnBeginImplFrameDeadline();
   ProcessScheduledActions();
 
   state_machine_.OnBeginImplFrameIdle();
   ProcessScheduledActions();
   state_machine_.SetResourcelessSoftwareDraw(false);
 }
 
-// BeginRetroFrame is called for BeginFrames that we've deferred because
-// the scheduler was in the middle of processing a previous BeginFrame.
-void Scheduler::BeginRetroFrame() {
-  TRACE_EVENT0("cc,benchmark", "Scheduler::BeginRetroFrame");
-  DCHECK(!settings_.using_synchronous_renderer_compositor);
-  DCHECK(!begin_retro_frame_args_.empty());
-  DCHECK(!begin_retro_frame_task_.IsCancelled());
+void Scheduler::BeginImplFrameWithDeadline(const BeginFrameArgs& args) {
+  // The storage for |args| is owned by the missed begin frame task. Therefore
+  // save |args| before cancelling the task either here or in the deadline.
+  BeginFrameArgs adjusted_args = args;
+  // Cancel the missed begin frame task in case the BFS sends a begin frame
+  // before the missed frame task runs.
+  missed_begin_frame_task_.Cancel();
+
+  // Discard missed begin frames in renderer scheduler if they are too late.

[brianderson, 2016/09/19 22:56:44]

Can you add a comment explaining why there's a difference in renderer vs.
browser here.

[sunnyps, 2016/09/19 23:59:04]

Actually I removed the exception for the browser. I couldn't reproduce a
difference by testing locally. I'll make this change in a separate CL.

+  if (!settings_.commit_to_active_tree &&
+      adjusted_args.type == BeginFrameArgs::MISSED &&
+      Now() > adjusted_args.deadline) {
+    begin_frame_source_->DidFinishFrame(this, 0);
+    return;
+  }
+
+  // Run the previous deadline if any.
+  if (state_machine_.begin_impl_frame_state() ==
+      SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME) {
+    OnBeginImplFrameDeadline();
+    // We may not need begin frames any longer.
+    if (!observing_begin_frame_source_) {
+      begin_frame_source_->DidFinishFrame(this, 0);
+      return;
+    }
+  }
   DCHECK_EQ(state_machine_.begin_impl_frame_state(),
             SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
 
-  begin_retro_frame_task_.Cancel();
-
-  // Discard expired BeginRetroFrames
-  // Today, we should always end up with at most one un-expired BeginRetroFrame
-  // because deadlines will not be greater than the next frame time. We don't
-  // DCHECK though because some systems don't always have monotonic timestamps.
-  // TODO(brianderson): In the future, long deadlines could result in us not
-  // draining the queue if we don't catch up. If we consistently can't catch
-  // up, our fallback should be to lower our frame rate.
-  base::TimeTicks now = Now();
-
-  while (!begin_retro_frame_args_.empty()) {
-    const BeginFrameArgs& args = begin_retro_frame_args_.front();
-    base::TimeTicks expiration_time = args.deadline;
-    if (now <= expiration_time)
-      break;
-    TRACE_EVENT_INSTANT2(
-        "cc", "Scheduler::BeginRetroFrame discarding", TRACE_EVENT_SCOPE_THREAD,
-        "expiration_time - now", (expiration_time - now).InMillisecondsF(),
-        "BeginFrameArgs", begin_retro_frame_args_.front().AsValue());
-    begin_retro_frame_args_.pop_front();
-    if (begin_frame_source_)
-      begin_frame_source_->DidFinishFrame(this, begin_retro_frame_args_.size());
-  }
-
-  if (begin_retro_frame_args_.empty()) {
-    TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginRetroFrames all expired",
-                         TRACE_EVENT_SCOPE_THREAD);
-  } else {
-    BeginFrameArgs front = begin_retro_frame_args_.front();
-    begin_retro_frame_args_.pop_front();
-    BeginImplFrameWithDeadline(front);
-  }
-}
-
-// There could be a race between the posted BeginRetroFrame and a new
-// BeginFrame arriving via the normal mechanism. Scheduler::BeginFrame
-// will check if there is a pending BeginRetroFrame to ensure we handle
-// BeginFrames in FIFO order.
-void Scheduler::PostBeginRetroFrameIfNeeded() {
-  TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler"),
-               "Scheduler::PostBeginRetroFrameIfNeeded", "state", AsValue());
-  if (!observing_begin_frame_source_)
-    return;
-
-  if (begin_retro_frame_args_.empty() || !begin_retro_frame_task_.IsCancelled())
-    return;
-
-  // begin_retro_frame_args_ should always be empty for the
-  // synchronous compositor.
-  DCHECK(!settings_.using_synchronous_renderer_compositor);
-
-  if (state_machine_.begin_impl_frame_state() !=
-      SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE)
-    return;
-
-  begin_retro_frame_task_.Reset(begin_retro_frame_closure_);
-
-  task_runner_->PostTask(FROM_HERE, begin_retro_frame_task_.callback());
-}
-
-void Scheduler::BeginImplFrameWithDeadline(const BeginFrameArgs& args) {
   bool main_thread_is_in_high_latency_mode =
       state_machine_.main_thread_missed_last_deadline();
   TRACE_EVENT2("cc,benchmark", "Scheduler::BeginImplFrame", "args",
-               args.AsValue(), "main_thread_missed_last_deadline",
+               adjusted_args.AsValue(), "main_thread_missed_last_deadline",
                main_thread_is_in_high_latency_mode);
   TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler"),
                  "MainThreadLatency", main_thread_is_in_high_latency_mode);
 
-  BeginFrameArgs adjusted_args = args;
+  DCHECK_EQ(state_machine_.begin_impl_frame_state(),
+            SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
+
   adjusted_args.deadline -= compositor_timing_history_->DrawDurationEstimate();
   adjusted_args.deadline -= kDeadlineFudgeFactor;
 
   base::TimeDelta bmf_start_to_activate =
       compositor_timing_history_
           ->BeginMainFrameStartToCommitDurationEstimate() +
       compositor_timing_history_->CommitToReadyToActivateDurationEstimate() +
       compositor_timing_history_->ActivateDurationEstimate();
 
   base::TimeDelta bmf_to_activate_estimate_critical =
       bmf_start_to_activate +
       compositor_timing_history_->BeginMainFrameQueueDurationCriticalEstimate();
 
   state_machine_.SetCriticalBeginMainFrameToActivateIsFast(
-      bmf_to_activate_estimate_critical < args.interval);
+      bmf_to_activate_estimate_critical < adjusted_args.interval);
 
   // Update the BeginMainFrame args now that we know whether the main
   // thread will be on the critical path or not.
   begin_main_frame_args_ = adjusted_args;
   begin_main_frame_args_.on_critical_path = !ImplLatencyTakesPriority();
 
   base::TimeDelta bmf_to_activate_estimate = bmf_to_activate_estimate_critical;
   if (!begin_main_frame_args_.on_critical_path) {
     bmf_to_activate_estimate =
         bmf_start_to_activate +
         compositor_timing_history_
             ->BeginMainFrameQueueDurationNotCriticalEstimate();
   }
 
   bool can_activate_before_deadline =
       CanBeginMainFrameAndActivateBeforeDeadline(adjusted_args,
                                                  bmf_to_activate_estimate);
 
   if (ShouldRecoverMainLatency(adjusted_args, can_activate_before_deadline)) {
     TRACE_EVENT_INSTANT0("cc", "SkipBeginMainFrameToReduceLatency",
                          TRACE_EVENT_SCOPE_THREAD);
     state_machine_.SetSkipNextBeginMainFrameToReduceLatency();
   } else if (ShouldRecoverImplLatency(adjusted_args,
                                       can_activate_before_deadline)) {
     TRACE_EVENT_INSTANT0("cc", "SkipBeginImplFrameToReduceLatency",
                          TRACE_EVENT_SCOPE_THREAD);
     if (begin_frame_source_)
-      begin_frame_source_->DidFinishFrame(this, begin_retro_frame_args_.size());
+      begin_frame_source_->DidFinishFrame(this, 0);
     return;
   }
 
   BeginImplFrame(adjusted_args);
 }
 
 void Scheduler::BeginImplFrameSynchronous(const BeginFrameArgs& args) {
   TRACE_EVENT1("cc,benchmark", "Scheduler::BeginImplFrame", "args",
                args.AsValue());
 
   // The main thread currently can't commit before we draw with the
   // synchronous compositor, so never consider the BeginMainFrame fast.
   state_machine_.SetCriticalBeginMainFrameToActivateIsFast(false);
   begin_main_frame_args_ = args;
   begin_main_frame_args_.on_critical_path = !ImplLatencyTakesPriority();
 
   BeginImplFrame(args);
   compositor_timing_history_->WillFinishImplFrame(
       state_machine_.needs_redraw());
   FinishImplFrame();
 }
 
 void Scheduler::FinishImplFrame() {
   state_machine_.OnBeginImplFrameIdle();
   ProcessScheduledActions();
 
   client_->DidFinishImplFrame();
   if (begin_frame_source_)
-    begin_frame_source_->DidFinishFrame(this, begin_retro_frame_args_.size());
+    begin_frame_source_->DidFinishFrame(this, 0);
   begin_impl_frame_tracker_.Finish();
 }
 
 // BeginImplFrame starts a compositor frame that will wait up until a deadline
 // for a BeginMainFrame+activation to complete before it times out and draws
 // any asynchronous animation and scroll/pinch updates.
 void Scheduler::BeginImplFrame(const BeginFrameArgs& args) {
   DCHECK_EQ(state_machine_.begin_impl_frame_state(),
             SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
   DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
@@ skipping 208 matching lines @@
   state_machine_.AsValueInto(state.get());
   state->EndDictionary();
 
   state->BeginDictionary("scheduler_state");
   state->SetBoolean("throttle_frame_production_",
                     settings_.throttle_frame_production);
   state->SetDouble("estimated_parent_draw_time_ms",
                    estimated_parent_draw_time_.InMillisecondsF());
   state->SetBoolean("observing_begin_frame_source",
                     observing_begin_frame_source_);
-  state->SetInteger("begin_retro_frame_args",
-                    static_cast<int>(begin_retro_frame_args_.size()));
-  state->SetBoolean("begin_retro_frame_task",
-                    !begin_retro_frame_task_.IsCancelled());
   state->SetBoolean("begin_impl_frame_deadline_task",
                     !begin_impl_frame_deadline_task_.IsCancelled());
+  state->SetBoolean("missed_begin_frame_task",
+                    !missed_begin_frame_task_.IsCancelled());
   state->SetString("inside_action",
                    SchedulerStateMachine::ActionToString(inside_action_));
 
   state->BeginDictionary("begin_impl_frame_args");
   begin_impl_frame_tracker_.AsValueInto(now, state.get());
   state->EndDictionary();
 
   state->SetString("begin_impl_frame_deadline_mode_",
                    SchedulerStateMachine::BeginImplFrameDeadlineModeToString(
                        begin_impl_frame_deadline_mode_));
@@ skipping 77 matching lines @@
 }
 
 bool Scheduler::IsBeginMainFrameSentOrStarted() const {
   return (state_machine_.begin_main_frame_state() ==
               SchedulerStateMachine::BEGIN_MAIN_FRAME_STATE_SENT ||
           state_machine_.begin_main_frame_state() ==
               SchedulerStateMachine::BEGIN_MAIN_FRAME_STATE_STARTED);
 }
 
 }  // namespace cc