Fixed HW video decode EOS/Flush-related bugs.

- VideoRendererBase needs to watch out for an EOS ready_frame_
  not at the front of the queue (since HW video decoder can
  generate multiple frames per Read() call).
- VideoRendererBase needs account for outstanding textures in
  current_frame_ & last_available_frame_ when deciding whether to
  Decoder::Read() some more.
- Made GpuVideoDecoder's implementation of EOS and Flush()
  handling more straightforward/explicit/robust.

BUG=109625
TEST=chrome play-to-EOS & replay works

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

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

5 files changed, 110 insertions, 74 deletions

diff --git a/media/filters/gpu_video_decoder.cc b/media/filters/gpu_video_decoder.cc
index 442f16f..a41f744 100644
--- a/media/filters/gpu_video_decoder.cc
+++ b/media/filters/gpu_video_decoder.cc
@@ -44,7 +44,7 @@ GpuVideoDecoder::GpuVideoDecoder(
     scoped_ptr<Factories> factories)
     : message_loop_(message_loop),
       factories_(factories.Pass()),
-      flush_in_progress_(false),
+      state_(kNormal),
       demuxer_read_in_progress_(false),
       next_picture_buffer_id_(0),
       next_bitstream_buffer_id_(0) {
@@ -102,22 +102,28 @@ void GpuVideoDecoder::Pause(const base::Closure& callback)  {
 }
 
 void GpuVideoDecoder::Flush(const base::Closure& callback)  {
-  if (MessageLoop::current() != message_loop_ || flush_in_progress_) {
+  // VRB::Flush() waits for pending reads to be satisfied, so it's important we
+  // don't reset the decoder while the renderer is still waiting.
+  // TODO(fischman): replace the pseudo-busy-loop here with a proper accounting
+  // of state transitions.
+  if (MessageLoop::current() != message_loop_ ||
+      state_ == kDrainingDecoder ||
+      !pending_read_cb_.is_null()) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &GpuVideoDecoder::Flush, this, callback));
     return;
   }
 
-  // Throw away any already-decoded frames.
+  // Throw away any already-decoded, not-yet-delivered frames.
   ready_video_frames_.clear();
 
   if (!vda_) {
     callback.Run();
     return;
   }
-  DCHECK(pending_flush_cb_.is_null());
+  DCHECK(pending_reset_cb_.is_null());
   DCHECK(!callback.is_null());
-  pending_flush_cb_ = callback;
+  pending_reset_cb_ = callback;
   vda_->Reset();
 }
 
@@ -175,16 +181,27 @@ void GpuVideoDecoder::Read(const ReadCB& callback) {
     return;
   }
 
-  DCHECK(pending_flush_cb_.is_null());
+  DCHECK(pending_reset_cb_.is_null());
   DCHECK(pending_read_cb_.is_null());
   pending_read_cb_ = callback;
 
   if (!ready_video_frames_.empty()) {
-    DeliverFrame(ready_video_frames_.front());
-    ready_video_frames_.pop_front();
+    EnqueueFrameAndTriggerFrameDelivery(NULL);
     return;
   }
-  EnsureDemuxOrDecode();
+
+  switch (state_) {
+    case kDecoderDrained:
+      state_ = kNormal;
+      // Fall-through.
+    case kNormal:
+      EnsureDemuxOrDecode();
+      break;
+    case kDrainingDecoder:
+      // Do nothing.  Will be satisfied either by a PictureReady or
+      // NotifyFlushDone below.
+      break;
+  }
 }
 
 void GpuVideoDecoder::RequestBufferDecode(const scoped_refptr<Buffer>& buffer) {
@@ -193,17 +210,16 @@ void GpuVideoDecoder::RequestBufferDecode(const scoped_refptr<Buffer>& buffer) {
         &GpuVideoDecoder::RequestBufferDecode, this, buffer));
     return;
   }
-
   demuxer_read_in_progress_ = false;
 
   if (!vda_) {
-    DeliverFrame(VideoFrame::CreateEmptyFrame());
+    EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
     return;
   }
 
   if (buffer->IsEndOfStream()) {
-    if (!flush_in_progress_) {
-      flush_in_progress_ = true;
+    if (state_ == kNormal) {
+      state_ = kDrainingDecoder;
       vda_->Flush();
     }
     return;
@@ -256,7 +272,6 @@ void GpuVideoDecoder::GetBufferTimeData(
   NOTREACHED() << "Missing bitstreambuffer id: " << id;
 }
 
-
 const gfx::Size& GpuVideoDecoder::natural_size() {
   return natural_size_;
 }
@@ -316,14 +331,6 @@ void GpuVideoDecoder::DismissPictureBuffer(int32 id) {
   picture_buffers_in_decoder_.erase(it);
 }
 
-static void ResetAndRunCB(VideoDecoder::ReadCB* cb,
-                          scoped_refptr<VideoFrame> frame) {
-  DCHECK(!cb->is_null());
-  VideoDecoder::ReadCB tmp_cb(*cb);
-  cb->Reset();
-  tmp_cb.Run(frame);
-}
-
 void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
   if (MessageLoop::current() != message_loop_) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
@@ -350,24 +357,30 @@ void GpuVideoDecoder::PictureReady(const media::Picture& picture) {
       base::Bind(&GpuVideoDecoder::ReusePictureBuffer, this,
                  picture.picture_buffer_id())));
 
-  // Deliver the frame.
-  DeliverFrame(frame);
+  EnqueueFrameAndTriggerFrameDelivery(frame);
 }
 
