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..e18fd99c66bb24b8260d15e5295feb9ce61e55ec 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,38 @@ 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;
-  callback_ = 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

[o1ka, 2016/05/03 15:47:32]

Could you explain here how exactly it can happen?

[Henrik Grunell, 2016/05/04 09:05:11]

Done.

+  // callbacks from previous runs (i.e. from before Stop() was called earlier).

[o1ka, 2016/05/03 15:47:32]

Taking into account current usage model, we are not expecting callback to be
changed during AOD lifetime, right?

[Henrik Grunell, 2016/05/04 09:05:11]

Yes, that's right, but we allow it. I suspect you were thinking of
checking/enforcing it? I don't think we should make that change now (if we want
to at all).

+  //
+  // Using the IO thread at all in this class will go away soon when we move IPC
+  // to use Mojo, and we'll then get rid of the thread hopping and locks. See
+  // http://crbug.com/606707.
+  {
+    base::AutoLock auto_lock(callback_lock_);
+    DCHECK(!callback_) << "Calling Initialize() twice?";
+    callback_ = callback;
+  }
+
+  task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&AudioOutputDevice::InitializeOnIOThread, this, params));
 }
 
 AudioOutputDevice::~AudioOutputDevice() {
   // The current design requires that the user calls Stop() before deleting
-  // this class.
-  DCHECK(audio_thread_.IsStopped());
+  // this class to ensure that stream and authorization data is cleaned up on
+  // the browser side, and that we don't get more IPC calls from AudioOutputIPC.
+  // With all posted tasks on IO thread done and no more IPC calls, it's safe to
+  // access |audio_thread_| here on any thread.
+  DCHECK(!callback_);
+
+  audio_thread_.Stop(base::MessageLoop::current());
 }
 
 void AudioOutputDevice::RequestDeviceAuthorization() {
@@ -100,21 +119,27 @@ 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() {
+  // Clear the callback synchronously to ensure no callbacks after returning.

[o1ka, 2016/05/03 15:47:32]

This is nice.

[Henrik Grunell, 2016/05/04 09:05:12]

Thanks.

   {
-    base::AutoLock auto_lock(audio_thread_lock_);
-    audio_thread_.Stop(base::MessageLoop::current());
-    stopping_hack_ = true;
+    base::AutoLock auto_lock(callback_lock_);
+    DCHECK(callback_) << "Initialize hasn't been called";
+    callback_ = nullptr;
   }
 
   task_runner()->PostTask(FROM_HERE,
-      base::Bind(&AudioOutputDevice::ShutDownOnIOThread, this));
+                          base::Bind(&AudioOutputDevice::StopOnIOThread, this));
 }
 
 void AudioOutputDevice::Play() {
@@ -149,27 +174,52 @@ 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;

[o1ka, 2016/05/03 15:47:32]

Shouldn't we zero-out the bus?

[Henrik Grunell, 2016/05/04 09:05:12]

That would not be correct, we should return the number of frames filled, if we
zero out we have actually filled it, literally speaking at least. It should in
any way be enough to say "I filled 0 frames".

+}
+
+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 restarting. http://crbug.com/608619.
   ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
                                    security_origin_);
 }
 
-void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+void AudioOutputDevice::InitializeOnIOThread(const AudioParameters& params) {
   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_);

[o1ka, 2016/05/03 15:47:32]

It's just AOD::OnRenderError()

[Henrik Grunell, 2016/05/04 09:05:12]

Of course, thanks.

       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 +232,7 @@ void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
 
     case AUTHORIZED:
       state_ = CREATING_STREAM;
-      ipc_->CreateStream(this, params);
+      ipc_->CreateStream(this, audio_parameters_);
       start_on_authorized_ = false;
       break;
 
@@ -218,29 +268,21 @@ 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.
+  // Close the stream, if we haven't already. After CloseStream(), we are
+  // guaranteed to not get IPC calls for any outstanding operations. This holds
+  // also if we restart, i.e. request authorization and create a new stream,
+  // before the previous request and/or create stream finished.
   if (state_ >= AUTHORIZING) {
     ipc_->CloseStream();
     state_ = IDLE;
   }
   start_on_authorized_ = false;
-
-  // We can run into an issue where ShutDownOnIOThread 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;
+  did_receive_auth_.Reset();

[o1ka, 2016/05/03 15:47:32]

Add a comment why it's needed?

[Henrik Grunell, 2016/05/04 09:05:11]

Done.

 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
@@ -264,14 +306,11 @@ 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();
+      {
+        base::AutoLock auto_lock(callback_lock_);

[o1ka, 2016/05/03 15:47:32]

AOD::OnRenderError()

[Henrik Grunell, 2016/05/04 09:05:12]

Done.

+        if (callback_)
+          callback_->OnRenderError();
+      }
       break;
     default:
       NOTREACHED();
@@ -317,12 +356,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_);

[o1ka, 2016/05/03 15:47:32]

ditto

[Henrik Grunell, 2016/05/04 09:05:11]

Done.

     if (callback_)
       callback_->OnRenderError();
   }
@@ -345,34 +385,32 @@ void AudioOutputDevice::OnStreamCreated(
     return;
 
   // We can receive OnStreamCreated() on the IO thread after the client has
-  // called Stop() but before ShutDownOnIOThread() 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.
+  // called Stop() but before StopOnIOThread() is processed. This is OK, since
+  // we pass callbacks though |this| and clear |callback_| in Stop().
   //
-  // 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
-  // freed memory is a mess. AudioRendererSink should be non-refcounted so that
-  // owners (WebRtcAudioDeviceImpl, AudioRendererImpl, etc...) can Stop() and
-  // delete as they see fit. AudioOutputDevice should internally use WeakPtr
-  // to handle teardown and thread hopping. See http://crbug.com/151051 for
-  // 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);
-    state_ = PAUSED;
+  // TODO(grunell): AudioRendererSink should be non-refcounted and
+  // AudioOutputDevice should internally use WeakPtr to handle teardown and
+  // thread hopping. See http://crbug.com/151051 for details.
+  // Note however that switching the IPC to Mojo will remove usage of the IO
+  // thread, so we'll eliminate thread concurrency issues here then. See
+  // http://crbug.com/606707.
 
-    // We handle the case where Play() and/or Pause() may have been called
-    // multiple times before OnStreamCreated() gets called.
-    if (play_on_start_)
-      PlayOnIOThread();
+  DCHECK(audio_thread_.IsStopped());
+  audio_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
+      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_.Resume(audio_callback_.get(), socket_handle);
+
+  state_ = PAUSED;
+
+  // We handle the case where Play() and/or Pause() may have been called
+  // multiple times before OnStreamCreated() gets called.
+  if (play_on_start_)
+    PlayOnIOThread();
 }
 
 void AudioOutputDevice::OnIPCClosed() {
@@ -386,7 +424,7 @@ void AudioOutputDevice::OnIPCClosed() {
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback