Make AudioOutputDevice restartable and reinitializable

media/audio/audio_output_device.cc

Index: media/audio/audio_output_device.cc
diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc
index 05b123f780a050021bdf54d40ddb311045e0984a..bef6d01aa4e2466a64ddceb57eb9c6691f9cc9dd 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -54,7 +54,7 @@ AudioOutputDevice::AudioOutputDevice(
     const std::string& device_id,
     const url::Origin& security_origin)
     : ScopedTaskRunnerObserver(io_task_runner),
-      callback_(NULL),
+      callback_(nullptr),
       ipc_(std::move(ipc)),
       state_(IDLE),
       start_on_authorized_(false),
@@ -62,7 +62,6 @@ AudioOutputDevice::AudioOutputDevice(
       session_id_(session_id),
       device_id_(device_id),
       security_origin_(security_origin),
-      stopping_hack_(false),
       did_receive_auth_(true, false),
       device_status_(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL) {
   CHECK(ipc_);
@@ -78,18 +77,35 @@ AudioOutputDevice::AudioOutputDevice(
   static_assert(PAUSED < PLAYING, "invalid enum value assignment 5");
 }
 
-void AudioOutputDevice::Initialize(const AudioParameters& params,
-                                   RenderCallback* callback) {
-  DCHECK(!callback_) << "Calling Initialize() twice?";
+void AudioOutputDevice::Initialize(
+    const AudioParameters& params,
+    AudioRendererSink::RenderCallback* callback) {
   DCHECK(params.IsValid());
-  audio_parameters_ = params;
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::InitializeOnIOThread, this,
+                            params, callback));
+
+  // We set the callback synchronously here, so that it's serialized with
+  // clearing it in Stop(). Note that after this it is possible to get stale
+  // callbacks from previous runs (i.e. from before Stop() was called earlier).
+  // Using the IO thread will go away soon when we move IPC to use Mojo, and
+  // we'll then get rid of the thread hopping and locks.
+  // http://crbug.com/606707.
+  base::AutoLock auto_lock(callback_lock_);
+  DCHECK(!callback_) << "Calling Initialize() twice?";
   callback_ = callback;
 }
 
 AudioOutputDevice::~AudioOutputDevice() {
+  // I'm not sure if Stop() should remain  mandatory. The purpose of that was
+  // to fix object lifetime issues. Probably we can do it in a cleaner way.
   // The current design requires that the user calls Stop() before deleting
-  // this class.
-  DCHECK(audio_thread_.IsStopped());
+  // this class. Since this object is reference counted, we're the only one
+  // accessing data when we destruct.
+  DCHECK(state_ == IDLE || state_ == IPC_CLOSED);
+  // We need to ensure all the object lifetime stuff still works.
+  // Or even better, fix it to be more clear.
+  audio_thread_.Stop(base::MessageLoop::current());
 }
 
 void AudioOutputDevice::RequestDeviceAuthorization() {
@@ -100,21 +116,26 @@ void AudioOutputDevice::RequestDeviceAuthorization() {
 }
 
 void AudioOutputDevice::Start() {
-  DCHECK(callback_) << "Initialize hasn't been called";
-  task_runner()->PostTask(FROM_HERE,
-      base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this,
-                 audio_parameters_));
+#if !defined(NDEBUG)
+  {
+    base::AutoLock auto_lock(callback_lock_);
+    DCHECK(callback_) << "Initialize hasn't been called";
+  }
+#endif
+
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this));
 }
 
 void AudioOutputDevice::Stop() {
   {
-    base::AutoLock auto_lock(audio_thread_lock_);
-    audio_thread_.Stop(base::MessageLoop::current());
-    stopping_hack_ = true;
+    // TODO: Add comment why we do this,
+    base::AutoLock auto_lock(callback_lock_);
+    callback_ = nullptr;
   }
 
   task_runner()->PostTask(FROM_HERE,
-      base::Bind(&AudioOutputDevice::ShutDownOnIOThread, this));
+                          base::Bind(&AudioOutputDevice::StopOnIOThread, this));
 }
 
 void AudioOutputDevice::Play() {
@@ -149,27 +170,58 @@ OutputDeviceInfo AudioOutputDevice::GetOutputDeviceInfo() {
                           device_status_, output_params_);
 }
 
+int AudioOutputDevice::Render(media::AudioBus* audio_bus,
+                              uint32_t frames_delayed,
+                              uint32_t frames_skipped) {
+  base::AutoLock auto_lock(callback_lock_);
+  return callback_
+             ? callback_->Render(audio_bus, frames_delayed, frames_skipped)
+             : 0;
+}
+
+void AudioOutputDevice::OnRenderError() {
+  base::AutoLock auto_lock(callback_lock_);
+  if (callback_)
+    callback_->OnRenderError();
+}
+
 void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK_EQ(state_, IDLE);
   state_ = AUTHORIZING;
+
+  // TODO(grunell): Store authorization so that we don't have to authorize
+  // again when re-starting. This involves mostly changes on the browser side:
+  // associate authorization by a new id (currently the stream id), don't forget
+  // authorization away after start or stop. Here we would have to explicitly
+  // make the browser side forget authorization in the dtor with a new
+  // AudioOutputIPC function.
   ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
                                    security_origin_);
 }
 
