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@59600 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 130 insertions, 114 deletions

diff --git a/chrome/browser/renderer_host/audio_renderer_host.cc b/chrome/browser/renderer_host/audio_renderer_host.cc
index 3219333..b4ba6ba 100644
--- a/chrome/browser/renderer_host/audio_renderer_host.cc
+++ b/chrome/browser/renderer_host/audio_renderer_host.cc
@@ -335,7 +335,14 @@ void AudioRendererHost::OnCreateStream(
   }
 
   scoped_ptr<AudioEntry> entry(new AudioEntry());
-  scoped_refptr<media::AudioOutputController> controller = NULL;
+  // Create the shared memory and share with the renderer process.
+  if (!entry->shared_memory.Create(L"", false, false, hardware_packet_size) ||
+      !entry->shared_memory.Map(entry->shared_memory.max_size())) {
+    // If creation of shared memory failed then send an error message.
+    SendErrorMessage(msg.routing_id(), stream_id);
+    return;
+  }
+
   if (low_latency) {
     // If this is the low latency mode, we need to construct a SyncReader first.
     scoped_ptr<AudioSyncReader> reader(
@@ -350,44 +357,32 @@ void AudioRendererHost::OnCreateStream(
     // If we have successfully created the SyncReader then assign it to the
     // entry and construct an AudioOutputController.
     entry->reader.reset(reader.release());
-    controller =
+    entry->controller =
         media::AudioOutputController::CreateLowLatency(
             this, params.params,
             hardware_packet_size,
             entry->reader.get());
   } else {
     // The choice of buffer capacity is based on experiment.
-    controller =
+    entry->controller =
         media::AudioOutputController::Create(this, params.params,
                                              hardware_packet_size,
                                              3 * hardware_packet_size);
   }
 
-  if (!controller) {
+  if (!entry->controller) {
     SendErrorMessage(msg.routing_id(), stream_id);
     return;
   }
 
   // If we have created the controller successfully create a entry and add it
   // to the map.
-  entry->controller = controller;
   entry->render_view_id = msg.routing_id();
   entry->stream_id = stream_id;
 
-  // Create the shared memory and share with the renderer process.
-  if (!entry->shared_memory.Create(L"", false, false, hardware_packet_size) ||
-      !entry->shared_memory.Map(entry->shared_memory.max_size())) {
-    // If creation of shared memory failed then close the controller and
-    // sends an error message.
-    controller->Close();
-    SendErrorMessage(msg.routing_id(), stream_id);
-    return;
-  }
-
-  // If everything is successful then add it to the map.
-  audio_entries_.insert(std::make_pair(
-      AudioEntryId(msg.routing_id(), stream_id),
-      entry.release()));
+ audio_entries_.insert(std::make_pair(
+     AudioEntryId(msg.routing_id(), stream_id),
+     entry.release()));
 }
 
 void AudioRendererHost::OnPlayStream(const IPC::Message& msg, int stream_id) {
@@ -431,10 +426,8 @@ void AudioRendererHost::OnCloseStream(const IPC::Message& msg, int stream_id) {
 
   AudioEntry* entry = LookupById(msg.routing_id(), stream_id);
 
-  // Note that closing an audio stream is a blocking operation. This call may
-  // block the IO thread for up to 100ms.
   if (entry)
-    DeleteEntry(entry);
+    CloseAndDeleteStream(entry);
 }
 
 void AudioRendererHost::OnSetVolume(const IPC::Message& msg, int stream_id,
@@ -519,10 +512,25 @@ void AudioRendererHost::SendErrorMessage(int32 render_view_id,
 void AudioRendererHost::DeleteEntries() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  while (!audio_entries_.empty()) {
-    DeleteEntry(audio_entries_.begin()->second);
+  for (AudioEntryMap::iterator i = audio_entries_.begin();
+       i != audio_entries_.end(); ++i) {
+    CloseAndDeleteStream(i->second);
   }
-  DCHECK(audio_entries_.empty());
+}
+
+void AudioRendererHost::CloseAndDeleteStream(AudioEntry* entry) {
+  if (!entry->pending_close) {
+    entry->controller->Close(
+        NewRunnableMethod(this, &AudioRendererHost::OnStreamClosed, entry));
+    entry->pending_close = true;
+  }
+}
+
+void AudioRendererHost::OnStreamClosed(AudioEntry* entry) {
+  // Delete the entry after we've closed the stream.
+  ChromeThread::PostTask(
+      ChromeThread::IO, FROM_HERE,
+      NewRunnableMethod(this, &AudioRendererHost::DeleteEntry, entry));
 }
 
 void AudioRendererHost::DeleteEntry(AudioEntry* entry) {
@@ -531,10 +539,7 @@ void AudioRendererHost::DeleteEntry(AudioEntry* entry) {
   // Delete the entry when this method goes out of scope.
   scoped_ptr<AudioEntry> entry_deleter(entry);
 
-  // Close the audio stream then remove the entry.
-  entry->controller->Close();
-
-  // Entry the entry from the map.
+  // Erase the entry from the map.
   audio_entries_.erase(
       AudioEntryId(entry->render_view_id, entry->stream_id));
 }
@@ -545,7 +550,7 @@ void AudioRendererHost::DeleteEntryOnError(AudioEntry* entry) {
   // Sends the error message first before we close the stream because
   // |entry| is destroyed in DeleteEntry().
   SendErrorMessage(entry->render_view_id, entry->stream_id);
-  DeleteEntry(entry);
+  CloseAndDeleteStream(entry);
 }
 
 AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(
diff --git a/chrome/browser/renderer_host/audio_renderer_host.h b/chrome/browser/renderer_host/audio_renderer_host.h
index 074ec66..c67e0e7 100644
--- a/chrome/browser/renderer_host/audio_renderer_host.h
+++ b/chrome/browser/renderer_host/audio_renderer_host.h
@@ -86,7 +86,8 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
     AudioEntry()
         : render_view_id(0),
           stream_id(0),
-          pending_buffer_request(false) {
+          pending_buffer_request(false),
+          pending_close(false) {
     }
 
     // The AudioOutputController that manages the audio stream.
@@ -106,6 +107,9 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
     scoped_ptr<media::AudioOutputController::SyncReader> reader;
 
     bool pending_buffer_request;
+
+    // Set to true after we called Close() for the controller.
+    bool pending_close;
   };
 
   typedef std::map<AudioEntryId, AudioEntry*> AudioEntryMap;
@@ -219,6 +223,13 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
   // Delete all audio entry and all audio streams
   void DeleteEntries();
 
+  // Closes the stream. The stream is then deleted in DeleteEntry() after it
+  // is closed.
+  void CloseAndDeleteStream(AudioEntry* entry);
+
+  // Called on the audio thread after the audio stream is closed.
+  void OnStreamClosed(AudioEntry* entry);
+
   // Delete an audio entry and close the related audio stream.
   void DeleteEntry(AudioEntry* entry);
 
diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index 6ff02aa..b3a42e9 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,
@@ -291,8 +271,6 @@ 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();
 
@@ -302,8 +280,10 @@ 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.
+    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.
     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