cc: Subtract estimated display draw time from begin frame deadline

Display draw time was being subtraced from display scheduler's deadline,
but a similar adjustment was missing from cc scheduler's deadline. To
fix this inconsistency, this CL applies the deadline adjustment at a
single place, the begin frame source so that there's no mismatch.

This makes cc scheduler have a later deadline and can cause it to wait
longer for the main thread. Recent experimentation has shown improved
latency in the absence of a scheduler deadline. It's hoped that some of
the improvement can be achieved by just fixing the deadline.

Bug: 1042584
Change-Id: Ia948b64c1dbba5239d1b97062224e73487e35fba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2045210
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781159}

cc/scheduler/scheduler.cc

@@ -460,28 +460,26 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
   adjusted_args.deadline -= compositor_timing_history_->DrawDurationEstimate();
   adjusted_args.deadline -= kDeadlineFudgeFactor;
 
-  // TODO(khushalsagar): We need to consider the deadline fudge factor here to
-  // match the deadline used in BeginImplFrameDeadlineMode::REGULAR mode
-  // (used in the case where the impl thread needs to redraw). In the case where
-  // main_frame_to_active is fast, we should consider using
-  // BeginImplFrameDeadlineMode::LATE instead to avoid putting the main
-  // thread in high latency mode. See crbug.com/753146.
-  base::TimeDelta bmf_to_activate_threshold =
-      adjusted_args.interval -
-      compositor_timing_history_->DrawDurationEstimate() - kDeadlineFudgeFactor;
+  // TODO(khushalsagar): We need to match the deadline used in
+  // BeginImplFrameDeadlineMode::REGULAR mode (used in the case where the impl
+  // thread needs to redraw). In the case where main_frame_to_active is fast, we
+  // should consider using BeginImplFrameDeadlineMode::LATE instead to avoid
+  // putting the main thread in high latency mode. See crbug.com/753146.
+  const base::TimeDelta bmf_to_activate_threshold =
+      adjusted_args.deadline - adjusted_args.frame_time;
 
   // An estimate of time from starting the main frame on the main thread to when
   // the resulting pending tree is activated. Note that this excludes the
   // durations where progress is blocked due to back pressure in the pipeline
   // (ready to commit to commit, ready to activate to activate, etc.)
-  base::TimeDelta bmf_start_to_activate =
+  const base::TimeDelta bmf_start_to_activate =
       compositor_timing_history_
           ->BeginMainFrameStartToReadyToCommitDurationEstimate() +
       compositor_timing_history_->CommitDurationEstimate() +
       compositor_timing_history_->CommitToReadyToActivateDurationEstimate() +
       compositor_timing_history_->ActivateDurationEstimate();
 