-void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+void AudioOutputDevice::InitializeOnIOThread(
+    const AudioParameters& params,
+    AudioRendererSink::RenderCallback* callback) {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  audio_parameters_ = params;
+}
+
+void AudioOutputDevice::CreateStreamOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
+
   switch (state_) {
-    case IPC_CLOSED:
+    case IPC_CLOSED: {
+      base::AutoLock auto_lock(callback_lock_);
       if (callback_)
         callback_->OnRenderError();
-      break;
+    } break;
 
     case IDLE:
       if (did_receive_auth_.IsSignaled() && device_id_.empty() &&
           security_origin_.unique()) {
         state_ = CREATING_STREAM;
-        ipc_->CreateStream(this, params);
+        ipc_->CreateStream(this, audio_parameters_);
       } else {
         RequestDeviceAuthorizationOnIOThread();
         start_on_authorized_ = true;
@@ -182,7 +234,9 @@ void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
 
     case AUTHORIZED:
       state_ = CREATING_STREAM;
-      ipc_->CreateStream(this, params);
+      // TODO: This could use new parameters for an old start operation. Needs
+      // to be fixed.
+      ipc_->CreateStream(this, audio_parameters_);
       start_on_authorized_ = false;
       break;
 
@@ -218,7 +272,7 @@ void AudioOutputDevice::PauseOnIOThread() {
   play_on_start_ = false;
 }
 
-void AudioOutputDevice::ShutDownOnIOThread() {
+void AudioOutputDevice::StopOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Close the stream, if we haven't already.
@@ -228,19 +282,14 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   }
   start_on_authorized_ = false;
 
-  // We can run into an issue where ShutDownOnIOThread is called right after
+  // We can run into an issue where StopOnIOThread is called right after
   // OnStreamCreated is called in cases where Start/Stop are called before we
   // get the OnStreamCreated callback.  To handle that corner case, we call
   // Stop(). In most cases, the thread will already be stopped.
-  //
-  // Another situation is when the IO thread goes away before Stop() is called
-  // in which case, we cannot use the message loop to close the thread handle
-  // and can't rely on the main thread existing either.
+
   base::AutoLock auto_lock_(audio_thread_lock_);
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-  audio_thread_.Stop(NULL);
+  audio_thread_.Pause();
   audio_callback_.reset();
-  stopping_hack_ = false;
 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
@@ -264,14 +313,12 @@ void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
       break;
     case AUDIO_OUTPUT_IPC_DELEGATE_STATE_ERROR:
       DLOG(WARNING) << "AudioOutputDevice::OnStateChanged(ERROR)";
-      // Don't dereference the callback object if the audio thread
-      // is stopped or stopping.  That could mean that the callback
-      // object has been deleted.
-      // TODO(tommi): Add an explicit contract for clearing the callback
-      // object.  Possibly require calling Initialize again or provide
-      // a callback object via Start() and clear it in Stop().
-      if (!audio_thread_.IsStopped())
-        callback_->OnRenderError();
+      // The callback object is cleared in Stop().
+      {
+        base::AutoLock auto_lock(callback_lock_);
+        if (callback_)
+          callback_->OnRenderError();
+      }
       break;
     default:
       NOTREACHED();
@@ -317,12 +364,13 @@ void AudioOutputDevice::OnDeviceAuthorized(
       did_receive_auth_.Signal();
     }
     if (start_on_authorized_)
-      CreateStreamOnIOThread(audio_parameters_);
+      CreateStreamOnIOThread();
   } else {
     // Closing IPC forces a Signal(), so no clients are locked waiting
     // indefinitely after this method returns.
     ipc_->CloseStream();
     OnIPCClosed();
+    base::AutoLock auto_lock(callback_lock_);
     if (callback_)
       callback_->OnRenderError();
   }
@@ -344,10 +392,12 @@ void AudioOutputDevice::OnStreamCreated(
   if (state_ != CREATING_STREAM)
     return;
 
+  // TODO: Update the comment.
+
   // We can receive OnStreamCreated() on the IO thread after the client has
-  // called Stop() but before ShutDownOnIOThread() is processed. In such a
+  // called Stop() but before StopOnIOThread() is processed. In such a
   // situation |callback_| might point to freed memory. Instead of starting
-  // |audio_thread_| do nothing and wait for ShutDownOnIOThread() to get called.
+  // |audio_thread_| do nothing and wait for StopOnIOThread() to get called.
   //
   // TODO(scherkus): The real fix is to have sane ownership semantics. The fact
   // that |callback_| (which should own and outlive this object!) can point to
@@ -358,14 +408,17 @@ void AudioOutputDevice::OnStreamCreated(
   // details.
   {
     base::AutoLock auto_lock(audio_thread_lock_);
-    if (stopping_hack_)
-      return;
 
     DCHECK(audio_thread_.IsStopped());
     audio_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
-        audio_parameters_, handle, length, callback_));
-    audio_thread_.Start(audio_callback_.get(), socket_handle,
-                        "AudioOutputDevice", true);
+        audio_parameters_, handle, length, this));
+
+    // If the audio thread hasn't been started before, start it.
+    if (audio_thread_.IsStopped()) {
+      audio_thread_.Start("AudioOutputDevice", true);
+    }
+    audio_thread_.Play(audio_callback_.get(), socket_handle);
+
     state_ = PAUSED;
 
     // We handle the case where Play() and/or Pause() may have been called
@@ -386,7 +439,7 @@ void AudioOutputDevice::OnIPCClosed() {
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback