Revert "cc: Subtract estimated display draw time from begin frame deadline"

This reverts commit fc04607a.

Reason for revert: Likely caused several regressions

Original change's description:
> 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}

TBR=sadrul@chromium.org,sunnyps@chromium.org,khushalsagar@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1042584, 1098444, 1098229
Change-Id: I3dcad61af5f1ef6d8a38a3290c6c2e04c503dbb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264629Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782068}

cc/scheduler/scheduler.cc

@@ -460,26 +460,28 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
   adjusted_args.deadline -= compositor_timing_history_->DrawDurationEstimate();
   adjusted_args.deadline -= 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;
+  // 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;
 
   // 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.)
-  const base::TimeDelta bmf_start_to_activate =
+  base::TimeDelta bmf_start_to_activate =
       compositor_timing_history_
           ->BeginMainFrameStartToReadyToCommitDurationEstimate() +
       compositor_timing_history_->CommitDurationEstimate() +
       compositor_timing_history_->CommitToReadyToActivateDurationEstimate() +
       compositor_timing_history_->ActivateDurationEstimate();
 
-  const base::TimeDelta bmf_to_activate_estimate_critical =
+  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 constexpr base::TimeDelta DefaultEstimatedDisplayDrawTime(
+  static base::TimeDelta DefaultEstimatedDisplayDrawTime(
       base::TimeDelta interval) {
     return interval * kDefaultEstimatedDisplayDrawTimeRatio;
   }

components/viz/common/frame_sinks/begin_frame_source.cc

@@ -102,7 +102,6 @@ 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 =
@@ -112,7 +111,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, deadline,
+                                sequence_number, frame_time, next_frame_time,
                                 vsync_interval, BeginFrameArgs::NORMAL);
 }
 
@@ -205,7 +204,8 @@ SyntheticBeginFrameSource::~SyntheticBeginFrameSource() = default;
 BackToBackBeginFrameSource::BackToBackBeginFrameSource(
     std::unique_ptr<DelayBasedTimeSource> time_source)
     : SyntheticBeginFrameSource(kNotRestartableId),
-      time_source_(std::move(time_source)) {
+      time_source_(std::move(time_source)),
+      next_sequence_number_(BeginFrameArgs::kStartingFrameNumber) {
   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,15 +250,12 @@ void BackToBackBeginFrameSource::OnGpuNoLongerBusy() {
 void BackToBackBeginFrameSource::OnTimerTick() {
   if (RequestCallbackOnGpuAvailable())
     return;
-  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);
+  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_++;
 
   // This must happen after getting the LastTickTime() from the time source.
   time_source_->SetActive(false);
@@ -296,12 +293,8 @@ 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, deadline, next_frame_time, interval);
+      source_id(), frame_time, time_source_->NextTickTime(), interval);
 }
 
 void DelayBasedBeginFrameSource::AddObserver(BeginFrameObserver* obs) {

components/viz/common/frame_sinks/begin_frame_source.h

@@ -137,7 +137,6 @@ 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);
 
@@ -283,10 +282,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);
@@ -316,11 +315,6 @@ 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,
@@ -337,7 +331,6 @@ 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,12 +59,8 @@ class TestTaskRunner : public base::TestMockTimeTaskRunner {
 // ------------------------------------------
 class BackToBackBeginFrameSourceTest : public ::testing::Test {
  protected:
-  static constexpr int64_t kInterval =
-      BeginFrameArgs::DefaultInterval().InMicroseconds();
-  static constexpr int64_t kDeadline =
-      kInterval - BeginFrameArgs::DefaultEstimatedDisplayDrawTime(
-                      base::TimeDelta::FromMicroseconds(kInterval))
-                      .InMicroseconds();
+  static const int64_t kDeadline;
+  static const int64_t kInterval;
 
   void SetUp() override {
     task_runner_ = base::MakeRefCounted<TestTaskRunner>();
@@ -85,6 +81,12 @@ 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());
@@ -351,8 +353,6 @@ 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,12 +360,10 @@ class DelayBasedBeginFrameSourceTest : public ::testing::Test {
             task_runner_->GetMockTickClock(), task_runner_.get());
 
     time_source->SetTimebaseAndInterval(
-        base::TimeTicks(), base::TimeDelta::FromMicroseconds(kInterval));
+        base::TimeTicks(), base::TimeDelta::FromMicroseconds(10000));
     source_ = std::make_unique<DelayBasedBeginFrameSource>(
         std::move(time_source), BeginFrameSource::kNotRestartableId);
-    // TODO(sunnyps): Apply deadline adjustment by default after fixing tests.
-    source_->set_apply_display_deadline_adjustment_for_testing(false);
-    obs_ = std::make_unique<MockBeginFrameObserver>();
+    obs_.reset(new MockBeginFrameObserver);
   }
 
   void TearDown() override { obs_.reset(); }
@@ -667,32 +665,6 @@ 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,6 +176,8 @@ 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.
-  AdvanceTimeAndBeginFrameForTest({root_surface_id2, sid1});
   late_deadline = now_src().NowTicks() + BeginFrameArgs::DefaultInterval();
+  AdvanceTimeAndBeginFrameForTest({root_surface_id2, sid1});
   SurfaceDamaged(sid1);
   EXPECT_GT(late_deadline, scheduler_.DesiredBeginFrameDeadlineTimeForTest());
   SurfaceDamaged(root_surface_id2);
@@ -734,7 +734,11 @@ TEST_F(DisplaySchedulerTest, DidSwapBuffers) {
   // Damage from previous BeginFrame should cary over, so don't damage again.
   scheduler_.DidReceiveSwapBuffersAck();
   AdvanceTimeAndBeginFrameForTest({sid2});
-  EXPECT_EQ(last_begin_frame_args_.deadline,
+  base::TimeTicks expected_deadline =
+      last_begin_frame_args_.deadline -
+      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(
+          last_begin_frame_args_.interval);
+  EXPECT_EQ(expected_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,13 +38,10 @@ void ExternalBeginFrameSourceAndroid::OnVSync(
   base::TimeDelta vsync_period(
       base::TimeDelta::FromMicroseconds(period_micros));
   // Calculate the next frame deadline:
-  base::TimeTicks next_frame_time = frame_time + vsync_period;
-  base::TimeTicks deadline =
-      next_frame_time -
-      BeginFrameArgs::DefaultEstimatedDisplayDrawTime(vsync_period);
+  base::TimeTicks deadline = frame_time + vsync_period;
 
   auto begin_frame_args = begin_frame_args_generator_.GenerateBeginFrameArgs(
-      source_id(), frame_time, deadline, next_frame_time, vsync_period);
+      source_id(), frame_time, deadline, vsync_period);
   OnBeginFrame(begin_frame_args);
 }
 

components/viz/service/frame_sinks/gpu_vsync_begin_frame_source.cc

@@ -36,12 +36,8 @@ 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, deadline, next_frame_time, vsync_interval);
+      source_id(), vsync_time, vsync_time + vsync_interval, vsync_interval);
   ExternalBeginFrameSource::OnBeginFrame(begin_frame_args);
 }
 
@@ -62,11 +58,8 @@ 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, deadline, next_frame_time, interval);
+        source_id(), frame_time, frame_time + interval, interval);
   }
 
   return ExternalBeginFrameSource::GetMissedBeginFrameArgs(obs);