Fix VideoRendererBase end of stream logic.

media/filters/video_renderer_base_unittest.cc

Index: media/filters/video_renderer_base_unittest.cc
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index d08670ebf347b749becafe58194f0e3d7e9ae202..1f41d9819737eed50aefee96b704c76149c0bf51 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -43,6 +43,7 @@ class VideoRendererBaseTest : public ::testing::Test {
         event_(false, false),
         timeout_(TestTimeouts::action_timeout()),
         prerolling_(false),
+        next_frame_timestamp_(0),
         paint_cv_(&lock_),
         paint_was_called_(false),
         should_queue_read_cb_(false) {
@@ -52,8 +53,6 @@ class VideoRendererBaseTest : public ::testing::Test {
         true);
 
     // We expect these to be called but we don't care how/when.
-    EXPECT_CALL(*decoder_, natural_size())
-        .WillRepeatedly(ReturnRef(kNaturalSize));
     EXPECT_CALL(*decoder_, Stop(_))
         .WillRepeatedly(RunClosure());
     EXPECT_CALL(statistics_cb_object_, OnStatistics(_))
@@ -82,6 +81,12 @@ class VideoRendererBaseTest : public ::testing::Test {
   MOCK_METHOD1(OnError, void(PipelineStatus));
 
   void Initialize() {
+    Initialize(kVideoDuration);
+  }
+
+  void Initialize(int64 duration) {
+    duration_ = duration;
+
     // TODO(scherkus): really, really, really need to inject a thread into
     // VideoRendererBase... it makes mocking much harder.
 
@@ -138,8 +143,9 @@ class VideoRendererBaseTest : public ::testing::Test {
   void StartPrerolling(int64 timestamp, PipelineStatus expected_status) {
     EXPECT_FALSE(prerolling_);
 
+    next_frame_timestamp_ = 0;
     prerolling_ = true;
-    renderer_->Preroll(base::TimeDelta::FromMicroseconds(timestamp),
+    renderer_->Preroll(base::TimeDelta::FromMilliseconds(timestamp),
                        base::Bind(&VideoRendererBaseTest::OnPrerollComplete,
                                   base::Unretained(this), expected_status));
   }
@@ -155,8 +161,10 @@ class VideoRendererBaseTest : public ::testing::Test {
   // Use |kEndOfStream| to preroll end of stream frames.
   void Preroll(int64 timestamp) {
     SCOPED_TRACE(base::StringPrintf("Preroll(%" PRId64 ")", timestamp));
-    StartPrerolling(timestamp, PIPELINE_OK);
-    FinishPrerolling(timestamp);
+    bool end_of_stream = (timestamp == kEndOfStream);
+    int preroll_timestamp = end_of_stream ? 0 : timestamp;

[DaleCurtis, 2012/08/07 20:32:14]

You're down converting from int64->int; did you mean to do this? Here and
elsewhere in the tests below.

[acolwell GONE FROM CHROMIUM, 2012/08/07 21:10:28]

Changed all int64s to int since int64 is overkill for these tests.

+    StartPrerolling(preroll_timestamp, PIPELINE_OK);
+    FinishPrerolling(end_of_stream);
   }
 
   void Pause() {
@@ -183,18 +191,27 @@ class VideoRendererBaseTest : public ::testing::Test {
     Stop();
   }
 
-  // Delivers a frame with the given timestamp to the video renderer.
-  //
-  // Use |kEndOfStream| to pass in an end of stream frame.
-  void DeliverFrame(int64 timestamp) {
-    // Lock+swap to avoid re-entrancy issues.
+  void DeliverNextFrame(bool end_of_stream) {
+    base::AutoLock l(lock_);
+    DeliverNextFrame_Locked(end_of_stream);
+  }
+
+  // Delivers the next frame to the video renderer. If |end_of_stream|
+  // is true then an "end or stream" frame will be returned. Otherwise
+  // A frame with |next_frame_timestamp_| will be returned.
+  void DeliverNextFrame_Locked(bool end_of_stream) {
+    lock_.AssertAcquired();
+
     VideoDecoder::ReadCB read_cb;
-    {
-      base::AutoLock l(lock_);
-      std::swap(read_cb, read_cb_);
-    }
+    std::swap(read_cb, read_cb_);
+
+    DCHECK_LT(next_frame_timestamp_, duration_);
+    int64 timestamp = next_frame_timestamp_;
+    next_frame_timestamp_ += kFrameDuration;
 
-    if (timestamp == kEndOfStream) {
+    // Unlock to deliver the frame to avoid re-entrancy issues.
+    base::AutoUnlock ul(lock_);
+    if (end_of_stream) {
       read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
     } else {
       read_cb.Run(VideoDecoder::kOk, CreateFrame(timestamp));
@@ -237,7 +254,7 @@ class VideoRendererBaseTest : public ::testing::Test {
   void ExpectCurrentTimestamp(int64 timestamp) {
     scoped_refptr<VideoFrame> frame;
     renderer_->GetCurrentFrame(&frame);
-    EXPECT_EQ(timestamp, frame->GetTimestamp().InMicroseconds());
+    EXPECT_EQ(timestamp, frame->GetTimestamp().InMilliseconds());
     renderer_->PutCurrentFrame(frame);
   }
 
@@ -254,7 +271,7 @@ class VideoRendererBaseTest : public ::testing::Test {
   scoped_refptr<VideoFrame> CreateFrame(int64 timestamp) {
     scoped_refptr<VideoFrame> frame =
         VideoFrame::CreateFrame(VideoFrame::RGB32, kNaturalSize, kNaturalSize,
-                                base::TimeDelta::FromMicroseconds(timestamp));
+                                base::TimeDelta::FromMilliseconds(timestamp));
     return frame;
   }
 
@@ -262,7 +279,7 @@ class VideoRendererBaseTest : public ::testing::Test {
   // rendered using |read_cb_| as the signal that the frame has rendered.
   void RenderFrame(int64 timestamp) {
     base::AutoLock l(lock_);
-    time_ = base::TimeDelta::FromMicroseconds(timestamp);
+    time_ = base::TimeDelta::FromMilliseconds(timestamp);
     paint_was_called_ = false;
     if (read_cb_.is_null()) {
       cv_.TimedWait(timeout_);
@@ -278,7 +295,7 @@ class VideoRendererBaseTest : public ::testing::Test {
         .WillOnce(Invoke(&event_, &base::WaitableEvent::Signal));
     {
       base::AutoLock l(lock_);
-      time_ = base::TimeDelta::FromMicroseconds(timestamp);
+      time_ = base::TimeDelta::FromMilliseconds(timestamp);
     }
     CHECK(event_.TimedWait(timeout_)) << "Timed out waiting for ended signal.";
   }
@@ -308,7 +325,7 @@ class VideoRendererBaseTest : public ::testing::Test {
   }
 
   base::TimeDelta GetDuration() {
-    return base::TimeDelta::FromMicroseconds(kVideoDuration);
+    return base::TimeDelta::FromMilliseconds(duration_);
   }
 
   // Called by VideoRendererBase when it wants a frame.
@@ -348,28 +365,16 @@ class VideoRendererBaseTest : public ::testing::Test {
     cv_.Signal();
   }
 
-  void FinishPrerolling(int64 timestamp) {
+  void FinishPrerolling(bool end_of_stream) {
     // Satisfy the read requests.  The callback must be executed in order
     // to exit the loop since VideoRendererBase can read a few extra frames
     // after |timestamp| in order to preroll.
     base::AutoLock l(lock_);
     EXPECT_TRUE(prerolling_);
     paint_was_called_ = false;
-    int i = 0;
     while (prerolling_) {
       if (!read_cb_.is_null()) {
-        VideoDecoder::ReadCB read_cb;
-        std::swap(read_cb, read_cb_);
-
-        // Unlock to deliver the frame to avoid re-entrancy issues.
-        base::AutoUnlock ul(lock_);
-        if (timestamp == kEndOfStream) {
-          read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
-        } else {
-          read_cb.Run(VideoDecoder::kOk,
-                      CreateFrame(i * kFrameDuration));
-          i++;
-        }
+        DeliverNextFrame_Locked(end_of_stream);
       } else {
         // We want to wait iff we're still prerolling but have no pending read.
         cv_.TimedWait(timeout_);
@@ -404,6 +409,8 @@ class VideoRendererBaseTest : public ::testing::Test {
   // Used in conjunction with |lock_| and |cv_| for satisfying reads.
   bool prerolling_;
   VideoDecoder::ReadCB read_cb_;
+  int64 next_frame_timestamp_;
+  int64 duration_;
   base::TimeDelta time_;
 
   // Used in conjunction with |lock_| to wait for Paint() calls.
@@ -429,18 +436,53 @@ TEST_F(VideoRendererBaseTest, Play) {
   Shutdown();
 }
 
-TEST_F(VideoRendererBaseTest, EndOfStream) {
+TEST_F(VideoRendererBaseTest, EndOfStream_DefaultFrameDuration) {
   Initialize();
   Play();
 
   // Finish rendering up to the next-to-last frame.
-  for (int i = 1; i < limits::kMaxVideoFrames; ++i)
-    RenderFrame(kFrameDuration * i);
+  int timestamp = kFrameDuration;
+  for (int i = 1; i < limits::kMaxVideoFrames; ++i) {
+    RenderFrame(timestamp);
+    timestamp += kFrameDuration;
+  }
+
+  // Deliver the end of stream frame.
+  DeliverNextFrame(true);
+
+  // Verify that the ended callback fires when the default last frame duration
+  // has elapsed.
+  int end_timestamp =
+      timestamp + VideoRendererBase::kLastFrameDuration().InMilliseconds();
+  EXPECT_LT(end_timestamp, kVideoDuration);
+  RenderLastFrame(end_timestamp);
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererBaseTest, EndOfStream_ClipDuration) {
+  int duration = kVideoDuration + kFrameDuration / 2;
+  Initialize(duration);
+  Play();
+
+  // Render all frames except for the last |limits::kMaxVideoFrames| frames
+  // and deliver all the frame between the start and |duration|. The preroll

[DaleCurtis, 2012/08/07 20:32:14]

s/frame/frames/

[acolwell GONE FROM CHROMIUM, 2012/08/07 21:10:28]

Done.

+  // inside Initialize() makes this a little confusing, but |timestamp| is
+  // the current render time and DeliverNextFrame() delivers a frame with a
+  // timestamp that is |timestamp| + limits::kMaxVideoFrames * kFrameDuration.
+  int timestamp = kFrameDuration;
+  int end_timestamp = duration - limits::kMaxVideoFrames * kFrameDuration;
+  for (; timestamp < end_timestamp; timestamp += kFrameDuration) {
+    RenderFrame(timestamp);
+    DeliverNextFrame(false);
+  }
+
+  // Render the next frame so that a Read() will get requested.
+  RenderFrame(timestamp);
 
-  // Finish rendering the last frame, we should NOT get a new frame but instead
-  // get notified of end of stream.
-  DeliverFrame(kEndOfStream);
-  RenderLastFrame(kVideoDuration);
+  // Deliver the end of stream frame and wait for the last frame to be rendered.
+  DeliverNextFrame(true);
+  RenderLastFrame(duration);
 
   Shutdown();
 }