-  base::TimeDelta bmf_to_activate_estimate_critical =
+  const base::TimeDelta bmf_to_activate_estimate_critical =
       bmf_start_to_activate +
       compositor_timing_history_->BeginMainFrameQueueDurationCriticalEstimate();
   state_machine_.SetCriticalBeginMainFrameToActivateIsFast(

components/viz/common/frame_sinks/begin_frame_args.h

@@ -131,7 +131,7 @@ struct VIZ_COMMON_EXPORT BeginFrameArgs {
 
   // Returns how much time the display should reserve for draw and swap if the
   // BeginFrame interval is |interval|.
-  static base::TimeDelta DefaultEstimatedDisplayDrawTime(
+  static constexpr base::TimeDelta DefaultEstimatedDisplayDrawTime(
       base::TimeDelta interval) {
     return interval * kDefaultEstimatedDisplayDrawTimeRatio;
   }

components/viz/common/frame_sinks/begin_frame_source.cc

@@ -102,6 +102,7 @@ BeginFrameArgs
 BeginFrameSource::BeginFrameArgsGenerator::GenerateBeginFrameArgs(
     uint64_t source_id,
     base::TimeTicks frame_time,
+    base::TimeTicks deadline,
     base::TimeTicks next_frame_time,
     base::TimeDelta vsync_interval) {
   uint64_t sequence_number =
@@ -111,7 +112,7 @@ BeginFrameSource::BeginFrameArgsGenerator::GenerateBeginFrameArgs(
   next_expected_frame_time_ = next_frame_time;
   next_sequence_number_ = sequence_number + 1;
   return BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, source_id,
-                                sequence_number, frame_time, next_frame_time,
+                                sequence_number, frame_time, deadline,
                                 vsync_interval, BeginFrameArgs::NORMAL);
 }
 
@@ -204,8 +205,7 @@ SyntheticBeginFrameSource::~SyntheticBeginFrameSource() = default;
 BackToBackBeginFrameSource::BackToBackBeginFrameSource(
     std::unique_ptr<DelayBasedTimeSource> time_source)
     : SyntheticBeginFrameSource(kNotRestartableId),
-      time_source_(std::move(time_source)),
-      next_sequence_number_(BeginFrameArgs::kStartingFrameNumber) {
+      time_source_(std::move(time_source)) {
   time_source_->SetClient(this);
   // The time_source_ ticks immediately, so we SetActive(true) for a single
   // tick when we need it, and keep it as SetActive(false) otherwise.
@@ -250,12 +250,15 @@ void BackToBackBeginFrameSource::OnGpuNoLongerBusy() {
 void BackToBackBeginFrameSource::OnTimerTick() {
   if (RequestCallbackOnGpuAvailable())
     return;
-  base::TimeTicks frame_time = time_source_->LastTickTime();
-  base::TimeDelta default_interval = BeginFrameArgs::DefaultInterval();
-  BeginFrameArgs args = BeginFrameArgs::Create(
-      BEGINFRAME_FROM_HERE, source_id(), next_sequence_number_, frame_time,
-      frame_time + default_interval, default_interval, BeginFrameArgs::NORMAL);
-  next_sequence_number_++;
+  const base::TimeTicks frame_time = time_source_->LastTickTime();
+  const base::TimeDelta interval = BeginFrameArgs::DefaultInterval();
+  const base::TimeTicks next_frame_time = frame_time + interval;
+  const base::TimeTicks deadline =
+      next_frame_time -
+      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
+
+  BeginFrameArgs args = begin_frame_args_generator_.GenerateBeginFrameArgs(
+      source_id(), frame_time, deadline, next_frame_time, interval);
 
   // This must happen after getting the LastTickTime() from the time source.
   time_source_->SetActive(false);
@@ -293,8 +296,12 @@ void DelayBasedBeginFrameSource::OnUpdateVSyncParameters(
 BeginFrameArgs DelayBasedBeginFrameSource::CreateBeginFrameArgs(
     base::TimeTicks frame_time) {
   base::TimeDelta interval = time_source_->Interval();
+  base::TimeTicks next_frame_time = time_source_->NextTickTime();
+  base::TimeTicks deadline = next_frame_time;
+  if (apply_display_deadline_adjustment_)
+    deadline -= BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
   return begin_frame_args_generator_.GenerateBeginFrameArgs(
-      source_id(), frame_time, time_source_->NextTickTime(), interval);
+      source_id(), frame_time, deadline, next_frame_time, interval);
 }
 
 void DelayBasedBeginFrameSource::AddObserver(BeginFrameObserver* obs) {

components/viz/common/frame_sinks/begin_frame_source.h

@@ -137,6 +137,7 @@ class VIZ_COMMON_EXPORT BeginFrameSource {
 
     BeginFrameArgs GenerateBeginFrameArgs(uint64_t source_id,
                                           base::TimeTicks frame_time,
+                                          base::TimeTicks deadline,
                                           base::TimeTicks next_frame_time,
                                           base::TimeDelta vsync_interval);
 
@@ -282,10 +283,10 @@ class VIZ_COMMON_EXPORT BackToBackBeginFrameSource
   void OnTimerTick() override;
 
  private:
+  BeginFrameArgsGenerator begin_frame_args_generator_;
   std::unique_ptr<DelayBasedTimeSource> time_source_;
   base::flat_set<BeginFrameObserver*> observers_;
   base::flat_set<BeginFrameObserver*> pending_begin_frame_observers_;
-  uint64_t next_sequence_number_;
   base::WeakPtrFactory<BackToBackBeginFrameSource> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(BackToBackBeginFrameSource);
@@ -315,6 +316,11 @@ class VIZ_COMMON_EXPORT DelayBasedBeginFrameSource
   // DelayBasedTimeSourceClient implementation.
   void OnTimerTick() override;
 
+  // TODO(sunnyps): Apply deadline adjustment by default after fixing tests.
+  void set_apply_display_deadline_adjustment_for_testing(bool apply) {
+    apply_display_deadline_adjustment_ = apply;
+  }
+
  private:
   // The created BeginFrameArgs' sequence_number is calculated based on what
   // interval |frame_time| is in. For example, if |last_frame_time_| is 100,
@@ -331,6 +337,7 @@ class VIZ_COMMON_EXPORT DelayBasedBeginFrameSource
   BeginFrameArgs last_begin_frame_args_;
 
   BeginFrameArgsGenerator begin_frame_args_generator_;
+  bool apply_display_deadline_adjustment_ = true;
 
   DISALLOW_COPY_AND_ASSIGN(DelayBasedBeginFrameSource);
 };

components/viz/common/frame_sinks/begin_frame_source_unittest.cc

@@ -59,8 +59,12 @@ class TestTaskRunner : public base::TestMockTimeTaskRunner {
 // ------------------------------------------
 class BackToBackBeginFrameSourceTest : public ::testing::Test {
  protected:
-  static const int64_t kDeadline;
-  static const int64_t kInterval;
+  static constexpr int64_t kInterval =
+      BeginFrameArgs::DefaultInterval().InMicroseconds();
+  static constexpr int64_t kDeadline =
+      kInterval - BeginFrameArgs::DefaultEstimatedDisplayDrawTime(
+                      base::TimeDelta::FromMicroseconds(kInterval))
+                      .InMicroseconds();
 
   void SetUp() override {
     task_runner_ = base::MakeRefCounted<TestTaskRunner>();
@@ -81,12 +85,6 @@ class BackToBackBeginFrameSourceTest : public ::testing::Test {
   FakeDelayBasedTimeSource* delay_based_time_source_;  // Owned by |source_|.
 };
 
-const int64_t BackToBackBeginFrameSourceTest::kDeadline =
-    BeginFrameArgs::DefaultInterval().InMicroseconds();
-
-const int64_t BackToBackBeginFrameSourceTest::kInterval =
-    BeginFrameArgs::DefaultInterval().InMicroseconds();
-
 TEST_F(BackToBackBeginFrameSourceTest, AddObserverSendsBeginFrame) {
   EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
   source_->AddObserver(obs_.get());
@@ -353,6 +351,8 @@ TEST_F(BackToBackBeginFrameSourceTest, MultipleObserversAtOnce) {
 // ------------------------------------------
 class DelayBasedBeginFrameSourceTest : public ::testing::Test {
  public:
+  static constexpr int64_t kInterval = 10000;
+
   void SetUp() override {
     task_runner_ = base::MakeRefCounted<TestTaskRunner>();
     std::unique_ptr<FakeDelayBasedTimeSource> time_source =
@@ -360,10 +360,12 @@ class DelayBasedBeginFrameSourceTest : public ::testing::Test {
             task_runner_->GetMockTickClock(), task_runner_.get());
 
     time_source->SetTimebaseAndInterval(
-        base::TimeTicks(), base::TimeDelta::FromMicroseconds(10000));
+        base::TimeTicks(), base::TimeDelta::FromMicroseconds(kInterval));
     source_ = std::make_unique<DelayBasedBeginFrameSource>(
         std::move(time_source), BeginFrameSource::kNotRestartableId);
-    obs_.reset(new MockBeginFrameObserver);
+    // TODO(sunnyps): Apply deadline adjustment by default after fixing tests.
+    source_->set_apply_display_deadline_adjustment_for_testing(false);
+    obs_ = std::make_unique<MockBeginFrameObserver>();
   }
 
   void TearDown() override { obs_.reset(); }
@@ -665,6 +667,32 @@ TEST_F(DelayBasedBeginFrameSourceTest, ConsecutiveArgsDelayedByMultipleVsyncs) {
   source_->AddObserver(&obs);
 }
 
+TEST_F(DelayBasedBeginFrameSourceTest, DisplayDeadlineAdjustment) {
+  source_->set_apply_display_deadline_adjustment_for_testing(true);
+
+  constexpr int64_t kDeadline =
+      kInterval - BeginFrameArgs::DefaultEstimatedDisplayDrawTime(
+                      base::TimeDelta::FromMicroseconds(kInterval))
+                      .InMicroseconds();
+
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(*obs_, source_->source_id(), 1, 0,
+                                 0 + kDeadline, kInterval);
+
+  source_->AddObserver(obs_.get());
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 2, 10000,
+                          10000 + kDeadline, kInterval);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 3, 20000,
+                          20000 + kDeadline, kInterval);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 4, 30000,
+                          30000 + kDeadline, kInterval);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(30001));
+
+  source_->RemoveObserver(obs_.get());
+  // No new frames....
+  task_runner_->FastForwardTo(TicksFromMicroseconds(60000));
+}
+
 // ExternalBeginFrameSource testing
 // --------------------------------------------
 class MockExternalBeginFrameSourceClient

components/viz/service/display/display_scheduler.cc

@@ -176,8 +176,6 @@ bool DisplayScheduler::OnBeginFrame(const BeginFrameArgs& args) {
 
   // Schedule the deadline.
   current_begin_frame_args_ = save_args;
-  current_begin_frame_args_.deadline -=
-      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(save_args.interval);
   inside_begin_frame_deadline_interval_ = true;
   UpdateHasPendingSurfaces();
   ScheduleBeginFrameDeadline();

components/viz/service/display/display_scheduler_unittest.cc

@@ -256,8 +256,8 @@ TEST_F(DisplaySchedulerTest, ResizeHasLateDeadlineUntilNewRootSurface) {
   scheduler_.BeginFrameDeadlineForTest();
 
   // Verify deadline goes back to normal after resize.
-  late_deadline = now_src().NowTicks() + BeginFrameArgs::DefaultInterval();
   AdvanceTimeAndBeginFrameForTest({root_surface_id2, sid1});
+  late_deadline = now_src().NowTicks() + BeginFrameArgs::DefaultInterval();
   SurfaceDamaged(sid1);
   EXPECT_GT(late_deadline, scheduler_.DesiredBeginFrameDeadlineTimeForTest());
   SurfaceDamaged(root_surface_id2);
@@ -734,11 +734,7 @@ TEST_F(DisplaySchedulerTest, DidSwapBuffers) {
   // Damage from previous BeginFrame should cary over, so don't damage again.
   scheduler_.DidReceiveSwapBuffersAck();
   AdvanceTimeAndBeginFrameForTest({sid2});
-  base::TimeTicks expected_deadline =
-      last_begin_frame_args_.deadline -
-      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(
-          last_begin_frame_args_.interval);
-  EXPECT_EQ(expected_deadline,
+  EXPECT_EQ(last_begin_frame_args_.deadline,
             scheduler_.DesiredBeginFrameDeadlineTimeForTest());
   // Still waiting for surface 2. Once it updates, deadline should trigger
   // immediately again.

components/viz/service/frame_sinks/external_begin_frame_source_android.cc

@@ -38,10 +38,13 @@ void ExternalBeginFrameSourceAndroid::OnVSync(
   base::TimeDelta vsync_period(
       base::TimeDelta::FromMicroseconds(period_micros));
   // Calculate the next frame deadline:
-  base::TimeTicks deadline = frame_time + vsync_period;
+  base::TimeTicks next_frame_time = frame_time + vsync_period;
+  base::TimeTicks deadline =
+      next_frame_time -
+      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(vsync_period);
 
   auto begin_frame_args = begin_frame_args_generator_.GenerateBeginFrameArgs(
-      source_id(), frame_time, deadline, vsync_period);
+      source_id(), frame_time, deadline, next_frame_time, vsync_period);
   OnBeginFrame(begin_frame_args);
 }
 

components/viz/service/frame_sinks/gpu_vsync_begin_frame_source.cc

@@ -36,8 +36,12 @@ void GpuVSyncBeginFrameSource::OnGpuVSync(base::TimeTicks vsync_time,
     skip_next_vsync_ = true;
     vsync_interval *= 2;
   }
+  auto next_frame_time = vsync_time + vsync_interval;
+  auto deadline =
+      next_frame_time -
+      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(vsync_interval);
   auto begin_frame_args = begin_frame_args_generator_.GenerateBeginFrameArgs(
-      source_id(), vsync_time, vsync_time + vsync_interval, vsync_interval);
+      source_id(), vsync_time, deadline, next_frame_time, vsync_interval);
   ExternalBeginFrameSource::OnBeginFrame(begin_frame_args);
 }
 
@@ -58,8 +62,11 @@ BeginFrameArgs GpuVSyncBeginFrameSource::GetMissedBeginFrameArgs(
   // Don't create new args unless we've actually moved past the previous frame.
   if (!last_begin_frame_args_.IsValid() ||
       frame_time > last_begin_frame_args_.frame_time) {
+    auto next_frame_time = frame_time + interval;
+    auto deadline = next_frame_time -
+                    BeginFrameArgs::DefaultEstimatedDisplayDrawTime(interval);
     last_begin_frame_args_ = begin_frame_args_generator_.GenerateBeginFrameArgs(
-        source_id(), frame_time, frame_time + interval, interval);
+        source_id(), frame_time, deadline, next_frame_time, interval);
   }
 
   return ExternalBeginFrameSource::GetMissedBeginFrameArgs(obs);