cc: Add readback and forced draw states to the Scheduler

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 c20238b3a589fd456a164e6783facc30eff34966..4ea105eed8f281f270a14e8a880534de1f38ca82 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -15,20 +15,19 @@ SchedulerStateMachine::SchedulerStateMachine(const SchedulerSettings& settings)
     : settings_(settings),
       output_surface_state_(OUTPUT_SURFACE_LOST),
       commit_state_(COMMIT_STATE_IDLE),
+      texture_state_(LAYER_TEXTURE_STATE_UNLOCKED),
+      forced_redraw_state_(FORCED_REDRAW_STATE_IDLE),
+      readback_state_(READBACK_STATE_IDLE),
       commit_count_(0),
       current_frame_number_(0),
       last_frame_number_where_begin_frame_sent_to_main_thread_(-1),
-      last_frame_number_where_draw_was_called_(-1),
+      last_frame_number_swap_performed_(-1),
       last_frame_number_where_update_visible_tiles_was_called_(-1),
       consecutive_failed_draws_(0),
       maximum_number_of_failed_draws_before_draw_is_forced_(3),
       needs_redraw_(false),
       swap_used_incomplete_tile_(false),
-      needs_forced_redraw_(false),
-      needs_forced_redraw_after_next_commit_(false),
       needs_commit_(false),
-      needs_forced_commit_(false),
-      expect_immediate_begin_frame_for_main_thread_(false),
       main_thread_needs_layer_textures_(false),
       inside_begin_frame_(false),
       visible_(false),
@@ -36,9 +35,8 @@ SchedulerStateMachine::SchedulerStateMachine(const SchedulerSettings& settings)
       can_draw_(false),
       has_pending_tree_(false),
       pending_tree_is_ready_for_activation_(false),
-      active_tree_has_been_drawn_(false),
+      active_tree_has_been_drawn_(true),

[danakj, 2013/08/30 15:56:41]

This is a bit magical. Can we do this another way?

[brianderson, 2013/09/03 22:41:12]

This is to avoid aborting a draw before the first commit, which is innocuous but
weird and not expected. I've found various ways to avoid aborting that first
draw in the past, but this solution is always so tempting when I can't think of
something right away. In this patch, I should be able to condition the aborted
draw on did_create_and_initialize_first_output_surface_.

       draw_if_possible_failed_(false),
