Fold VideoRenderer::Stop() into the dtor.

BUG=349211
TEST=All existing tests pass.

Review URL: https://codereview.chromium.org/409813003

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

7 files changed, 51 insertions, 118 deletions

diff --git a/media/base/mock_filters.h b/media/base/mock_filters.h
index dd66e2f..0755e46 100644
--- a/media/base/mock_filters.h
+++ b/media/base/mock_filters.h
@@ -128,7 +128,6 @@ class MockVideoRenderer : public VideoRenderer {
                                  const TimeDeltaCB& get_duration_cb));
   MOCK_METHOD1(Flush, void(const base::Closure& callback));
   MOCK_METHOD0(StartPlaying, void());
-  MOCK_METHOD1(Stop, void(const base::Closure& callback));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockVideoRenderer);
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index f238d66..e11fe32 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -496,6 +496,7 @@ void Pipeline::DoStop(const PipelineStatusCB& done_cb) {
   DCHECK(!pending_callbacks_.get());
 
   audio_renderer_.reset();
+  video_renderer_.reset();
 
   SerialRunner::Queue bound_fns;
 
@@ -504,11 +505,6 @@ void Pipeline::DoStop(const PipelineStatusCB& done_cb) {
         &Demuxer::Stop, base::Unretained(demuxer_)));
   }
 
-  if (video_renderer_) {
-    bound_fns.Push(base::Bind(
-        &VideoRenderer::Stop, base::Unretained(video_renderer_.get())));
-  }
-
   if (text_renderer_) {
     bound_fns.Push(base::Bind(
         &TextRenderer::Stop, base::Unretained(text_renderer_.get())));
@@ -521,6 +517,7 @@ void Pipeline::OnStopCompleted(PipelineStatus status) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kStopping);
   DCHECK(!audio_renderer_);
