cc: Add SchedulerStateMachine::DidDraw and use for forced draws

cc/scheduler/scheduler_state_machine.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_state_machine.h"
 
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 231 matching lines @@
   state->SetBoolean("main_thread_missed_last_deadline",
                     main_thread_missed_last_deadline_);
   state->SetBoolean("skip_next_begin_main_frame_to_reduce_latency",
                     skip_next_begin_main_frame_to_reduce_latency_);
   state->SetBoolean("children_need_begin_frames", children_need_begin_frames_);
   state->SetBoolean("video_needs_begin_frames", video_needs_begin_frames_);
   state->SetBoolean("defer_commits", defer_commits_);
   state->SetBoolean("last_commit_had_no_updates", last_commit_had_no_updates_);
   state->SetBoolean("did_request_swap_in_last_frame",
                     did_request_swap_in_last_frame_);
   state->SetBoolean("did_perform_swap_in_last_draw",

[sunnyps, 2015/11/24 22:42:44]

nit: rename did_perform_swap_in_last_draw to did_swap_in_last_draw

[brianderson, 2015/11/24 23:19:38]

Renamed to did_swap_in_last_frame to be consistent with the
did_draw_in_last_frame naming.

                     did_perform_swap_in_last_draw_);
   state->EndDictionary();
 }
 
 bool SchedulerStateMachine::PendingDrawsShouldBeAborted() const {
   // Normally when |visible_| is false, pending activations will be forced and
   // draws will be aborted. However, when the embedder is Android WebView,
   // software draws could be scheduled by the Android OS at any time and draws
   // should not be aborted in this case.
   bool is_output_surface_lost = (output_surface_state_ == OUTPUT_SURFACE_NONE);
@@ skipping 378 matching lines @@
 
   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_ACTIVATION)
     forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_DRAW;
 
   has_pending_tree_ = false;
   pending_tree_is_ready_for_activation_ = false;
   active_tree_needs_first_draw_ = true;
   needs_redraw_ = true;
 }
 
