cc: Always use SetNeedsBeginFrame to request the next BeginFrame

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/values.h"
@@ skipping 722 matching lines @@
   // 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;
 
+  // If we just swapped, it's likely that we are going to produce another
+  // frame soon.
+  if (last_frame_number_swap_performed_ == current_frame_number_)

[enne (OOO), 2013/09/13 17:48:44]

Just as a sanity check, the bugfix here is that although the state machine is
swapping, maybe some output surface implementation doesn't swap, and thus the
next begin frame isn't unblocked.  By moving the check down a level (or two) to
the state machine, it knows when swaps have really happened and can request the
next begin frame without depending on the output surface to also unblock.

[brianderson, 2013/09/13 18:24:46]

Your understand is correct. The Scheduler can accept the next BeginFrame after a
swap request whether or not the swap actually occurred. ToT has the Scheduler
explicitly request the next BeginFrame when the OutputSurface's swap didn't
implicitly request the next BeginFrame. Having the Scheduler detect that
implicit request is too confusing and error prone, so this patch only uses the
explicit request.

This particular diff, however, is used to avoid negative glitches in
SetNeedsBeginFrame between two BeginFrames. Since we just drew, it's likely that
we don't think we need another BeginFrame for a fraction of a VSync until we get
another user-input or draw trigger.

A negative glitch is bad because it will propagate all the way to the Browser,
at which point the Browser might sample the SetNeedsBeginFrame value at an
inopportune time, delaying the BeginFrame for a VSync.

+    return true;
+
   return false;
 }
 
 void SchedulerStateMachine::DidEnterBeginFrame(const BeginFrameArgs& args) {
   current_frame_number_++;
   inside_begin_frame_ = true;
   last_begin_frame_args_ = args;
 }
 
 void SchedulerStateMachine::DidLeaveBeginFrame() {
@@ skipping 108 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