Snap sequence numbers based on vsync

Sequence numbers are now incremented based on vsync intervals. If two
consecutive BeginFrameArgs are created 5 ticks apart, then their
sequence numbers will differ by 5 instead of 1.


Doc: https://docs.google.com/document/d/1rvC3iBqL41R2dg_QN92yAFxU9ic6Irk89-9LBlyBJ_U/edit?usp=sharing

Change-Id: I8b3d8614079cc2ef44f8fad6420314a97b12f260
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1538540
Commit-Queue: Kaan Alsan <alsan@google.com>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652847}

components/viz/common/frame_sinks/begin_frame_source.cc

@@ -23,6 +23,11 @@ namespace {
 // often to an observer.
 constexpr double kDoubleTickDivisor = 2.0;
 
+// kErrorMarginIntervalPct used to determine what percentage of the time tick
+// interval should be used as a margin of error when comparing times to
+// deadlines.
+constexpr double kErrorMarginIntervalPct = 0.05;
+
 base::AtomicSequenceNumber g_next_source_id;
 
 // Generates a source_id with upper 32 bits from |restart_id| and lower 32 bits
@@ -244,11 +249,30 @@ void DelayBasedBeginFrameSource::OnUpdateVSyncParameters(
 
 BeginFrameArgs DelayBasedBeginFrameSource::CreateBeginFrameArgs(
     base::TimeTicks frame_time) {
-  uint64_t sequence_number = next_sequence_number_++;
+  base::TimeDelta interval = time_source_->Interval();
+  uint64_t sequence_number = next_sequence_number_;
+
+  base::TimeDelta error_margin = interval * kErrorMarginIntervalPct;
+
+  // We expect |sequence_number| to be the number for the frame at
+  // |expected_frame_time|. We adjust this sequence number according to the
+  // actual frame time in case it is later than expected.
+  if (next_expected_frame_time_ != base::TimeTicks()) {
+    // Add |error_margin| to round |frame_time| up to the next tick if it is
+    // close to the end of an interval. This happens when a timebase is a bit
+    // off because of an imperfect presentation timestamp that may be a bit
+    // later than the beginning of the next interval.
+    int ticks_since_estimated_frame_time =
+        (frame_time + error_margin - next_expected_frame_time_) / interval;
+    sequence_number += std::max(0, ticks_since_estimated_frame_time);
+  }
+
+  next_expected_frame_time_ = time_source_->NextTickTime();
+  next_sequence_number_ = sequence_number + 1;
+
   return BeginFrameArgs::Create(
       BEGINFRAME_FROM_HERE, source_id(), sequence_number, frame_time,
-      time_source_->NextTickTime(), time_source_->Interval(),
-      BeginFrameArgs::NORMAL);
+      time_source_->NextTickTime(), interval, BeginFrameArgs::NORMAL);
 }
 
 void DelayBasedBeginFrameSource::AddObserver(BeginFrameObserver* obs) {

components/viz/common/frame_sinks/begin_frame_source.h

@@ -264,6 +264,11 @@ class VIZ_COMMON_EXPORT DelayBasedBeginFrameSource
   void OnTimerTick() override;
 
  private:
+  // The created BeginFrameArgs' sequence_number is calculated based on what
+  // interval |frame_time| is in. For example, if |last_frame_time_| is 100,
+  // |next_sequence_number_| is 5, |last_timebase_| is 110 and the interval is
+  // 20, then a |frame_time| of 175 would result in the sequence number being 8
+  // (3 intervals since 110).
   BeginFrameArgs CreateBeginFrameArgs(base::TimeTicks frame_time);
   void IssueBeginFrameToObserver(BeginFrameObserver* obs,
                                  const BeginFrameArgs& args);
@@ -272,6 +277,16 @@ class VIZ_COMMON_EXPORT DelayBasedBeginFrameSource
   base::flat_set<BeginFrameObserver*> observers_;
   base::TimeTicks last_timebase_;
   BeginFrameArgs last_begin_frame_args_;
+
+  // Used for determining what the sequence number should be on
+  // CreateBeginFrameArgs.
+  base::TimeTicks next_expected_frame_time_;
+
+  // This is what the sequence number should be for any args created between
+  // |next_expected_frame_time_| to |next_expected_frame_time_| + vsync
+  // interval. Args created outside of this range will have their sequence
+  // number assigned relative to this, based on how many intervals the frame
+  // time is off.
   uint64_t next_sequence_number_;
 
   DISALLOW_COPY_AND_ASSIGN(DelayBasedBeginFrameSource);

components/viz/common/frame_sinks/begin_frame_source_unittest.cc

@@ -431,6 +431,105 @@ TEST_F(DelayBasedBeginFrameSourceTest, VSyncChanges) {
   task_runner_->FastForwardTo(TicksFromMicroseconds(60000));
 }
 
+TEST_F(DelayBasedBeginFrameSourceTest, VSyncChangeTimebaseBeforeLastTick) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(*obs_, source_->source_id(), 1, 0, 10000,
+                                 10000);
+  source_->AddObserver(obs_.get());
+
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 2, 10000, 20000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 3, 20000, 30000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 4, 30000, 40000, 10000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(30000));
+
+  // Update the vsync information such that timebase is before last tick time,
+  // and next tick happens within less than the new interval of the following
+  // tick (i.e. next_tick -> 40000, following_tick -> 41000)
+  // Begin frame won't be used at 41000 because this is a double-tick.
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(26000),
+                                   base::TimeDelta::FromMicroseconds(5000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 5, 40000, 41000, 5000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 7, 46000, 51000, 5000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(46000));
+
+  // Update the vsync information such that timebase is before last tick time,
+  // and next tick happens exactly one interval before the following tick
+  // tick (i.e. next_tick -> 51000, following_tick -> 60000)
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(42000),
+                                   base::TimeDelta::FromMicroseconds(9000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 8, 51000, 60000, 9000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 9, 60000, 69000, 9000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 10, 69000, 78000, 9000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(70000));
+}
+
+TEST_F(DelayBasedBeginFrameSourceTest, VSyncChangeTimebaseAfterNextTick) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(*obs_, source_->source_id(), 1, 0, 10000,
+                                 10000);
+  source_->AddObserver(obs_.get());
+
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 2, 10000, 20000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 3, 20000, 30000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 4, 30000, 40000, 10000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(30000));
+
+  // Update the vsync information such that timebase is after next tick time,
+  // and next tick happens within less than one interval of the new timebase
+  // Begin frame won't be used at 41000 because this is a double-tick.
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(41000),
+                                   base::TimeDelta::FromMicroseconds(5000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 5, 40000, 41000, 5000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 7, 46000, 51000, 5000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(46000));
+
+  // Update the vsync information such that timebase is after next tick time,
+  // and next tick happens exactly one interval before the new timebase
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(60000),
+                                   base::TimeDelta::FromMicroseconds(9000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 8, 51000, 60000, 9000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 9, 60000, 69000, 9000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 10, 69000, 78000, 9000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(70000));
+
+  // Update the vsync information such that timebase is after next tick time,
+  // and next tick happens more than one interval before the new timebase
+  // Begin frame won't be used at 80000 because this is a double-tick.
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(100000),
+                                   base::TimeDelta::FromMicroseconds(5000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 11, 78000, 80000, 5000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 13, 85000, 90000, 5000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(85000));
+}
+
+TEST_F(DelayBasedBeginFrameSourceTest, VSyncChangeTimebaseBetweenTicks) {
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(*obs_, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(*obs_, source_->source_id(), 1, 0, 10000,
+                                 10000);
+  source_->AddObserver(obs_.get());
+
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 2, 10000, 20000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 3, 20000, 30000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 4, 30000, 40000, 10000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(30000));
+
+  // Update the vsync information such that timebase is between next tick time,
+  // and last tick time.
+  // Begin frame won't be used at 41000 because this is a double-tick.
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(35000),
+                                   base::TimeDelta::FromMicroseconds(6000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 5, 40000, 41000, 6000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 7, 47000, 53000, 6000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(47000));
+
+  source_->OnUpdateVSyncParameters(TicksFromMicroseconds(49000),
+                                   base::TimeDelta::FromMicroseconds(10000));
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 8, 53000, 59000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 9, 59000, 69000, 10000);
+  EXPECT_BEGIN_FRAME_USED(*obs_, source_->source_id(), 10, 69000, 79000, 10000);
+  task_runner_->FastForwardTo(TicksFromMicroseconds(70000));
+}
+
 TEST_F(DelayBasedBeginFrameSourceTest, MultipleObservers) {
   NiceMock<MockBeginFrameObserver> obs1, obs2;
 
@@ -519,6 +618,52 @@ TEST_F(DelayBasedBeginFrameSourceTest, DoubleTickMissedFrame) {
   source_->RemoveObserver(&obs);
 }
 
+TEST_F(DelayBasedBeginFrameSourceTest, MultipleArgsInSameInterval) {
+  NiceMock<MockBeginFrameObserver> obs;
+  NiceMock<MockBeginFrameObserver> obs2;
+
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(obs, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(obs, source_->source_id(), 1, 0, 10000, 10000);
+  source_->AddObserver(&obs);
+  task_runner_->RunUntilIdle();
+
+  EXPECT_BEGIN_FRAME_USED(obs, source_->source_id(), 2, 10000, 20000, 10000);
+  task_runner_->AdvanceMockTickClock(base::TimeDelta::FromMicroseconds(9000));
+  task_runner_->RunUntilIdle();
+
+  // Sequence number should stay the same within same interval.
+  EXPECT_BEGIN_FRAME_USED_MISSED(obs2, source_->source_id(), 2, 10000, 20000,
+                                 10000);
+  source_->AddObserver(&obs2);
+
+  EXPECT_BEGIN_FRAME_USED(obs, source_->source_id(), 3, 20000, 30000, 10000);
+  EXPECT_BEGIN_FRAME_USED(obs2, source_->source_id(), 3, 20000, 30000, 10000);
+  task_runner_->AdvanceMockTickClock(base::TimeDelta::FromMicroseconds(10000));
+  task_runner_->RunUntilIdle();
+}
+
+TEST_F(DelayBasedBeginFrameSourceTest, ConsecutiveArgsDelayedByMultipleVsyncs) {
+  NiceMock<MockBeginFrameObserver> obs;
+
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(obs, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(obs, source_->source_id(), 1, 0, 10000, 10000);
+  source_->AddObserver(&obs);
+  task_runner_->RunUntilIdle();
+
+  EXPECT_BEGIN_FRAME_USED(obs, source_->source_id(), 2, 10000, 20000, 10000);
+  task_runner_->AdvanceMockTickClock(base::TimeDelta::FromMicroseconds(9000));
+  task_runner_->RunUntilIdle();
+  source_->RemoveObserver(&obs);
+
+  // New args created 8 intervals later.
+  // Sequence number should increase bt this much.
+  EXPECT_BEGIN_FRAME_SOURCE_PAUSED(obs, false);
+  EXPECT_BEGIN_FRAME_USED_MISSED(obs, source_->source_id(), 10, 90000, 100000,
+                                 10000);
+  task_runner_->AdvanceMockTickClock(base::TimeDelta::FromMicroseconds(80000));
+  source_->AddObserver(&obs);
+}
+
 // ExternalBeginFrameSource testing
 // --------------------------------------------
 class MockExternalBeginFrameSourceClient