Use FFmpeg's reordered_opaque for presentation timestamp reordering.

This fixes numerous audio/video synchronization issues caused by PtsHeap getting out of sync due to the decoder silently dropping/reordering frames.

Most decoder libraries (including FFmpeg and OpenMAX) feature some form of presentation timestamp reordering.  This is the first step in moving away from using PtsHeap and instead requireing VideoDecodeEngines + their libraries to handle presentation timestamp reordering.

This change also removes VideoFrame::GetRepeatCount() as it is an FFmpeg-specific detail.  The duration is now properly calculated inside FFmpegVideoDecodeEngine.

BUG=26036
TEST=videos linked in bugs remain in sync, as do all other videos

Review URL: http://codereview.chromium.org/1726015

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

6 files changed, 88 insertions, 43 deletions

diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index 6767ee4..0b8239c 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -152,7 +152,6 @@ VideoFrame::VideoFrame(VideoFrame::Format format,
   width_ = width;
   height_ = height;
   planes_ = 0;
-  repeat_count_ = 0;
   memset(&strides_, 0, sizeof(strides_));
   memset(&data_, 0, sizeof(data_));
 }
diff --git a/media/base/video_frame.h b/media/base/video_frame.h
index 0ad0628..e5cb8ba 100644
--- a/media/base/video_frame.h
+++ b/media/base/video_frame.h
@@ -80,14 +80,6 @@ class VideoFrame : public StreamSample {
   // StreamSample interface.
   virtual bool IsEndOfStream() const;
 
-  int GetRepeatCount() const {
-    return repeat_count_;
-  }
-
-  void SetRepeatCount(int repeat_count) {
-    repeat_count_ = repeat_count;
-  }
-
  protected:
   // Clients must use the static CreateFrame() method to create a new frame.
   VideoFrame(Format format,
@@ -119,9 +111,6 @@ class VideoFrame : public StreamSample {
   // Array of data pointers to each plane.
   uint8* data_[kMaxPlanes];
 
-  // Display meta data
-  int repeat_count_;
-
   DISALLOW_COPY_AND_ASSIGN(VideoFrame);
 };
 
diff --git a/media/filters/ffmpeg_video_decode_engine.cc b/media/filters/ffmpeg_video_decode_engine.cc
index 15cddc3..bf6089a 100644
--- a/media/filters/ffmpeg_video_decode_engine.cc
+++ b/media/filters/ffmpeg_video_decode_engine.cc
@@ -107,6 +107,9 @@ void FFmpegVideoDecodeEngine::DecodeFrame(
   packet.data = const_cast<uint8*>(buffer->GetData());
   packet.size = buffer->GetDataSize();
 
+  // Let FFmpeg handle presentation timestamp reordering.
+  codec_context_->reordered_opaque = buffer->GetTimestamp().InMicroseconds();
+
   int frame_decoded = 0;
   int result = avcodec_decode_video2(codec_context_,
                                      av_frame_.get(),
@@ -142,11 +145,24 @@ void FFmpegVideoDecodeEngine::DecodeFrame(
     return;
   }
 
+  // Determine timestamp and calculate the duration based on the repeat picture
+  // count.  According to FFmpeg docs, the total duration can be calculated as
+  // follows:
+  //   duration = (1 / fps) + (repeat_pict) / (2 * fps)
+  //            = (2 + repeat_pict) / (2 * fps)
+  DCHECK_LE(av_frame_->repeat_pict, 2);  // Sanity check.
+  AVRational doubled_time_base = codec_context_->time_base;
+  doubled_time_base.den *= 2;
+  base::TimeDelta timestamp =
+      base::TimeDelta::FromMicroseconds(av_frame_->reordered_opaque);
+  base::TimeDelta duration =
+      ConvertTimestamp(doubled_time_base, 2 + av_frame_->repeat_pict);
+
   VideoFrame::CreateFrame(GetSurfaceFormat(),
                           codec_context_->width,
                           codec_context_->height,
-                          StreamSample::kInvalidTimestamp,
-                          StreamSample::kInvalidTimestamp,
+                          timestamp,
+                          duration,
                           video_frame);
   if (!video_frame->get()) {
     // TODO(jiesun): this is also an error case handled as normal.
@@ -162,9 +178,6 @@ void FFmpegVideoDecodeEngine::DecodeFrame(
   CopyPlane(VideoFrame::kYPlane, video_frame->get(), av_frame_.get());
   CopyPlane(VideoFrame::kUPlane, video_frame->get(), av_frame_.get());
   CopyPlane(VideoFrame::kVPlane, video_frame->get(), av_frame_.get());
-
-  DCHECK_LE(av_frame_->repeat_pict, 2);  // Sanity check.
-  (*video_frame)->SetRepeatCount(av_frame_->repeat_pict);
 }
 
 void FFmpegVideoDecodeEngine::Flush(Task* done_cb) {
diff --git a/media/filters/ffmpeg_video_decode_engine_unittest.cc b/media/filters/ffmpeg_video_decode_engine_unittest.cc
index 533edd0..f1d1680 100644
--- a/media/filters/ffmpeg_video_decode_engine_unittest.cc
+++ b/media/filters/ffmpeg_video_decode_engine_unittest.cc
@@ -22,6 +22,7 @@ namespace media {
 
 static const int kWidth = 320;
 static const int kHeight = 240;
+static const AVRational kTimeBase = { 1, 100 };
 
 class FFmpegVideoDecodeEngineTest : public testing::Test {
  protected:
@@ -29,14 +30,18 @@ class FFmpegVideoDecodeEngineTest : public testing::Test {
     // Setup FFmpeg structures.
     frame_buffer_.reset(new uint8[kWidth * kHeight]);
     memset(&yuv_frame_, 0, sizeof(yuv_frame_));
+
     // DecodeFrame will check these pointers as non-NULL value.
     yuv_frame_.data[0] = yuv_frame_.data[1] = yuv_frame_.data[2]
         = frame_buffer_.get();
     yuv_frame_.linesize[0] = kWidth;
     yuv_frame_.linesize[1] = yuv_frame_.linesize[2] = kWidth >> 1;
+
     memset(&codec_context_, 0, sizeof(codec_context_));
     codec_context_.width = kWidth;
     codec_context_.height = kHeight;
+    codec_context_.time_base = kTimeBase;
+
     memset(&codec_, 0, sizeof(codec_));
     memset(&stream_, 0, sizeof(stream_));
     stream_.codec = &codec_context_;
@@ -151,6 +156,13 @@ TEST_F(FFmpegVideoDecodeEngineTest, Initialize_OpenDecoderFails) {
 TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_Normal) {
   Initialize();
 
+  // We rely on FFmpeg for timestamp and duration reporting.  The one tricky
+  // bit is calculating the duration when |repeat_pict| > 0.
+  const base::TimeDelta kTimestamp = base::TimeDelta::FromMicroseconds(123);
+  const base::TimeDelta kDuration = base::TimeDelta::FromMicroseconds(15000);
+  yuv_frame_.repeat_pict = 1;
+  yuv_frame_.reordered_opaque = kTimestamp.InMicroseconds();
+
   // Expect a bunch of avcodec calls.
   EXPECT_CALL(mock_ffmpeg_, AVInitPacket(_));
   EXPECT_CALL(mock_ffmpeg_,
@@ -166,6 +178,10 @@ TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_Normal) {
   test_engine_->DecodeFrame(buffer_, &video_frame, &got_result,
                             done_cb.CreateTask());
   EXPECT_TRUE(got_result);
+  EXPECT_EQ(kTimestamp.InMicroseconds(),
+            video_frame->GetTimestamp().ToInternalValue());
+  EXPECT_EQ(kDuration.ToInternalValue(),
+            video_frame->GetDuration().ToInternalValue());
 }
 
 TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_0ByteFrame) {
diff --git a/media/filters/video_decoder_impl.cc b/media/filters/video_decoder_impl.cc
index a4a655e..9932620 100644
--- a/media/filters/video_decoder_impl.cc
+++ b/media/filters/video_decoder_impl.cc
@@ -212,29 +212,38 @@ VideoDecoderImpl::TimeTuple VideoDecoderImpl::FindPtsAndDuration(
 
   // First search the VideoFrame for the pts. This is the most authoritative.
   // Make a special exclusion for the value pts == 0.  Though this is
-  // technically a valid value, it seems a number of ffmpeg codecs will
+  // technically a valid value, it seems a number of FFmpeg codecs will
   // mistakenly always set pts to 0.
+  //
+  // TODO(scherkus): FFmpegVideoDecodeEngine should be able to detect this
+  // situation and set the timestamp to kInvalidTimestamp.
   DCHECK(frame);
   base::TimeDelta timestamp = frame->GetTimestamp();
   if (timestamp != StreamSample::kInvalidTimestamp &&
       timestamp.ToInternalValue() != 0) {
-    pts.timestamp = ConvertTimestamp(time_base, timestamp.ToInternalValue());
-    pts.duration = ConvertTimestamp(time_base, 1 + frame->GetRepeatCount());
-    return pts;
-  }
-
-  if (!pts_heap.IsEmpty()) {
+    pts.timestamp = timestamp;
+  } else if (!pts_heap.IsEmpty()) {
     // If the frame did not have pts, try to get the pts from the |pts_heap|.
     pts.timestamp = pts_heap.Top();
-  } else {
-    DCHECK(last_pts.timestamp != StreamSample::kInvalidTimestamp);
-    DCHECK(last_pts.duration != StreamSample::kInvalidTimestamp);
-    // Unable to read the pts from anywhere. Time to guess.
+  } else if (last_pts.timestamp != StreamSample::kInvalidTimestamp &&
+             last_pts.duration != StreamSample::kInvalidTimestamp) {
+    // Guess assuming this frame was the same as the last frame.
     pts.timestamp = last_pts.timestamp + last_pts.duration;
+  } else {
+    // Now we really have no clue!!!  Mark an invalid timestamp and let the
+    // video renderer handle it (i.e., drop frame).
+    pts.timestamp = StreamSample::kInvalidTimestamp;
   }
 
-  // Fill in the duration while accounting for repeated frames.
-  pts.duration = ConvertTimestamp(time_base, 1);
+  // Fill in the duration, using the frame itself as the authoratative source.
+  base::TimeDelta duration = frame->GetDuration();
+  if (duration != StreamSample::kInvalidTimestamp &&
+      duration.ToInternalValue() != 0) {
+    pts.duration = duration;
+  } else {
+    // Otherwise assume a normal frame duration.
+    pts.duration = ConvertTimestamp(time_base, 1);
+  }
 
   return pts;
 }
diff --git a/media/filters/video_decoder_impl_unittest.cc b/media/filters/video_decoder_impl_unittest.cc
index 7c321c5..5bef51c 100644
--- a/media/filters/video_decoder_impl_unittest.cc
+++ b/media/filters/video_decoder_impl_unittest.cc
@@ -258,20 +258,35 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   // Start with an empty timestamp queue.
   PtsHeap pts_heap;
 
-  // Use 1/2 second for simple results.  Thus, calculated Durations should be
+  // Use 1/2 second for simple results.  Thus, calculated durations should be
   // 500000 microseconds.
   AVRational time_base = {1, 2};
 
-  // Setup the last known pts to be at 100 microseconds with a have 16
-  // duration.
   VideoDecoderImpl::TimeTuple last_pts;
+  last_pts.timestamp = StreamSample::kInvalidTimestamp;
+  last_pts.duration = StreamSample::kInvalidTimestamp;
+
+  // Simulate an uninitialized |video_frame| and |last_pts| where we cannot
+  // determine a timestamp at all.
+  video_frame_->SetTimestamp(StreamSample::kInvalidTimestamp);
+  video_frame_->SetDuration(StreamSample::kInvalidTimestamp);
+  VideoDecoderImpl::TimeTuple result_pts =
+      decoder_->FindPtsAndDuration(time_base, pts_heap,
+                                   last_pts, video_frame_.get());
+  EXPECT_EQ(StreamSample::kInvalidTimestamp.InMicroseconds(),
+            result_pts.timestamp.InMicroseconds());
+  EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
+
+  // Setup the last known pts to be at 100 microseconds with 16 microsecond
+  // duration.
   last_pts.timestamp = base::TimeDelta::FromMicroseconds(100);
   last_pts.duration = base::TimeDelta::FromMicroseconds(16);
 
-  // Simulate an uninitialized yuv_frame.
-  video_frame_->SetTimestamp(
-      base::TimeDelta::FromMicroseconds(AV_NOPTS_VALUE));
-  VideoDecoderImpl::TimeTuple result_pts =
+  // Simulate an uninitialized |video_frame| where |last_pts| will be used to
+  // generate a timestamp and |time_base| will be used to generate a duration.
+  video_frame_->SetTimestamp(StreamSample::kInvalidTimestamp);
+  video_frame_->SetDuration(StreamSample::kInvalidTimestamp);
+  result_pts =
       decoder_->FindPtsAndDuration(time_base, pts_heap,
                                    last_pts, video_frame_.get());
   EXPECT_EQ(116, result_pts.timestamp.InMicroseconds());
@@ -280,6 +295,9 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   // Test that having pts == 0 in the frame also behaves like the pts is not
   // provided.  This is because FFmpeg set the pts to zero when there is no
   // data for the frame, which means that value is useless to us.
+  //
+  // TODO(scherkus): FFmpegVideoDecodeEngine should be able to detect this
+  // situation and set the timestamp to kInvalidTimestamp.
   video_frame_->SetTimestamp(base::TimeDelta::FromMicroseconds(0));
   result_pts =
       decoder_->FindPtsAndDuration(time_base, pts_heap,
@@ -287,7 +305,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   EXPECT_EQ(116, result_pts.timestamp.InMicroseconds());
   EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
 
-  // Add a pts to the timequeue and make sure it overrides estimation.
+  // Add a pts to the |pts_heap| and make sure it overrides estimation.
   pts_heap.Push(base::TimeDelta::FromMicroseconds(123));
   result_pts = decoder_->FindPtsAndDuration(time_base,
                                             pts_heap,
@@ -296,15 +314,16 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   EXPECT_EQ(123, result_pts.timestamp.InMicroseconds());
   EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
 
-  // Add a pts into the frame and make sure it overrides the timequeue.
-  video_frame_->SetTimestamp(base::TimeDelta::FromMicroseconds(333));
-  video_frame_->SetRepeatCount(2);
+  // Set a pts and duration on |video_frame_| and make sure it overrides
+  // |pts_heap|.
+  video_frame_->SetTimestamp(base::TimeDelta::FromMicroseconds(456));
+  video_frame_->SetDuration(base::TimeDelta::FromMicroseconds(789));
   result_pts = decoder_->FindPtsAndDuration(time_base,
                                             pts_heap,
                                             last_pts,
                                             video_frame_.get());
-  EXPECT_EQ(166500000, result_pts.timestamp.InMicroseconds());
-  EXPECT_EQ(1500000, result_pts.duration.InMicroseconds());
+  EXPECT_EQ(456, result_pts.timestamp.InMicroseconds());
+  EXPECT_EQ(789, result_pts.duration.InMicroseconds());
 }
 
 TEST_F(VideoDecoderImplTest, DoDecode_TestStateTransition) {