cc: Always use SetNeedsBeginFrame to request the next BeginFrame

This avoids relying on SwapBuffers to implicitly trigger the next
BeginFrame. Only a single code path is used to request the next
BeginFrame now: SetNeedsBeginFrame(true).

This avoids issues where OutputSurface subclasses might not call
OutputSurface::DidSwap() when they need to or when we early out
a swap request and don't actually swap anything.

BUG=289755

Review URL: https://chromiumcodereview.appspot.com/23498035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223218 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 92 insertions, 46 deletions

diff --git a/cc/output/output_surface.cc b/cc/output/output_surface.cc
index 2ffcf95..812f46b 100644
--- a/cc/output/output_surface.cc
+++ b/cc/output/output_surface.cc
@@ -39,7 +39,7 @@ OutputSurface::OutputSurface(scoped_refptr<ContextProvider> context_provider)
       max_frames_pending_(0),
       pending_swap_buffers_(0),
       needs_begin_frame_(false),
-      begin_frame_pending_(false),
+      client_ready_for_begin_frame_(true),
       client_(NULL),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false) {}
@@ -54,7 +54,7 @@ OutputSurface::OutputSurface(
       max_frames_pending_(0),
       pending_swap_buffers_(0),
       needs_begin_frame_(false),
-      begin_frame_pending_(false),
+      client_ready_for_begin_frame_(true),
       client_(NULL),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false) {}
