cc: Make scheduling be driven by vsync for android webview.

cc/scheduler/scheduler_state_machine.cc

Index: cc/scheduler/scheduler_state_machine.cc
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index c512987bd23f501abdaf1fceab783945f8dc1ae4..37f7bd6089ae4372a18d554cb93d002c7892c3d1 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -26,10 +26,12 @@ SchedulerStateMachine::SchedulerStateMachine(const SchedulerSettings& settings)
       last_frame_number_swap_performed_(-1),
       last_frame_number_swap_requested_(-1),
       last_frame_number_begin_main_frame_sent_(-1),
+      last_frame_number_invalidate_output_surface_performed_(-1),
       animate_funnel_(false),
       perform_swap_funnel_(false),
       request_swap_funnel_(false),
       send_begin_main_frame_funnel_(false),
+      invalidate_output_surface_funnel_(false),
       prepare_tiles_funnel_(0),
       consecutive_checkerboard_animations_(0),
       max_pending_swaps_(1),
@@ -38,7 +40,6 @@ SchedulerStateMachine::SchedulerStateMachine(const SchedulerSettings& settings)
       needs_animate_(false),
       needs_prepare_tiles_(false),
       needs_commit_(false),
-      inside_poll_for_anticipated_draw_triggers_(false),
       visible_(false),
       can_start_(false),
       can_draw_(false),
@@ -89,6 +90,22 @@ const char* SchedulerStateMachine::BeginImplFrameStateToString(
   return "???";
 }
 