-      texture_state_(LAYER_TEXTURE_STATE_UNLOCKED),
       did_create_and_initialize_first_output_surface_(false) {}
 
 const char* SchedulerStateMachine::OutputSurfaceStateToString(
@@ -69,8 +67,6 @@ const char* SchedulerStateMachine::CommitStateToString(CommitState state) {
       return "COMMIT_STATE_READY_TO_COMMIT";
     case COMMIT_STATE_WAITING_FOR_FIRST_DRAW:
       return "COMMIT_STATE_WAITING_FOR_FIRST_DRAW";
-    case COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW:
-      return "COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW";
   }
   NOTREACHED();
   return "???";
@@ -89,6 +85,44 @@ const char* SchedulerStateMachine::TextureStateToString(TextureState state) {
   return "???";
 }
 
+const char* SchedulerStateMachine::SynchronousReadbackStateToString(
+    SynchronousReadbackState state) {
+  switch (state) {
+    case READBACK_STATE_IDLE:
+      return "READBACK_STATE_IDLE";
+    case READBACK_STATE_FORCED_COMMIT_REQUESTED:
+      return "READBACK_STATE_FORCED_COMMIT_REQUESTED";
+    case READBACK_STATE_FORCED_COMMIT_PENDING:
+      return "READBACK_STATE_FORCED_COMMIT_PENDING";
+    case READBACK_STATE_WAITING_FOR_ACTIVATION:
+      return "READBACK_STATE_WAITING_FOR_ACTIVATION";
+    case READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK:
+      return "READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK";
+    case READBACK_STATE_REPLACEMENT_COMMIT_PENDING:
+      return "READBACK_STATE_REPLACEMENT_COMMIT_PENDING";
+    case READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING:
+      return "READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING";
+  }
+  NOTREACHED();
+  return "???";
+}
+
+const char* SchedulerStateMachine::ForcedRedrawOnTimeoutStateToString(
+    ForcedRedrawOnTimeoutState state) {
+  switch (state) {
+    case FORCED_REDRAW_STATE_IDLE:
+      return "FORCED_REDRAW_STATE_IDLE";
+    case FORCED_REDRAW_STATE_WAITING_FOR_COMMIT:
+      return "FORCED_REDRAW_STATE_WAITING_FOR_COMMIT";
+    case FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION:
+      return "FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION";
+    case FORCED_REDRAW_STATE_WAITING_FOR_DRAW:
+      return "FORCED_REDRAW_STATE_WAITING_FOR_DRAW";
+  }
+  NOTREACHED();
+  return "???";
+}
+
 const char* SchedulerStateMachine::ActionToString(Action action) {
   switch (action) {
     case ACTION_NONE:
@@ -101,12 +135,14 @@ const char* SchedulerStateMachine::ActionToString(Action action) {
       return "ACTION_UPDATE_VISIBLE_TILES";
     case ACTION_ACTIVATE_PENDING_TREE:
       return "ACTION_ACTIVATE_PENDING_TREE";
-    case ACTION_DRAW_IF_POSSIBLE:
-      return "ACTION_DRAW_IF_POSSIBLE";
-    case ACTION_DRAW_FORCED:
-      return "ACTION_DRAW_FORCED";
+    case ACTION_DRAW_AND_SWAP_IF_POSSIBLE:
+      return "ACTION_DRAW_AND_SWAP_IF_POSSIBLE";
+    case ACTION_DRAW_AND_SWAP_FORCED:
+      return "ACTION_DRAW_AND_SWAP_FORCED";
     case ACTION_DRAW_AND_SWAP_ABORT:
       return "ACTION_DRAW_AND_SWAP_ABORT";
+    case ACTION_DRAW_AND_READBACK:
+      return "ACTION_DRAW_AND_READBACK";
     case ACTION_BEGIN_OUTPUT_SURFACE_CREATION:
       return "ACTION_BEGIN_OUTPUT_SURFACE_CREATION";
     case ACTION_ACQUIRE_LAYER_TEXTURES_FOR_MAIN_THREAD:
@@ -126,6 +162,11 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
                          TextureStateToString(texture_state_));
   major_state->SetString("output_surface_state_",
                          OutputSurfaceStateToString(output_surface_state_));
+  major_state->SetString(
+      "forced_redraw_state",
+      ForcedRedrawOnTimeoutStateToString(forced_redraw_state_));
+  major_state->SetString("readback_state",
+                         SynchronousReadbackStateToString(readback_state_));
   state->Set("major_state", major_state.release());
 
   scoped_ptr<base::DictionaryValue> timestamps_state(new base::DictionaryValue);
@@ -137,7 +178,7 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
       (last_begin_frame_args_.deadline - now).InMicroseconds() / 1000.0L);
   timestamps_state->SetDouble(
       "2_frame_time_to_now",
-      (last_begin_frame_args_.deadline - now).InMicroseconds() / 1000.0L);
+      (now - last_begin_frame_args_.frame_time).InMicroseconds() / 1000.0L);
   timestamps_state->SetDouble(
       "3_frame_time_to_deadline",
       (last_begin_frame_args_.deadline - last_begin_frame_args_.frame_time)
@@ -162,7 +203,7 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
       "last_frame_number_where_begin_frame_sent_to_main_thread",
       last_frame_number_where_begin_frame_sent_to_main_thread_);
   minor_state->SetInteger("last_frame_number_where_draw_was_called",

[danakj, 2013/08/30 15:56:41]

change this too?

[brianderson, 2013/09/03 22:41:12]

OK.

-                          last_frame_number_where_draw_was_called_);
+                          last_frame_number_swap_performed_);
   minor_state->SetInteger(
       "last_frame_number_where_update_visible_tiles_was_called",
       last_frame_number_where_update_visible_tiles_was_called_);
@@ -174,13 +215,7 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
   minor_state->SetBoolean("needs_redraw", needs_redraw_);
   minor_state->SetBoolean("swap_used_incomplete_tile",
                           swap_used_incomplete_tile_);
-  minor_state->SetBoolean("needs_forced_redraw", needs_forced_redraw_);
-  minor_state->SetBoolean("needs_forced_redraw_after_next_commit",
-                          needs_forced_redraw_after_next_commit_);
   minor_state->SetBoolean("needs_commit", needs_commit_);
-  minor_state->SetBoolean("needs_forced_commit", needs_forced_commit_);
-  minor_state->SetBoolean("expect_immediate_begin_frame_for_main_thread",
-                          expect_immediate_begin_frame_for_main_thread_);
   minor_state->SetBoolean("main_thread_needs_layer_textures",
                           main_thread_needs_layer_textures_);
   minor_state->SetBoolean("inside_begin_frame", inside_begin_frame_);