+  DCHECK(!video_renderer_);
   {
     base::AutoLock l(lock_);
     running_ = false;
@@ -529,7 +526,6 @@ void Pipeline::OnStopCompleted(PipelineStatus status) {
   SetState(kStopped);
   pending_callbacks_.reset();
   filter_collection_.reset();
-  video_renderer_.reset();
   text_renderer_.reset();
   demuxer_ = NULL;
 
diff --git a/media/base/pipeline_unittest.cc b/media/base/pipeline_unittest.cc
index 42db189..6a12a11 100644
--- a/media/base/pipeline_unittest.cc
+++ b/media/base/pipeline_unittest.cc
@@ -311,9 +311,6 @@ class PipelineTest : public ::testing::Test {
   void ExpectStop() {
     if (demuxer_)
       EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
-
-    if (video_stream_)
-      EXPECT_CALL(*video_renderer_, Stop(_)).WillOnce(RunClosure<0>());
   }
 
   MOCK_METHOD2(OnAddTextTrack, void(const TextTrackConfig&,
@@ -979,7 +976,6 @@ class PipelineTeardownTest : public PipelineTest {
       }
 
       EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
-      EXPECT_CALL(*video_renderer_, Stop(_)).WillOnce(RunClosure<0>());
       return status;
     }
 
@@ -1014,7 +1010,6 @@ class PipelineTeardownTest : public PipelineTest {
     PipelineStatus status = SetSeekExpectations(state, stop_or_error);
 
     EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
-    EXPECT_CALL(*video_renderer_, Stop(_)).WillOnce(RunClosure<0>());
     EXPECT_CALL(callbacks_, OnSeek(status));
 
     if (status == PIPELINE_OK) {
@@ -1084,7 +1079,6 @@ class PipelineTeardownTest : public PipelineTest {
     InSequence s;
 
     EXPECT_CALL(*demuxer_, Stop(_)).WillOnce(RunClosure<0>());
-    EXPECT_CALL(*video_renderer_, Stop(_)).WillOnce(RunClosure<0>());
 
     switch (stop_or_error) {
       case kStop:
diff --git a/media/base/video_renderer.h b/media/base/video_renderer.h
index a3f7b07..ff675b9 100644
--- a/media/base/video_renderer.h
+++ b/media/base/video_renderer.h
@@ -27,9 +27,13 @@ class MEDIA_EXPORT VideoRenderer {
   typedef base::Callback<base::TimeDelta()> TimeDeltaCB;
 
   VideoRenderer();
+
+  // Stops all operations and drops all pending callbacks.
+  // TODO(xhwang): Fires all pending callbacks to be consistent with the rest of
+  // media pipeline.
   virtual ~VideoRenderer();
 
-  // Initialize a VideoRenderer with |stream|, executing |init_cb| upon
+  // Initializes a VideoRenderer with |stream|, executing |init_cb| upon
   // completion.
   //
   // |statistics_cb| is executed periodically with video rendering stats, such
@@ -58,8 +62,8 @@ class MEDIA_EXPORT VideoRenderer {
                           const TimeDeltaCB& get_time_cb,
                           const TimeDeltaCB& get_duration_cb) = 0;
 
-  // Discard any video data and stop reading from |stream|, executing |callback|
-  // when completed.
+  // Discards any video data and stops reading from |stream|, executing
+  // |callback| when completed.
   //
   // Clients should expect |buffering_state_cb| to be called with
   // BUFFERING_HAVE_NOTHING while flushing is in progress.
@@ -70,10 +74,6 @@ class MEDIA_EXPORT VideoRenderer {
   // Only valid to call after a successful Initialize() or Flush().
   virtual void StartPlaying() = 0;
 
-  // Stop all operations in preparation for being deleted, executing |callback|
-  // when complete.
-  virtual void Stop(const base::Closure& callback) = 0;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoRenderer);
 };
diff --git a/media/filters/video_renderer_impl.cc b/media/filters/video_renderer_impl.cc
index c5be9f5..f65cf2f 100644
--- a/media/filters/video_renderer_impl.cc
+++ b/media/filters/video_renderer_impl.cc
@@ -41,14 +41,22 @@ VideoRendererImpl::VideoRendererImpl(
       last_timestamp_(kNoTimestamp()),
       frames_decoded_(0),
       frames_dropped_(0),
+      is_shutting_down_(false),
       weak_factory_(this) {
   DCHECK(!paint_cb_.is_null());
 }
 
 VideoRendererImpl::~VideoRendererImpl() {
-  base::AutoLock auto_lock(lock_);
-  CHECK(state_ == kStopped || state_ == kUninitialized) << state_;
-  CHECK(thread_.is_null());
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  {
+    base::AutoLock auto_lock(lock_);
+    is_shutting_down_ = true;
+    frame_available_.Signal();
+  }
+
+  if (!thread_.is_null())
+    base::PlatformThread::Join(thread_);
 }
 
 void VideoRendererImpl::Flush(const base::Closure& callback) {
@@ -73,41 +81,6 @@ void VideoRendererImpl::Flush(const base::Closure& callback) {
                  weak_factory_.GetWeakPtr()));
 }
 
-void VideoRendererImpl::Stop(const base::Closure& callback) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
-  if (state_ == kUninitialized || state_ == kStopped) {
-    task_runner_->PostTask(FROM_HERE, callback);
-    return;
-  }
-
-  // TODO(scherkus): Consider invalidating |weak_factory_| and replacing
-  // task-running guards that check |state_| with DCHECK().
-
-  state_ = kStopped;
-
-  statistics_cb_.Reset();
-  max_time_cb_.Reset();
-  DoStopOrError_Locked();
-
-  // Clean up our thread if present.
-  base::PlatformThreadHandle thread_to_join = base::PlatformThreadHandle();
-  if (!thread_.is_null()) {
-    // Signal the thread since it's possible to get stopped with the video
-    // thread waiting for a read to complete.
-    frame_available_.Signal();
-    std::swap(thread_, thread_to_join);
-  }
-
-  if (!thread_to_join.is_null()) {
-    base::AutoUnlock auto_unlock(lock_);
-    base::PlatformThread::Join(thread_to_join);
-  }
-
-  video_frame_stream_.reset();
-  task_runner_->PostTask(FROM_HERE, callback);
-}
-
 void VideoRendererImpl::StartPlaying() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
@@ -167,10 +140,6 @@ void VideoRendererImpl::Initialize(DemuxerStream* stream,
 void VideoRendererImpl::OnVideoFrameStreamInitialized(bool success) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
-
-  if (state_ == kStopped)
-    return;
-
   DCHECK_EQ(state_, kInitializing);
 
   if (!success) {
@@ -213,7 +182,7 @@ void VideoRendererImpl::ThreadMain() {
     base::AutoLock auto_lock(lock_);
 
     // Thread exit condition.
-    if (state_ == kStopped)
+    if (is_shutting_down_)
       return;
 
     // Remain idle as long as we're not playing.
@@ -327,7 +296,7 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
 
   // Already-queued VideoFrameStream ReadCB's can fire after various state
   // transitions have happened; in that case just drop those frames immediately.
-  if (state_ == kStopped || state_ == kFlushing)
+  if (state_ == kFlushing)
     return;
 
   DCHECK_EQ(state_, kPlaying);
@@ -451,16 +420,12 @@ void VideoRendererImpl::AttemptRead_Locked() {
     case kInitializing:
     case kFlushing:
     case kFlushed:
-    case kStopped:
       return;
   }
 }
 
 void VideoRendererImpl::OnVideoFrameStreamResetDone() {
   base::AutoLock auto_lock(lock_);
-  if (state_ == kStopped)
-    return;
-
   DCHECK_EQ(kFlushing, state_);
   DCHECK(!pending_read_);
   DCHECK(ready_frames_.empty());
@@ -473,12 +438,6 @@ void VideoRendererImpl::OnVideoFrameStreamResetDone() {
   base::ResetAndReturn(&flush_cb_).Run();
 }
 
-void VideoRendererImpl::DoStopOrError_Locked() {
-  lock_.AssertAcquired();
-  last_timestamp_ = kNoTimestamp();
-  ready_frames_.clear();
-}
-
 void VideoRendererImpl::UpdateStatsAndWait_Locked(
     base::TimeDelta wait_duration) {
   lock_.AssertAcquired();
diff --git a/media/filters/video_renderer_impl.h b/media/filters/video_renderer_impl.h
index 9f3b671..eef3d0b 100644
--- a/media/filters/video_renderer_impl.h
+++ b/media/filters/video_renderer_impl.h
@@ -69,7 +69,6 @@ class MEDIA_EXPORT VideoRendererImpl
                           const TimeDeltaCB& get_duration_cb) OVERRIDE;
   virtual void Flush(const base::Closure& callback) OVERRIDE;
   virtual void StartPlaying() OVERRIDE;
-  virtual void Stop(const base::Closure& callback) OVERRIDE;
 
   // PlatformThread::Delegate implementation.
   virtual void ThreadMain() OVERRIDE;
@@ -95,9 +94,6 @@ class MEDIA_EXPORT VideoRendererImpl
   // Called when VideoFrameStream::Reset() completes.
   void OnVideoFrameStreamResetDone();
 
-  // Helper function that flushes the buffers when a Stop() or error occurs.
-  void DoStopOrError_Locked();
-
   // Runs |paint_cb_| with the next frame from |ready_frames_|.
   //
   // A read is scheduled to replace the frame.
@@ -164,8 +160,7 @@ class MEDIA_EXPORT VideoRendererImpl
     kInitializing,
     kFlushing,
     kFlushed,
-    kPlaying,
-    kStopped,
+    kPlaying
   };
   State state_;
 
@@ -208,6 +203,8 @@ class MEDIA_EXPORT VideoRendererImpl
   int frames_decoded_;
   int frames_dropped_;
 
+  bool is_shutting_down_;
+
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<VideoRendererImpl> weak_factory_;
 
diff --git a/media/filters/video_renderer_impl_unittest.cc b/media/filters/video_renderer_impl_unittest.cc
index 1ecf6c3..5bb5f30 100644
--- a/media/filters/video_renderer_impl_unittest.cc
+++ b/media/filters/video_renderer_impl_unittest.cc
@@ -138,15 +138,9 @@ class VideoRendererImplTest : public ::testing::Test {
     event.RunAndWait();
   }
 
-  void Stop() {
-    SCOPED_TRACE("Stop()");
-    WaitableMessageLoopEvent event;
-    renderer_->Stop(event.GetClosure());
-    event.RunAndWait();
-  }
-
-  void Shutdown() {
-    Stop();
+  void Destroy() {
+    SCOPED_TRACE("Destroy()");
+    renderer_.reset();
   }
 
   // Parses a string representation of video frames and generates corresponding
@@ -342,16 +336,16 @@ class VideoRendererImplTest : public ::testing::Test {
 
 TEST_F(VideoRendererImplTest, DoNothing) {
   // Test that creation and deletion doesn't depend on calls to Initialize()
-  // and/or Stop().
+  // and/or Destroy().
 }
 
-TEST_F(VideoRendererImplTest, StopWithoutInitialize) {
-  Stop();
+TEST_F(VideoRendererImplTest, DestroyWithoutInitialize) {
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, Initialize) {
   Initialize();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, InitializeAndStartPlaying) {
@@ -360,7 +354,7 @@ TEST_F(VideoRendererImplTest, InitializeAndStartPlaying) {
   EXPECT_CALL(mock_cb_, Display(HasTimestamp(0)));
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH));
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 static void ExpectNotCalled(PipelineStatus) {
@@ -368,14 +362,12 @@ static void ExpectNotCalled(PipelineStatus) {
   ADD_FAILURE() << "Expected callback not to be called\n" << stack.ToString();
 }
 
-TEST_F(VideoRendererImplTest, StopWhileInitializing) {
+TEST_F(VideoRendererImplTest, DestroyWhileInitializing) {
   CallInitialize(base::Bind(&ExpectNotCalled), false, PIPELINE_OK);
-  Stop();
-
-  // ~VideoRendererImpl() will CHECK() if we left anything initialized.
+  Destroy();
 }
 
-TEST_F(VideoRendererImplTest, StopWhileFlushing) {
+TEST_F(VideoRendererImplTest, DestroyWhileFlushing) {
   Initialize();
   QueueFrames("0 10 20 30");
   EXPECT_CALL(mock_cb_, Display(HasTimestamp(0)));
@@ -383,9 +375,7 @@ TEST_F(VideoRendererImplTest, StopWhileFlushing) {
   StartPlaying();
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING));
   renderer_->Flush(base::Bind(&ExpectNotCalled, PIPELINE_OK));
-  Stop();
-
-  // ~VideoRendererImpl() will CHECK() if we left anything initialized.
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, Play) {
@@ -394,7 +384,7 @@ TEST_F(VideoRendererImplTest, Play) {
   EXPECT_CALL(mock_cb_, Display(HasTimestamp(0)));
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH));
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, FlushWithNothingBuffered) {
@@ -404,7 +394,7 @@ TEST_F(VideoRendererImplTest, FlushWithNothingBuffered) {
   // We shouldn't expect a buffering state change since we never reached
   // BUFFERING_HAVE_ENOUGH.
   Flush();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, EndOfStream_ClipDuration) {
@@ -427,7 +417,7 @@ TEST_F(VideoRendererImplTest, EndOfStream_ClipDuration) {
   AdvanceTimeInMs(kVideoDurationInMs);
   WaitForEnded();
 
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, DecodeError_Playing) {
@@ -440,14 +430,14 @@ TEST_F(VideoRendererImplTest, DecodeError_Playing) {
   QueueFrames("error");
   SatisfyPendingRead();
   WaitForError(PIPELINE_ERROR_DECODE);
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, DecodeError_DuringStartPlaying) {
   Initialize();
   QueueFrames("error");
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, StartPlaying_Exact) {
@@ -458,7 +448,7 @@ TEST_F(VideoRendererImplTest, StartPlaying_Exact) {
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH));
   AdvanceTimeInMs(60);
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, StartPlaying_RightBefore) {
@@ -469,7 +459,7 @@ TEST_F(VideoRendererImplTest, StartPlaying_RightBefore) {
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH));
   AdvanceTimeInMs(59);
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, StartPlaying_RightAfter) {
@@ -480,7 +470,7 @@ TEST_F(VideoRendererImplTest, StartPlaying_RightAfter) {
   EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_ENOUGH));
   AdvanceTimeInMs(61);
   StartPlaying();
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, StartPlaying_LowDelay) {
@@ -505,7 +495,7 @@ TEST_F(VideoRendererImplTest, StartPlaying_LowDelay) {
   AdvanceTimeInMs(10);
   event.RunAndWait();
 
-  Shutdown();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, PlayAfterStartPlaying) {
@@ -518,11 +508,11 @@ TEST_F(VideoRendererImplTest, PlayAfterStartPlaying) {
   // Check that there is an outstanding Read() request.
   EXPECT_TRUE(IsReadPending());
 
-  Shutdown();
+  Destroy();
 }
 
 // Verify that a late decoder response doesn't break invariants in the renderer.
-TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) {
+TEST_F(VideoRendererImplTest, DestroyDuringOutstandingRead) {
   Initialize();
   QueueFrames("0 10 20 30");
   EXPECT_CALL(mock_cb_, Display(HasTimestamp(0)));
@@ -532,14 +522,12 @@ TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) {
   // Check that there is an outstanding Read() request.
   EXPECT_TRUE(IsReadPending());
 
-  WaitableMessageLoopEvent event;
-  renderer_->Stop(event.GetClosure());
-  event.RunAndWait();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, VideoDecoder_InitFailure) {
   InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED, false);
-  Stop();
+  Destroy();
 }
 
 TEST_F(VideoRendererImplTest, Underflow) {
@@ -570,7 +558,7 @@ TEST_F(VideoRendererImplTest, Underflow) {
   }
 
   WaitForEnded();
-  Shutdown();
+  Destroy();
 }
 
 }  // namespace media