Make AudioOutputController.Close() truly asynchronous.

Added closed_task parameter in this method. This parameter is used to
notify the caller when the stream is actually closed. Callbacks may
be called until closed_task is executed.

BUG=55755
TEST=Unittests, audio still works, doesn't crash

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

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

3 files changed, 81 insertions, 81 deletions

diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index 6ff02aa..439583c 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -115,18 +115,12 @@ void AudioOutputController::Flush() {
       NewRunnableMethod(this, &AudioOutputController::DoFlush));
 }
 
-void AudioOutputController::Close() {
-  {
-    AutoLock auto_lock(lock_);
-    // Don't do anything if the stream is already closed.
-    if (state_ == kClosed)
-      return;
-    state_ = kClosed;
-  }
-
+void AudioOutputController::Close(Task* closed_task) {
+  DCHECK(closed_task);
+  DCHECK(message_loop_);
   message_loop_->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &AudioOutputController::DoClose));
+      NewRunnableMethod(this, &AudioOutputController::DoClose, closed_task));
 }
 
 void AudioOutputController::SetVolume(double volume) {
@@ -147,8 +141,6 @@ void AudioOutputController::DoCreate(AudioParameters params,
                                      uint32 hardware_buffer_size) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  AutoLock auto_lock(lock_);
-
   // Close() can be called before DoCreate() is executed.
   if (state_ == kClosed)
     return;
@@ -180,6 +172,7 @@ void AudioOutputController::DoCreate(AudioParameters params,
 
   // If in normal latency mode then start buffering.
   if (!LowLatencyMode()) {
+    AutoLock auto_lock(lock_);
     SubmitOnMoreData_Locked();
   }
 }
@@ -187,35 +180,25 @@ void AudioOutputController::DoCreate(AudioParameters params,
 void AudioOutputController::DoPlay() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  {
-    AutoLock auto_lock(lock_);
-    // We can start from created or paused state.
-    if (state_ != kCreated && state_ != kPaused)
-      return;
-    state_ = kPlaying;
-  }
+  // We can start from created or paused state.
+  if (state_ != kCreated && state_ != kPaused)
+    return;
+  state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
   stream_->Start(this);
 
-  {
-    AutoLock auto_lock(lock_);
-    // Tell the event handler that we are now playing.
-    if (state_ != kClosed)
-      handler_->OnPlaying(this);
-  }
+  // Tell the event handler that we are now playing.
+  handler_->OnPlaying(this);
 }
 
 void AudioOutputController::DoPause() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  {
-    AutoLock auto_lock(lock_);
-    // We can pause from started state.
-    if (state_ != kPlaying)
-      return;
-    state_ = kPaused;
-  }
+  // We can pause from started state.
+  if (state_ != kPlaying)
+    return;
+  state_ = kPaused;
 
   // Then we stop the audio device. This is not the perfect solution because
   // it discards all the internal buffer in the audio device.
@@ -227,11 +210,7 @@ void AudioOutputController::DoPause() {
     sync_reader_->UpdatePendingBytes(kPauseMark);
   }
 
-  {
-    AutoLock auto_lock(lock_);
-    if (state_ != kClosed)
-      handler_->OnPaused(this);
-  }
+  handler_->OnPaused(this);
 }
 
 void AudioOutputController::DoFlush() {
@@ -241,23 +220,30 @@ void AudioOutputController::DoFlush() {
 
   // If we are in the regular latency mode then flush the push source.
   if (!sync_reader_) {
-    AutoLock auto_lock(lock_);
     if (state_ != kPaused)
       return;
     push_source_.ClearAll();
   }
 }
 
-void AudioOutputController::DoClose() {
+void AudioOutputController::DoClose(Task* closed_task) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
-  DCHECK_EQ(kClosed, state_);
-  // |stream_| can be null if creating the device failed in DoCreate().
-  if (stream_) {
-    stream_->Stop();
-    stream_->Close();
-    // After stream is closed it is destroyed, so don't keep a reference to it.
-    stream_ = NULL;
+
+  if (state_ != kClosed) {
+    // |stream_| can be null if creating the device failed in DoCreate().
+    if (stream_) {
+      stream_->Stop();
+      stream_->Close();
+      // After stream is closed it is destroyed, so don't keep a reference to
+      // it.
+      stream_ = NULL;
+    }
+
+    state_ = kClosed;
   }
+
+  closed_task->Run();
+  delete closed_task;
 }
 
 void AudioOutputController::DoSetVolume(double volume) {
@@ -267,22 +253,16 @@ void AudioOutputController::DoSetVolume(double volume) {
   // right away but when the stream is created we'll set the volume.
   volume_ = volume;
 
-  {
-    AutoLock auto_lock(lock_);
-    if (state_ != kPlaying && state_ != kPaused && state_ != kCreated)
-      return;
-  }
+  if (state_ != kPlaying && state_ != kPaused && state_ != kCreated)
+    return;
 
   stream_->SetVolume(volume_);
 }
 
 void AudioOutputController::DoReportError(int code) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
-  {
-    AutoLock auto_lock(lock_);
-    if (state_ != kClosed)
-      handler_->OnError(this, code);
-  }
+  if (state_ != kClosed)
+    handler_->OnError(this, code);
 }
 
 uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
@@ -302,8 +282,8 @@ uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
       return 0;
     }
 
-    // Push source doesn't need to know the stream and number of pending bytes.
-    // So just pass in NULL and 0.
+    // Push source doesn't need to know the stream and number of pending
+    // bytes. So just pass in NULL and 0.
     uint32 size = push_source_.OnMoreData(NULL, dest, max_size, 0);
     hardware_pending_bytes_ = pending_bytes + size;
     SubmitOnMoreData_Locked();
@@ -317,6 +297,8 @@ uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
 }
 
 void AudioOutputController::OnClose(AudioOutputStream* stream) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
   // Push source doesn't need to know the stream so just pass in NULL.
   if (LowLatencyMode()) {
     sync_reader_->Close();
@@ -343,6 +325,10 @@ void AudioOutputController::SubmitOnMoreData_Locked() {
   uint32 pending_bytes = hardware_pending_bytes_ +
       push_source_.UnProcessedBytes();
 
+  // If we need more data then call the event handler to ask for more data.
+  // It is okay that we don't lock in this block because the parameters are
+  // correct and in the worst case we are just asking more data than needed.
+  AutoUnlock auto_unlock(lock_);
   handler_->OnMoreData(this, timestamp, pending_bytes);
 }
 
diff --git a/media/audio/audio_output_controller.h b/media/audio/audio_output_controller.h
index e9ed321..8208e28 100644
--- a/media/audio/audio_output_controller.h
+++ b/media/audio/audio_output_controller.h
@@ -14,6 +14,7 @@
 #include "media/audio/simple_sources.h"
 
 class MessageLoop;
+class Task;
 
 // An AudioOutputController controls an AudioOutputStream and provides data
 // to this output stream. It has an important function that it executes
@@ -134,13 +135,14 @@ class AudioOutputController
   // has effect when the stream is paused.
   void Flush();
 
-  // Closes the audio output stream. It changes state to kClosed and returns
-  // right away. The physical resources are freed on the audio thread if
-  // neccessary.
+  // Closes the audio output stream. The state is changed and the resources
+  // are freed on the audio thread. closed_task is executed after that.
+  // Callbacks (EventHandler and SyncReader) must exist until closed_task is
+  // called.
   //
   // It is safe to call this method more than once. Calls after the first one
   // will have no effect.
-  void Close();
+  void Close(Task* closed_task);
 
   // Sets the volume of the audio output stream.
   void SetVolume(double volume);
@@ -168,7 +170,7 @@ class AudioOutputController
   void DoPlay();
   void DoPause();
   void DoFlush();
-  void DoClose();
+  void DoClose(Task* closed_task);
   void DoSetVolume(double volume);
   void DoReportError(int code);
 
@@ -190,8 +192,7 @@ class AudioOutputController
   uint32 hardware_pending_bytes_;
   base::Time last_callback_time_;
 
-  // The |lock_| must be acquired whenever we access |state_| or call
-  // |handler_|.
+  // The |lock_| must be acquired whenever we access |push_source_|.
   Lock lock_;
 
   // PushSource role is to buffer and it's only used in regular latency mode.
diff --git a/media/audio/audio_output_controller_unittest.cc b/media/audio/audio_output_controller_unittest.cc
index db1716c..b479974d 100644
--- a/media/audio/audio_output_controller_unittest.cc
+++ b/media/audio/audio_output_controller_unittest.cc
@@ -5,6 +5,7 @@
 #include "base/environment.h"
 #include "base/basictypes.h"
 #include "base/logging.h"
+#include "base/task.h"
 #include "base/waitable_event.h"
 #include "media/audio/audio_output_controller.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -80,6 +81,18 @@ ACTION_P3(SignalEvent, event, count, limit) {
   }
 }
 