@@ -201,7 +236,7 @@ scoped_ptr<base::Value> SchedulerStateMachine::AsValue() const  {
 }
 
 bool SchedulerStateMachine::HasDrawnThisFrame() const {

[danakj, 2013/08/30 15:56:41]

HasDrawnAndSwappedThisFrame?

[brianderson, 2013/09/03 22:41:12]

Ok. That would match the surrounding code better.

-  return current_frame_number_ == last_frame_number_where_draw_was_called_;
+  return current_frame_number_ == last_frame_number_swap_performed_;
 }
 
 bool SchedulerStateMachine::HasUpdatedVisibleTilesThisFrame() const {
@@ -231,8 +266,6 @@ bool SchedulerStateMachine::PendingDrawsShouldBeAborted() const {
     return true;
   if (!visible_)
     return true;
-  if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
-    return true;
   return false;
 }
 
@@ -241,10 +274,11 @@ bool SchedulerStateMachine::PendingActivationsShouldBeForced() const {
   // forward progress, we might deadlock with the main thread.
   if (texture_state_ == LAYER_TEXTURE_STATE_ACQUIRED_BY_MAIN_THREAD)
     return true;
-  if (output_surface_state_ == OUTPUT_SURFACE_LOST ||
-      output_surface_state_ == OUTPUT_SURFACE_CREATING ||
-      output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT)
+
+  // There is no output surface to trigger our activations.
+  if (output_surface_state_ == OUTPUT_SURFACE_LOST)
     return true;
+
   return false;
 }
 
@@ -258,28 +292,31 @@ bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const {
   if (commit_state_ != COMMIT_STATE_IDLE)
     return false;
 
+  // We want to clear the pipline of any pending draws and activations
+  // before starting output surface initialization. This allows us to avoid
+  // weird corner cases where we abort draws or force activation while we
+  // are initializing the output surface and can potentially have a pending
+  // readback.
+  if (!active_tree_has_been_drawn_ || has_pending_tree_)

[danakj, 2013/08/30 15:56:41]

what if the active tree is invalid and can't be drawn?

[brianderson, 2013/09/03 22:41:12]

The draw will be aborted and will set active_tree_has_been_drawn_ to true, which
will unblock activations, which will allow has_pending_tree_ to also go false.

+    return false;
+
   // We need to create the output surface if we don't have one and we haven't
   // started creating one yet.
   return output_surface_state_ == OUTPUT_SURFACE_LOST;
 }
 
 bool SchedulerStateMachine::ShouldDraw() const {
-  // We should not draw or abort draws while we are waiting for the
-  // first activation. Doing so will cause us to transition to
-  // COMMIT_STATE_IDLE and start the 2nd commit before the 1st commit
-  // has been drawn. It will also cause us to fail readbacks when we could
-  // have succeeded by waiting a little longer.
-  if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
+  // After a readback, make sure not to draw again until we've replaced the
+  // readback commit with a real one.
+  if (readback_state_ == READBACK_STATE_REPLACEMENT_COMMIT_PENDING ||
+      readback_state_ == READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING)
     return false;
 
-  // When we are waiting for a forced draw, only force draw once
-  // needs_forced_redraw_ is true.
-  if (commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW)
-    return needs_forced_redraw_;
-
-  // Always handle forced draws ASAP.
-  if (needs_forced_redraw_)
+  // Draw immediately for readbacks to unblock the main thread quickly.
+  if (readback_state_ == READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK) {
+    DCHECK_EQ(commit_state_, COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
     return true;
+  }
 
   // If we need to abort draws, we should do so ASAP since the draw could
   // be blocking other important actions (like output surface initialization),
@@ -287,7 +324,7 @@ bool SchedulerStateMachine::ShouldDraw() const {
   // aborted draw to keep things moving. If we are not waiting for the first
   // draw however, we don't want to abort for no reason.
   if (PendingDrawsShouldBeAborted())
-    return commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
+    return !active_tree_has_been_drawn_;
 
   // After this line, we only want to draw once per frame.
   if (HasDrawnThisFrame())
@@ -297,6 +334,12 @@ bool SchedulerStateMachine::ShouldDraw() const {
   if (!inside_begin_frame_)
     return false;
 
+  // Only handle forced redraws due to timeouts on the regular deadline.
+  if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW) {
+    DCHECK_EQ(commit_state_, COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
+    return true;
+  }
+
   return needs_redraw_;
 }
 
@@ -314,12 +357,12 @@ bool SchedulerStateMachine::ShouldActivatePendingTree() const {
   if (!has_pending_tree_)
     return false;
 
+  // If this is our first activation, we don't want to activate early.

[danakj, 2013/08/30 15:56:41]

We now have two conditions that trump "Forcing" an activate. Can you explain in
a comment for each why it's ok to ignore the Force state? Maybe some words like
"can't" or "must"?

[brianderson, 2013/09/03 22:41:12]

Will do.

+  if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
+    return pending_tree_is_ready_for_activation_;
+
   // We don't want to activate a second tree before drawing the first one.
-  // Note: It is possible that there is no active tree to draw when
-  // output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION,
-  // so we don't block activation on draw in that case.
-  if (!active_tree_has_been_drawn_ &&
-      output_surface_state_ != OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
+  if (!active_tree_has_been_drawn_)
     return false;
 
   // If we want to force activation, do so ASAP.
@@ -366,9 +409,12 @@ bool SchedulerStateMachine::ShouldSendBeginFrameToMainThread() const {
   if (has_pending_tree_)
     return false;
 
-  // We want to start forced commits ASAP.
-  if (needs_forced_commit_)
-    return true;
+  // We want to handle readback commits immediately to unblock the main thread.
+  // Note: This BeginFrame will correspond to the replacement commit that comes
+  // after the readback commit itself, so we only send the BeginFrame if a
+  // commit isn't already pending behind the readback.
+  if (readback_state_ == READBACK_STATE_FORCED_COMMIT_REQUESTED)
+    return !CommitPending();
 
   // We do not need commits if we are not visible, unless there's a
   // request for a forced commit.
@@ -379,6 +425,11 @@ bool SchedulerStateMachine::ShouldSendBeginFrameToMainThread() const {
   if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT)
     return true;
 
+  // We need a new commit for the forced redraw. This honors the
+  // single commit per interval because the result will be swapped to screen.
+  if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT)
+    return true;
+
   // After this point, we only start a commit once per frame.
   if (HasSentBeginFrameToMainThreadThisFrame())
     return false;
@@ -404,12 +455,14 @@ SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
   if (ShouldCommit())
     return ACTION_COMMIT;
   if (ShouldDraw()) {
-    if (needs_forced_redraw_)
-      return ACTION_DRAW_FORCED;
+    if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)
+      return ACTION_DRAW_AND_SWAP_FORCED;
+    else if (readback_state_ == READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK)

[danakj, 2013/08/30 15:56:41]

Seems like this one should come first, we don't want to swap to the screen
during a readback until the replacement commit? If you agree, can you add a
test?

Oh, but I see your CheckInvariants below. Can you explain that?

[brianderson, 2013/09/03 22:41:12]

I will go through the code and make sure that the readback logic is always
handled first, unless it absolutely cannot be, since it generally takes
precedence over everything else anyway.

In this case, the order does not matter because of the reason you alluded to in
CheckInvariants(). If we are in READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK, we
cannot be in FORCED_REDRAW_STATE_WAITING_FOR_DRAW because the readback commit
must never be swapped to screen. This is enforced in UpdateStateOnCommit by only
advancing the other states that might cause a swap when the readback state is
idle.

+      return ACTION_DRAW_AND_READBACK;
     else if (PendingDrawsShouldBeAborted())
       return ACTION_DRAW_AND_SWAP_ABORT;
     else
-      return ACTION_DRAW_IF_POSSIBLE;
+      return ACTION_DRAW_AND_SWAP_IF_POSSIBLE;
   }
   if (ShouldSendBeginFrameToMainThread())
     return ACTION_SEND_BEGIN_FRAME_TO_MAIN_THREAD;
@@ -418,6 +471,13 @@ SchedulerStateMachine::Action SchedulerStateMachine::NextAction() const {
   return ACTION_NONE;
 }
 
+void SchedulerStateMachine::CheckInvariants() {
+  // We should never try to perform a draw for readback and forced draw due to
+  // timeout simultaneously.
+  DCHECK(!(forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW &&
+           readback_state_ == READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK));
+}
+
 void SchedulerStateMachine::UpdateState(Action action) {
   switch (action) {
     case ACTION_NONE:
@@ -434,15 +494,14 @@ void SchedulerStateMachine::UpdateState(Action action) {
 
     case ACTION_SEND_BEGIN_FRAME_TO_MAIN_THREAD:
       DCHECK(!has_pending_tree_);
-      if (!needs_forced_commit_ &&
-          output_surface_state_ != OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT) {
-        DCHECK(visible_);
-        DCHECK_GT(current_frame_number_,
-                  last_frame_number_where_begin_frame_sent_to_main_thread_);
-      }
+      DCHECK(visible_ ||
+             readback_state_ == READBACK_STATE_FORCED_COMMIT_REQUESTED ||
+             forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT ||

[danakj, 2013/08/30 15:56:41]

I don't think we commit for forced redraws when not visible, do we?

[brianderson, 2013/09/03 22:41:12]

Good catch. The main thread would just abort the commit anway. This version of
the patch shouldn't commit in these cases anymore, so I will remove them from
the DCHECK.

+             output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT);

[danakj, 2013/08/30 15:56:41]

Same here?

       commit_state_ = COMMIT_STATE_FRAME_IN_PROGRESS;
       needs_commit_ = false;
-      needs_forced_commit_ = false;
+      if (readback_state_ == READBACK_STATE_FORCED_COMMIT_REQUESTED)
+        readback_state_ = READBACK_STATE_FORCED_COMMIT_PENDING;
       last_frame_number_where_begin_frame_sent_to_main_thread_ =
           current_frame_number_;
       return;
@@ -453,14 +512,15 @@ void SchedulerStateMachine::UpdateState(Action action) {
       return;
     }
 
-    case ACTION_DRAW_FORCED:
-    case ACTION_DRAW_IF_POSSIBLE: {
+    case ACTION_DRAW_AND_SWAP_FORCED:
+    case ACTION_DRAW_AND_SWAP_IF_POSSIBLE: {
       bool did_swap = true;
       UpdateStateOnDraw(did_swap);
       return;
     }
 
-    case ACTION_DRAW_AND_SWAP_ABORT: {
+    case ACTION_DRAW_AND_SWAP_ABORT:
+    case ACTION_DRAW_AND_READBACK: {
       bool did_swap = false;
       UpdateStateOnDraw(did_swap);
       return;
@@ -469,6 +529,8 @@ void SchedulerStateMachine::UpdateState(Action action) {
     case ACTION_BEGIN_OUTPUT_SURFACE_CREATION:
       DCHECK_EQ(commit_state_, COMMIT_STATE_IDLE);
       DCHECK_EQ(output_surface_state_, OUTPUT_SURFACE_LOST);
+      DCHECK(!has_pending_tree_);
+      DCHECK(active_tree_has_been_drawn_);

[danakj, 2013/08/30 15:56:41]

Can you explain?

[brianderson, 2013/09/03 22:41:12]

I added logic to flush the pipeline before starting output surface
initialization to avoid really complicated corner cases where we want to do all
of the following at the same time:
1) Abort a draw.
2) Force an activation.
3) Initialize the output surface.
4) Perform a readback.

These DCHECKs verify that the pipeline was actually flushed and that we only
have to worry about doing 3 and 4 at the same time.

       output_surface_state_ = OUTPUT_SURFACE_CREATING;
       return;
 
@@ -487,30 +549,62 @@ void SchedulerStateMachine::UpdateStateOnCommit(bool commit_was_aborted) {
   has_pending_tree_ =
       settings_.impl_side_painting && !commit_was_aborted;
 
-  // Update the commit state.
-  if (expect_immediate_begin_frame_for_main_thread_)
-    commit_state_ = COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW;
-  else if (!commit_was_aborted)
-    commit_state_ = COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
-  else
-    commit_state_ = COMMIT_STATE_IDLE;
+  // Update state related to readbacks.
+  if (readback_state_ == READBACK_STATE_FORCED_COMMIT_PENDING) {
+    // Update the state if this is the readback commit.
+    readback_state_ = has_pending_tree_
+                          ? READBACK_STATE_WAITING_FOR_ACTIVATION
+                          : READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK;
+  } else if (readback_state_ == READBACK_STATE_REPLACEMENT_COMMIT_PENDING) {
+    // Update the state if this is the commit replacing the readback commit.
+    readback_state_ = has_pending_tree_
+                          ? READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
+                          : READBACK_STATE_IDLE;
+  } else {
+    DCHECK(readback_state_ == READBACK_STATE_IDLE);
+  }
+
+  // Readbacks can interrupt output surface initialization and forced draws,
+  // so we do not want to advance those states unless readback is idle.
+  // Note: It is possible for the readback's replacement commit to be the
+  // output surface's first commit and/or the forced redraw's commit.
+  if (readback_state_ == READBACK_STATE_IDLE) {

[danakj, 2013/08/30 15:56:41]

What if the readback state is READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING then?
The comment seems to call out explicitly that we should advance then?

[brianderson, 2013/09/03 22:41:12]

Good catch again. I should include READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING
in this if statement as well.

[danakj, 2013/09/03 22:45:01]

Can you add a test for this and any other mistakes we uncover that weren't
covered by a test?

+    // Update state related to forced draws.
+    if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT) {
+      forced_redraw_state_ = has_pending_tree_
+                                 ? FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION
+                                 : FORCED_REDRAW_STATE_WAITING_FOR_DRAW;
+    }
 
-  // Update the output surface state.
-  DCHECK_NE(output_surface_state_,
-            OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION);
-  if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT) {
-    if (has_pending_tree_) {
-      output_surface_state_ = OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION;
-    } else {
-      output_surface_state_ = OUTPUT_SURFACE_ACTIVE;
-      needs_redraw_ = true;
+    // Update the output surface state.
+    DCHECK_NE(output_surface_state_,
+              OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION);
+    if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT) {
+      if (has_pending_tree_) {
+        output_surface_state_ = OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION;
+      } else {
+        output_surface_state_ = OUTPUT_SURFACE_ACTIVE;
+        needs_redraw_ = true;
+      }
     }
   }
 
+  // Update the commit state. We expect and wait for a draw if the commit
+  // was not aborted or if we in a readback or forced draw.

[danakj, 2013/08/30 15:56:41]

nit: "if we in a"

[brianderson, 2013/09/03 22:41:12]

Done.

+  if (!commit_was_aborted)
+    commit_state_ = COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
+  else if (readback_state_ != READBACK_STATE_IDLE ||
+           forced_redraw_state_ != FORCED_REDRAW_STATE_IDLE)
+    commit_state_ = COMMIT_STATE_WAITING_FOR_FIRST_DRAW;
+  else
+    commit_state_ = COMMIT_STATE_IDLE;
+
   // Update state if we have a new active tree to draw, or if the active tree
-  // was unchanged but we need to do a readback.
+  // was unchanged but we need to do a readback or forced draw.
   if (!has_pending_tree_ &&
-      (!commit_was_aborted || expect_immediate_begin_frame_for_main_thread_)) {
+      (!commit_was_aborted ||
+       readback_state_ == READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK ||
+       forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)) {
     needs_redraw_ = true;
     active_tree_has_been_drawn_ = false;
   }
@@ -519,25 +613,31 @@ void SchedulerStateMachine::UpdateStateOnCommit(bool commit_was_aborted) {
   pending_tree_is_ready_for_activation_ = false;
 
   if (draw_if_possible_failed_)
-    last_frame_number_where_draw_was_called_ = -1;
-
-  if (needs_forced_redraw_after_next_commit_) {
-    needs_forced_redraw_after_next_commit_ = false;
-    needs_forced_redraw_ = true;
-  }
+    last_frame_number_swap_performed_ = -1;
 
   // If we are planing to draw with the new commit, lock the layer textures for
   // use on the impl thread. Otherwise, leave them unlocked.
-  if (has_pending_tree_ || needs_redraw_ || needs_forced_redraw_)
+  if (has_pending_tree_ || needs_redraw_)
     texture_state_ = LAYER_TEXTURE_STATE_ACQUIRED_BY_IMPL_THREAD;
   else
     texture_state_ = LAYER_TEXTURE_STATE_UNLOCKED;
 }
 
 void SchedulerStateMachine::UpdateStateOnActivation() {
+  // Update output surface state.
   if (output_surface_state_ == OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION)
     output_surface_state_ = OUTPUT_SURFACE_ACTIVE;
 
+  // Update readback state
+  if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION)
+    forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_DRAW;
+
+  // Update forced redraw state
+  if (readback_state_ == READBACK_STATE_WAITING_FOR_ACTIVATION)
+    readback_state_ = READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK;
+  else if (readback_state_ == READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING)
+    readback_state_ = READBACK_STATE_IDLE;
+
   has_pending_tree_ = false;
   pending_tree_is_ready_for_activation_ = false;
   active_tree_has_been_drawn_ = false;
@@ -545,25 +645,40 @@ void SchedulerStateMachine::UpdateStateOnActivation() {
 }
 
 void SchedulerStateMachine::UpdateStateOnDraw(bool did_swap) {
-  if (inside_begin_frame_)
-    last_frame_number_where_draw_was_called_ = current_frame_number_;
-  if (commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_FORCED_DRAW) {
-    DCHECK(expect_immediate_begin_frame_for_main_thread_);
+  DCHECK(readback_state_ != READBACK_STATE_REPLACEMENT_COMMIT_PENDING &&
+         readback_state_ != READBACK_STATE_REPLACEMENT_COMMIT_ACTIVATING)
+      << *AsValue();
+
+  if (readback_state_ == READBACK_STATE_WAITING_FOR_DRAW_AND_READBACK) {
+    // The draw correspons to a readback commit.
+    DCHECK_EQ(commit_state_, COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
+    // We are blocking commits from the main thread until after this draw, so
+    // we should not have a pending tree.
+    DCHECK(!has_pending_tree_);
+    // We transition to COMMIT_STATE_FRAME_IN_PROGRESS because there is a
+    // pending BeginFrame on the main thread behind the readback request.
     commit_state_ = COMMIT_STATE_FRAME_IN_PROGRESS;
-    expect_immediate_begin_frame_for_main_thread_ = false;
-  } else if (commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_DRAW) {
+    readback_state_ = READBACK_STATE_REPLACEMENT_COMMIT_PENDING;
+  } else if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW) {
+    DCHECK_EQ(commit_state_, COMMIT_STATE_WAITING_FOR_FIRST_DRAW);
+    commit_state_ = COMMIT_STATE_IDLE;
+    forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
+  } else if (commit_state_ == COMMIT_STATE_WAITING_FOR_FIRST_DRAW &&
+             !has_pending_tree_) {
     commit_state_ = COMMIT_STATE_IDLE;
   }
+
   if (texture_state_ == LAYER_TEXTURE_STATE_ACQUIRED_BY_IMPL_THREAD)
     texture_state_ = LAYER_TEXTURE_STATE_UNLOCKED;
 
   needs_redraw_ = false;
-  needs_forced_redraw_ = false;
   draw_if_possible_failed_ = false;
   active_tree_has_been_drawn_ = true;

[danakj, 2013/08/30 15:56:41]

This bool seems to now be shadowed by 3 enum states: all the waiting for draw
states. Do we need it?

[brianderson, 2013/09/03 22:41:12]

This probably could be removed at ToT, but the upcoming patches to start the
next commit before the previous commit has been drawn will de-shadow the commit
state from active_tree_has_been_drawn_. Do you mind if I keep the variable to
avoid churn?

 
-  if (did_swap)
+  if (did_swap) {
     swap_used_incomplete_tile_ = false;
+    last_frame_number_swap_performed_ = current_frame_number_;
+  }
 }
 
 void SchedulerStateMachine::SetMainThreadNeedsLayerTextures() {
@@ -572,20 +687,43 @@ void SchedulerStateMachine::SetMainThreadNeedsLayerTextures() {
   main_thread_needs_layer_textures_ = true;
 }
 
+// These are the cases where we definitely (or almost definitely) have a
+// new frame to draw and can draw.
 bool SchedulerStateMachine::BeginFrameNeededToDrawByImplThread() const {
+  // The output surface is the provider of BeginFrames for the impl thread,
+  // so we are not going to get them even if we ask for them.
+  if (!HasInitializedOutputSurface())
+    return false;
+
   // If we can't draw, don't tick until we are notified that we can draw again.
   if (!can_draw_)
     return false;
 
-  if (needs_forced_redraw_)
+  // The forced draw respects our normal draw scheduling, so we need to
+  // request a BeginFrame on the impl thread for it.
+  if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)
     return true;
 
-  if (visible_ && swap_used_incomplete_tile_)
+  // There's no need to produce frames if we are not visible.
+  if (!visible_)
+    return false;
+
+  // We need to draw a more complete frame than we did the last BeginFrame,
+  // so request another BeginFrame in anticipation that we will have
+  // additional visible tiles.
+  if (swap_used_incomplete_tile_)
     return true;
 
-  return needs_redraw_ && visible_ && HasInitializedOutputSurface();
+  return needs_redraw_;
 }
 
+// These are cases where we are very likely to draw soon, but might not
+// actually have a new frame to draw when we receive the next BeginFrame.
+// Proactively requesting the BeginFrame helps hide the round trip latency of
+// the SetNeedsBeginFrame request that has to go to the Browser.
+// However, this is bad for the synchronous compoisitor because we have to
+// draw when we get the BeginFrame and could end up drawing many duplicate
+// frames.
 bool SchedulerStateMachine::ProactiveBeginFrameWantedByImplThread() const {
   // The output surface is the provider of BeginFrames for the impl thread,
   // so we are not going to get them even if we ask for them.
@@ -596,10 +734,17 @@ bool SchedulerStateMachine::ProactiveBeginFrameWantedByImplThread() const {
   if (!visible_)
     return false;
 
-  // We should proactively request a BeginFrame if a commit or a tree activation
-  // is pending.
-  return (needs_commit_ || needs_forced_commit_ ||
-          commit_state_ != COMMIT_STATE_IDLE || has_pending_tree_);
+  // We should proactively request a BeginFrame if a commit is pending
+  // because we will want to draw if the commit completes quickly.
+  if (needs_commit_ || commit_state_ != COMMIT_STATE_IDLE)
+    return true;
+
+  // If the pending tree activates quickly, we'll want a BeginFrame soon
+  // to draw the new active tree.
+  if (has_pending_tree_)
+    return true;
+
+  return false;
 }
 
 void SchedulerStateMachine::DidEnterBeginFrame(const BeginFrameArgs& args) {
@@ -620,10 +765,6 @@ void SchedulerStateMachine::DidSwapUseIncompleteTile() {
   swap_used_incomplete_tile_ = true;
 }
 
-void SchedulerStateMachine::SetNeedsForcedRedraw() {
-  needs_forced_redraw_ = true;
-}
-
 void SchedulerStateMachine::DidDrawIfPossibleCompleted(bool success) {
   draw_if_possible_failed_ = !success;
   if (draw_if_possible_failed_) {
@@ -636,7 +777,7 @@ void SchedulerStateMachine::DidDrawIfPossibleCompleted(bool success) {
       consecutive_failed_draws_ = 0;
       // We need to force a draw, but it doesn't make sense to do this until
       // we've committed and have new textures.
-      needs_forced_redraw_after_next_commit_ = true;
+      forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
     }
   } else {
     consecutive_failed_draws_ = 0;
@@ -645,16 +786,29 @@ void SchedulerStateMachine::DidDrawIfPossibleCompleted(bool success) {
 
 void SchedulerStateMachine::SetNeedsCommit() { needs_commit_ = true; }
 
-void SchedulerStateMachine::SetNeedsForcedCommit() {
-  needs_forced_commit_ = true;
-  expect_immediate_begin_frame_for_main_thread_ = true;
+void SchedulerStateMachine::SetNeedsForcedCommitForReadback() {
+  // If this is called in READBACK_STATE_IDLE, this is a "first" readback
+  // request.
+  // If this is called in READBACK_STATE_REPLACEMENT_COMMIT_PENDING, this
+  // is a back-to-back readback request that started before the replacement
+  // commit had a chance to land.
+  DCHECK(readback_state_ == READBACK_STATE_IDLE ||
+         readback_state_ == READBACK_STATE_REPLACEMENT_COMMIT_PENDING);
+
+  // If there is already a commit in progress when we get the readback request
+  // (we are in COMMIT_STATE_FRAME_IN_PROGRESS), then we don't need to send a
+  // BeginFrame for the replacement commit, since there's already a BeginFrame
+  // behind the readback request. In that case, we can skip
+  // READBACK_STATE_FORCED_COMMIT_REQUESTED and go directly to
+  // READBACK_STATE_FORCED_COMMIT_PENDING
+  if (commit_state_ == COMMIT_STATE_FRAME_IN_PROGRESS)
+    readback_state_ = READBACK_STATE_FORCED_COMMIT_PENDING;
+  else
+    readback_state_ = READBACK_STATE_FORCED_COMMIT_REQUESTED;
 }
 
 void SchedulerStateMachine::FinishCommit() {
-  DCHECK(commit_state_ == COMMIT_STATE_FRAME_IN_PROGRESS ||
-         (expect_immediate_begin_frame_for_main_thread_ &&
-          commit_state_ != COMMIT_STATE_IDLE))
-      << *AsValue();
+  DCHECK(commit_state_ == COMMIT_STATE_FRAME_IN_PROGRESS) << *AsValue();
   commit_state_ = COMMIT_STATE_READY_TO_COMMIT;
 }
 
@@ -663,8 +817,8 @@ void SchedulerStateMachine::BeginFrameAbortedByMainThread(bool did_handle) {
   if (did_handle) {
     bool commit_was_aborted = true;
     UpdateStateOnCommit(commit_was_aborted);
-  } else if (expect_immediate_begin_frame_for_main_thread_) {
-    expect_immediate_begin_frame_for_main_thread_ = false;
+  } else if (readback_state_ == READBACK_STATE_FORCED_COMMIT_PENDING) {
+    readback_state_ = READBACK_STATE_IDLE;
   } else {
     commit_state_ = COMMIT_STATE_IDLE;
     SetNeedsCommit();