Don't carry over frame overage unless frame is selected by cadence.

The overage values should only be applied if the frame is actually
chosen by cadence.  This is most wrong when there is video at some
non zero time and audio at time zero.  If cadence is present the
first frame will have an obscene display count which carries over
and forever wrecks cadence :(

We should also not track render counts until the first frame has
reached its display period.

BUG=439548
TEST=new unittest

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

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

media/filters/video_renderer_algorithm.cc

@@ -94,7 +94,9 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
   base::TimeDelta selected_frame_drift;
 
   // Step 4: Attempt to find the best frame by cadence.
-  int frame_to_render = FindBestFrameByCadence();
+  int cadence_overage = 0;
+  int frame_to_render =
+      FindBestFrameByCadence(first_frame_ ? nullptr : &cadence_overage);
   if (frame_to_render >= 0) {
     selected_frame_drift =
         CalculateAbsoluteDriftForFrame(deadline_min, frame_to_render);
@@ -119,6 +121,7 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
     }
 
     if (frame_to_render >= 0) {
+      cadence_overage = 0;
       selected_frame_drift =
           CalculateAbsoluteDriftForFrame(deadline_min, frame_to_render);
     }
@@ -128,8 +131,10 @@ 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 (frame_to_render < 0 || selected_frame_drift > max_acceptable_drift_) {
+    cadence_overage = 0;
     frame_to_render = FindBestFrameByDrift(deadline_min, &selected_frame_drift);
+  }
 
   last_render_had_glitch_ = selected_frame_drift > max_acceptable_drift_;
   DVLOG_IF(2, last_render_had_glitch_)
@@ -197,7 +202,22 @@ scoped_refptr<VideoFrame> VideoRendererAlgorithm::Render(
 
   // Step 8: Congratulations, the frame selection gauntlet has been passed!
   last_frame_index_ = 0;
-  ++frame_queue_.front().render_count;
+
+  // If we ended up choosing a frame selected by cadence, carry over the overage
+  // values from the previous frame.  Overage is treated as having been
+  // displayed and dropped for each count.  If the frame wasn't selected by
+  // cadence, |cadence_overage| will be zero.
+  //
+  // We also don't want to start counting render counts until the first frame
+  // has reached its presentation time; which is considered to be when its
+  // start time is at most |render_interval_| / 2 before |deadline_min|.
+  if (!first_frame_ ||
+      deadline_min >= frame_queue_.front().start_time - render_interval_ / 2) {
+    frame_queue_.front().render_count += cadence_overage + 1;
+    frame_queue_.front().drop_count += cadence_overage;
+    first_frame_ = false;
+  }
+
   DCHECK(frame_queue_.front().frame);
   return frame_queue_.front().frame;
 }
@@ -261,6 +281,7 @@ void VideoRendererAlgorithm::Reset() {
   frame_queue_.clear();
   cadence_estimator_.Reset();
   frame_duration_calculator_.Reset();
+  first_frame_ = true;
 
   // Default to ATSC IS/191 recommendations for maximum acceptable drift before
   // we have enough frames to base the maximum on frame duration.
@@ -287,7 +308,7 @@ size_t VideoRendererAlgorithm::EffectiveFramesQueued() const {
   }
 
   // Find the first usable frame to start counting from.
-  const int start_index = FindBestFrameByCadenceInternal(nullptr);
+  const int start_index = FindBestFrameByCadence(nullptr);
   if (start_index < 0)
     return 0;
 
@@ -473,27 +494,12 @@ void VideoRendererAlgorithm::UpdateCadenceForFrames() {
   }
 }
 
-int VideoRendererAlgorithm::FindBestFrameByCadence() {
+int VideoRendererAlgorithm::FindBestFrameByCadence(
+    int* remaining_overage) const {
   DCHECK(!frame_queue_.empty());
   if (!cadence_estimator_.has_cadence())
     return -1;
 
-  int remaining_overage = 0;
-  const int best_frame =
-      FindBestFrameByCadenceInternal(&remaining_overage);
-  if (best_frame < 0)
-    return -1;
-
-  DCHECK_GE(remaining_overage, 0);
-
-  // Overage is treated as having been displayed and dropped for each count.
-  frame_queue_[best_frame].render_count += remaining_overage;
-  frame_queue_[best_frame].drop_count += remaining_overage;
-  return best_frame;
-}
-
-int VideoRendererAlgorithm::FindBestFrameByCadenceInternal(
-    int* remaining_overage) const {
   DCHECK(!frame_queue_.empty());
   DCHECK(cadence_estimator_.has_cadence());
   const ReadyFrame& current_frame = frame_queue_[last_frame_index_];

media/filters/video_renderer_algorithm.h

@@ -194,20 +194,15 @@ class MEDIA_EXPORT VideoRendererAlgorithm {
 
   // If |cadence_estimator_| has detected a valid cadence, attempts to find the
   // next frame which should be rendered.  Returns -1 if not enough frames are
-  // available for cadence selection or there is no cadence.  Will adjust the
-  // selected frame's ideal render count if the last rendered frame has been
-  // over selected.
-  int FindBestFrameByCadence();
-
-  // Similar to FindBestFrameByCadence(), but instead of adjusting the last
-  // rendered frame's ideal render count in the case of over selection.
-  // Optionally returns the number of times a prior frame was over displayed and
-  // ate into the returned frames ideal render count via |remaining_overage|.
+  // available for cadence selection or there is no cadence.
+  //
+  // Returns the number of times a prior frame was over displayed and ate into
+  // the returned frames ideal render count via |remaining_overage|.
   //
   // For example, if we have 2 frames and each has an ideal display count of 3,
   // but the first was displayed 4 times, the best frame is the second one, but
   // it should only be displayed twice instead of thrice, so it's overage is 1.
-  int FindBestFrameByCadenceInternal(int* remaining_overage) const;
+  int FindBestFrameByCadence(int* remaining_overage) const;
 
   // Iterates over |frame_queue_| and finds the frame which covers the most of
   // the deadline interval.  If multiple frames have coverage of the interval,
@@ -293,6 +288,10 @@ class MEDIA_EXPORT VideoRendererAlgorithm {
   // to the queue.  Callers are told about these frames during Render().
   size_t frames_dropped_during_enqueue_;
 
+  // When cadence is present, we don't want to start counting against cadence
+  // until the first frame has reached its presentation time.
+  bool first_frame_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoRendererAlgorithm);
 };
 

media/filters/video_renderer_algorithm_unittest.cc

@@ -1165,4 +1165,51 @@ TEST_F(VideoRendererAlgorithmTest, EnqueueFrames) {
   EXPECT_EQ(1u, frames_dropped);
 }
 
+TEST_F(VideoRendererAlgorithmTest, CadenceForFutureFrames) {
+  TickGenerator tg(base::TimeTicks(), 50);
+  time_source_.StartTicking();
+
+  disable_cadence_hysteresis();
+
+  algorithm_.EnqueueFrame(CreateFrame(tg.interval(10)));
+  algorithm_.EnqueueFrame(CreateFrame(tg.interval(11)));
+  algorithm_.EnqueueFrame(CreateFrame(tg.interval(12)));
+  EXPECT_EQ(3u, frames_queued());
+
+  // Call Render() a few times to increment the render count.
+  for (int i = 0; i < 10; ++i) {
+    size_t frames_dropped = 0;
+    scoped_refptr<VideoFrame> rendered_frame =
+        RenderAndStep(&tg, &frames_dropped);
+    EXPECT_EQ(3u, frames_queued());
+    EXPECT_EQ(tg.interval(10), rendered_frame->timestamp());
+    ASSERT_TRUE(is_using_cadence());
+  }
+
+  // Add some noise to the tick generator so it our first frame
+  // doesn't line up evenly on a deadline.
+  tg.Reset(tg.current() + base::TimeDelta::FromMilliseconds(5));
+
+  // We're now at the first frame, cadence should be one, so
+  // it should only be displayed once.
+  size_t frames_dropped = 0;
+  scoped_refptr<VideoFrame> rendered_frame =
+      RenderAndStep(&tg, &frames_dropped);
+  EXPECT_EQ(3u, frames_queued());
+  EXPECT_EQ(tg.interval(10), rendered_frame->timestamp());
+  ASSERT_TRUE(is_using_cadence());
+
+  // Then the next frame should be displayed.
+  rendered_frame = RenderAndStep(&tg, &frames_dropped);
+  EXPECT_EQ(2u, frames_queued());
+  EXPECT_EQ(tg.interval(11), rendered_frame->timestamp());
+  ASSERT_TRUE(is_using_cadence());
+
+  // Finally the last frame.
+  rendered_frame = RenderAndStep(&tg, &frames_dropped);
+  EXPECT_EQ(1u, frames_queued());
+  EXPECT_EQ(tg.interval(12), rendered_frame->timestamp());
+  ASSERT_TRUE(is_using_cadence());
+}
+
 }  // namespace media