Remove completion callbacks from AudioRenderer::Play/Pause().

They're only used as a synchronous signal to start/stop the audio sink.

BUG=144683

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

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

media/base/audio_renderer.h

@@ -47,13 +47,13 @@ class MEDIA_EXPORT AudioRenderer {
                           const base::Closure& ended_cb,
                           const PipelineStatusCB& error_cb) = 0;
 
-  // Start audio decoding and rendering at the current playback rate, executing
-  // |callback| when playback is underway.
-  virtual void Play(const base::Closure& callback) = 0;
+  // Signal audio playback to start at the current rate. It is expected that
+  // |time_cb| will eventually start being run with time updates.
+  virtual void Play() = 0;
 
-  // Temporarily suspend decoding and rendering audio, executing |callback| when
-  // playback has been suspended.
-  virtual void Pause(const base::Closure& callback) = 0;
+  // Signal audio playback to stop until further notice. It is expected that
+  // |time_cb| will no longer be run.
+  virtual void Pause() = 0;
 
   // Discard any audio data, executing |callback| when completed.
   virtual void Flush(const base::Closure& callback) = 0;

media/base/mock_filters.h

@@ -146,8 +146,8 @@ class MockAudioRenderer : public AudioRenderer {
                                 const TimeCB& time_cb,
                                 const base::Closure& ended_cb,
                                 const PipelineStatusCB& error_cb));
-  MOCK_METHOD1(Play, void(const base::Closure& callback));
-  MOCK_METHOD1(Pause, void(const base::Closure& callback));
+  MOCK_METHOD0(Play, void());
+  MOCK_METHOD0(Pause, void());
   MOCK_METHOD1(Flush, void(const base::Closure& callback));
   MOCK_METHOD1(Stop, void(const base::Closure& callback));
   MOCK_METHOD1(SetPlaybackRate, void(float playback_rate));

media/base/pipeline_unittest.cc

@@ -208,8 +208,7 @@ class PipelineTest : public ::testing::Test {
         // Startup sequence.
         EXPECT_CALL(*audio_renderer_, Preroll(base::TimeDelta(), _))
             .WillOnce(RunCallback<1>(PIPELINE_OK));
-        EXPECT_CALL(*audio_renderer_, Play(_))
-            .WillOnce(RunClosure<0>());
+        EXPECT_CALL(*audio_renderer_, Play());
       }
       EXPECT_CALL(callbacks_, OnPrerollCompleted());
     }