-void SchedulerStateMachine::WillDraw(bool did_request_swap) {
+void SchedulerStateMachine::WillDraw() {
+  // We need to reset needs_redraw_ before we draw since the
+  // draw itself might request another draw.
+  needs_redraw_ = false;
+}
+
+void SchedulerStateMachine::DidDraw(bool did_request_swap,
+                                    DrawResult draw_result) {
+  active_tree_needs_first_draw_ = false;
+
   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)
     forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
 
   if (begin_main_frame_state_ == BEGIN_MAIN_FRAME_STATE_WAITING_FOR_DRAW)
     begin_main_frame_state_ = BEGIN_MAIN_FRAME_STATE_IDLE;
 
-  needs_redraw_ = false;
-  active_tree_needs_first_draw_ = false;
-
   if (did_request_swap) {

[sunnyps, 2015/11/20 23:34:07]

Doesn't draw_result != DRAW_SUCCESS imply that did_request_swap is false and
that it's true otherwise?

[brianderson, 2015/11/21 00:31:33]

It's a bit confusing, and probably deserves some cleaning up:

There's "did request swap", which is true whether or not DrawAndSwap returns
early because of no damage, and "did swap", which is only true when there is
damage and we swap.

Also, we get DRAW_SUCCESS even if the draw returns early because of no damage:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Let me see what I can do to make it less confusing.

     DCHECK(!request_swap_funnel_);
     request_swap_funnel_ = true;
     did_request_swap_in_last_frame_ = true;
     last_frame_number_swap_requested_ = current_frame_number_;
   }
+
+  switch (draw_result) {
+    case INVALID_RESULT:
+      NOTREACHED() << "Uninitialized DrawResult.";
+      break;
+    case DRAW_ABORTED_CANT_DRAW:
+    case DRAW_ABORTED_CONTEXT_LOST:
+      NOTREACHED() << "Invalid return value from DrawAndSwapIfPossible:"
+                   << draw_result;
+      break;
+    case DRAW_SUCCESS:
+      consecutive_checkerboard_animations_ = 0;
+      forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
+      break;
+    case DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
+      needs_redraw_ = true;
+
+      // If we're already in the middle of a redraw, we don't need to
+      // restart it.
+      if (forced_redraw_state_ != FORCED_REDRAW_STATE_IDLE)
+        return;
+
+      needs_begin_main_frame_ = true;
+      consecutive_checkerboard_animations_++;

[sunnyps, 2015/11/20 23:34:07]

nit: needs_begin_main_frame should be inside the if (settings_...) block and
consecutive_checkerboard_animations_ should be incremented above the if
(forced_redraw_state_...) block. See the next comment.

[brianderson, 2015/11/21 00:31:33]

Sg.

+      if (settings_.timeout_and_draw_when_animation_checkerboards &&
+          consecutive_checkerboard_animations_ >=
+              settings_.maximum_number_of_failed_draws_before_draw_is_forced) {
+        consecutive_checkerboard_animations_ = 0;

[sunnyps, 2015/11/20 23:34:07]

nit: We shouldn't have to set consecutive_checkerboard_animations_ here. Setting
forced_redraw_state_ is enough. Having the correct value for
consecutive_checkerboard_animations_ might be useful for UMA.

[brianderson, 2015/11/21 00:31:33]

Also sg.

+        // We need to force a draw, but it doesn't make sense to do this until
+        // we've committed and have new textures.
+        forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
+      }
+      break;
+    case DRAW_ABORTED_MISSING_HIGH_RES_CONTENT:
+      // It's not clear whether this missing content is because of missing
+      // pictures (which requires a commit) or because of memory pressure
+      // removing textures (which might not).  To be safe, request a commit
+      // anyway.
+      needs_begin_main_frame_ = true;
+      break;
+  }
 }
 
 void SchedulerStateMachine::WillPrepareTiles() {
   needs_prepare_tiles_ = false;
 }
 
 void SchedulerStateMachine::WillBeginOutputSurfaceCreation() {
   DCHECK_EQ(output_surface_state_, OUTPUT_SURFACE_NONE);
   output_surface_state_ = OUTPUT_SURFACE_CREATING;
 
@@ skipping 299 matching lines @@
 void SchedulerStateMachine::DidSwapBuffersComplete() {
   TRACE_EVENT_ASYNC_END0("cc", "Scheduler:pending_swaps", this);
   pending_swaps_--;
 }
 
 void SchedulerStateMachine::SetImplLatencyTakesPriority(
     bool impl_latency_takes_priority) {
   impl_latency_takes_priority_ = impl_latency_takes_priority;
 }
 
-void SchedulerStateMachine::DidDrawIfPossibleCompleted(DrawResult result) {
-  switch (result) {
-    case INVALID_RESULT:
-      NOTREACHED() << "Uninitialized DrawResult.";
-      break;
-    case DRAW_ABORTED_CANT_DRAW:
-    case DRAW_ABORTED_CONTEXT_LOST:
-      NOTREACHED() << "Invalid return value from DrawAndSwapIfPossible:"
-                   << result;
-      break;
-    case DRAW_SUCCESS:
-      consecutive_checkerboard_animations_ = 0;
-      forced_redraw_state_ = FORCED_REDRAW_STATE_IDLE;
-      break;
-    case DRAW_ABORTED_CHECKERBOARD_ANIMATIONS:
-      needs_redraw_ = true;
-
-      // If we're already in the middle of a redraw, we don't need to
-      // restart it.
-      if (forced_redraw_state_ != FORCED_REDRAW_STATE_IDLE)
-        return;
-
-      needs_begin_main_frame_ = true;
-      consecutive_checkerboard_animations_++;
-      if (settings_.timeout_and_draw_when_animation_checkerboards &&
-          consecutive_checkerboard_animations_ >=
-              settings_.maximum_number_of_failed_draws_before_draw_is_forced) {
-        consecutive_checkerboard_animations_ = 0;
-        // We need to force a draw, but it doesn't make sense to do this until
-        // we've committed and have new textures.
-        forced_redraw_state_ = FORCED_REDRAW_STATE_WAITING_FOR_COMMIT;
-      }
-      break;
-    case DRAW_ABORTED_MISSING_HIGH_RES_CONTENT:
-      // It's not clear whether this missing content is because of missing
-      // pictures (which requires a commit) or because of memory pressure
-      // removing textures (which might not).  To be safe, request a commit
-      // anyway.
-      needs_begin_main_frame_ = true;
-      break;
-  }
-}
-
 void SchedulerStateMachine::SetNeedsBeginMainFrame() {
   needs_begin_main_frame_ = true;
 }
 
 void SchedulerStateMachine::SetNeedsOneBeginImplFrame() {
   needs_one_begin_impl_frame_ = true;
 }
 
 void SchedulerStateMachine::NotifyReadyToCommit() {
   DCHECK_EQ(begin_main_frame_state_, BEGIN_MAIN_FRAME_STATE_STARTED)
@@ skipping 75 matching lines @@
     case OUTPUT_SURFACE_ACTIVE:
     case OUTPUT_SURFACE_WAITING_FOR_FIRST_COMMIT:
     case OUTPUT_SURFACE_WAITING_FOR_FIRST_ACTIVATION:
       return true;
   }
   NOTREACHED();
   return false;
 }
 
 }  // namespace cc