Fix VideoRendererBase to allow the decoder to be flushed with a pending read.

BUG=123134
TEST=media_unittests


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

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

4 files changed, 58 insertions, 18 deletions

diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h
index 5162675..409d09d 100644
--- a/media/base/mock_filters.h
+++ b/media/base/mock_filters.h
@@ -147,6 +147,7 @@ class MockVideoDecoder : public VideoDecoder {
   MockVideoDecoder();
 
   // Filter implementation.
+  MOCK_METHOD1(Flush, void(const base::Closure& callback));
   MOCK_METHOD1(Stop, void(const base::Closure& callback));
   MOCK_METHOD1(SetPlaybackRate, void(float playback_rate));
   MOCK_METHOD2(Seek, void(base::TimeDelta time, const PipelineStatusCB& cb));
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index 30b2995..51c51b0 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -54,9 +54,13 @@ void VideoRendererBase::Flush(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(state_, kPaused);
   flush_cb_ = callback;
-  state_ = kFlushing;
+  state_ = kFlushingDecoder;
 
-  AttemptFlush_Locked();
+  // We must unlock here because the callback might run within the Flush()
+  // call.
+  // TODO: Remove this line when fixing http://crbug.com/125020
+  base::AutoUnlock auto_unlock(lock_);
+  decoder_->Flush(base::Bind(&VideoRendererBase::OnDecoderFlushDone, this));
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
@@ -342,9 +346,15 @@ void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) {
   // frame is timed-out. We will wake up our main thread to advance the current
   // frame when this is true.
   frame_available_.Signal();
+  if (state_ == kFlushingDecoder)
+    return;
+
   if (state_ == kFlushing) {
     AttemptFlush_Locked();
-  } else if (state_ == kError || state_ == kStopped) {
+    return;
+  }
+
+  if (state_ == kError || state_ == kStopped) {
     DoStopOrError_Locked();
   }
 }
@@ -358,7 +368,8 @@ void VideoRendererBase::FrameReady(scoped_refptr<VideoFrame> frame) {
 
   // Already-queued Decoder ReadCB's can fire after various state transitions
   // have happened; in that case just drop those frames immediately.
-  if (state_ == kStopped || state_ == kError || state_ == kFlushed)
+  if (state_ == kStopped || state_ == kError || state_ == kFlushed ||
+      state_ == kFlushingDecoder)
     return;
 
   if (state_ == kFlushing) {
@@ -446,14 +457,25 @@ void VideoRendererBase::AttemptRead_Locked() {
 
   if (pending_read_ ||
       NumFrames_Locked() == limits::kMaxVideoFrames ||
-      (!ready_frames_.empty() && ready_frames_.back()->IsEndOfStream())) {
-    return;
-  }
+      (!ready_frames_.empty() && ready_frames_.back()->IsEndOfStream()) ||
+      state_ == kFlushingDecoder ||
+      state_ == kFlushing) {
+     return;
+   }
 
   pending_read_ = true;
   decoder_->Read(base::Bind(&VideoRendererBase::FrameReady, this));
 }
 
+void VideoRendererBase::OnDecoderFlushDone() {
+  base::AutoLock auto_lock(lock_);
+  DCHECK_EQ(kFlushingDecoder, state_);
+  DCHECK(!pending_read_);
+
+  state_ = kFlushing;
+  AttemptFlush_Locked();
+}
+
 void VideoRendererBase::AttemptFlush_Locked() {
   lock_.AssertAcquired();
   DCHECK_EQ(kFlushing, state_);
@@ -464,10 +486,7 @@ void VideoRendererBase::AttemptFlush_Locked() {
   if (!pending_paint_ && !pending_read_) {
     state_ = kFlushed;
     current_frame_ = NULL;
-
-    base::Closure flush_cb = flush_cb_;
-    flush_cb_.Reset();
-    decoder_->Flush(flush_cb);
+    base::ResetAndReturn(&flush_cb_).Run();
   }
 }
 
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index ca9ebea..2746ccb 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -81,6 +81,9 @@ class MEDIA_EXPORT VideoRendererBase
   // as there isn't a pending read and we have capacity.
   void AttemptRead_Locked();
 
+  // Called when the VideoDecoder Flush() completes.
+  void OnDecoderFlushDone();
+
   // Attempts to complete flushing and transition into the flushed state.
   void AttemptFlush_Locked();
 
@@ -133,11 +136,11 @@ class MEDIA_EXPORT VideoRendererBase
   //              |
   //              | Initialize()
   //              V        All frames returned
-  //   +------[kFlushed]<----------------------[kFlushing]
+  //   +------[kFlushed]<-----[kFlushing]<--- OnDecoderFlushDone()
   //   |          | Seek() or upon                  ^
-  //   |          V got first frame                 |
-  //   |      [kSeeking]                            | Flush()
-  //   |          |                                 |
+  //   |          V got first frame           [kFlushingDecoder]
+  //   |      [kSeeking]                            ^
+  //   |          |                                 | Flush()
   //   |          V Got enough frames               |
   //   |      [kPrerolled]---------------------->[kPaused]
   //   |          |                Pause()          ^
@@ -156,6 +159,7 @@ class MEDIA_EXPORT VideoRendererBase
     kUninitialized,
     kPrerolled,
     kPaused,
+    kFlushingDecoder,
     kFlushing,
     kFlushed,
     kSeeking,
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index c5507a4..b7be329 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -98,6 +98,9 @@ class VideoRendererBaseTest : public ::testing::Test {
     EXPECT_CALL(*decoder_, Read(_))
         .WillRepeatedly(Invoke(this, &VideoRendererBaseTest::FrameRequested));
 
+    EXPECT_CALL(*decoder_, Flush(_))
+        .WillRepeatedly(Invoke(this, &VideoRendererBaseTest::FlushRequested));
+
     InSequence s;
 
     // We expect the video size to be set.
@@ -322,6 +325,21 @@ class VideoRendererBaseTest : public ::testing::Test {
     cv_.Signal();
   }
 
+  void FlushRequested(const base::Closure& callback) {
+    // Lock+swap to avoid re-entrancy issues.
+    VideoDecoder::ReadCB read_cb;
+    {
+      base::AutoLock l(lock_);
+      std::swap(read_cb, read_cb_);
+    }
+
+    // Abort pending read.
+    if (!read_cb.is_null())
+      read_cb.Run(NULL);
+
+    callback.Run();
+  }
+
   void OnSeekComplete(PipelineStatus expected_status, PipelineStatus status) {
     base::AutoLock l(lock_);
     EXPECT_EQ(status, expected_status);
@@ -579,9 +597,7 @@ TEST_F(VideoRendererBaseTest, AbortPendingRead_Flush) {
   RenderFrame(kFrameDuration);
 
   Pause();
-  renderer_->Flush(NewWaitableClosure());
-  AbortRead();
-  WaitForClosure();
+  Flush();
   Shutdown();
 }