cc: Prevent retro frames from being discarded too early.

Retro frames should not be discarded if there's a possibility
that they can be drawn. This CL also adds tests for the
expiration logic of retro frames.

BUG=413484

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

Cr-Commit-Position: refs/heads/master@{#305873}

2 files changed, 169 insertions, 33 deletions

diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc
index 3e6f6c6a..7f0232d 100644
--- a/cc/scheduler/scheduler.cc
+++ b/cc/scheduler/scheduler.cc
@@ -470,20 +470,16 @@ void Scheduler::BeginRetroFrame() {
   // draining the queue if we don't catch up. If we consistently can't catch
   // up, our fallback should be to lower our frame rate.
   base::TimeTicks now = Now();
-  base::TimeDelta draw_duration_estimate = client_->DrawDurationEstimate();
+
   while (!begin_retro_frame_args_.empty()) {
-    base::TimeTicks adjusted_deadline = AdjustedBeginImplFrameDeadline(
-        begin_retro_frame_args_.front(), draw_duration_estimate);
-    if (now <= adjusted_deadline)
+    const BeginFrameArgs& args = begin_retro_frame_args_.front();
+    base::TimeTicks expiration_time = args.frame_time + args.interval;
+    if (now <= expiration_time)
       break;
-
-    TRACE_EVENT_INSTANT2("cc",
-                         "Scheduler::BeginRetroFrame discarding",
-                         TRACE_EVENT_SCOPE_THREAD,
-                         "deadline - now",
-                         (adjusted_deadline - now).InMicroseconds(),
-                         "BeginFrameArgs",
-                         begin_retro_frame_args_.front().AsValue());
+    TRACE_EVENT_INSTANT2(
+        "cc", "Scheduler::BeginRetroFrame discarding", TRACE_EVENT_SCOPE_THREAD,
+        "expiration_time - now", (expiration_time - now).InMillisecondsF(),
+        "BeginFrameArgs", begin_retro_frame_args_.front().AsValue());
     begin_retro_frame_args_.pop_front();
     frame_source_->DidFinishFrame(begin_retro_frame_args_.size());
   }
@@ -564,18 +560,28 @@ void Scheduler::BeginImplFrame(const BeginFrameArgs& args) {
   ProcessScheduledActions();
 
   state_machine_.OnBeginImplFrameDeadlinePending();
-  ScheduleBeginImplFrameDeadline(
-      AdjustedBeginImplFrameDeadline(args, draw_duration_estimate));
+
+  if (settings_.using_synchronous_renderer_compositor) {
+    // The synchronous renderer compositor has to make its GL calls
+    // within this call.
+    // TODO(brianderson): Have the OutputSurface initiate the deadline tasks
+    // so the synchronous renderer compositor can take advantage of splitting
+    // up the BeginImplFrame and deadline as well.
+    OnBeginImplFrameDeadline();
+  } else {
+    ScheduleBeginImplFrameDeadline(
+        AdjustedBeginImplFrameDeadline(args, draw_duration_estimate));
+  }
 }
 
 base::TimeTicks Scheduler::AdjustedBeginImplFrameDeadline(
     const BeginFrameArgs& args,
     base::TimeDelta draw_duration_estimate) const {
-  if (settings_.using_synchronous_renderer_compositor) {
-    // The synchronous compositor needs to draw right away.
-    return base::TimeTicks();
-  } else if (state_machine_.ShouldTriggerBeginImplFrameDeadlineEarly()) {
+  // The synchronous compositor does not post a deadline task.
+  DCHECK(!settings_.using_synchronous_renderer_compositor);
+  if (state_machine_.ShouldTriggerBeginImplFrameDeadlineEarly()) {
     // We are ready to draw a new active tree immediately.
+    // We don't use Now() here because it's somewhat expensive to call.
     return base::TimeTicks();
   } else if (state_machine_.needs_redraw()) {
     // We have an animation or fast input path on the impl thread that wants
@@ -596,15 +602,7 @@ base::TimeTicks Scheduler::AdjustedBeginImplFrameDeadline(
 void Scheduler::ScheduleBeginImplFrameDeadline(base::TimeTicks deadline) {
   TRACE_EVENT1(
       "cc", "Scheduler::ScheduleBeginImplFrameDeadline", "deadline", deadline);
-  if (settings_.using_synchronous_renderer_compositor) {
-    // The synchronous renderer compositor has to make its GL calls
-    // within this call.
-    // TODO(brianderson): Have the OutputSurface initiate the deadline tasks
-    // so the sychronous renderer compositor can take advantage of splitting
-    // up the BeginImplFrame and deadline as well.
-    OnBeginImplFrameDeadline();
-    return;
-  }
+
   begin_impl_frame_deadline_task_.Cancel();
   begin_impl_frame_deadline_task_.Reset(begin_impl_frame_deadline_closure_);
 
diff --git a/cc/scheduler/scheduler_unittest.cc b/cc/scheduler/scheduler_unittest.cc
index 636beb0..deea726 100644
--- a/cc/scheduler/scheduler_unittest.cc
+++ b/cc/scheduler/scheduler_unittest.cc
@@ -168,19 +168,32 @@ class FakeSchedulerClient : public SchedulerClient {
   void AdvanceFrame() {
     TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"),
                  "FakeSchedulerClient::AdvanceFrame");
+    bool previous_deadline_pending =
+        scheduler_->BeginImplFrameDeadlinePending();
     if (ExternalBeginFrame()) {
-      // Creep the time forward so that any BeginFrameArgs is not equal to the
-      // last one otherwise we violate the BeginFrameSource contract.
-      now_src_->AdvanceNowMicroseconds(1);
-      fake_external_begin_frame_source_->TestOnBeginFrame(
-          CreateBeginFrameArgsForTesting(now_src_));
+      SendNextBeginFrame();
+      // This could be the previous deadline or a new one.
       EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
     }
-
+    // Consume previous deadline first. It is important that we check for the
+    // existence of a previous deadline so that we do not consume the new one.
+    if (previous_deadline_pending) {
+      EXPECT_TRUE(task_runner().RunTasksWhile(ImplFrameDeadlinePending(true)));
+    }
+    // Then run tasks until new deadline is scheduled.
     EXPECT_TRUE(task_runner().RunTasksWhile(ImplFrameDeadlinePending(false)));
     EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
   }
 
+  void SendNextBeginFrame() {
+    DCHECK(ExternalBeginFrame());
+    // Creep the time forward so that any BeginFrameArgs is not equal to the
+    // last one otherwise we violate the BeginFrameSource contract.
+    now_src_->AdvanceNow(BeginFrameArgs::DefaultInterval());
+    fake_external_begin_frame_source_->TestOnBeginFrame(
+        CreateBeginFrameArgsForTesting(now_src_));
+  }
+
   OrderedSimpleTaskRunner& task_runner() { return *task_runner_; }
   TestNowSource* now_src() { return now_src_.get(); }
 
@@ -1387,6 +1400,131 @@ TEST(SchedulerTest, BeginRetroFrame_SwapThrottled) {
   client.Reset();
 }
 
+TEST(SchedulerTest, RetroFrameDoesNotExpireTooEarly) {
+  FakeSchedulerClient client;
+  SchedulerSettings scheduler_settings;
+  scheduler_settings.use_external_begin_frame_source = true;
+  TestScheduler* scheduler = client.CreateScheduler(scheduler_settings);
+  scheduler->SetCanStart();
+  scheduler->SetVisible(true);
+  scheduler->SetCanDraw(true);
+  InitializeOutputSurfaceAndFirstCommit(scheduler, &client);
+
+  client.Reset();
+  scheduler->SetNeedsCommit();
+  EXPECT_TRUE(client.needs_begin_frames());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrames(true)", client);
+
+  client.Reset();
+  client.AdvanceFrame();
+  EXPECT_ACTION("WillBeginImplFrame", client, 0, 2);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client, 1, 2);
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+
+  client.Reset();
+  scheduler->NotifyBeginMainFrameStarted();
+
+  client.Reset();
+  client.SendNextBeginFrame();
+  // This BeginFrame is queued up as a retro frame.
+  EXPECT_NO_ACTION(client);
+  // The previous deadline is still pending.
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+
+  client.Reset();
+  // This commit should schedule the (previous) deadline to trigger immediately.
+  scheduler->NotifyReadyToCommit();
+  EXPECT_SINGLE_ACTION("ScheduledActionCommit", client);
+
+  client.Reset();
+  // The deadline task should trigger causing a draw.
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+  client.task_runner().RunTasksWhile(client.ImplFrameDeadlinePending(true));
+  EXPECT_ACTION("ScheduledActionAnimate", client, 0, 2);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 1, 2);
+
+  // Keep animating.
+  client.Reset();
+  scheduler->SetNeedsAnimate();
+  scheduler->SetNeedsRedraw();
+  EXPECT_NO_ACTION(client);
+
+  // Let's advance sufficiently past the next frame's deadline.
+  client.now_src()->AdvanceNow(
+      BeginFrameArgs::DefaultInterval() -
+      BeginFrameArgs::DefaultEstimatedParentDrawTime() +
+      base::TimeDelta::FromMicroseconds(1));
+
+  // The retro frame hasn't expired yet.
+  client.task_runner().RunTasksWhile(client.ImplFrameDeadlinePending(false));
+  EXPECT_ACTION("WillBeginImplFrame", client, 0, 2);
+  EXPECT_ACTION("ScheduledActionAnimate", client, 1, 2);
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+
+  // This is an immediate deadline case.
+  client.Reset();
+  client.task_runner().RunPendingTasks();
+  EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending());
+  EXPECT_SINGLE_ACTION("ScheduledActionDrawAndSwapIfPossible", client);
+}
+
+TEST(SchedulerTest, RetroFrameDoesNotExpireTooLate) {
+  FakeSchedulerClient client;
+  SchedulerSettings scheduler_settings;
+  scheduler_settings.use_external_begin_frame_source = true;
+  TestScheduler* scheduler = client.CreateScheduler(scheduler_settings);
+  scheduler->SetCanStart();
+  scheduler->SetVisible(true);
+  scheduler->SetCanDraw(true);
+  InitializeOutputSurfaceAndFirstCommit(scheduler, &client);
+
+  client.Reset();
+  scheduler->SetNeedsCommit();
+  EXPECT_TRUE(client.needs_begin_frames());
+  EXPECT_SINGLE_ACTION("SetNeedsBeginFrames(true)", client);
+
+  client.Reset();
+  client.AdvanceFrame();
+  EXPECT_ACTION("WillBeginImplFrame", client, 0, 2);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client, 1, 2);
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+
+  client.Reset();
+  scheduler->NotifyBeginMainFrameStarted();
+
+  client.Reset();
+  client.SendNextBeginFrame();
+  // This BeginFrame is queued up as a retro frame.
+  EXPECT_NO_ACTION(client);
+  // The previous deadline is still pending.
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+
+  client.Reset();
+  // This commit should schedule the (previous) deadline to trigger immediately.
+  scheduler->NotifyReadyToCommit();
+  EXPECT_SINGLE_ACTION("ScheduledActionCommit", client);
+
+  client.Reset();
+  // The deadline task should trigger causing a draw.
+  EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
+  client.task_runner().RunTasksWhile(client.ImplFrameDeadlinePending(true));
+  EXPECT_ACTION("ScheduledActionAnimate", client, 0, 2);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client, 1, 2);
+
+  // Keep animating.
+  client.Reset();
+  scheduler->SetNeedsAnimate();
+  scheduler->SetNeedsRedraw();
+  EXPECT_NO_ACTION(client);
+
+  // Let's advance sufficiently past the next frame's deadline.
+  client.now_src()->AdvanceNow(BeginFrameArgs::DefaultInterval() +
+                               base::TimeDelta::FromMicroseconds(1));
+
+  // The retro frame should've expired.
+  EXPECT_NO_ACTION(client);
+}
+
 void BeginFramesNotFromClient(bool use_external_begin_frame_source,
                               bool throttle_frame_production) {
   FakeSchedulerClient client;