Fix VideoRendererBase end of stream logic to use a default

last frame duration if the last frame is far away from the clip duration.


BUG=None
TEST=VideoRendererBaseTest.EndOfStream_DefaultFrameDuration, VideoRendererBaseTest.EndOfStream_DefaultFrameDuration

Review URL: https://chromiumcodereview.appspot.com/10829200

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150467 0039d316-1c4b-4281-b951-d872f2087c98

media/base/mock_filters.h

@@ -102,7 +102,6 @@ class MockVideoDecoder : public VideoDecoder {
   MOCK_METHOD1(Read, void(const ReadCB&));
   MOCK_METHOD1(Reset, void(const base::Closure&));
   MOCK_METHOD1(Stop, void(const base::Closure&));
-  MOCK_METHOD0(natural_size, const gfx::Size&());
   MOCK_CONST_METHOD0(HasAlpha, bool());
 
  protected:

media/filters/video_renderer_base.cc

@@ -15,6 +15,10 @@
 
 namespace media {
 
+base::TimeDelta VideoRendererBase::kMaxLastFrameDuration() {
+  return base::TimeDelta::FromMilliseconds(250);
+}
+
 VideoRendererBase::VideoRendererBase(const base::Closure& paint_cb,
                                      const SetOpaqueCB& set_opaque_cb,
                                      bool drop_frames)
@@ -70,7 +74,7 @@ void VideoRendererBase::Stop(const base::Closure& callback) {
     state_ = kStopped;
 
     statistics_cb_.Reset();
-    time_cb_.Reset();
+    max_time_cb_.Reset();
     if (!pending_paint_ && !pending_paint_with_last_available_)
       DoStopOrError_Locked();
 
@@ -111,7 +115,7 @@ void VideoRendererBase::Preroll(base::TimeDelta time,
 void VideoRendererBase::Initialize(const scoped_refptr<VideoDecoder>& decoder,
                                    const PipelineStatusCB& init_cb,
                                    const StatisticsCB& statistics_cb,
-                                   const TimeCB& time_cb,
+                                   const TimeCB& max_time_cb,
                                    const NaturalSizeChangedCB& size_changed_cb,
                                    const base::Closure& ended_cb,
                                    const PipelineStatusCB& error_cb,
@@ -121,7 +125,7 @@ void VideoRendererBase::Initialize(const scoped_refptr<VideoDecoder>& decoder,
   DCHECK(decoder);
   DCHECK(!init_cb.is_null());
   DCHECK(!statistics_cb.is_null());
-  DCHECK(!time_cb.is_null());
+  DCHECK(!max_time_cb.is_null());
   DCHECK(!size_changed_cb.is_null());
   DCHECK(!ended_cb.is_null());
   DCHECK(!get_time_cb.is_null());
@@ -130,7 +134,7 @@ void VideoRendererBase::Initialize(const scoped_refptr<VideoDecoder>& decoder,
   decoder_ = decoder;
 
   statistics_cb_ = statistics_cb;
-  time_cb_ = time_cb;
+  max_time_cb_ = max_time_cb;
   size_changed_cb_ = size_changed_cb;
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
@@ -484,13 +488,30 @@ void VideoRendererBase::AddReadyFrame(const scoped_refptr<VideoFrame>& frame) {
   // frame rate.  Another way for this to happen is for the container to state a
   // smaller duration than the largest packet timestamp.
   base::TimeDelta duration = get_duration_cb_.Run();
-  if (frame->GetTimestamp() > duration || frame->IsEndOfStream()) {
+  if (frame->IsEndOfStream()) {
+    base::TimeDelta end_timestamp = kNoTimestamp();
+    if (!ready_frames_.empty()) {
+      end_timestamp = std::min(
+          duration,
+          ready_frames_.back()->GetTimestamp() + kMaxLastFrameDuration());
+    } else if (current_frame_) {
+      end_timestamp =
+          std::min(duration,
+                   current_frame_->GetTimestamp() + kMaxLastFrameDuration());
+    }
+    frame->SetTimestamp(end_timestamp);
+  } else if (frame->GetTimestamp() > duration) {
     frame->SetTimestamp(duration);
   }
 
   ready_frames_.push_back(frame);
   DCHECK_LE(NumFrames_Locked(), limits::kMaxVideoFrames);
-  time_cb_.Run(frame->GetTimestamp());
+
+  base::TimeDelta max_clock_time =
+      frame->IsEndOfStream() ? duration : frame->GetTimestamp();
+  DCHECK(max_clock_time != kNoTimestamp());
+  max_time_cb_.Run(max_clock_time);
+
   frame_available_.Signal();
 }
 

media/filters/video_renderer_base.h

@@ -28,6 +28,9 @@ class MEDIA_EXPORT VideoRendererBase
  public:
   typedef base::Callback<void(bool)> SetOpaqueCB;
 
+  // Maximum duration of the last frame.
+  static base::TimeDelta kMaxLastFrameDuration();
+
   // |paint_cb| is executed on the video frame timing thread whenever a new
   // frame is available for painting via GetCurrentFrame().
   //
@@ -50,7 +53,7 @@ class MEDIA_EXPORT VideoRendererBase
   virtual void Initialize(const scoped_refptr<VideoDecoder>& decoder,
                           const PipelineStatusCB& init_cb,
                           const StatisticsCB& statistics_cb,
-                          const TimeCB& time_cb,
+                          const TimeCB& max_time_cb,
                           const NaturalSizeChangedCB& size_changed_cb,
                           const base::Closure& ended_cb,
                           const PipelineStatusCB& error_cb,
@@ -209,7 +212,7 @@ class MEDIA_EXPORT VideoRendererBase
 
   // Event callbacks.
   StatisticsCB statistics_cb_;
-  TimeCB time_cb_;
+  TimeCB max_time_cb_;
   NaturalSizeChangedCB size_changed_cb_;
   base::Closure ended_cb_;
   PipelineStatusCB error_cb_;

media/filters/video_renderer_base_unittest.cc

@@ -4,7 +4,6 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
@@ -30,9 +29,9 @@ using ::testing::StrictMock;
 
 namespace media {
 
-static const int64 kFrameDuration = 10;
-static const int64 kVideoDuration = kFrameDuration * 100;
-static const int64 kEndOfStream = kint64min;
+static const int kFrameDuration = 10;
+static const int kVideoDuration = kFrameDuration * 100;
+static const int kEndOfStream = -1;
 static const gfx::Size kNaturalSize(16u, 16u);
 
 class VideoRendererBaseTest : public ::testing::Test {
@@ -43,6 +42,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 +52,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 +80,12 @@ class VideoRendererBaseTest : public ::testing::Test {
   MOCK_METHOD1(OnError, void(PipelineStatus));
 
   void Initialize() {
+    Initialize(kVideoDuration);
+  }
+
+  void Initialize(int duration) {
+    duration_ = duration;
+
     // TODO(scherkus): really, really, really need to inject a thread into
     // VideoRendererBase... it makes mocking much harder.
 
@@ -135,11 +139,12 @@ class VideoRendererBaseTest : public ::testing::Test {
     read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
   }
 
-  void StartPrerolling(int64 timestamp, PipelineStatus expected_status) {
+  void StartPrerolling(int 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));
   }
@@ -153,10 +158,12 @@ class VideoRendererBaseTest : public ::testing::Test {
   // Preroll to the given timestamp.
   //
   // 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);
+  void Preroll(int timestamp) {
+    SCOPED_TRACE(base::StringPrintf("Preroll(%d)", timestamp));
+    bool end_of_stream = (timestamp == kEndOfStream);
+    int preroll_timestamp = end_of_stream ? 0 : timestamp;
+    StartPrerolling(preroll_timestamp, PIPELINE_OK);
+    FinishPrerolling(end_of_stream);
   }
 
   void Pause() {
@@ -183,18 +190,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_);
+    int 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));
@@ -234,10 +250,10 @@ class VideoRendererBaseTest : public ::testing::Test {
     renderer_->PutCurrentFrame(frame);
   }
 
-  void ExpectCurrentTimestamp(int64 timestamp) {
+  void ExpectCurrentTimestamp(int timestamp) {
     scoped_refptr<VideoFrame> frame;
     renderer_->GetCurrentFrame(&frame);
-    EXPECT_EQ(timestamp, frame->GetTimestamp().InMicroseconds());
+    EXPECT_EQ(timestamp, frame->GetTimestamp().InMilliseconds());
     renderer_->PutCurrentFrame(frame);
   }
 
@@ -251,18 +267,18 @@ class VideoRendererBaseTest : public ::testing::Test {
   }
 
   // Creates a frame with given timestamp.
-  scoped_refptr<VideoFrame> CreateFrame(int64 timestamp) {
+  scoped_refptr<VideoFrame> CreateFrame(int timestamp) {
     scoped_refptr<VideoFrame> frame =
         VideoFrame::CreateFrame(VideoFrame::RGB32, kNaturalSize, kNaturalSize,
-                                base::TimeDelta::FromMicroseconds(timestamp));
+                                base::TimeDelta::FromMilliseconds(timestamp));
     return frame;
   }
 
   // Advances clock to |timestamp| and waits for the frame at |timestamp| to get
   // rendered using |read_cb_| as the signal that the frame has rendered.
-  void RenderFrame(int64 timestamp) {
+  void RenderFrame(int 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_);
@@ -273,12 +289,12 @@ class VideoRendererBaseTest : public ::testing::Test {
 
   // Advances clock to |timestamp| (which should be the timestamp of the last
   // frame plus duration) and waits for the ended signal before returning.
-  void RenderLastFrame(int64 timestamp) {
+  void RenderLastFrame(int timestamp) {
     EXPECT_CALL(*this, OnEnded())
         .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 +324,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 +364,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 +408,8 @@ class VideoRendererBaseTest : public ::testing::Test {
   // Used in conjunction with |lock_| and |cv_| for satisfying reads.
   bool prerolling_;
   VideoDecoder::ReadCB read_cb_;
+  int next_frame_timestamp_;
+  int duration_;
   base::TimeDelta time_;
 
   // Used in conjunction with |lock_| to wait for Paint() calls.
@@ -429,18 +435,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::kMaxLastFrameDuration().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 frames between the start and |duration|. The preroll
+  // 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();
 }