@@ -259,16 +258,14 @@ class PipelineTest : public ::testing::Test {
         .WillOnce(RunCallback<1>(PIPELINE_OK));
 
     if (audio_stream_) {
-      EXPECT_CALL(*audio_renderer_, Pause(_))
-          .WillOnce(RunClosure<0>());
+      EXPECT_CALL(*audio_renderer_, Pause());
       EXPECT_CALL(*audio_renderer_, Flush(_))
           .WillOnce(RunClosure<0>());
       EXPECT_CALL(*audio_renderer_, Preroll(seek_time, _))
           .WillOnce(RunCallback<1>(PIPELINE_OK));
       EXPECT_CALL(*audio_renderer_, SetPlaybackRate(_));
       EXPECT_CALL(*audio_renderer_, SetVolume(_));
-      EXPECT_CALL(*audio_renderer_, Play(_))
-          .WillOnce(RunClosure<0>());
+      EXPECT_CALL(*audio_renderer_, Play());
     }
 
     if (video_stream_) {
@@ -669,8 +666,7 @@ TEST_F(PipelineTest, ErrorDuringSeek) {
   base::TimeDelta seek_time = base::TimeDelta::FromSeconds(5);
 
   // Preroll() isn't called as the demuxer errors out first.
-  EXPECT_CALL(*audio_renderer_, Pause(_))
-      .WillOnce(RunClosure<0>());
+  EXPECT_CALL(*audio_renderer_, Pause());
   EXPECT_CALL(*audio_renderer_, Flush(_))
       .WillOnce(RunClosure<0>());
   EXPECT_CALL(*audio_renderer_, Stop(_))
@@ -724,8 +720,7 @@ TEST_F(PipelineTest, NoMessageDuringTearDownFromError) {
   base::TimeDelta seek_time = base::TimeDelta::FromSeconds(5);
 
   // Seek() isn't called as the demuxer errors out first.
-  EXPECT_CALL(*audio_renderer_, Pause(_))
-      .WillOnce(RunClosure<0>());
+  EXPECT_CALL(*audio_renderer_, Pause());
   EXPECT_CALL(*audio_renderer_, Flush(_))
       .WillOnce(RunClosure<0>());
   EXPECT_CALL(*audio_renderer_, Stop(_))
@@ -815,16 +810,14 @@ TEST_F(PipelineTest, AudioTimeUpdateDuringSeek) {
       .WillOnce(DoAll(InvokeWithoutArgs(&closure, &base::Closure::Run),
                       RunCallback<1>(PIPELINE_OK)));
 
-  EXPECT_CALL(*audio_renderer_, Pause(_))
-      .WillOnce(RunClosure<0>());
+  EXPECT_CALL(*audio_renderer_, Pause());
   EXPECT_CALL(*audio_renderer_, Flush(_))
       .WillOnce(RunClosure<0>());
   EXPECT_CALL(*audio_renderer_, Preroll(seek_time, _))
       .WillOnce(RunCallback<1>(PIPELINE_OK));
   EXPECT_CALL(*audio_renderer_, SetPlaybackRate(_));
   EXPECT_CALL(*audio_renderer_, SetVolume(_));
-  EXPECT_CALL(*audio_renderer_, Play(_))
-      .WillOnce(RunClosure<0>());
+  EXPECT_CALL(*audio_renderer_, Play());
 
   EXPECT_CALL(callbacks_, OnPrerollCompleted());
   EXPECT_CALL(callbacks_, OnSeek(PIPELINE_OK));
@@ -1010,8 +1003,7 @@ class PipelineTeardownTest : public PipelineTest {
     EXPECT_CALL(*video_renderer_, SetPlaybackRate(0.0f));
     EXPECT_CALL(*audio_renderer_, SetVolume(1.0f));
 
-    EXPECT_CALL(*audio_renderer_, Play(_))
-        .WillOnce(RunClosure<0>());
+    EXPECT_CALL(*audio_renderer_, Play());
     EXPECT_CALL(*video_renderer_, Play(_))
         .WillOnce(RunClosure<0>());
 
@@ -1047,18 +1039,18 @@ class PipelineTeardownTest : public PipelineTest {
 
     if (state == kPausing) {
       if (stop_or_error == kStop) {
-        EXPECT_CALL(*audio_renderer_, Pause(_))
-            .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb), RunClosure<0>()));
+        EXPECT_CALL(*audio_renderer_, Pause())
+            .WillOnce(Stop(pipeline_.get(), stop_cb));
       } else {
         status = PIPELINE_ERROR_READ;
-        EXPECT_CALL(*audio_renderer_, Pause(_)).WillOnce(
-            DoAll(SetError(pipeline_.get(), status), RunClosure<0>()));
+        EXPECT_CALL(*audio_renderer_, Pause())
+            .WillOnce(SetError(pipeline_.get(), status));
       }
 
       return status;
     }
 
-    EXPECT_CALL(*audio_renderer_, Pause(_)).WillOnce(RunClosure<0>());
+    EXPECT_CALL(*audio_renderer_, Pause());
     EXPECT_CALL(*video_renderer_, Pause(_)).WillOnce(RunClosure<0>());
 
     if (state == kFlushing) {
@@ -1120,12 +1112,12 @@ class PipelineTeardownTest : public PipelineTest {
 
     if (state == kStarting) {
       if (stop_or_error == kStop) {
-        EXPECT_CALL(*audio_renderer_, Play(_))
-            .WillOnce(DoAll(Stop(pipeline_.get(), stop_cb), RunClosure<0>()));
+        EXPECT_CALL(*audio_renderer_, Play())
+            .WillOnce(Stop(pipeline_.get(), stop_cb));
       } else {
         status = PIPELINE_ERROR_READ;
-        EXPECT_CALL(*audio_renderer_, Play(_)).WillOnce(
-            DoAll(SetError(pipeline_.get(), status), RunClosure<0>()));
+        EXPECT_CALL(*audio_renderer_, Play())
+            .WillOnce(SetError(pipeline_.get(), status));
       }
       return status;
     }

media/base/serial_runner.cc

@@ -12,6 +12,14 @@
 
 namespace media {
 
+// Converts a Closure into a bound function accepting a PipelineStatusCB.
+static void RunClosure(
+    const base::Closure& closure,
+    const PipelineStatusCB& status_cb) {
+  closure.Run();
+  status_cb.Run(PIPELINE_OK);
+}
+
 // Converts a bound function accepting a Closure into a bound function
 // accepting a PipelineStatusCB. Since closures have no way of reporting a
 // status |status_cb| is executed with PIPELINE_OK.
@@ -34,6 +42,10 @@ static void RunOnTaskRunner(
 SerialRunner::Queue::Queue() {}
 SerialRunner::Queue::~Queue() {}
 
+void SerialRunner::Queue::Push(const base::Closure& closure) {
+  bound_fns_.push(base::Bind(&RunClosure, closure));
+}
+
 void SerialRunner::Queue::Push(
     const BoundClosure& bound_closure) {
   bound_fns_.push(base::Bind(&RunBoundClosure, bound_closure));

media/base/serial_runner.h

@@ -34,6 +34,7 @@ class MEDIA_EXPORT SerialRunner {
     Queue();
     ~Queue();
 
+    void Push(const base::Closure& closure);
     void Push(const BoundClosure& bound_fn);
     void Push(const BoundPipelineStatusCB& bound_fn);
 

media/base/serial_runner_unittest.cc

@@ -35,6 +35,20 @@ class SerialRunnerTest : public ::testing::Test {
     called_.push_back(false);
   }
 
+  void PushBoundClosure() {
+    bound_fns_.Push(base::Bind(&SerialRunnerTest::RunBoundClosure,
+                               base::Unretained(this),
+                               called_.size()));
+    called_.push_back(false);
+  }
+
+  void PushClosure() {
+    bound_fns_.Push(base::Bind(&SerialRunnerTest::RunClosure,
+                               base::Unretained(this),
+                               called_.size()));
+    called_.push_back(false);
+  }
+
   // Push a bound function to the queue that will delete the SerialRunner,
   // which should cancel all remaining queued work.
   void PushCancellation() {
@@ -61,6 +75,26 @@ class SerialRunnerTest : public ::testing::Test {
     status_cb.Run(status);
   }
 
+  void RunBoundClosure(size_t index,
+                       const base::Closure& done_cb) {
+    EXPECT_EQ(index == 0u, inside_start_)
+        << "First bound function should run on same stack as "
+        << "SerialRunner::Run() while all others should not\n"
+        << base::debug::StackTrace().ToString();
+
+    called_[index] = true;
+    done_cb.Run();
+  }
+
+  void RunClosure(size_t index) {
+    EXPECT_EQ(index == 0u, inside_start_)
+        << "First bound function should run on same stack as "
+        << "SerialRunner::Run() while all others should not\n"
+        << base::debug::StackTrace().ToString();
+
+    called_[index] = true;
+  }
+
   void StartRunnerInternal(const SerialRunner::Queue& bound_fns) {
     inside_start_ = true;
     runner_ = SerialRunner::Run(bound_fns_, base::Bind(
@@ -173,4 +207,22 @@ TEST_F(SerialRunnerTest, Multiple_Cancel) {
   EXPECT_FALSE(done_called());
 }
 
+TEST_F(SerialRunnerTest, BoundClosure) {
+  PushBoundClosure();
+  RunSerialRunner();
+
+  EXPECT_TRUE(called(0));
+  EXPECT_TRUE(done_called());
+  EXPECT_EQ(PIPELINE_OK, done_status());
+}
+
+TEST_F(SerialRunnerTest, Closure) {
+  PushClosure();
+  RunSerialRunner();
+
+  EXPECT_TRUE(called(0));
+  EXPECT_TRUE(done_called());
+  EXPECT_EQ(PIPELINE_OK, done_status());
+}
+
 }  // namespace media

media/filters/audio_renderer_impl.cc

@@ -73,13 +73,12 @@ AudioRendererImpl::~AudioRendererImpl() {
   DCHECK(!algorithm_.get());
 }
 
-void AudioRendererImpl::Play(const base::Closure& callback) {
+void AudioRendererImpl::Play() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(state_, kPaused);
   ChangeState_Locked(kPlaying);
-  callback.Run();
   earliest_end_time_ = now_cb_.Run();
 
   if (algorithm_->playback_rate() != 0)
@@ -104,7 +103,7 @@ void AudioRendererImpl::DoPlay_Locked() {
   }
 }
 
-void AudioRendererImpl::Pause(const base::Closure& callback) {
+void AudioRendererImpl::Pause() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
@@ -113,8 +112,6 @@ void AudioRendererImpl::Pause(const base::Closure& callback) {
   ChangeState_Locked(kPaused);
 
   DoPause_Locked();
-
-  callback.Run();
 }
 
 void AudioRendererImpl::DoPause_Locked() {

media/filters/audio_renderer_impl.h

@@ -72,8 +72,8 @@ class MEDIA_EXPORT AudioRendererImpl
                           const TimeCB& time_cb,
                           const base::Closure& ended_cb,
                           const PipelineStatusCB& error_cb) OVERRIDE;
-  virtual void Play(const base::Closure& callback) OVERRIDE;
-  virtual void Pause(const base::Closure& callback) OVERRIDE;
+  virtual void Play() OVERRIDE;
+  virtual void Pause() OVERRIDE;
   virtual void Flush(const base::Closure& callback) OVERRIDE;
   virtual void Stop(const base::Closure& callback) OVERRIDE;
   virtual void SetPlaybackRate(float rate) OVERRIDE;

media/filters/audio_renderer_impl_unittest.cc

@@ -229,17 +229,12 @@ class AudioRendererImplTest : public ::testing::Test {
   }
 
   void Play() {
-    SCOPED_TRACE("Play()");
-    WaitableMessageLoopEvent event;
-    renderer_->Play(event.GetClosure());
+    renderer_->Play();
     renderer_->SetPlaybackRate(1.0f);
-    event.RunAndWait();
   }
 
   void Pause() {
-    WaitableMessageLoopEvent pause_event;
-    renderer_->Pause(pause_event.GetClosure());
-    pause_event.RunAndWait();
+    renderer_->Pause();
   }
 
   void Seek() {
@@ -766,31 +761,6 @@ TEST_F(AudioRendererImplTest, AbortPendingRead_Preroll) {
   Preroll(1000, PIPELINE_OK);
 }
 
-TEST_F(AudioRendererImplTest, AbortPendingRead_Pause) {
-  Initialize();
-
-  Preroll();
-  Play();
-
-  // Partially drain internal buffer so we get a pending read.
-  EXPECT_TRUE(ConsumeBufferedData(frames_buffered() / 2, NULL));
-  WaitForPendingRead();
-
-  // Start pausing.
-  WaitableMessageLoopEvent event;
-  renderer_->Pause(event.GetClosure());
-
-  // Simulate the decoder aborting the pending read.
-  AbortPendingRead();
-  event.RunAndWait();
-
-  Flush();
-
-  // Preroll again to a different timestamp and verify it completed normally.
-  Preroll(1000, PIPELINE_OK);
-}
-
-
 TEST_F(AudioRendererImplTest, AbortPendingRead_Flush) {
   Initialize();
 
@@ -819,30 +789,6 @@ TEST_F(AudioRendererImplTest, AbortPendingRead_Flush) {
   Preroll(1000, PIPELINE_OK);
 }
 
-TEST_F(AudioRendererImplTest, PendingRead_Pause) {
-  Initialize();
-
-  Preroll();
-  Play();
-
-  // Partially drain internal buffer so we get a pending read.
-  EXPECT_TRUE(ConsumeBufferedData(frames_buffered() / 2, NULL));
-  WaitForPendingRead();
-
-  // Start pausing.
-  WaitableMessageLoopEvent event;
-  renderer_->Pause(event.GetClosure());
-
-  SatisfyPendingRead(kDataSize);
-
-  event.RunAndWait();
-
-  Flush();
-
-  // Preroll again to a different timestamp and verify it completed normally.
-  Preroll(1000, PIPELINE_OK);
-}
-
 TEST_F(AudioRendererImplTest, PendingRead_Flush) {
   Initialize();