-void GpuVideoDecoder::DeliverFrame(const scoped_refptr<VideoFrame>& frame) {
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::DeliverFrameOutOfLine, this, frame));
-}
-
-void GpuVideoDecoder::DeliverFrameOutOfLine(
+void GpuVideoDecoder::EnqueueFrameAndTriggerFrameDelivery(
     const scoped_refptr<VideoFrame>& frame) {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  if (!pending_read_cb_.is_null()) {
-    ResetAndRunCB(&pending_read_cb_, frame);
+  DCHECK(MessageLoop::current() == message_loop_);
+
+  // During a pending vda->Reset(), we don't accumulate frames.  Drop it on the
+  // floor and return.
+  if (!pending_reset_cb_.is_null())
     return;
-  }
-  if (pending_flush_cb_.is_null())
+
+  if (frame)
     ready_video_frames_.push_back(frame);
+  else
+    DCHECK(!ready_video_frames_.empty());
+
+  if (pending_read_cb_.is_null())
+    return;
+
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      pending_read_cb_, ready_video_frames_.front()));
+  pending_read_cb_.Reset();
+  ready_video_frames_.pop_front();
 }
 
 void GpuVideoDecoder::ReusePictureBuffer(int64 picture_buffer_id) {
@@ -401,12 +414,8 @@ void GpuVideoDecoder::PutSHM(SHMBuffer* shm_buffer) {
 }
 
 void GpuVideoDecoder::NotifyEndOfStream() {
-  if (MessageLoop::current() != message_loop_) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &GpuVideoDecoder::NotifyEndOfStream, this));
-    return;
-  }
-  DeliverFrame(VideoFrame::CreateEmptyFrame());
+  // TODO(fischman): remove with http://crbug.com/109819
+  NOTREACHED() << "NotifyEndOfStream is never called.";
 }
 
 void GpuVideoDecoder::NotifyEndOfBitstreamBuffer(int32 id) {
@@ -433,7 +442,9 @@ void GpuVideoDecoder::NotifyEndOfBitstreamBuffer(int32 id) {
   }
   bitstream_buffers_in_decoder_.erase(it);
 
-  if (!pending_read_cb_.is_null() && pending_flush_cb_.is_null()) {
+  if (!pending_read_cb_.is_null() && pending_reset_cb_.is_null() &&
+      state_ != kDrainingDecoder &&
+      bitstream_buffers_in_decoder_.empty()) {
     DCHECK(ready_video_frames_.empty());
     EnsureDemuxOrDecode();
   }
@@ -441,7 +452,7 @@ void GpuVideoDecoder::NotifyEndOfBitstreamBuffer(int32 id) {
 
 void GpuVideoDecoder::EnsureDemuxOrDecode() {
   DCHECK(MessageLoop::current() == message_loop_);
-  if (demuxer_read_in_progress_ || !bitstream_buffers_in_decoder_.empty())
+  if (demuxer_read_in_progress_)
     return;
   demuxer_read_in_progress_ = true;
   demuxer_stream_->Read(base::Bind(
@@ -454,9 +465,9 @@ void GpuVideoDecoder::NotifyFlushDone() {
         &GpuVideoDecoder::NotifyFlushDone, this));
     return;
   }
-  DCHECK(flush_in_progress_);
-  flush_in_progress_ = false;
-  DeliverFrame(VideoFrame::CreateEmptyFrame());
+  DCHECK_EQ(state_, kDrainingDecoder);
+  state_ = kDecoderDrained;
+  EnqueueFrameAndTriggerFrameDelivery(VideoFrame::CreateEmptyFrame());
 }
 
 void GpuVideoDecoder::NotifyResetDone() {
@@ -471,7 +482,7 @@ void GpuVideoDecoder::NotifyResetDone() {
   // delivered during the reset can find their time data.
   input_buffer_time_data_.clear();
 
-  ResetAndRunCB(&pending_flush_cb_);
+  ResetAndRunCB(&pending_reset_cb_);
 }
 
 void GpuVideoDecoder::NotifyError(media::VideoDecodeAccelerator::Error error) {
diff --git a/media/filters/gpu_video_decoder.h b/media/filters/gpu_video_decoder.h
index ea10251..ffe508c 100644
--- a/media/filters/gpu_video_decoder.h
+++ b/media/filters/gpu_video_decoder.h
@@ -80,6 +80,17 @@ class MEDIA_EXPORT GpuVideoDecoder
   virtual void NotifyError(media::VideoDecodeAccelerator::Error error) OVERRIDE;
 
  private:
+  enum State {
+    kNormal,
+    // Avoid the use of "flush" in these enums because the term is overloaded:
+    // Filter::Flush() means drop pending data on the floor, but
+    // VideoDecodeAccelerator::Flush() means drain pending data (Filter::Flush()
+    // actually corresponds to VideoDecodeAccelerator::Reset(), confusingly
+    // enough).
+    kDrainingDecoder,
+    kDecoderDrained,
+  };
+
   // If no demuxer read is in flight and no bitstream buffers are in the
   // decoder, kick some off demuxing/decoding.
   void EnsureDemuxOrDecode();
@@ -87,10 +98,13 @@ class MEDIA_EXPORT GpuVideoDecoder
   // Callback to pass to demuxer_stream_->Read() for receiving encoded bits.
   void RequestBufferDecode(const scoped_refptr<Buffer>& buffer);
 
-  // Deliver a frame to the client.  Because VideoDecoder::Read() promises not
-  // to run its callback before returning, we need an out-of-line helper here.
-  void DeliverFrame(const scoped_refptr<VideoFrame>& frame);
-  void DeliverFrameOutOfLine(const scoped_refptr<VideoFrame>& frame);
+  // Enqueue a frame for later delivery (or drop it on the floor if a
+  // vda->Reset() is in progress) and trigger out-of-line delivery of the oldest
+  // ready frame to the client if there is a pending read.  A NULL |frame|
+  // merely triggers delivery, and requires the ready_video_frames_ queue not be
+  // empty.
+  void EnqueueFrameAndTriggerFrameDelivery(
+      const scoped_refptr<VideoFrame>& frame);
 
   // Indicate the picturebuffer can be reused by the decoder.
   void ReusePictureBuffer(int64 picture_buffer_id);
@@ -140,10 +154,9 @@ class MEDIA_EXPORT GpuVideoDecoder
   // Callbacks that are !is_null() only during their respective operation being
   // asynchronously executed.
   ReadCB pending_read_cb_;
-  base::Closure pending_flush_cb_;
+  base::Closure pending_reset_cb_;
 
-  // Status of the decoder.
-  bool flush_in_progress_;
+  State state_;
 
   // Is a demuxer read in flight?
   bool demuxer_read_in_progress_;
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index 0202269..8248f88 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -192,6 +192,7 @@ void VideoRendererBase::ThreadMain() {
       if (ready_frames_.front()->IsEndOfStream()) {
         state_ = kEnded;
         host()->NotifyEnded();
+        ready_frames_.clear();
 
         // No need to sleep here as we idle when |state_ != kPlaying|.
         continue;
@@ -230,6 +231,7 @@ void VideoRendererBase::ThreadMain() {
     if (ready_frames_.front()->IsEndOfStream()) {
       state_ = kEnded;
       host()->NotifyEnded();
+      ready_frames_.clear();
 
       // No need to sleep here as we idle when |state_ != kPlaying|.
       continue;
@@ -356,34 +358,32 @@ void VideoRendererBase::FrameReady(scoped_refptr<VideoFrame> frame) {
     return;
   }
 
-  // Adjust the incoming frame if it's rendering stop time is past the duration
+  // Adjust the incoming frame if its rendering stop time is past the duration
   // of the video itself. This is typically the last frame of the video and
   // occurs if the container specifies a duration that isn't a multiple of the
-  // frame rate.
-  if (!frame->IsEndOfStream() &&
-      (frame->GetTimestamp() + frame->GetDuration()) > host()->GetDuration()) {
-    frame->SetDuration(host()->GetDuration() - frame->GetTimestamp());
+  // frame rate.  Another way for this to happen is for the container to state a
+  // smaller duration than the largest packet timestamp.
+  if (!frame->IsEndOfStream()) {
+    if (frame->GetTimestamp() > host()->GetDuration())
+      frame->SetTimestamp(host()->GetDuration());
+    if ((frame->GetTimestamp() + frame->GetDuration()) > host()->GetDuration())
+      frame->SetDuration(host()->GetDuration() - frame->GetTimestamp());
   }
 
   // This one's a keeper! Place it in the ready queue.
   ready_frames_.push_back(frame);
-  DCHECK_LE(ready_frames_.size(),
-            static_cast<size_t>(limits::kMaxVideoFrames));
+  DCHECK_LE(NumFrames_Locked(), limits::kMaxVideoFrames);
   frame_available_.Signal();
 
   PipelineStatistics statistics;
   statistics.video_frames_decoded = 1;
   statistics_callback_.Run(statistics);
 
-  int outstanding_frames =
-      (current_frame_ ? 1 : 0) + (last_available_frame_ ? 1 : 0) +
-      (current_frame_ && (current_frame_ == last_available_frame_) ? -1 : 0);
   // Always request more decoded video if we have capacity. This serves two
   // purposes:
   //   1) Prerolling while paused
   //   2) Keeps decoding going if video rendering thread starts falling behind
-  if ((ready_frames_.size() + outstanding_frames) < limits::kMaxVideoFrames &&
-      !frame->IsEndOfStream()) {
+  if (NumFrames_Locked() < limits::kMaxVideoFrames && !frame->IsEndOfStream()) {
     AttemptRead_Locked();
     return;
   }
@@ -415,8 +415,11 @@ void VideoRendererBase::AttemptRead_Locked() {
   lock_.AssertAcquired();
   DCHECK_NE(kEnded, state_);
 
-  if (pending_read_ || ready_frames_.size() == limits::kMaxVideoFrames)
+  if (pending_read_ ||
+      NumFrames_Locked() == limits::kMaxVideoFrames ||
+      (!ready_frames_.empty() && ready_frames_.back()->IsEndOfStream())) {
     return;
+  }
 
   pending_read_ = true;
   decoder_->Read(read_cb_);
@@ -427,9 +430,7 @@ void VideoRendererBase::AttemptFlush_Locked() {
   DCHECK_EQ(kFlushing, state_);
 
   // Get rid of any ready frames.
-  while (!ready_frames_.empty()) {
-    ready_frames_.pop_front();
-  }
+  ready_frames_.clear();
 
   if (!pending_paint_ && !pending_read_) {
     state_ = kFlushed;
@@ -463,6 +464,15 @@ void VideoRendererBase::DoStopOrError_Locked() {
   lock_.AssertAcquired();
   current_frame_ = NULL;
   last_available_frame_ = NULL;
+  ready_frames_.clear();
+}
+
+int VideoRendererBase::NumFrames_Locked() const {
+  lock_.AssertAcquired();
+  int outstanding_frames =
+      (current_frame_ ? 1 : 0) + (last_available_frame_ ? 1 : 0) +
+      (current_frame_ && (current_frame_ == last_available_frame_) ? -1 : 0);
+  return ready_frames_.size() + outstanding_frames;
 }
 
 }  // namespace media
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index d6dad42..047e079 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -94,6 +94,9 @@ class MEDIA_EXPORT VideoRendererBase
   // Helper function that flushes the buffers when a Stop() or error occurs.
   void DoStopOrError_Locked();
 
+  // Return the number of frames currently held by this class.
+  int NumFrames_Locked() const;
+
   // Used for accessing data members.
   base::Lock lock_;
 
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 25d002e..5f395c7 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -338,19 +338,18 @@ TEST_F(VideoRendererBaseTest, EndOfStream) {
   Initialize();
   Play();
 
-  // Finish rendering up to the next-to-last frame, delivering end of stream
-  // frames as we go along.
+  // Finish rendering up to the next-to-last frame.
   //
   // Put the gmock expectation here to avoid racing with the rendering thread.
   EXPECT_CALL(*this, PaintCBWasCalled())
       .Times(limits::kMaxVideoFrames - 1);
   for (int i = 1; i < limits::kMaxVideoFrames; ++i) {
     RenderFrame(kFrameDuration * i);
-    DeliverFrame(kEndOfStream);
   }
 
   // Finish rendering the last frame, we should NOT get a new frame but instead
   // get notified of end of stream.
+  DeliverFrame(kEndOfStream);
   RenderLastFrame(kFrameDuration * limits::kMaxVideoFrames);
 
   Shutdown();