+const char* SchedulerStateMachine::BeginImplFrameDeadlineModeToString(
+    BeginImplFrameDeadlineMode mode) {
+  switch (mode) {
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_IMMEDIATE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_IMMEDIATE";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_REGULAR:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_REGULAR";
+    case BEGIN_IMPL_FRAME_DEADLINE_MODE_LATE:
+      return "BEGIN_IMPL_FRAME_DEADLINE_MODE_LATE";
+  }
+  NOTREACHED();
+  return "???";
+}
+
 const char* SchedulerStateMachine::CommitStateToString(CommitState state) {
   switch (state) {
     case COMMIT_STATE_IDLE:
@@ -146,6 +163,8 @@ const char* SchedulerStateMachine::ActionToString(Action action) {
       return "ACTION_BEGIN_OUTPUT_SURFACE_CREATION";
     case ACTION_PREPARE_TILES:
       return "ACTION_PREPARE_TILES";
+    case ACTION_INVALIDATE_OUTPUT_SURFACE:
+      return "ACTION_INVALIDATE_OUTPUT_SURFACE";
   }
   NOTREACHED();
   return "???";
@@ -189,6 +208,8 @@ void SchedulerStateMachine::AsValueInto(
   state->SetBoolean("funnel: send_begin_main_frame_funnel",
                     send_begin_main_frame_funnel_);
   state->SetInteger("funnel: prepare_tiles_funnel", prepare_tiles_funnel_);
+  state->SetBoolean("funnel: invalidate_output_surface_funnel",
+                    invalidate_output_surface_funnel_);
   state->SetInteger("consecutive_checkerboard_animations",
                     consecutive_checkerboard_animations_);
   state->SetInteger("max_pending_swaps_", max_pending_swaps_);
@@ -221,23 +242,6 @@ void SchedulerStateMachine::AsValueInto(
   state->EndDictionary();
 }
 
-void SchedulerStateMachine::AdvanceCurrentFrameNumber() {
-  current_frame_number_++;
-
-  animate_funnel_ = false;
-  perform_swap_funnel_ = false;
-  request_swap_funnel_ = false;
-  send_begin_main_frame_funnel_ = false;
-
-  // "Drain" the PrepareTiles funnel.
-  if (prepare_tiles_funnel_ > 0)
-    prepare_tiles_funnel_--;
-
-  skip_begin_main_frame_to_reduce_latency_ =
-      skip_next_begin_main_frame_to_reduce_latency_;
-  skip_next_begin_main_frame_to_reduce_latency_ = false;
-}
-
 bool SchedulerStateMachine::PendingDrawsShouldBeAborted() const {
   // These are all the cases where we normally cannot or do not want to draw
   // but, if needs_redraw_ is true and we do not draw to make forward progress,
@@ -462,14 +466,30 @@ bool SchedulerStateMachine::ShouldPrepareTiles() const {
     return false;
 
   // Limiting to once per-frame is not enough, since we only want to
-  // prepare tiles _after_ draws. Polling for draw triggers and
-  // begin-frame are mutually exclusive, so we limit to these two cases.
-  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE &&
-      !inside_poll_for_anticipated_draw_triggers_)
+  // prepare tiles _after_ draws.
+  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE)
     return false;
+
   return needs_prepare_tiles_;
 }
 
+bool SchedulerStateMachine::ShouldInvalidateOutputSurface() const {
+  // Do not invalidate too many times in a frame.
+  if (invalidate_output_surface_funnel_)
+    return false;
+
+  // Only the synchronous compositor requires invalidations.
+  if (!settings_.using_synchronous_renderer_compositor)
+    return false;
+
+  // Invalidations are only performed inside a BeginFrame or inside deadline.
+  if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING &&
+      begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE)
+    return false;
+
+  return needs_redraw_ || needs_prepare_tiles_;

[brianderson, 2015/03/25 23:10:37]

Why needs_prepare_tiles_?

[sunnyps, 2015/03/27 00:52:48]

The previous draw might have caused tile properties to become dirty and set
needs_prepare_tiles_. It's possible we didn't prepare tiles then because of the
prepare tiles funnel. If we don't ask webview for a draw again we will never
call PrepareTiles but still keep asking for BeginFrames indefinitely.

In practice not having this causes the scheduler to never go to idle on certain
apps like Play Books.

[brianderson, 2015/03/27 21:24:24]

I think you can remove needs_prepare_tiles_ from here and still avoid the
scheduler never going idle if you drain the prepare_tiles_funnel__ in
BeginImplFrame rather than the deadline.

[sunnyps, 2015/03/27 22:38:24]

I moved the prepare_tiles_funnel_ decrement to OnBeginImplFrame but that does
not alleviate the need for checking needs_prepare_tiles here.

If needs_prepare_tiles is true we keep getting BeginFrames but unless we enter
the deadline phase we don't call PrepareTiles.

I know this causes some extra drawing to happen just for the sake of calling
PrepareTiles but it should be okay.

Aside: It would be great if we could call UDP+PrepareTiles on BeginImplFrame if
commit state is idle. That would solve this problem and probably other problems
as well.

+}
+
 SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
   if (ShouldActivatePendingTree())
     return ACTION_ACTIVATE_SYNC_TREE;
@@ -489,6 +509,8 @@ SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
     return ACTION_PREPARE_TILES;
   if (ShouldSendBeginMainFrame())
     return ACTION_SEND_BEGIN_MAIN_FRAME;
+  if (ShouldInvalidateOutputSurface())
+    return ACTION_INVALIDATE_OUTPUT_SURFACE;
   if (ShouldBeginOutputSurfaceCreation())
     return ACTION_BEGIN_OUTPUT_SURFACE_CREATION;
   return ACTION_NONE;
@@ -504,25 +526,11 @@ void SchedulerStateMachine::UpdateState(Action action) {
       return;
 
     case ACTION_ANIMATE:
-      DCHECK(!animate_funnel_);
-      last_frame_number_animate_performed_ = current_frame_number_;
-      animate_funnel_ = true;
-      needs_animate_ = false;
-      // TODO(skyostil): Instead of assuming this, require the client to tell
-      // us.
-      SetNeedsRedraw();
+      UpdateStateOnAnimate();
       return;
 
     case ACTION_SEND_BEGIN_MAIN_FRAME:
-      DCHECK(!has_pending_tree_ ||
-             settings_.main_frame_before_activation_enabled);
-      DCHECK(visible_);
-      DCHECK(!send_begin_main_frame_funnel_);
-      commit_state_ = COMMIT_STATE_BEGIN_MAIN_FRAME_SENT;
-      needs_commit_ = false;
-      send_begin_main_frame_funnel_ = true;
-      last_frame_number_begin_main_frame_sent_ =
-          current_frame_number_;
+      UpdateStateOnSendBeginMainFrame();
       return;
 
     case ACTION_COMMIT: {
@@ -545,23 +553,39 @@ void SchedulerStateMachine::UpdateState(Action action) {
     }
 
     case ACTION_BEGIN_OUTPUT_SURFACE_CREATION:
-      DCHECK_EQ(output_surface_state_, OUTPUT_SURFACE_LOST);
-      output_surface_state_ = OUTPUT_SURFACE_CREATING;
-
-      // The following DCHECKs make sure we are in the proper quiescent state.
-      // The pipeline should be flushed entirely before we start output
-      // surface creation to avoid complicated corner cases.
-      DCHECK_EQ(commit_state_, COMMIT_STATE_IDLE);
-      DCHECK(!has_pending_tree_);
-      DCHECK(!active_tree_needs_first_draw_);
+      UpdateStateOnBeginOutputSurfaceCreation();
       return;
 
     case ACTION_PREPARE_TILES:
       UpdateStateOnPrepareTiles();
       return;
+
+    case ACTION_INVALIDATE_OUTPUT_SURFACE:

[brianderson, 2015/03/25 23:10:37]

Thanks for making UpdateStateOn* method usage consistent.

+      UpdateStateOnInvalidateOutputSurface();
+      return;
   }
 }
 
+void SchedulerStateMachine::UpdateStateOnAnimate() {
+  DCHECK(!animate_funnel_);
+  last_frame_number_animate_performed_ = current_frame_number_;
+  animate_funnel_ = true;
+  needs_animate_ = false;
+  // TODO(skyostil): Instead of assuming this, require the client to tell
+  // us.

[brianderson, 2015/03/25 23:10:37]

Comment can be one line now.

[sunnyps, 2015/03/27 00:52:48]

Done.

+  SetNeedsRedraw();
+}
+
+void SchedulerStateMachine::UpdateStateOnSendBeginMainFrame() {
+  DCHECK(!has_pending_tree_ || settings_.main_frame_before_activation_enabled);
+  DCHECK(visible_);
+  DCHECK(!send_begin_main_frame_funnel_);
+  commit_state_ = COMMIT_STATE_BEGIN_MAIN_FRAME_SENT;
+  needs_commit_ = false;
+  send_begin_main_frame_funnel_ = true;
+  last_frame_number_begin_main_frame_sent_ = current_frame_number_;
+}
+
 void SchedulerStateMachine::UpdateStateOnCommit(bool commit_has_no_updates) {
   commit_count_++;
 
@@ -657,6 +681,28 @@ void SchedulerStateMachine::UpdateStateOnPrepareTiles() {
   needs_prepare_tiles_ = false;
 }
 
+void SchedulerStateMachine::UpdateStateOnBeginOutputSurfaceCreation() {
+  DCHECK_EQ(output_surface_state_, OUTPUT_SURFACE_LOST);
+  output_surface_state_ = OUTPUT_SURFACE_CREATING;
+
+  // The following DCHECKs make sure we are in the proper quiescent state.
+  // The pipeline should be flushed entirely before we start output
+  // surface creation to avoid complicated corner cases.
+  DCHECK_EQ(commit_state_, COMMIT_STATE_IDLE);
+  DCHECK(!has_pending_tree_);
+  DCHECK(!active_tree_needs_first_draw_);
+}
+
+void SchedulerStateMachine::UpdateStateOnInvalidateOutputSurface() {
+  DCHECK(!invalidate_output_surface_funnel_);
+  invalidate_output_surface_funnel_ = true;
+  last_frame_number_invalidate_output_surface_performed_ =
+      current_frame_number_;
+  // There is no guarantee that a draw will arrive from the synchronous
+  // compositor. We clear this flag here so that the pipeline isn't blocked.
+  active_tree_needs_first_draw_ = false;

[brianderson, 2015/03/25 23:10:37]

I'm worried that this will make us skip frames or draw duplicate frames. For
example, if we activated since the last draw, clear
active_tree_needs_first_draw_ here and then activate again before the next draw.

How often might we not get a draw from WebView after an Invalidate? If WebView
knows it's in this state, can it call something like DrawAbort() instead? That
way we wouldn't have to unconditionally remove the back pressure.

[sunnyps, 2015/03/27 00:52:48]

Webview can't even tell if it's in this state. Since webview allows apps to call
Java code from JS - it's possible to stop drawing at any point. Not setting this
flag to false can cause commit to be blocked indefinitely. An alternative to
this would be to not block commit due to this flag.

[brianderson, 2015/03/27 21:24:24]

Is the problem that we are blocking the main thread after sending a
BeginMainFrame? Or is it that we never send a BeginMainFrame in the first place?

In the first case, I think that could only happen because of the
SetNextCommitWaitsForActivation logic. In which case, maybe we should remove it.

In the second case, I think that is what we want. If the app isn't drawing, we
shouldn't start new commits.

[sunnyps, 2015/03/27 22:38:24]

The problem is that the main thread is blocked indefinitely while it's waiting
for a commit to happen.
The first case also happens if commit is blocked indefinitely.

[sunnyps, 2015/03/30 21:49:18]

I removed this piece of code and found that I could not reproduce the bug that
it fixed any more. So I'm removing it - we can add it later if we see the same
symptoms again.

+}
+
 void SchedulerStateMachine::SetSkipNextBeginMainFrameToReduceLatency() {
   TRACE_EVENT_INSTANT0("cc",
                        "Scheduler: SkipNextBeginMainFrameToReduceLatency",
@@ -676,44 +722,8 @@ bool SchedulerStateMachine::BeginFrameNeeded() const {
   // TODO(brianderson): Support output surface creation inside a BeginFrame.
   if (!HasInitializedOutputSurface())
     return false;
-
-  if (SupportsProactiveBeginFrame()) {
-    return (BeginFrameNeededToAnimateOrDraw() ||
-            BeginFrameNeededForChildren() ||
-            ProactiveBeginFrameWanted());
-  }
-
-  // Proactive BeginFrames are bad for the synchronous compositor because we
-  // have to draw when we get the BeginFrame and could end up drawing many
-  // duplicate frames if our new frame isn't ready in time.
-  // To poll for state with the synchronous compositor without having to draw,
-  // we rely on ShouldPollForAnticipatedDrawTriggers instead.
-  // Synchronous compositor doesn't have a browser.
-  DCHECK(!children_need_begin_frames_);
-  return BeginFrameNeededToAnimateOrDraw();
-}
-
-bool SchedulerStateMachine::ShouldPollForAnticipatedDrawTriggers() const {
-  // ShouldPollForAnticipatedDrawTriggers is what we use in place of
-  // ProactiveBeginFrameWanted when we are using the synchronous
-  // compositor.
-  if (!SupportsProactiveBeginFrame()) {
-    return !BeginFrameNeededToAnimateOrDraw() && ProactiveBeginFrameWanted();
-  }
-
-  // Non synchronous compositors should rely on
-  // ProactiveBeginFrameWanted to poll for state instead.
-  return false;
-}
-
-// Note: If SupportsProactiveBeginFrame is false, the scheduler should poll
-// for changes in it's draw state so it can request a BeginFrame when it's
-// actually ready.
-bool SchedulerStateMachine::SupportsProactiveBeginFrame() const {
-  // It is undesirable to proactively request BeginFrames if we are
-  // using a synchronous compositor because we *must* draw for every
-  // BeginFrame, which could cause duplicate draws.
-  return !settings_.using_synchronous_renderer_compositor;
+  return (BeginFrameNeededToAnimateOrDraw() || BeginFrameNeededForChildren() ||

[brianderson, 2015/03/25 23:10:37]

So much easier to understand!

+          ProactiveBeginFrameWanted());
 }
 
 void SchedulerStateMachine::SetChildrenNeedBeginFrames(
@@ -777,35 +787,50 @@ bool SchedulerStateMachine::ProactiveBeginFrameWanted() const {
 }
 
 void SchedulerStateMachine::OnBeginImplFrame() {
-  AdvanceCurrentFrameNumber();
-  DCHECK_EQ(begin_impl_frame_state_, BEGIN_IMPL_FRAME_STATE_IDLE)
-      << AsValue()->ToString();
   begin_impl_frame_state_ = BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING;
+  current_frame_number_++;
+
   last_commit_had_no_updates_ = false;
+
+  // Clear funnels for any actions we perform during the frame.
+  animate_funnel_ = false;
+  perform_swap_funnel_ = false;
+  request_swap_funnel_ = false;
+  send_begin_main_frame_funnel_ = false;
+  invalidate_output_surface_funnel_ = false;
+
+  skip_begin_main_frame_to_reduce_latency_ =
+      skip_next_begin_main_frame_to_reduce_latency_;
+  skip_next_begin_main_frame_to_reduce_latency_ = false;
 }
 
 void SchedulerStateMachine::OnBeginImplFrameDeadlinePending() {
-  DCHECK_EQ(begin_impl_frame_state_,
-            BEGIN_IMPL_FRAME_STATE_BEGIN_FRAME_STARTING)
-      << AsValue()->ToString();
   begin_impl_frame_state_ = BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME;
 }
 
 void SchedulerStateMachine::OnBeginImplFrameDeadline() {
-  DCHECK_EQ(begin_impl_frame_state_, BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME)
-      << AsValue()->ToString();
   begin_impl_frame_state_ = BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE;
+
+  // Clear funnels for any actions we perform during the deadline.

[brianderson, 2015/03/25 23:10:37]

Why are these cleared in both the deadline and BeginImplFrame?

[sunnyps, 2015/03/27 00:52:48]

For invalidate_output_surface_funnel_ the reason is that we can invalidate in
both the BeginImplFrame phase and the Draw phase.

[brianderson, 2015/03/27 21:24:24]

Can add a comment about that and also explain in what cases it makes sense to
invalidate twice in one frame?

Without knowing details, I feel like resetting the funnel only in the deadline
would be sufficient. If we invalidate just after draw1, we should be able to
expect a draw2 even if we don't invalidate in the BeginImplFrame2.  If we don't
invalidate just after draw1, the funnel will be empty, allowing BeginImplFrame2
to invalidate if needed.

Is there a hole in my understanding of WebView?

[sunnyps, 2015/03/27 22:38:24]

I changed this so that it's invalidated only once, I think you're looking at old
patch set.

+  perform_swap_funnel_ = false;
+  request_swap_funnel_ = false;
+  invalidate_output_surface_funnel_ = false;
+
+  // "Drain" the PrepareTiles funnel.

[brianderson, 2015/03/25 23:10:37]

Why is this moved to the deadline rather than the BeginImplFrame where it was
before?

[sunnyps, 2015/03/27 00:52:48]

Because prepare tiles (when initiated by the scheduler) is performed only in the
deadline. We should revisit this if we make changes to when PrepareTiles gets
called.

[brianderson, 2015/03/27 21:24:24]

Was the intent not to have a functional change by moving it here?

I think this will sometimes call prepare tiles inside of a draw just after
having prepared tiles for a commit, which is something we want to avoid.

[sunnyps, 2015/03/27 22:38:24]

Good catch. Done.

+  if (prepare_tiles_funnel_ > 0)
+    prepare_tiles_funnel_--;
 }
 
 void SchedulerStateMachine::OnBeginImplFrameIdle() {
-  DCHECK_EQ(begin_impl_frame_state_, BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE)
-      << AsValue()->ToString();
   begin_impl_frame_state_ = BEGIN_IMPL_FRAME_STATE_IDLE;
 }
 
 SchedulerStateMachine::BeginImplFrameDeadlineMode
 SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode() const {
-  if (ShouldTriggerBeginImplFrameDeadlineImmediately()) {
+  if (settings_.using_synchronous_renderer_compositor) {
+    // No deadline for synchronous compositor.
+    return BEGIN_IMPL_FRAME_DEADLINE_MODE_NONE;
+  } else if (ShouldTriggerBeginImplFrameDeadlineImmediately()) {
     return BEGIN_IMPL_FRAME_DEADLINE_MODE_IMMEDIATE;
   } else if (needs_redraw_ && pending_swaps_ < max_pending_swaps_) {
     // We have an animation or fast input path on the impl thread that wants
@@ -823,7 +848,6 @@ SchedulerStateMachine::CurrentBeginImplFrameDeadlineMode() const {
 bool SchedulerStateMachine::ShouldTriggerBeginImplFrameDeadlineImmediately()
     const {
   // TODO(brianderson): This should take into account multiple commit sources.
-
   if (begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME)
     return false;
 
@@ -898,15 +922,6 @@ bool SchedulerStateMachine::MainThreadIsInHighLatencyMode() const {
   return active_tree_needs_first_draw_;
 }
 
-void SchedulerStateMachine::DidEnterPollForAnticipatedDrawTriggers() {
-  AdvanceCurrentFrameNumber();
-  inside_poll_for_anticipated_draw_triggers_ = true;
-}
-
-void SchedulerStateMachine::DidLeavePollForAnticipatedDrawTriggers() {
-  inside_poll_for_anticipated_draw_triggers_ = false;
-}
-
 void SchedulerStateMachine::SetVisible(bool visible) { visible_ = visible; }
 
 void SchedulerStateMachine::SetCanDraw(bool can_draw) { can_draw_ = can_draw; }