@@ -71,7 +71,7 @@ OutputSurface::OutputSurface(
       max_frames_pending_(0),
       pending_swap_buffers_(0),
       needs_begin_frame_(false),
-      begin_frame_pending_(false),
+      client_ready_for_begin_frame_(true),
       client_(NULL),
       check_for_retroactive_begin_frame_pending_(false),
       external_stencil_test_enabled_(false) {}
@@ -133,7 +133,7 @@ void OutputSurface::SetNeedsRedrawRect(gfx::Rect damage_rect) {
 void OutputSurface::SetNeedsBeginFrame(bool enable) {
   TRACE_EVENT1("cc", "OutputSurface::SetNeedsBeginFrame", "enable", enable);
   needs_begin_frame_ = enable;
-  begin_frame_pending_ = false;
+  client_ready_for_begin_frame_ = true;
   if (frame_rate_controller_) {
     BeginFrameArgs skipped = frame_rate_controller_->SetActive(enable);
     if (skipped.IsValid())
@@ -145,14 +145,14 @@ void OutputSurface::SetNeedsBeginFrame(bool enable) {
 
 void OutputSurface::BeginFrame(const BeginFrameArgs& args) {
   TRACE_EVENT2("cc", "OutputSurface::BeginFrame",
-               "begin_frame_pending_", begin_frame_pending_,
+               "client_ready_for_begin_frame_", client_ready_for_begin_frame_,
                "pending_swap_buffers_", pending_swap_buffers_);
-  if (!needs_begin_frame_ || begin_frame_pending_ ||
+  if (!needs_begin_frame_ || !client_ready_for_begin_frame_ ||
       (pending_swap_buffers_ >= max_frames_pending_ &&
        max_frames_pending_ > 0)) {
     skipped_begin_frame_args_ = args;
   } else {
-    begin_frame_pending_ = true;
+    client_ready_for_begin_frame_ = false;
     client_->BeginFrame(args);
     // args might be an alias for skipped_begin_frame_args_.
     // Do not reset it before calling BeginFrame!
@@ -192,7 +192,6 @@ void OutputSurface::CheckForRetroactiveBeginFrame() {
 }
 
 void OutputSurface::DidSwapBuffers() {
-  begin_frame_pending_ = false;
   pending_swap_buffers_++;
   TRACE_EVENT1("cc", "OutputSurface::DidSwapBuffers",
                "pending_swap_buffers_", pending_swap_buffers_);
@@ -213,7 +212,7 @@ void OutputSurface::OnSwapBuffersComplete(const CompositorFrameAck* ack) {
 
 void OutputSurface::DidLoseOutputSurface() {
   TRACE_EVENT0("cc", "OutputSurface::DidLoseOutputSurface");
-  begin_frame_pending_ = false;
+  client_ready_for_begin_frame_ = true;
   pending_swap_buffers_ = 0;
   client_->DidLoseOutputSurface();
 }
diff --git a/cc/output/output_surface.h b/cc/output/output_surface.h
index f30d3f3..c75092d 100644
--- a/cc/output/output_surface.h
+++ b/cc/output/output_surface.h
@@ -163,7 +163,7 @@ class CC_EXPORT OutputSurface : public FrameRateControllerClient {
   int max_frames_pending_;
   int pending_swap_buffers_;
   bool needs_begin_frame_;
-  bool begin_frame_pending_;
+  bool client_ready_for_begin_frame_;
 
   // Forwarded to OutputSurfaceClient but threaded through OutputSurface
   // first so OutputSurface has a chance to update the FrameRateController
diff --git a/cc/output/output_surface_unittest.cc b/cc/output/output_surface_unittest.cc
index 54fd3a1..cad5e4e 100644
--- a/cc/output/output_surface_unittest.cc
+++ b/cc/output/output_surface_unittest.cc
@@ -210,8 +210,10 @@ TEST(OutputSurfaceTest, BeginFrameEmulation) {
   EXPECT_EQ(client.begin_frame_count(), 1);
   EXPECT_EQ(output_surface.pending_swap_buffers(), 0);
 
-  // DidSwapBuffers should clear the pending BeginFrame.
+  // SetNeedsBeginFrame should clear the pending BeginFrame after
+  // a SwapBuffers.
   output_surface.DidSwapBuffersForTesting();
+  output_surface.SetNeedsBeginFrame(true);
   EXPECT_EQ(client.begin_frame_count(), 1);
   EXPECT_EQ(output_surface.pending_swap_buffers(), 1);
   task_runner->RunPendingTasks();
@@ -220,6 +222,7 @@ TEST(OutputSurfaceTest, BeginFrameEmulation) {
 
   // BeginFrame should be throttled by pending swap buffers.
   output_surface.DidSwapBuffersForTesting();
+  output_surface.SetNeedsBeginFrame(true);
   EXPECT_EQ(client.begin_frame_count(), 2);
   EXPECT_EQ(output_surface.pending_swap_buffers(), 2);
   task_runner->RunPendingTasks();
@@ -284,12 +287,14 @@ TEST(OutputSurfaceTest, OptimisticAndRetroactiveBeginFrames) {
   output_surface.BeginFrameForTesting();
   EXPECT_EQ(client.begin_frame_count(), 2);
   output_surface.DidSwapBuffersForTesting();
+  output_surface.SetNeedsBeginFrame(true);
   EXPECT_EQ(client.begin_frame_count(), 3);
   EXPECT_EQ(output_surface.pending_swap_buffers(), 1);
 
   // Optimistically injected BeginFrames should be by throttled by pending
   // swap buffers...
   output_surface.DidSwapBuffersForTesting();
+  output_surface.SetNeedsBeginFrame(true);
   EXPECT_EQ(client.begin_frame_count(), 3);
   EXPECT_EQ(output_surface.pending_swap_buffers(), 2);
   output_surface.BeginFrameForTesting();
diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc
index d638f6d..14f6c7f 100644
--- a/cc/scheduler/scheduler.cc
+++ b/cc/scheduler/scheduler.cc
@@ -17,7 +17,6 @@ Scheduler::Scheduler(SchedulerClient* client,
       client_(client),
       weak_factory_(this),
       last_set_needs_begin_frame_(false),
-      has_pending_begin_frame_(false),
       state_machine_(scheduler_settings),
       inside_process_scheduled_actions_(false),
       inside_action_(SchedulerStateMachine::ACTION_NONE) {
@@ -102,7 +101,6 @@ void Scheduler::DidLoseOutputSurface() {
 void Scheduler::DidCreateAndInitializeOutputSurface() {
   TRACE_EVENT0("cc", "Scheduler::DidCreateAndInitializeOutputSurface");
   state_machine_.DidCreateAndInitializeOutputSurface();
-  has_pending_begin_frame_ = false;
   last_set_needs_begin_frame_ = false;
   ProcessScheduledActions();
 }
@@ -137,29 +135,18 @@ void Scheduler::SetupNextBeginFrameIfNeeded() {
       settings_.throttle_frame_production;
   bool needs_begin_frame = needs_begin_frame_to_draw ||
                            proactive_begin_frame_wanted;
-  bool immediate_disables_needed =
-      settings_.using_synchronous_renderer_compositor;
-
-  // Determine if we need BeginFrame notifications.
-  // If we do, always request the BeginFrame immediately.
-  // If not, only disable on the next BeginFrame to avoid unnecessary toggles.
-  // The synchronous renderer compositor requires immediate disables though.
-  if ((needs_begin_frame ||
-       state_machine_.inside_begin_frame() ||
-       immediate_disables_needed) &&
-      (needs_begin_frame != last_set_needs_begin_frame_)) {
-    has_pending_begin_frame_ = false;
+
+  bool should_call_set_needs_begin_frame =
+      // Always request the BeginFrame immediately if it wasn't needed before.
+      (needs_begin_frame && !last_set_needs_begin_frame_) ||
+      // We always need to explicitly request our next BeginFrame.
+      state_machine_.inside_begin_frame();
+
+  if (should_call_set_needs_begin_frame) {
     client_->SetNeedsBeginFrameOnImplThread(needs_begin_frame);
     last_set_needs_begin_frame_ = needs_begin_frame;
   }
 
-  // Request another BeginFrame if we haven't drawn for now until we have
-  // deadlines implemented.
-  if (state_machine_.inside_begin_frame() && has_pending_begin_frame_) {
-    has_pending_begin_frame_ = false;
-    client_->SetNeedsBeginFrameOnImplThread(true);
-  }
-
   // Setup PollForAnticipatedDrawTriggers for cases where we want a proactive
   // BeginFrame but aren't requesting one.
   if (!needs_begin_frame &&
@@ -180,8 +167,7 @@ void Scheduler::SetupNextBeginFrameIfNeeded() {
 
 void Scheduler::BeginFrame(const BeginFrameArgs& args) {
   TRACE_EVENT0("cc", "Scheduler::BeginFrame");
-  DCHECK(!has_pending_begin_frame_);
-  has_pending_begin_frame_ = true;
+  DCHECK(!state_machine_.inside_begin_frame());
   last_begin_frame_args_ = args;
   state_machine_.DidEnterBeginFrame(args);
   ProcessScheduledActions();
@@ -199,14 +185,10 @@ void Scheduler::DrawAndSwapIfPossible() {
   DrawSwapReadbackResult result =
       client_->ScheduledActionDrawAndSwapIfPossible();
   state_machine_.DidDrawIfPossibleCompleted(result.did_draw);
-  if (result.did_swap)
-    has_pending_begin_frame_ = false;
 }
 
 void Scheduler::DrawAndSwapForced() {
-  DrawSwapReadbackResult result = client_->ScheduledActionDrawAndSwapForced();
-  if (result.did_swap)
-    has_pending_begin_frame_ = false;
+  client_->ScheduledActionDrawAndSwapForced();
 }
 
 void Scheduler::DrawAndReadback() {
diff --git a/cc/scheduler/scheduler.h b/cc/scheduler/scheduler.h
index 0f216b4..4d1b0b8 100644
--- a/cc/scheduler/scheduler.h
+++ b/cc/scheduler/scheduler.h
@@ -129,7 +129,6 @@ class CC_EXPORT Scheduler {
 
   base::WeakPtrFactory<Scheduler> weak_factory_;
   bool last_set_needs_begin_frame_;
-  bool has_pending_begin_frame_;
   BeginFrameArgs last_begin_frame_args_;
   base::CancelableClosure poll_for_draw_triggers_closure_;
 
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index c2cb977..c80d9cc 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -769,6 +769,13 @@ bool SchedulerStateMachine::ProactiveBeginFrameWantedByImplThread() const {
   if (needs_manage_tiles_)
     return true;
 
+  // If we just swapped, it's likely that we are going to produce another
+  // frame soon. This helps avoid negative glitches in our SetNeedsBeginFrame
+  // requests, which may propagate to the BeginFrame provider and get sampled
+  // at an inopportune time, delaying the next BeginFrame.
+  if (last_frame_number_swap_performed_ == current_frame_number_)
+    return true;
+
   return false;
 }
 
diff --git a/cc/scheduler/scheduler_unittest.cc b/cc/scheduler/scheduler_unittest.cc
index eba7f34..ff8b11d 100644
--- a/cc/scheduler/scheduler_unittest.cc
+++ b/cc/scheduler/scheduler_unittest.cc
@@ -209,7 +209,7 @@ TEST(SchedulerTest, RequestCommit) {
   scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
   EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 2);
   EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 1, 2);
-  EXPECT_FALSE(client.needs_begin_frame());
+  EXPECT_TRUE(client.needs_begin_frame());
   client.Reset();
 }
 
@@ -240,16 +240,27 @@ TEST(SchedulerTest, RequestCommitAfterBeginFrameSentToMainThread) {
   client.Reset();
 
   // Tick should draw but then begin another frame for the second commit.
+  // Because we just swapped, the Scheduler should also request the next
+  // BeginFrame from the OutputSurface.
   scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
   EXPECT_TRUE(client.needs_begin_frame());
-  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 2);
-  EXPECT_ACTION("ScheduledActionSendBeginFrameToMainThread", client, 1, 2);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 3);
+  EXPECT_ACTION("ScheduledActionSendBeginFrameToMainThread", client, 1, 3);
+  EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 2, 3);
   client.Reset();
 
-  // Finish the second commit to go back to quiescent state and verify we no
-  // longer request BeginFrames.
+  // Finish the second commit.
   scheduler->FinishCommit();
   scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_ACTION("ScheduledActionCommit", client, 0, 3);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 1, 3);
+  EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 2, 3);
+  EXPECT_TRUE(client.needs_begin_frame());
+  client.Reset();
+
+  // On the next BeginFrame, verify we go back to a quiescent state and
+  // no longer request BeginFrames.
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
   EXPECT_FALSE(client.needs_begin_frame());
 }
 
@@ -274,6 +285,12 @@ TEST(SchedulerTest, TextureAcquisitionCausesCommitInsteadOfDraw) {
   EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 2);
   EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 1, 2);
   EXPECT_FALSE(scheduler->RedrawPending());
+  EXPECT_TRUE(client.needs_begin_frame());
+
+  client.Reset();
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrameOnImplThread", client);
+  EXPECT_FALSE(scheduler->RedrawPending());
   EXPECT_FALSE(client.needs_begin_frame());
 
   client.Reset();
@@ -313,8 +330,13 @@ TEST(SchedulerTest, TextureAcquisitionCausesCommitInsteadOfDraw) {
   EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 2);
   EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 1, 2);
   EXPECT_FALSE(scheduler->RedrawPending());
-  EXPECT_FALSE(client.needs_begin_frame());
+  EXPECT_TRUE(client.needs_begin_frame());
+
+  // Make sure we stop requesting BeginFrames if we don't swap.
   client.Reset();
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrameOnImplThread", client);
+  EXPECT_FALSE(client.needs_begin_frame());
 }
 
 TEST(SchedulerTest, TextureAcquisitionCollision) {
@@ -361,11 +383,15 @@ TEST(SchedulerTest, TextureAcquisitionCollision) {
                 1,
                 3);
   EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 2, 3);
+  EXPECT_TRUE(client.needs_begin_frame());
   client.Reset();
 
   // Compositor not scheduled to draw because textures are locked by main
   // thread.
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrameOnImplThread", client);
   EXPECT_FALSE(client.needs_begin_frame());
+  client.Reset();
 
   // Needs an explicit commit from the main thread.
   scheduler->SetNeedsCommit();
@@ -375,7 +401,16 @@ TEST(SchedulerTest, TextureAcquisitionCollision) {
 
   // Trigger the commit
   scheduler->FinishCommit();
+  EXPECT_SINGLE_ACTION("ScheduledActionCommit", client);
   EXPECT_TRUE(client.needs_begin_frame());
+  client.Reset();
+
+  // Verify we draw on the next BeginFrame.
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 0, 2);
+  EXPECT_ACTION("SetNeedsBeginFrameOnImplThread", client, 1, 2);
+  EXPECT_TRUE(client.needs_begin_frame());
+  client.Reset();
 }
 
 TEST(SchedulerTest, VisibilitySwitchWithTextureAcquisition) {
@@ -466,6 +501,12 @@ TEST(SchedulerTest, RequestRedrawInsideDraw) {
   scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
   EXPECT_EQ(2, client.num_draws());
   EXPECT_FALSE(scheduler->RedrawPending());
+  EXPECT_TRUE(client.needs_begin_frame());
+
+  // We stop requesting BeginFrames after a BeginFrame where we don't swap.
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_EQ(2, client.num_draws());
+  EXPECT_FALSE(scheduler->RedrawPending());
   EXPECT_FALSE(client.needs_begin_frame());
 }
 
@@ -575,6 +616,13 @@ TEST(SchedulerTest, RequestCommitInsideDraw) {
   EXPECT_EQ(2, client.num_draws());;
   EXPECT_FALSE(scheduler->RedrawPending());
   EXPECT_FALSE(scheduler->CommitPending());
+  EXPECT_TRUE(client.needs_begin_frame());
+
+  // We stop requesting BeginFrames after a BeginFrame where we don't swap.
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_EQ(2, client.num_draws());
+  EXPECT_FALSE(scheduler->RedrawPending());
+  EXPECT_FALSE(scheduler->CommitPending());
   EXPECT_FALSE(client.needs_begin_frame());
 }
 
@@ -756,6 +804,12 @@ TEST(SchedulerTest, ManageTiles) {
   EXPECT_FALSE(scheduler->RedrawPending());
   EXPECT_FALSE(scheduler->ManageTilesPending());
 
+  // We need a BeginFrame where we don't swap to go idle.
+  client.Reset();
+  scheduler->BeginFrame(BeginFrameArgs::CreateForTesting());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrameOnImplThread", client);
+  EXPECT_EQ(0, client.num_draws());
+
   // Now trigger a ManageTiles outside of a draw. We will then need
   // a begin-frame for the ManageTiles, but we don't need a draw.
   client.Reset();