Make DataSource::Stop() synchronous.

In a lot of filters in media code, Stop() has been folded into the dtor.
This model doesn't apply directly to DataSource because DataSource::Stop()
is called by the FFmpegDemuxer, but the DataSource is owned by
WebMediaPlayerImpl.

TBR=gbillock@chromium.org
BUG=349211
TEST=All existing tests pass.

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

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

chrome/utility/media_galleries/ipc_data_source.cc

@@ -21,9 +21,8 @@ IPCDataSource::~IPCDataSource() {
   DCHECK(utility_thread_checker_.CalledOnValidThread());
 }
 
-void IPCDataSource::Stop(const base::Closure& callback) {
+void IPCDataSource::Stop() {
   DCHECK(data_source_thread_checker_.CalledOnValidThread());
-  callback.Run();
 }
 
 void IPCDataSource::Read(int64 position, int size, uint8* data,

chrome/utility/media_galleries/ipc_data_source.h

@@ -32,7 +32,7 @@ class IPCDataSource: public media::DataSource,
 
   // Implementation of DataSource. These methods may be called on any single
   // thread. First usage of these methods attaches a thread checker.
-  virtual void Stop(const base::Closure& callback) OVERRIDE;
+  virtual void Stop() OVERRIDE;
   virtual void Read(int64 position, int size, uint8* data,
                     const ReadCB& read_cb) OVERRIDE;
   virtual bool GetSize(int64* size_out) OVERRIDE;

content/renderer/media/buffered_data_source.cc

@@ -207,12 +207,11 @@ void BufferedDataSource::MediaIsPaused() {
 
 /////////////////////////////////////////////////////////////////////////////
 // media::DataSource implementation.
-void BufferedDataSource::Stop(const base::Closure& closure) {
+void BufferedDataSource::Stop() {
   {
     base::AutoLock auto_lock(lock_);
     StopInternal_Locked();
   }
-  closure.Run();
 
   render_loop_->PostTask(
       FROM_HERE,

content/renderer/media/buffered_data_source.h

@@ -98,7 +98,7 @@ class CONTENT_EXPORT BufferedDataSource : public media::DataSource {
 
   // media::DataSource implementation.
   // Called from demuxer thread.
-  virtual void Stop(const base::Closure& closure) OVERRIDE;
+  virtual void Stop() OVERRIDE;
 
   virtual void Read(int64 position, int size, uint8* data,
                     const media::DataSource::ReadCB& read_cb) OVERRIDE;

content/renderer/media/buffered_data_source_unittest.cc

@@ -178,7 +178,7 @@ class BufferedDataSourceTest : public testing::Test {
       message_loop_.RunUntilIdle();
     }
 
-    data_source_->Stop(media::NewExpectedClosure());
+    data_source_->Stop();
     message_loop_.RunUntilIdle();
   }
 
@@ -505,31 +505,6 @@ TEST_F(BufferedDataSourceTest, File_Successful) {
   Stop();
 }
 
-static void SetTrue(bool* value) {
-  *value = true;
-}
-
-// This test makes sure that Stop() does not require a task to run on
-// |message_loop_| before it calls its callback. This prevents accidental
-// introduction of a pipeline teardown deadlock. The pipeline owner blocks
-// the render message loop while waiting for Stop() to complete. Since this
-// object runs on the render message loop, Stop() will not complete if it
-// requires a task to run on the the message loop that is being blocked.
-TEST_F(BufferedDataSourceTest, StopDoesNotUseMessageLoopForCallback) {
-  InitializeWith206Response();
-
-  // Stop() the data source, using a callback that lets us verify that it was
-  // called before Stop() returns. This is to make sure that the callback does
-  // not require |message_loop_| to execute tasks before being called.
-  bool stop_done_called = false;
-  EXPECT_TRUE(data_source_->loading());
-  data_source_->Stop(base::Bind(&SetTrue, &stop_done_called));
-
-  // Verify that the callback was called inside the Stop() call.
-  EXPECT_TRUE(stop_done_called);
-  message_loop_.RunUntilIdle();
-}
-
 TEST_F(BufferedDataSourceTest, StopDuringRead) {
   InitializeWith206Response();
 
@@ -541,7 +516,7 @@ TEST_F(BufferedDataSourceTest, StopDuringRead) {
   {
     InSequence s;
     EXPECT_CALL(*this, ReadCallback(media::DataSource::kReadError));
-    data_source_->Stop(media::NewExpectedClosure());
+    data_source_->Stop();
   }
   message_loop_.RunUntilIdle();
 }

media/base/data_source.h

@@ -28,7 +28,7 @@ class MEDIA_EXPORT DataSource {
 
   // Stops the DataSource. Once this is called all future Read() calls will
   // return an error.
-  virtual void Stop(const base::Closure& callback) = 0;
+  virtual void Stop() = 0;
 
   // Returns true and the file size, false if the file size could not be
   // retrieved.

media/filters/blocking_url_protocol_unittest.cc

@@ -24,10 +24,7 @@ class BlockingUrlProtocolTest : public testing::Test {
   }
 
   virtual ~BlockingUrlProtocolTest() {
-    base::WaitableEvent stop_event(false, false);
-    data_source_.Stop(base::Bind(
-        &base::WaitableEvent::Signal, base::Unretained(&stop_event)));
-    stop_event.Wait();
+    data_source_.Stop();
   }
 
   MOCK_METHOD0(OnDataSourceError, void());

media/filters/ffmpeg_demuxer.cc

@@ -530,11 +530,22 @@ FFmpegDemuxer::~FFmpegDemuxer() {}
 void FFmpegDemuxer::Stop(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   url_protocol_->Abort();
-  data_source_->Stop(
-      BindToCurrentLoop(base::Bind(&FFmpegDemuxer::OnDataSourceStopped,
-                                   weak_factory_.GetWeakPtr(),
-                                   BindToCurrentLoop(callback))));
+  data_source_->Stop();
+
+  // This will block until all tasks complete. Note that after this returns it's
+  // possible for reply tasks (e.g., OnReadFrameDone()) to be queued on this
+  // thread. Each of the reply task methods must check whether we've stopped the
+  // thread and drop their results on the floor.
+  blocking_thread_.Stop();
+
+  StreamVector::iterator iter;
+  for (iter = streams_.begin(); iter != streams_.end(); ++iter) {
+    if (*iter)
+      (*iter)->Stop();
+  }
+
   data_source_ = NULL;
+  task_runner_->PostTask(FROM_HERE, callback);
 }
 
 void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
@@ -1120,23 +1131,6 @@ void FFmpegDemuxer::OnReadFrameDone(ScopedAVPacket packet, int result) {
   ReadFrameIfNeeded();
 }
 
-void FFmpegDemuxer::OnDataSourceStopped(const base::Closure& callback) {
-  // This will block until all tasks complete. Note that after this returns it's
-  // possible for reply tasks (e.g., OnReadFrameDone()) to be queued on this
-  // thread. Each of the reply task methods must check whether we've stopped the
-  // thread and drop their results on the floor.
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  blocking_thread_.Stop();
-
-  StreamVector::iterator iter;
-  for (iter = streams_.begin(); iter != streams_.end(); ++iter) {
-    if (*iter)
-      (*iter)->Stop();
-  }
-
-  callback.Run();
-}
-
 bool FFmpegDemuxer::StreamsHaveAvailableCapacity() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   StreamVector::iterator iter;

media/filters/ffmpeg_demuxer.h

@@ -197,9 +197,6 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   void ReadFrameIfNeeded();
   void OnReadFrameDone(ScopedAVPacket packet, int result);
 
-  // DataSource callbacks during stopping.
-  void OnDataSourceStopped(const base::Closure& callback);
-
   // Returns true iff any stream has additional capacity. Note that streams can
   // go over capacity depending on how the file is muxed.
   bool StreamsHaveAvailableCapacity();

media/filters/file_data_source.cc

@@ -26,8 +26,7 @@ bool FileDataSource::Initialize(const base::FilePath& file_path) {
   return file_.Initialize(file_path);
 }
 
-void FileDataSource::Stop(const base::Closure& callback) {
-  callback.Run();
+void FileDataSource::Stop() {
 }
 
 void FileDataSource::Read(int64 position, int size, uint8* data,

media/filters/file_data_source.h

@@ -25,7 +25,7 @@ class MEDIA_EXPORT FileDataSource : public DataSource {
   bool Initialize(const base::FilePath& file_path);
 
   // Implementation of DataSource.
-  virtual void Stop(const base::Closure& callback) OVERRIDE;
+  virtual void Stop() OVERRIDE;
   virtual void Read(int64 position, int size, uint8* data,
                     const DataSource::ReadCB& read_cb) OVERRIDE;
   virtual bool GetSize(int64* size_out) OVERRIDE;

media/filters/file_data_source_unittest.cc

@@ -77,7 +77,7 @@ TEST(FileDataSourceTest, ReadData) {
       &ReadCBHandler::ReadCB, base::Unretained(&handler)));
   EXPECT_EQ('5', ten_bytes[0]);
 
-  data_source.Stop(NewExpectedClosure());
+  data_source.Stop();
 }
 
 }  // namespace media

media/tools/player_x11/data_source_logger.cc

@@ -6,11 +6,6 @@
 #include "base/logging.h"
 #include "media/tools/player_x11/data_source_logger.h"
 
-static void LogAndRunStopClosure(const base::Closure& closure) {
-  VLOG(1) << "Stop() finished";
-  closure.Run();
-}
-
 static void LogAndRunReadCB(
     int64 position, int size,
     const media::DataSource::ReadCB& read_cb, int result) {
@@ -25,9 +20,9 @@ DataSourceLogger::DataSourceLogger(
       streaming_(streaming) {
 }
 
-void DataSourceLogger::Stop(const base::Closure& closure) {
-  VLOG(1) << "Stop() started";
-  data_source_->Stop(base::Bind(&LogAndRunStopClosure, closure));
+void DataSourceLogger::Stop() {
+  VLOG(1) << "Stop()";
+  data_source_->Stop();
 }
 
 void DataSourceLogger::Read(

media/tools/player_x11/data_source_logger.h

@@ -22,7 +22,7 @@ class DataSourceLogger : public media::DataSource {
   virtual ~DataSourceLogger();
 
   // media::DataSource implementation.
-  virtual void Stop(const base::Closure& closure) OVERRIDE;
+  virtual void Stop() OVERRIDE;
   virtual void Read(
       int64 position, int size, uint8* data,
       const media::DataSource::ReadCB& read_cb) OVERRIDE;