Fix gradual deterioration of cadence based rendering.

When a cadence based rendering sequence exceeds drift, we should not
count the over-rendered frame against the next in sequence.  Doing
so causes an accumulation of errors.

Also extends the moving average for durations to 32 values and the
allowable drift to 16.66ms to stabilize the Catzilla 60fps and
Elite: Dangerous 60fps clips, which were hitting hysteresis very
frequently.

BUG=439548
TEST=new unittest.

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

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

media/filters/video_renderer_algorithm.cc

@@ -11,7 +11,7 @@ namespace media {
 
 // The number of frames to store for moving average calculations.  Value picked
 // after experimenting with playback of various local media and YouTube clips.
-const int kMovingAverageSamples = 25;
+const int kMovingAverageSamples = 32;
 
 VideoRendererAlgorithm::ReadyFrame::ReadyFrame(
     const scoped_refptr<VideoFrame>& ready_frame)
@@ -94,12 +94,11 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
   base::TimeDelta selected_frame_drift;
 
   // Step 4: Attempt to find the best frame by cadence.
-  bool was_frame_selected_by_cadence = false;
   int cadence_overage = 0;
-  int frame_to_render =
+  const int cadence_frame =
       FindBestFrameByCadence(first_frame_ ? nullptr : &cadence_overage);
+  int frame_to_render = cadence_frame;
   if (frame_to_render >= 0) {
-    was_frame_selected_by_cadence = true;
     selected_frame_drift =
         CalculateAbsoluteDriftForFrame(deadline_min, frame_to_render);
   }
@@ -123,12 +122,6 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
     }
 
     if (frame_to_render >= 0) {
-      if (was_frame_selected_by_cadence) {
-        cadence_overage = 0;
-        was_frame_selected_by_cadence = false;
-        DVLOG(2) << "Overriding frame selected by cadence because of drift: "
-                 << selected_frame_drift;
-      }
       selected_frame_drift =
           CalculateAbsoluteDriftForFrame(deadline_min, frame_to_render);
     }
@@ -138,12 +131,14 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
   // least crappy option based on the drift from the deadline. If we're here the
   // selection is going to be bad because it means no suitable frame has any
   // coverage of the deadline interval.
-  if (frame_to_render < 0 || selected_frame_drift > max_acceptable_drift_) {
-    if (was_frame_selected_by_cadence) {
-      cadence_overage = 0;
-      was_frame_selected_by_cadence = false;
-    }
+  if (frame_to_render < 0 || selected_frame_drift > max_acceptable_drift_)
     frame_to_render = FindBestFrameByDrift(deadline_min, &selected_frame_drift);
+
+  const bool ignored_cadence_frame =
+      cadence_frame >= 0 && frame_to_render != cadence_frame;
+  if (ignored_cadence_frame) {
+    cadence_overage = 0;
+    DVLOG(2) << "Cadence frame overridden by drift: " << selected_frame_drift;
   }
 
   last_render_had_glitch_ = selected_frame_drift > max_acceptable_drift_;
@@ -226,12 +221,18 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
   // start time is at most |render_interval_| / 2 before |deadline_min|.
   if (!first_frame_ ||
       deadline_min >= frame_queue_.front().start_time - render_interval_ / 2) {
+    // Ignore one frame of overage if the last call to Render() ignored the
+    // frame selected by cadence due to drift.
+    if (last_render_ignored_cadence_frame_ && cadence_overage > 0)
+      cadence_overage -= 1;
+
+    last_render_ignored_cadence_frame_ = ignored_cadence_frame;
     frame_queue_.front().render_count += cadence_overage + 1;
     frame_queue_.front().drop_count += cadence_overage;
 
     // Once we reach a glitch in our cadence sequence, reset the base frame
     // number used for defining the cadence sequence.
-    if (!was_frame_selected_by_cadence && cadence_estimator_.has_cadence()) {
+    if (ignored_cadence_frame) {
       cadence_frame_counter_ = 0;
       UpdateCadenceForFrames();
     }
@@ -306,6 +307,7 @@ void VideoRendererAlgorithm::Reset() {
   frame_duration_calculator_.Reset();
   first_frame_ = true;
   cadence_frame_counter_ = 0;
+  last_render_ignored_cadence_frame_ = false;
 
   // Default to ATSC IS/191 recommendations for maximum acceptable drift before
   // we have enough frames to base the maximum on frame duration.
@@ -431,8 +433,8 @@ void VideoRendererAlgorithm::AccountForMissedIntervals(
   // If the frame was never really rendered since it was dropped each attempt,
   // we need to increase the drop count as well to match the new render count.
   // Otherwise we won't properly count the frame as dropped when it's discarded.
-  // We always update the render count so FindBestFrameByCadenceInternal() can
-  // properly account for potentially over-rendered frames.
+  // We always update the render count so FindBestFrameByCadence() can properly
+  // account for potentially over-rendered frames.
   if (ready_frame.render_count == ready_frame.drop_count)
     ready_frame.drop_count += render_cycle_count;
   ready_frame.render_count += render_cycle_count;
@@ -480,10 +482,11 @@ bool VideoRendererAlgorithm::UpdateFrameStatistics() {
   // duration; there are other asymmetric, more lenient measures, that we're
   // forgoing in favor of simplicity.
   //
-  // We'll always allow at least 8.33ms of drift since literature suggests it's
-  // well below the floor of detection.
+  // We'll always allow at least 16.66ms of drift since literature suggests it's
+  // well below the floor of detection and is high enough to ensure stability
+  // for 60fps content.
   max_acceptable_drift_ = std::max(average_frame_duration_ / 2,
-                                   base::TimeDelta::FromSecondsD(1.0 / 120));
+                                   base::TimeDelta::FromSecondsD(1.0 / 60));
 
   // If we were called via RemoveExpiredFrames() and Render() was never called,
   // we may not have a render interval yet.

media/filters/video_renderer_algorithm.h

@@ -300,6 +300,10 @@ class MEDIA_EXPORT VideoRendererAlgorithm {
   // ReadyFrameQueue.  Cleared when a new cadence is detected.
   uint64_t cadence_frame_counter_;
 
+  // Tracks whether the last call to Render() choose to ignore the frame chosen
+  // by cadence in favor of one by drift or coverage.
+  bool last_render_ignored_cadence_frame_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoRendererAlgorithm);
 };
 

media/filters/video_renderer_algorithm_unittest.cc

@@ -732,6 +732,55 @@ TEST_F(VideoRendererAlgorithmTest, BestFrameByCadenceOverdisplayed) {
   ASSERT_EQ(2, GetCurrentFrameIdealDisplayCount());
 }
 
+TEST_F(VideoRendererAlgorithmTest, BestFrameByCadenceOverdisplayedForDrift) {
+  // Use 24.94 to ensure drift expires pretty rapidly (8.36s in this case).
+  TickGenerator frame_tg(base::TimeTicks(), 24.94);
+  TickGenerator display_tg(tick_clock_->NowTicks(), 50);
+  time_source_.StartTicking();
+  disable_cadence_hysteresis();
+
+  scoped_refptr<VideoFrame> last_frame;
+  bool have_overdisplayed_frame = false;
+  while (!have_overdisplayed_frame) {
+    while (algorithm_.EffectiveFramesQueued() < 2) {
+      algorithm_.EnqueueFrame(
+          CreateFrame(frame_tg.current() - base::TimeTicks()));
+      frame_tg.step();
+    }
+
+    size_t frames_dropped = 0;
+    last_frame = RenderAndStep(&display_tg, &frames_dropped);
+    ASSERT_TRUE(last_frame);
+    ASSERT_TRUE(is_using_cadence());
+    ASSERT_EQ(0u, frames_dropped);
+    ASSERT_EQ(2, GetCurrentFrameIdealDisplayCount());
+    have_overdisplayed_frame = GetCurrentFrameDisplayCount() > 2;
+  }
+
+  ASSERT_TRUE(last_render_had_glitch());
+
+  // We've reached the point where the current frame is over displayed due to
+  // drift, the next frame should resume cadence without accounting for the
+  // overdisplayed frame.
+
+  size_t frames_dropped = 0;
+  scoped_refptr<VideoFrame> next_frame =
+      RenderAndStep(&display_tg, &frames_dropped);
+  ASSERT_EQ(0u, frames_dropped);
+  ASSERT_NE(last_frame, next_frame);
+  ASSERT_TRUE(is_using_cadence());
+  ASSERT_EQ(2, GetCurrentFrameIdealDisplayCount());
+  ASSERT_EQ(1, GetCurrentFrameDisplayCount());
+  last_frame = next_frame;
+
+  next_frame = RenderAndStep(&display_tg, &frames_dropped);
+  ASSERT_EQ(0u, frames_dropped);
+  ASSERT_EQ(last_frame, next_frame);
+  ASSERT_TRUE(is_using_cadence());
+  ASSERT_EQ(2, GetCurrentFrameIdealDisplayCount());
+  ASSERT_EQ(2, GetCurrentFrameDisplayCount());
+}
+
 TEST_F(VideoRendererAlgorithmTest, BestFrameByCoverage) {
   TickGenerator tg(tick_clock_->NowTicks(), 50);
   time_source_.StartTicking();
@@ -984,7 +1033,7 @@ TEST_F(VideoRendererAlgorithmTest, RemoveExpiredFrames) {
   // Advance expiry enough that one frame is removed, but one remains and is
   // still counted as effective.
   ASSERT_EQ(
-      1u, algorithm_.RemoveExpiredFrames(tg.current() + tg.interval(1) * 0.75));
+      1u, algorithm_.RemoveExpiredFrames(tg.current() + tg.interval(1) * 0.9));
   EXPECT_EQ(1u, frames_queued());
   EXPECT_EQ(1u, algorithm_.EffectiveFramesQueued());