media: fix timestamp issues.

1. use av_stream->r_frame_rate as time_base because it is more accurate in some cases.
2. do not use pts heap when timestamp is invalid

TEST=video stack matrix
BUGS=45347

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

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

6 files changed, 39 insertions, 33 deletions

diff --git a/media/filters/ffmpeg_video_decode_engine.cc b/media/filters/ffmpeg_video_decode_engine.cc
index 6c1eafb..d84d56c 100644
--- a/media/filters/ffmpeg_video_decode_engine.cc
+++ b/media/filters/ffmpeg_video_decode_engine.cc
@@ -18,6 +18,7 @@ namespace media {
 
 FFmpegVideoDecodeEngine::FFmpegVideoDecodeEngine()
     : codec_context_(NULL),
+      av_stream_(NULL),
       state_(kCreated) {
 }
 
@@ -50,8 +51,8 @@ void FFmpegVideoDecodeEngine::Initialize(
   static const int kMaxDecodeThreads = 16;
 
 
-  AVStream* stream = av_stream;
-  codec_context_ = stream->codec;
+  av_stream_ = av_stream;
+  codec_context_ = av_stream_->codec;
   codec_context_->flags2 |= CODEC_FLAG2_FAST;  // Enable faster H264 decode.
   // Enable motion vector search (potentially slow), strong deblocking filter
   // for damaged macroblocks, and set our error detection sensitivity.
@@ -172,8 +173,16 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) {
   //   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;
+  // Even frame rate is fixed, for some streams and codecs, the value of
+  // |codec_context_->time_base| and |av_stream_->time_base| are not the
+  // inverse of the |av_stream_->r_frame_rate|. They use 1 milli-second as
+  // time-base units and use increment of av_packet->pts which is not one.
+  // Use the inverse of |av_stream_->r_frame_rate| instead of time_base.
+  AVRational doubled_time_base;
+  doubled_time_base.den = av_stream_->r_frame_rate.num;
+  doubled_time_base.num = av_stream_->r_frame_rate.den;
   doubled_time_base.den *= 2;
+
   base::TimeDelta timestamp =
       base::TimeDelta::FromMicroseconds(av_frame_->reordered_opaque);
   base::TimeDelta duration =
diff --git a/media/filters/ffmpeg_video_decode_engine.h b/media/filters/ffmpeg_video_decode_engine.h
index a96dd06..9b0c186 100644
--- a/media/filters/ffmpeg_video_decode_engine.h
+++ b/media/filters/ffmpeg_video_decode_engine.h
@@ -46,6 +46,7 @@ class FFmpegVideoDecodeEngine : public VideoDecodeEngine {
   void DecodeFrame(scoped_refptr<Buffer> buffer);
 
   AVCodecContext* codec_context_;
+  AVStream* av_stream_;
   State state_;
   scoped_ptr_malloc<AVFrame, ScopedPtrAVFree> av_frame_;
   scoped_ptr<FillThisBufferCallback> fill_this_buffer_callback_;
diff --git a/media/filters/ffmpeg_video_decode_engine_unittest.cc b/media/filters/ffmpeg_video_decode_engine_unittest.cc
index 12051c1..b774e65 100644
--- a/media/filters/ffmpeg_video_decode_engine_unittest.cc
+++ b/media/filters/ffmpeg_video_decode_engine_unittest.cc
@@ -45,6 +45,8 @@ class FFmpegVideoDecodeEngineTest : public testing::Test {
     memset(&codec_, 0, sizeof(codec_));
     memset(&stream_, 0, sizeof(stream_));
     stream_.codec = &codec_context_;
+    stream_.r_frame_rate.num = kTimeBase.den;
+    stream_.r_frame_rate.den = kTimeBase.num;
 
     buffer_ = new DataBuffer(1);
 
diff --git a/media/filters/video_decoder_impl.cc b/media/filters/video_decoder_impl.cc
index 3f7b745..5f3dbf0 100644
--- a/media/filters/video_decoder_impl.cc
+++ b/media/filters/video_decoder_impl.cc
@@ -39,7 +39,8 @@ void VideoDecoderImpl::DoInitialize(DemuxerStream* demuxer_stream,
   }
   AVStream* av_stream = av_stream_provider->GetAVStream();
 
-  *time_base_ = av_stream->time_base;
+  time_base_->den = av_stream->r_frame_rate.num;
+  time_base_->num = av_stream->r_frame_rate.den;
 
   // TODO(ajwong): We don't need these extra variables if |media_format_| has
   // them.  Remove.
@@ -138,7 +139,8 @@ void VideoDecoderImpl::DoDecode(Buffer* buffer) {
   //
   // TODO(ajwong): This push logic, along with the pop logic below needs to
   // be reevaluated to correctly handle decode errors.
-  if (state_ == kNormal) {
+  if (state_ == kNormal &&
+      buffer->GetTimestamp() != StreamSample::kInvalidTimestamp) {
     pts_heap_.Push(buffer->GetTimestamp());
   }
 
@@ -149,23 +151,9 @@ void VideoDecoderImpl::DoDecode(Buffer* buffer) {
 void VideoDecoderImpl::OnDecodeComplete(scoped_refptr<VideoFrame> video_frame) {
   if (video_frame.get()) {
     // If we actually got data back, enqueue a frame.
-    last_pts_ = FindPtsAndDuration(*time_base_, pts_heap_, last_pts_,
+    last_pts_ = FindPtsAndDuration(*time_base_, &pts_heap_, last_pts_,
                                    video_frame.get());
 
-    // Pop off a pts on a successful decode since we are "using up" one
-    // timestamp.
-    //
-    // TODO(ajwong): Do we need to pop off a pts when avcodec_decode_video2()
-    // returns < 0?  The rationale is that when get_picture_ptr == 0, we skip
-    // popping a pts because no frame was produced.  However, when
-    // avcodec_decode_video2() returns false, it is a decode error, which
-    // if it means a frame is dropped, may require us to pop one more time.
-    if (!pts_heap_.IsEmpty()) {
-      pts_heap_.Pop();
-    } else {
-      NOTREACHED() << "Attempting to decode more frames than were input.";
-    }
-
     video_frame->SetTimestamp(last_pts_.timestamp);
     video_frame->SetDuration(last_pts_.duration);
     EnqueueVideoFrame(video_frame);
@@ -201,7 +189,7 @@ void VideoDecoderImpl::EnqueueEmptyFrame() {
 
 VideoDecoderImpl::TimeTuple VideoDecoderImpl::FindPtsAndDuration(
     const AVRational& time_base,
-    const PtsHeap& pts_heap,
+    PtsHeap* pts_heap,
     const TimeTuple& last_pts,
     const VideoFrame* frame) {
   TimeTuple pts;
@@ -218,9 +206,10 @@ VideoDecoderImpl::TimeTuple VideoDecoderImpl::FindPtsAndDuration(
   if (timestamp != StreamSample::kInvalidTimestamp &&
       timestamp.ToInternalValue() != 0) {
     pts.timestamp = timestamp;
-  } else if (!pts_heap.IsEmpty()) {
+  } 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();
+    pts.timestamp = pts_heap->Top();
+    pts_heap->Pop();
   } else if (last_pts.timestamp != StreamSample::kInvalidTimestamp &&
              last_pts.duration != StreamSample::kInvalidTimestamp) {
     // Guess assuming this frame was the same as the last frame.
diff --git a/media/filters/video_decoder_impl.h b/media/filters/video_decoder_impl.h
index 811d141..d972a8f 100644
--- a/media/filters/video_decoder_impl.h
+++ b/media/filters/video_decoder_impl.h
@@ -83,7 +83,7 @@ class VideoDecoderImpl : public DecoderBase<VideoDecoder, VideoFrame> {
   // by data from the packet stream.  Estimation based on the |last_pts| is
   // reserved as a last-ditch effort.
   virtual TimeTuple FindPtsAndDuration(const AVRational& time_base,
-                                       const PtsHeap& pts_heap,
+                                       PtsHeap* pts_heap,
                                        const TimeTuple& last_pts,
                                        const VideoFrame* frame);
 
diff --git a/media/filters/video_decoder_impl_unittest.cc b/media/filters/video_decoder_impl_unittest.cc
index c509dd8..f2161ed 100644
--- a/media/filters/video_decoder_impl_unittest.cc
+++ b/media/filters/video_decoder_impl_unittest.cc
@@ -77,7 +77,7 @@ class DecoderPrivateMock : public VideoDecoderImpl {
   MOCK_METHOD0(EnqueueEmptyFrame, void());
   MOCK_METHOD4(FindPtsAndDuration, TimeTuple(
       const AVRational& time_base,
-      const PtsHeap& pts_heap,
+      PtsHeap* pts_heap,
       const TimeTuple& last_pts,
       const VideoFrame* frame));
   MOCK_METHOD0(SignalPipelineError, void());
@@ -293,7 +293,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   video_frame_->SetTimestamp(StreamSample::kInvalidTimestamp);
   video_frame_->SetDuration(StreamSample::kInvalidTimestamp);
   VideoDecoderImpl::TimeTuple result_pts =
-      decoder_->FindPtsAndDuration(time_base, pts_heap,
+      decoder_->FindPtsAndDuration(time_base, &pts_heap,
                                    last_pts, video_frame_.get());
   EXPECT_EQ(StreamSample::kInvalidTimestamp.InMicroseconds(),
             result_pts.timestamp.InMicroseconds());
@@ -309,7 +309,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   video_frame_->SetTimestamp(StreamSample::kInvalidTimestamp);
   video_frame_->SetDuration(StreamSample::kInvalidTimestamp);
   result_pts =
-      decoder_->FindPtsAndDuration(time_base, pts_heap,
+      decoder_->FindPtsAndDuration(time_base, &pts_heap,
                                    last_pts, video_frame_.get());
   EXPECT_EQ(116, result_pts.timestamp.InMicroseconds());
   EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
@@ -322,7 +322,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   // situation and set the timestamp to kInvalidTimestamp.
   video_frame_->SetTimestamp(base::TimeDelta::FromMicroseconds(0));
   result_pts =
-      decoder_->FindPtsAndDuration(time_base, pts_heap,
+      decoder_->FindPtsAndDuration(time_base,&pts_heap,
                                    last_pts, video_frame_.get());
   EXPECT_EQ(116, result_pts.timestamp.InMicroseconds());
   EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
@@ -330,7 +330,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   // 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,
+                                            &pts_heap,
                                             last_pts,
                                             video_frame_.get());
   EXPECT_EQ(123, result_pts.timestamp.InMicroseconds());
@@ -341,7 +341,7 @@ TEST_F(VideoDecoderImplTest, FindPtsAndDuration) {
   video_frame_->SetTimestamp(base::TimeDelta::FromMicroseconds(456));
   video_frame_->SetDuration(base::TimeDelta::FromMicroseconds(789));
   result_pts = decoder_->FindPtsAndDuration(time_base,
-                                            pts_heap,
+                                            &pts_heap,
                                             last_pts,
                                             video_frame_.get());
   EXPECT_EQ(456, result_pts.timestamp.InMicroseconds());
@@ -357,6 +357,10 @@ ACTION_P2(DecodeNotComplete, decoder, video_frame) {
   decoder->OnDecodeComplete(null_frame);
 }
 
+ACTION_P(ConsumePTS, pts_heap) {
+  pts_heap->Pop();
+}
+
 TEST_F(VideoDecoderImplTest, DoDecode_TestStateTransition) {
   // Simulates a input sequence of three buffers, and six decode requests to
   // exercise the state transitions, and bookkeeping logic of DoDecode.
@@ -382,10 +386,11 @@ TEST_F(VideoDecoderImplTest, DoDecode_TestStateTransition) {
       .WillOnce(DecodeComplete(mock_decoder.get(), video_frame_))
       .WillOnce(DecodeComplete(mock_decoder.get(), video_frame_))
       .WillOnce(DecodeNotComplete(mock_decoder.get(), video_frame_));
+  PtsHeap* pts_heap = &mock_decoder->pts_heap_;
   EXPECT_CALL(*mock_decoder, FindPtsAndDuration(_, _, _, _))
-      .WillOnce(Return(kTestPts1))
-      .WillOnce(Return(kTestPts2))
-      .WillOnce(Return(kTestPts1));
+      .WillOnce(DoAll(ConsumePTS(pts_heap), Return(kTestPts1)))
+      .WillOnce(DoAll(ConsumePTS(pts_heap), Return(kTestPts2)))
+      .WillOnce(DoAll(ConsumePTS(pts_heap), Return(kTestPts1)));
   EXPECT_CALL(*mock_decoder, EnqueueVideoFrame(_))
       .Times(3);
   EXPECT_CALL(*mock_decoder, EnqueueEmptyFrame())