+// Helper functions used to close audio controller.
+static void SignalClosedEvent(base::WaitableEvent* event) {
+  event->Signal();
+}
+
+// Closes AudioOutputController synchronously.
+static void CloseAudioController(AudioOutputController* controller) {
+  base::WaitableEvent closed_event(true, false);
+  controller->Close(NewRunnableFunction(&SignalClosedEvent, &closed_event));
+  closed_event.Wait();
+}
+
 TEST(AudioOutputControllerTest, CreateAndClose) {
   if (!HasAudioOutputDevices() || IsRunningHeadless())
     return;
@@ -92,11 +105,8 @@ TEST(AudioOutputControllerTest, CreateAndClose) {
                                     kHardwareBufferSize, kBufferCapacity);
   ASSERT_TRUE(controller.get());
 
-  // Close the controller immediately. At this point, chances are that
-  // DoCreate() hasn't been called yet. In any case, it should be safe to call
-  // Close() and it should not try to call |event_handler| later (the test
-  // would crash otherwise).
-  controller->Close();
+  // Close the controller immediately.
+  CloseAudioController(controller);
 }
 
 TEST(AudioOutputControllerTest, PlayAndClose) {
@@ -134,9 +144,8 @@ TEST(AudioOutputControllerTest, PlayAndClose) {
   controller->Play();
   event.Wait();
 
-  // Now stop the controller. The object is freed later after DoClose() is
-  // executed.
-  controller->Close();
+  // Now stop the controller.
+  CloseAudioController(controller);
 }
 
 TEST(AudioOutputControllerTest, PlayPauseClose) {
@@ -186,9 +195,8 @@ TEST(AudioOutputControllerTest, PlayPauseClose) {
   controller->Pause();
   event.Wait();
 
-  // Now stop the controller. The object is freed later after DoClose() is
-  // executed.
-  controller->Close();
+  // Now stop the controller.
+  CloseAudioController(controller);
 }
 
 TEST(AudioOutputControllerTest, PlayPausePlay) {
@@ -250,9 +258,8 @@ TEST(AudioOutputControllerTest, PlayPausePlay) {
   controller->Play();
   event.Wait();
 
-  // Now stop the controller. The object is freed later after DoClose() is
-  // executed.
-  controller->Close();
+  // Now stop the controller.
+  CloseAudioController(controller);
 }
 
 TEST(AudioOutputControllerTest, HardwareBufferTooLarge) {
@@ -302,8 +309,14 @@ TEST(AudioOutputControllerTest, CloseTwice) {
   // Wait for OnMoreData() to be called.
   event.Wait();
 
-  controller->Close();
-  controller->Close();
+  base::WaitableEvent closed_event_1(true, false);
+  controller->Close(NewRunnableFunction(&SignalClosedEvent, &closed_event_1));
+
+  base::WaitableEvent closed_event_2(true, false);
+  controller->Close(NewRunnableFunction(&SignalClosedEvent, &closed_event_2));
+
+  closed_event_1.Wait();
+  closed_event_2.Wait();
 }
 
 }  // namespace media