Revert 59600 - 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

TBR=sergeyu@chromium.org
Review URL: http://codereview.chromium.org/3425008

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

3 files changed, 83 insertions, 83 deletions

diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index b3a42e9..6ff02aa 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -115,12 +115,18 @@ void AudioOutputController::Flush() {
       NewRunnableMethod(this, &AudioOutputController::DoFlush));
 }
 
-void AudioOutputController::Close(Task* closed_task) {
-  DCHECK(closed_task);
-  DCHECK(message_loop_);
+void AudioOutputController::Close() {
+  {
+    AutoLock auto_lock(lock_);
+    // Don't do anything if the stream is already closed.
+    if (state_ == kClosed)
+      return;
+    state_ = kClosed;
+  }
+
   message_loop_->PostTask(
       FROM_HERE,
-      NewRunnableMethod(this, &AudioOutputController::DoClose, closed_task));
+      NewRunnableMethod(this, &AudioOutputController::DoClose));
 }
 
 void AudioOutputController::SetVolume(double volume) {
@@ -141,6 +147,8 @@ 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;
@@ -172,7 +180,6 @@ void AudioOutputController::DoCreate(AudioParameters params,
 
   // If in normal latency mode then start buffering.
   if (!LowLatencyMode()) {
-    AutoLock auto_lock(lock_);
     SubmitOnMoreData_Locked();
   }
 }
@@ -180,25 +187,35 @@ void AudioOutputController::DoCreate(AudioParameters params,
 void AudioOutputController::DoPlay() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  // We can start from created or paused state.
-  if (state_ != kCreated && state_ != kPaused)
-    return;
-  state_ = kPlaying;
+  {
+    AutoLock auto_lock(lock_);
+    // We can start from created or paused state.
+    if (state_ != kCreated && state_ != kPaused)
+      return;
+    state_ = kPlaying;
+  }
 
   // We start the AudioOutputStream lazily.
   stream_->Start(this);
 
-  // Tell the event handler that we are now playing.
-  handler_->OnPlaying(this);
+  {
+    AutoLock auto_lock(lock_);
+    // Tell the event handler that we are now playing.
+    if (state_ != kClosed)
+      handler_->OnPlaying(this);
+  }
 }
 
 void AudioOutputController::DoPause() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
-  // We can pause from started state.
-  if (state_ != kPlaying)
-    return;
-  state_ = kPaused;
+  {
+    AutoLock auto_lock(lock_);
+    // 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.
@@ -210,7 +227,11 @@ void AudioOutputController::DoPause() {
     sync_reader_->UpdatePendingBytes(kPauseMark);
   }
 
-  handler_->OnPaused(this);
+  {
+    AutoLock auto_lock(lock_);
+    if (state_ != kClosed)
+      handler_->OnPaused(this);
+  }
 }
 
 void AudioOutputController::DoFlush() {
@@ -220,30 +241,23 @@ 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(Task* closed_task) {
+void AudioOutputController::DoClose() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
-
-  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;
+  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;
   }
-
-  closed_task->Run();
-  delete closed_task;
 }
 
 void AudioOutputController::DoSetVolume(double volume) {
@@ -253,16 +267,22 @@ void AudioOutputController::DoSetVolume(double volume) {
   // right away but when the stream is created we'll set the volume.
   volume_ = volume;
 
-  if (state_ != kPlaying && state_ != kPaused && state_ != kCreated)
-    return;
+  {
+    AutoLock auto_lock(lock_);
+    if (state_ != kPlaying && state_ != kPaused && state_ != kCreated)
+      return;
+  }
 
   stream_->SetVolume(volume_);
 }
 
 void AudioOutputController::DoReportError(int code) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
-  if (state_ != kClosed)
-    handler_->OnError(this, code);
+  {
+    AutoLock auto_lock(lock_);
+    if (state_ != kClosed)
+      handler_->OnError(this, code);
+  }
 }
 
 uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
@@ -271,6 +291,8 @@ uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
                                          uint32 pending_bytes) {
   // If regular latency mode is used.
   if (!sync_reader_) {
+    AutoLock auto_lock(lock_);
+
     // Record the callback time.
     last_callback_time_ = base::Time::Now();
 
@@ -280,10 +302,8 @@ uint32 AudioOutputController::OnMoreData(AudioOutputStream* stream,
       return 0;
     }
 
-    AutoLock auto_lock(lock_);
-
-    // 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();
@@ -297,8 +317,6 @@ 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();
@@ -325,10 +343,6 @@ 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 8208e28..e9ed321 100644
--- a/media/audio/audio_output_controller.h
+++ b/media/audio/audio_output_controller.h
@@ -14,7 +14,6 @@
 #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
@@ -135,14 +134,13 @@ class AudioOutputController
   // has effect when the stream is paused.
   void Flush();
 
-  // 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.
+  // 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.
   //
   // It is safe to call this method more than once. Calls after the first one
   // will have no effect.
-  void Close(Task* closed_task);
+  void Close();
 
   // Sets the volume of the audio output stream.
   void SetVolume(double volume);
@@ -170,7 +168,7 @@ class AudioOutputController
   void DoPlay();
   void DoPause();
   void DoFlush();
-  void DoClose(Task* closed_task);
+  void DoClose();
   void DoSetVolume(double volume);
   void DoReportError(int code);
 
@@ -192,7 +190,8 @@ class AudioOutputController
   uint32 hardware_pending_bytes_;
   base::Time last_callback_time_;
 
-  // The |lock_| must be acquired whenever we access |push_source_|.
+  // The |lock_| must be acquired whenever we access |state_| or call
+  // |handler_|.
   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 b479974d..db1716c 100644
--- a/media/audio/audio_output_controller_unittest.cc
+++ b/media/audio/audio_output_controller_unittest.cc
@@ -5,7 +5,6 @@
 #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"
@@ -81,18 +80,6 @@ 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;
@@ -105,8 +92,11 @@ TEST(AudioOutputControllerTest, CreateAndClose) {
                                     kHardwareBufferSize, kBufferCapacity);
   ASSERT_TRUE(controller.get());
 
-  // Close the controller immediately.
-  CloseAudioController(controller);
+  // 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();
 }
 
 TEST(AudioOutputControllerTest, PlayAndClose) {
@@ -144,8 +134,9 @@ TEST(AudioOutputControllerTest, PlayAndClose) {
   controller->Play();
   event.Wait();
 
-  // Now stop the controller.
-  CloseAudioController(controller);
+  // Now stop the controller. The object is freed later after DoClose() is
+  // executed.
+  controller->Close();
 }
 
 TEST(AudioOutputControllerTest, PlayPauseClose) {
@@ -195,8 +186,9 @@ TEST(AudioOutputControllerTest, PlayPauseClose) {
   controller->Pause();
   event.Wait();
 
-  // Now stop the controller.
-  CloseAudioController(controller);
+  // Now stop the controller. The object is freed later after DoClose() is
+  // executed.
+  controller->Close();
 }
 
 TEST(AudioOutputControllerTest, PlayPausePlay) {
@@ -258,8 +250,9 @@ TEST(AudioOutputControllerTest, PlayPausePlay) {
   controller->Play();
   event.Wait();
 
-  // Now stop the controller.
-  CloseAudioController(controller);
+  // Now stop the controller. The object is freed later after DoClose() is
+  // executed.
+  controller->Close();
 }
 
 TEST(AudioOutputControllerTest, HardwareBufferTooLarge) {
@@ -309,14 +302,8 @@ TEST(AudioOutputControllerTest, CloseTwice) {
   // Wait for OnMoreData() to be called.
   event.Wait();
 
-  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();
+  controller->Close();
+  controller->Close();
 }
 
 }  // namespace media