Make MediaFilter::Stop() asynchronous.

This is the second issue regarding making Stop() asynchronous.

All Stop() in subclasses of MediaFilter is made to accept a callback and
runs the callback at the end of its stop.

BUG=16059
TEST=media_unittest, unit_tests, test_shell_tests still passes

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

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

20 files changed, 92 insertions, 47 deletions

diff --git a/media/base/filters.h b/media/base/filters.h
index 141a753..80a3bd2 100644
--- a/media/base/filters.h
+++ b/media/base/filters.h
@@ -91,7 +91,9 @@ class MediaFilter : public base::RefCountedThreadSafe<MediaFilter> {
 
   // The pipeline has resumed playback.  Filters can continue requesting reads.
   // Filters may implement this method if they need to respond to this call.
+  // TODO(boliu): Check that callback is not NULL in sublcasses.
   virtual void Play(FilterCallback* callback) {
+    DCHECK(callback);
     if (callback) {
       callback->Run();
       delete callback;
@@ -101,24 +103,20 @@ class MediaFilter : public base::RefCountedThreadSafe<MediaFilter> {
   // The pipeline has paused playback.  Filters should fulfill any existing read
   // requests and then idle.  Filters may implement this method if they need to
   // respond to this call.
+  // TODO(boliu): Check that callback is not NULL in sublcasses.
   virtual void Pause(FilterCallback* callback) {
+    DCHECK(callback);
     if (callback) {
       callback->Run();
       delete callback;
     }
   }
 
-  // TODO(boliu): Remove once Stop() is asynchronous in subclasses.
-  virtual void Stop() {}
-
   // The pipeline is being stopped either as a result of an error or because
   // the client called Stop().
-  // TODO(boliu): No implementation in subclasses yet.
+  // TODO(boliu): Check that callback is not NULL in sublcasses.
   virtual void Stop(FilterCallback* callback) {
-    // TODO(boliu): Call the synchronous version for now. Remove once
-    // all filters have asynchronous stop.
-    Stop();
-
+    DCHECK(callback);
     if (callback) {
       callback->Run();
       delete callback;
diff --git a/media/filters/audio_renderer_base.cc b/media/filters/audio_renderer_base.cc
index 7e530a8..f3f9294 100644
--- a/media/filters/audio_renderer_base.cc
+++ b/media/filters/audio_renderer_base.cc
@@ -48,11 +48,17 @@ void AudioRendererBase::Pause(FilterCallback* callback) {
   }
 }
 
-void AudioRendererBase::Stop() {
+void AudioRendererBase::Stop(FilterCallback* callback) {
   OnStop();
-  AutoLock auto_lock(lock_);
-  state_ = kStopped;
-  algorithm_.reset(NULL);
+  {
+    AutoLock auto_lock(lock_);
+    state_ = kStopped;
+    algorithm_.reset(NULL);
+  }
+  if (callback) {
+    callback->Run();
+    delete callback;
+  }
 }
 
 void AudioRendererBase::Seek(base::TimeDelta time, FilterCallback* callback) {
diff --git a/media/filters/audio_renderer_base.h b/media/filters/audio_renderer_base.h
index 3762679..279d59f 100644
--- a/media/filters/audio_renderer_base.h
+++ b/media/filters/audio_renderer_base.h
@@ -33,7 +33,7 @@ class AudioRendererBase : public AudioRenderer {
   // MediaFilter implementation.
   virtual void Play(FilterCallback* callback);
   virtual void Pause(FilterCallback* callback);
-  virtual void Stop();
+  virtual void Stop(FilterCallback* callback);
 
   virtual void Seek(base::TimeDelta time, FilterCallback* callback);
 
diff --git a/media/filters/audio_renderer_base_unittest.cc b/media/filters/audio_renderer_base_unittest.cc
index 064f7a6..20d2912 100644
--- a/media/filters/audio_renderer_base_unittest.cc
+++ b/media/filters/audio_renderer_base_unittest.cc
@@ -70,7 +70,9 @@ class AudioRendererBaseTest : public ::testing::Test {
   virtual ~AudioRendererBaseTest() {
     // Expect a call into the subclass.
     EXPECT_CALL(*renderer_, OnStop());
-    renderer_->Stop();
+    EXPECT_CALL(callback_, OnFilterCallback());
+    EXPECT_CALL(callback_, OnCallbackDestroyed());
+    renderer_->Stop(callback_.NewCallback());
   }
 
  protected:
diff --git a/media/filters/decoder_base.h b/media/filters/decoder_base.h
index cc0e3cb..82f767e 100644
--- a/media/filters/decoder_base.h
+++ b/media/filters/decoder_base.h
@@ -30,9 +30,9 @@ class DecoderBase : public Decoder {
   typedef CallbackRunner< Tuple1<Output*> > ReadCallback;
 
   // MediaFilter implementation.
-  virtual void Stop() {
+  virtual void Stop(FilterCallback* callback) {
     this->message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &DecoderBase::StopTask));
+        NewRunnableMethod(this, &DecoderBase::StopTask, callback));
   }
 
   virtual void Seek(base::TimeDelta time,
@@ -146,7 +146,7 @@ class DecoderBase : public Decoder {
         NewRunnableMethod(this, &DecoderBase::ReadCompleteTask, buffer_ref));
   }
 
-  void StopTask() {
+  void StopTask(FilterCallback* callback) {
     DCHECK_EQ(MessageLoop::current(), this->message_loop());
 
     // Delegate to the subclass first.
@@ -155,6 +155,11 @@ class DecoderBase : public Decoder {
     // Throw away all buffers in all queues.
     result_queue_.clear();
     state_ = kStopped;
+
+    if (callback) {
+      callback->Run();
+      delete callback;
+    }
   }
 
   void SeekTask(base::TimeDelta time, FilterCallback* callback) {
diff --git a/media/filters/decoder_base_unittest.cc b/media/filters/decoder_base_unittest.cc
index e771989..4bc0531 100644
--- a/media/filters/decoder_base_unittest.cc
+++ b/media/filters/decoder_base_unittest.cc
@@ -153,7 +153,9 @@ TEST(DecoderBaseTest, FlowControl) {
 
   // Stop.
   EXPECT_CALL(*decoder, DoStop());
-  decoder->Stop();
+  EXPECT_CALL(callback, OnFilterCallback());
+  EXPECT_CALL(callback, OnCallbackDestroyed());
+  decoder->Stop(callback.NewCallback());
   message_loop.RunAllPending();
 }
 
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index c472504..27d8878 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -265,10 +265,10 @@ void FFmpegDemuxer::PostDemuxTask() {
       NewRunnableMethod(this, &FFmpegDemuxer::DemuxTask));
 }
 
-void FFmpegDemuxer::Stop() {
+void FFmpegDemuxer::Stop(FilterCallback* callback) {
   // Post a task to notify the streams to stop as well.
   message_loop()->PostTask(FROM_HERE,
-      NewRunnableMethod(this, &FFmpegDemuxer::StopTask));
+      NewRunnableMethod(this, &FFmpegDemuxer::StopTask, callback));
 
   // Then wakes up the thread from reading.
   SignalReadCompleted(DataSource::kReadError);
@@ -561,12 +561,16 @@ void FFmpegDemuxer::DemuxTask() {
   }
 }
 
-void FFmpegDemuxer::StopTask() {
+void FFmpegDemuxer::StopTask(FilterCallback* callback) {
   DCHECK_EQ(MessageLoop::current(), message_loop());
   StreamVector::iterator iter;
   for (iter = streams_.begin(); iter != streams_.end(); ++iter) {
     (*iter)->Stop();
   }
+  if (callback) {
+    callback->Run();
+    delete callback;
+  }
 }
 
 void FFmpegDemuxer::DisableAudioStreamTask() {
diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h
index bc5cc62..c43bd20 100644
--- a/media/filters/ffmpeg_demuxer.h
+++ b/media/filters/ffmpeg_demuxer.h
@@ -131,7 +131,7 @@ class FFmpegDemuxer : public Demuxer,
   virtual void PostDemuxTask();
 
   // MediaFilter implementation.
-  virtual void Stop();
+  virtual void Stop(FilterCallback* callback);
   virtual void Seek(base::TimeDelta time, FilterCallback* callback);
   virtual void OnAudioRendererDisabled();
 
@@ -166,7 +166,7 @@ class FFmpegDemuxer : public Demuxer,
   void DemuxTask();
 
   // Carries out stopping the demuxer streams on the demuxer thread.
-  void StopTask();
+  void StopTask(FilterCallback* callback);
 
   // Carries out disabling the audio stream on the demuxer thread.
   void DisableAudioStreamTask();
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index ec263a6..c6c8e74 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -117,7 +117,9 @@ class FFmpegDemuxerTest : public testing::Test {
 
   virtual ~FFmpegDemuxerTest() {
     // Call Stop() to shut down internal threads.
-    demuxer_->Stop();
+    EXPECT_CALL(callback_, OnFilterCallback());
+    EXPECT_CALL(callback_, OnCallbackDestroyed());
+    demuxer_->Stop(callback_.NewCallback());
 
     // Finish up any remaining tasks.
     message_loop_.RunAllPending();
@@ -623,7 +625,9 @@ TEST_F(FFmpegDemuxerTest, Stop) {
   ASSERT_TRUE(audio);
 
   // Stop the demuxer.
-  demuxer_->Stop();
+  EXPECT_CALL(callback_, OnFilterCallback());
+  EXPECT_CALL(callback_, OnCallbackDestroyed());
+  demuxer_->Stop(callback_.NewCallback());
 
   // Expect all calls in sequence.
   InSequence s;
@@ -732,9 +736,6 @@ TEST_F(FFmpegDemuxerTest, ProtocolRead) {
   EXPECT_CALL(*data_source_, GetSize(_))
       .WillOnce(DoAll(SetArgumentPointee<0>(1024), Return(true)));
 
-  // This read complete signal is generated when demuxer is stopped.
-  EXPECT_CALL(*demuxer, SignalReadCompleted(DataSource::kReadError));
-
   // First read.
   EXPECT_EQ(512, demuxer->Read(512, kBuffer));
   int64 position;
@@ -749,7 +750,12 @@ TEST_F(FFmpegDemuxerTest, ProtocolRead) {
   // Third read will get an end-of-file error.
   EXPECT_EQ(AVERROR_EOF, demuxer->Read(512, kBuffer));
 
-  demuxer->Stop();
+  // This read complete signal is generated when demuxer is stopped.
+  EXPECT_CALL(*demuxer, SignalReadCompleted(DataSource::kReadError));
+  EXPECT_CALL(callback_, OnFilterCallback());
+  EXPECT_CALL(callback_, OnCallbackDestroyed());
+  demuxer->Stop(callback_.NewCallback());
+  message_loop_.RunAllPending();
 }
 
 TEST_F(FFmpegDemuxerTest, ProtocolGetSetPosition) {
diff --git a/media/filters/file_data_source.cc b/media/filters/file_data_source.cc
index 901e107..181149d 100644
--- a/media/filters/file_data_source.cc
+++ b/media/filters/file_data_source.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -19,7 +19,7 @@ FileDataSource::FileDataSource()
 }
 
 FileDataSource::~FileDataSource() {
-  Stop();
+  DCHECK(!file_);
 }
 
 void FileDataSource::Initialize(const std::string& url,
@@ -48,13 +48,17 @@ void FileDataSource::Initialize(const std::string& url,
   callback->Run();
 }
 
-void FileDataSource::Stop() {
+void FileDataSource::Stop(FilterCallback* callback) {
   AutoLock l(lock_);
   if (file_) {
     file_util::CloseFile(file_);
     file_ = NULL;
     file_size_ = 0;
   }
+  if (callback) {
+    callback->Run();
+    delete callback;
+  }
 }
 
 const MediaFormat& FileDataSource::media_format() {
diff --git a/media/filters/file_data_source.h b/media/filters/file_data_source.h
index 164e74c..182d5be 100644
--- a/media/filters/file_data_source.h
+++ b/media/filters/file_data_source.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -23,7 +23,7 @@ class FileDataSource : public DataSource {
   }
 
   // Implementation of MediaFilter.
-  virtual void Stop();
+  virtual void Stop(FilterCallback* callback);
 
   // Implementation of DataSource.
   virtual void Initialize(const std::string& url, FilterCallback* callback);
diff --git a/media/filters/file_data_source_unittest.cc b/media/filters/file_data_source_unittest.cc
index 80df9b7..48d999e 100644
--- a/media/filters/file_data_source_unittest.cc
+++ b/media/filters/file_data_source_unittest.cc
@@ -65,6 +65,10 @@ TEST(FileDataSourceTest, OpenFile) {
   scoped_refptr<FileDataSource> filter = new FileDataSource();
   filter->set_host(&host);
   filter->Initialize(TestFileURL(), callback.NewCallback());
+
+  EXPECT_CALL(callback, OnFilterCallback());
+  EXPECT_CALL(callback, OnCallbackDestroyed());
+  filter->Stop(callback.NewCallback());
 }
 
 // Use the mock filter host to directly call the Read and GetPosition methods.
@@ -99,6 +103,10 @@ TEST(FileDataSourceTest, ReadData) {
   filter->Read(5, 10, ten_bytes,
                NewCallback(&handler, &ReadCallbackHandler::ReadCallback));
   EXPECT_EQ('5', ten_bytes[0]);
+
+  EXPECT_CALL(callback, OnFilterCallback());
+  EXPECT_CALL(callback, OnCallbackDestroyed());
+  filter->Stop(callback.NewCallback());
 }
 
 // Test that FileDataSource does nothing on Seek().
@@ -110,6 +118,10 @@ TEST(FileDataSourceTest, Seek) {
 
   scoped_refptr<FileDataSource> filter = new FileDataSource();
   filter->Seek(kZero, callback.NewCallback());
+
+  EXPECT_CALL(callback, OnFilterCallback());
+  EXPECT_CALL(callback, OnCallbackDestroyed());
+  filter->Stop(callback.NewCallback());
 }
 
 }  // namespace media
diff --git a/media/filters/null_audio_renderer.cc b/media/filters/null_audio_renderer.cc
index 544b588..62cbeee 100644
--- a/media/filters/null_audio_renderer.cc
+++ b/media/filters/null_audio_renderer.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -23,7 +23,7 @@ NullAudioRenderer::NullAudioRenderer()
 }
 
 NullAudioRenderer::~NullAudioRenderer() {
-  Stop();
+  DCHECK_EQ(kNullThreadHandle, thread_);
 }
 
 // static
diff --git a/media/filters/omx_video_decode_engine.h b/media/filters/omx_video_decode_engine.h
index 6cdfe73..d1c5619 100644
--- a/media/filters/omx_video_decode_engine.h
+++ b/media/filters/omx_video_decode_engine.h
@@ -53,6 +53,7 @@ class OmxVideoDecodeEngine :
   //
   // TODO(ajwong): Normalize this interface with Task like the others, and
   // promote to the abstract interface.
+  // TODO(boliu): Should the callback type be FilterCallback*?
   virtual void Stop(Callback0::Type* done_cb);
 
   // Subclass can provide a different value.
@@ -258,4 +259,3 @@ class OmxVideoDecodeEngine :
 }  // namespace media
 
 #endif  // MEDIA_FILTERS_OMX_VIDEO_DECODE_ENGINE_H_
-
diff --git a/media/filters/omx_video_decoder.cc b/media/filters/omx_video_decoder.cc
index d969cb6..48d2cb6 100644
--- a/media/filters/omx_video_decoder.cc
+++ b/media/filters/omx_video_decoder.cc
@@ -72,11 +72,8 @@ void OmxVideoDecoder::FillThisBuffer(scoped_refptr<VideoFrame> frame) {
                        &OmxVideoDecodeEngine::FillThisBuffer, frame));
 }
 
-void OmxVideoDecoder::Stop() {
-  // TODO(ajwong): This is a total hack. Make async.
-  base::WaitableEvent event(false, false);
-  omx_engine_->Stop(NewCallback(&event, &base::WaitableEvent::Signal));
-  event.Wait();
+void OmxVideoDecoder::Stop(FilterCallback* callback) {
+  omx_engine_->Stop(callback);
 }
 
 void OmxVideoDecoder::DoInitialize(DemuxerStream* demuxer_stream,
diff --git a/media/filters/omx_video_decoder.h b/media/filters/omx_video_decoder.h
index 18c8b6b..236130b 100644
--- a/media/filters/omx_video_decoder.h
+++ b/media/filters/omx_video_decoder.h
@@ -28,7 +28,7 @@ class OmxVideoDecoder : public VideoDecoder {
   virtual ~OmxVideoDecoder();
 
   virtual void Initialize(DemuxerStream* stream, FilterCallback* callback);
-  virtual void Stop();
+  virtual void Stop(FilterCallback* callback);
   virtual void FillThisBuffer(scoped_refptr<VideoFrame> frame);
   virtual const MediaFormat& media_format() { return media_format_; }
 
diff --git a/media/filters/video_decoder_impl_unittest.cc b/media/filters/video_decoder_impl_unittest.cc
index f2161ed..d84067c 100644
--- a/media/filters/video_decoder_impl_unittest.cc
+++ b/media/filters/video_decoder_impl_unittest.cc
@@ -139,7 +139,9 @@ class VideoDecoderImplTest : public testing::Test {
 
   virtual ~VideoDecoderImplTest() {
     // Call Stop() to shut down internal threads.
-    decoder_->Stop();
+    EXPECT_CALL(callback_, OnFilterCallback());
+    EXPECT_CALL(callback_, OnCallbackDestroyed());
+    decoder_->Stop(callback_.NewCallback());
 
     // Finish up any remaining tasks.
     message_loop_.RunAllPending();
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index d3c1daa..13b6dee 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -99,7 +99,7 @@ void VideoRendererBase::Pause(FilterCallback* callback) {
   }
 }
 
-void VideoRendererBase::Stop() {
+void VideoRendererBase::Stop(FilterCallback* callback) {
   AutoLock auto_lock(lock_);
   state_ = kStopped;
 
@@ -119,6 +119,11 @@ void VideoRendererBase::Stop() {
     }
     thread_ = kNullThreadHandle;
   }
+
+  if (callback) {
+    callback->Run();
+    delete callback;
+  }
 }
 
 void VideoRendererBase::SetPlaybackRate(float playback_rate) {
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index 551d234..ba30f52 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -41,7 +41,7 @@ class VideoRendererBase : public VideoRenderer,
   // MediaFilter implementation.
   virtual void Play(FilterCallback* callback);
   virtual void Pause(FilterCallback* callback);
-  virtual void Stop();
+  virtual void Stop(FilterCallback* callback);
   virtual void SetPlaybackRate(float playback_rate);
   virtual void Seek(base::TimeDelta time, FilterCallback* callback);
 
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 111df56..81af7d7 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -65,7 +65,9 @@ class VideoRendererBaseTest : public ::testing::Test {
 
     // Expect a call into the subclass.
     EXPECT_CALL(*renderer_, OnStop());
-    renderer_->Stop();
+    EXPECT_CALL(callback_, OnFilterCallback());
+    EXPECT_CALL(callback_, OnCallbackDestroyed());
+    renderer_->Stop(callback_.NewCallback());
   }
 
  protected: