CC: Add a scheduled action for ManageTiles

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 a608fb902c74864a3203db522e5f72f367a091eb..282eaf7cfae50a2ece9035cb221ca93986ae91fc 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -146,6 +146,8 @@ const char* SchedulerStateMachine::ActionToString(Action action) {
       return "ACTION_BEGIN_OUTPUT_SURFACE_CREATION";
     case ACTION_ACQUIRE_LAYER_TEXTURES_FOR_MAIN_THREAD:
       return "ACTION_ACQUIRE_LAYER_TEXTURES_FOR_MAIN_THREAD";
+    case ACTION_MANAGE_TILES:
+      return "ACTION_MANAGE_TILES";
   }
   NOTREACHED();
   return "???";
@@ -209,6 +211,7 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
   minor_state->SetInteger("consecutive_failed_draws",
                           consecutive_failed_draws_);
   minor_state->SetBoolean("needs_redraw", needs_redraw_);
+  minor_state->SetBoolean("needs_manage_tiles", needs_manage_tiles_);
   minor_state->SetBoolean("swap_used_incomplete_tile",
                           swap_used_incomplete_tile_);
   minor_state->SetBoolean("needs_commit", needs_commit_);
@@ -442,6 +445,12 @@ bool SchedulerStateMachine::ShouldCommit() const {
   return commit_state_ == COMMIT_STATE_READY_TO_COMMIT;
 }
 
+bool SchedulerStateMachine::ShouldManageTiles() const {

[enne (OOO), 2013/09/11 23:45:11]

Yeah, I agree strongly with brianderson's suggestion about throttling this to
happen once per frame.  Without that, this seems like this patch could
accidentally regress current behavior.

It'd also be nice if the ManageTiles from commit could be wrapped into the
scheduler too so that the manual ManageTiles there wouldn't happen in the same
frame as a scheduler-initiated one.  I think you could do this if you added a
needs_post_commit_manage_tiles_ variable.  You could update its value to
needs_manage_tiles_ in OnUpdateState, and then check it here before checking
inside_begin_frame.  What do you think?

[epennerAtGoogle, 2013/09/12 00:09:59]

Just for clarity: The extra throttling is only required for the synchronous
compositor right? IIUC throttling by inside_begin_frame_ is enough in other
cases?

Regarding the commit ManageTiles: I initially thought we could merge the
synchronous commit ManageTiles... However, I think there is a problem: The
commit changes tile priorities, so the last ManageTiles isn't sufficient.
Therefore, waiting for the next ManageTiles effectively delays commits due to
delaying painting the new tiles etc.

So unless my understanding above is off, we can't skip the commit ManageTiles
unless we are in an accelerate gesture (don't care about commit latency)...
Alternatively maybe we could skip the next post-draw ManageTiles, but I think
that has the same problem (CC may have changed scroll offsets etc. and we would
miss those for one frame).

[brianderson, 2013/09/12 00:24:08]

Yes, the extra throttling is only required for the synchronous compositor, but
should also work without the synchronous compositor and make the check for
inside_begin_frame unnecessary.

I agree that we should not skip the ManageTiles after the commit as it would
delay activation. It would be nice if the ManageTiles after the commit cleared
the Scheduler's needs_manage_tiles and also blocked future ManageTiles from the
Scheduler until the next frame though.

+  if (!inside_begin_frame_)
+    return false;
+  return needs_manage_tiles_;
+}
+
 SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
   if (ShouldAcquireLayerTexturesForMainThread())
     return ACTION_ACQUIRE_LAYER_TEXTURES_FOR_MAIN_THREAD;
@@ -461,6 +470,8 @@ SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
     else
       return ACTION_DRAW_AND_SWAP_IF_POSSIBLE;
   }
+  if (ShouldManageTiles())
+    return ACTION_MANAGE_TILES;
   if (ShouldSendBeginFrameToMainThread())
     return ACTION_SEND_BEGIN_FRAME_TO_MAIN_THREAD;
   if (ShouldBeginOutputSurfaceCreation())
@@ -536,6 +547,10 @@ void SchedulerStateMachine::UpdateState(Action action) {
       texture_state_ = LAYER_TEXTURE_STATE_ACQUIRED_BY_MAIN_THREAD;
       main_thread_needs_layer_textures_ = false;
       return;
+
+    case ACTION_MANAGE_TILES:
+      UpdateStateOnManageTiles();
+      return;
   }
 }
 
@@ -677,6 +692,10 @@ void SchedulerStateMachine::UpdateStateOnDraw(bool did_swap) {
     last_frame_number_swap_performed_ = current_frame_number_;
 }
 
+void SchedulerStateMachine::UpdateStateOnManageTiles() {
+  needs_manage_tiles_ = false;
+}
+
 void SchedulerStateMachine::SetMainThreadNeedsLayerTextures() {
   DCHECK(!main_thread_needs_layer_textures_);
   DCHECK_NE(texture_state_, LAYER_TEXTURE_STATE_ACQUIRED_BY_MAIN_THREAD);
@@ -740,6 +759,11 @@ bool SchedulerStateMachine::ProactiveBeginFrameWantedByImplThread() const {
   if (has_pending_tree_)
     return true;
 
+  // Changing priorities may allow us to activate (given the new priorities),
+  // which may result in a new frame.
+  if (needs_manage_tiles_)
+    return true;

[epenner, 2013/09/11 22:55:47]

Brianderson@, is there anything I need to worry about here regarding the
synchronous compositor. The comment references this being an issue, but I don't
know understand how that case should be handled (or whether it's handled in
general somewhere else).

[brianderson, 2013/09/11 23:14:37]

I'm working on a patch to fix issues like this, when we need to poll for
actions/state that might produce a new frame with the synchronous compositor:
https://codereview.chromium.org/23463014/

For that to work with this patch, you will need to throttle ShouldManageTiles by
the current_frame_number_.

+
   return false;
 }
 
@@ -757,6 +781,10 @@ void SchedulerStateMachine::SetVisible(bool visible) { visible_ = visible; }
 
 void SchedulerStateMachine::SetNeedsRedraw() { needs_redraw_ = true; }
 
+void SchedulerStateMachine::SetNeedsManageTiles() {
+  needs_manage_tiles_ = true;
+}
+
 void SchedulerStateMachine::SetSwapUsedIncompleteTile(
     bool used_incomplete_tile) {
   swap_used_incomplete_tile_ = used_incomplete_tile;