cc: Don't double-tick unthrottled FrameRateController

When the FrameRateController is configured with a non-throttling time
source (i.e., vsync is off), it posts the next tick task manually
whenever it is active at the end of a previous tick. However, it also
posts a manual tick task when a swap buffers call completes to make sure
rendering doesn't stall.

These two tick task sources combined can lead to an expential growth of
tick tasks. Consider the following call tree where each edge is an
asynchronous task:

    OnTimerTick
        |  |
        |   `--------------------.
        |                        |
    BeginImplFrame               |
    Deadline                     |
        |                        |
    OnSwapBuffers                |
    Complete                     |
        |                        |
    OnTimerTick              OnTimerTick
        |  |                     |  |
        |   `--------.           |   `----------.
        |            |           |              |
    BeginImplFrame   |       BeginImplFrame     |
    Deadline         |       Deadline           |
        |            |           |              |
    OnSwapBuffers    |       OnSwapBuffers      |
    Complete         |       Complete           |
        |            |           |              |
    OnTimerTick  OnTimerTick OnTimerTick    OnTimerTick
        |  |         |  |        |  |           |  |
        :  :         :  :        :  :           :  :

    (etc.)

In practice this situation happens if both the impl and main threads request a
BeginFrame and when the tick task runs, the scheduler isn't ready to draw
because the previous BeginFrame deadline hasn't triggered yet. This means the
the pending swap count never increases to the throttling limit and each timer
tick causes another one to get queued. Once the deadline triggers and we do
swap, the swap completion spawns a parallel line of tick tasks.

This patch fixes the problem by only queueing a new tick task if we haven't
queued one already.

BUG=312960

Review URL: https://codereview.chromium.org/67023003

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

3 files changed, 39 insertions, 7 deletions

diff --git a/cc/scheduler/frame_rate_controller.cc b/cc/scheduler/frame_rate_controller.cc
index 2729748..243ef6b 100644
--- a/cc/scheduler/frame_rate_controller.cc
+++ b/cc/scheduler/frame_rate_controller.cc
@@ -44,6 +44,7 @@ FrameRateController::FrameRateController(scoped_refptr<TimeSource> timer)
       time_source_(timer),
       active_(false),
       is_time_source_throttling_(true),
+      manual_tick_pending_(false),
       task_runner_(NULL),
       weak_factory_(this) {
   time_source_client_adapter_ =
@@ -59,6 +60,7 @@ FrameRateController::FrameRateController(
       interval_(BeginFrameArgs::DefaultInterval()),
       active_(false),
       is_time_source_throttling_(false),
+      manual_tick_pending_(false),
       task_runner_(task_runner),
       weak_factory_(this) {}
 
@@ -81,10 +83,12 @@ BeginFrameArgs FrameRateController::SetActive(bool active) {
           missed_tick_time, deadline + deadline_adjustment_, interval_);
     }
   } else {
-    if (active)
+    if (active) {
       PostManualTick();
-    else
+    } else {
       weak_factory_.InvalidateWeakPtrs();
+      manual_tick_pending_ = false;
+    }
   }
 
   return BeginFrameArgs();
@@ -129,7 +133,8 @@ void FrameRateController::OnTimerTick() {
 }
 
 void FrameRateController::PostManualTick() {
-  if (active_) {
+  if (active_ && !manual_tick_pending_) {
+    manual_tick_pending_ = true;
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&FrameRateController::ManualTick,
                                       weak_factory_.GetWeakPtr()));
@@ -137,6 +142,7 @@ void FrameRateController::PostManualTick() {
 }
 
 void FrameRateController::ManualTick() {
+  manual_tick_pending_ = false;
   OnTimerTick();
 }
 
diff --git a/cc/scheduler/frame_rate_controller.h b/cc/scheduler/frame_rate_controller.h
index 4d2e307..6d86d97 100644
--- a/cc/scheduler/frame_rate_controller.h
+++ b/cc/scheduler/frame_rate_controller.h
@@ -88,6 +88,7 @@ class CC_EXPORT FrameRateController {
 
   // Members for unthrottled frame-rate.
   bool is_time_source_throttling_;
+  bool manual_tick_pending_;
   base::SingleThreadTaskRunner* task_runner_;
   base::WeakPtrFactory<FrameRateController> weak_factory_;
 
diff --git a/cc/scheduler/frame_rate_controller_unittest.cc b/cc/scheduler/frame_rate_controller_unittest.cc
index 353d984..ea5f52f 100644
--- a/cc/scheduler/frame_rate_controller_unittest.cc
+++ b/cc/scheduler/frame_rate_controller_unittest.cc
@@ -15,16 +15,17 @@ class FakeFrameRateControllerClient : public cc::FrameRateControllerClient {
  public:
   FakeFrameRateControllerClient() { Reset(); }
 
-  void Reset() { began_frame_ = false; }
-  bool BeganFrame() const { return began_frame_; }
+  void Reset() { frame_count_ = 0; }
+  bool BeganFrame() const { return frame_count_ > 0; }
+  int frame_count() const { return frame_count_; }
 
   virtual void FrameRateControllerTick(
       bool throttled, const BeginFrameArgs& args) OVERRIDE {
-    began_frame_ = !throttled;
+    frame_count_ += throttled ? 0 : 1;
   }
 
  protected:
-  bool began_frame_;
+  int frame_count_;
 };
 
 TEST(FrameRateControllerTest, TestFrameThrottling_ImmediateAck) {
@@ -182,5 +183,29 @@ TEST(FrameRateControllerTest, TestFrameThrottling_Unthrottled) {
   EXPECT_TRUE(client.BeganFrame());
 }
 
+TEST(FrameRateControllerTest, TestFrameThrottling_NoDoubleTicking) {
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner =
+      new base::TestSimpleTaskRunner;
+  FakeFrameRateControllerClient client;
+  FrameRateController controller(task_runner.get());
+  controller.SetClient(&client);
+
+  // SetActive triggers 1st frame and queues another tick task since the time
+  // source isn't throttling.
+  controller.SetActive(true);
+  task_runner->RunPendingTasks();
+  EXPECT_TRUE(client.BeganFrame());
+  client.Reset();
+  EXPECT_TRUE(task_runner->HasPendingTask());
+
+  // Simulate a frame swap. This shouldn't queue a second tick task.
+  controller.DidSwapBuffers();
+  controller.DidSwapBuffersComplete();
+
+  // The client should only be ticked once.
+  task_runner->RunPendingTasks();
+  EXPECT_EQ(1, client.frame_count());
+}
+
 }  // namespace
 }  // namespace cc