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 a3effebfb43af118500197a6eed9a48093563f7d..b913a463b3a7e2cf28ad38a14c46dff8c5632ccc 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),
+      render_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_(base::WaitableEvent::ResetPolicy::MANUAL,
                         base::WaitableEvent::InitialState::NOT_SIGNALED),
       device_status_(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL) {
@@ -79,17 +78,40 @@ 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
+  // callbacks from previous runs (i.e. from before Stop() was called earlier).
+  // This is because Stop() and Initialize() could be called before
+  // StopOnIOThread() gets to execute; it's in that function we pause the
+  // AudioDeviceThread. We gate callbacks in this class, but after setting a
+  // callback again here, the gate is open.
+  //
+  // 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(render_callback_lock_);
+    DCHECK(!render_callback_) << "Already initialized.";
+    render_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.
+  // 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(!render_callback_);
   DCHECK(audio_thread_.IsStopped());

[DaleCurtis, 2016/06/09 22:37:27]

This won't be true with the code you have currently, since the OnCreated() may
come in later and start the thread (you removed stopping_hack_)

[Henrik Grunell, 2016/06/13 13:43:43]

This class is ref counted so all pending tasks will complete first. After
StopOnIOThread() has run there will be no calls from the IPC implementation, so
no OnXyz() functions will be called after that, thus when we are in the
destructor we can assert that we are stopped.

 }
 
@@ -101,21 +123,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(render_callback_lock_);

[DaleCurtis, 2016/06/09 22:37:27]

This doesn't need a lock since it can only be read on the same thread Stop() is
called on. It might be worth having a thread checker for these methods.

[Henrik Grunell, 2016/06/13 13:43:43]

Done.

+    DCHECK(render_callback_) << "Not initialized.";
+  }
+#endif
+
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this));
 }
 
 void AudioOutputDevice::Stop() {
+  // Clear the callback synchronously to ensure no callbacks after returning.
   {
-    base::AutoLock auto_lock(audio_thread_lock_);
-    audio_thread_.Stop(base::MessageLoop::current());
-    stopping_hack_ = true;
+    base::AutoLock auto_lock(render_callback_lock_);
+    render_callback_ = nullptr;
   }
 
   task_runner()->PostTask(FROM_HERE,
-      base::Bind(&AudioOutputDevice::ShutDownOnIOThread, this));
+                          base::Bind(&AudioOutputDevice::StopOnIOThread, this));
 }
 
 void AudioOutputDevice::Play() {
@@ -150,27 +177,51 @@ 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(render_callback_lock_);
+  return render_callback_
+             ? render_callback_->Render(audio_bus, frames_delayed,
+                                        frames_skipped)
+             : 0;
+}
+
+void AudioOutputDevice::OnRenderError() {
+  base::AutoLock auto_lock(render_callback_lock_);
+  if (render_callback_)
+    render_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:
-      if (callback_)
-        callback_->OnRenderError();
+      OnRenderError();
       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;
@@ -183,7 +234,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;
 
@@ -198,8 +249,8 @@ void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
 void AudioOutputDevice::PlayOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ == PAUSED) {
-    TRACE_EVENT_ASYNC_BEGIN0(
-        "audio", "StartingPlayback", audio_callback_.get());
+    TRACE_EVENT_ASYNC_BEGIN0("audio", "StartingPlayback",
+                             audio_thread_callback_.get());
     ipc_->PlayStream();
     state_ = PLAYING;
     play_on_start_ = false;
@@ -211,37 +262,34 @@ void AudioOutputDevice::PlayOnIOThread() {
 void AudioOutputDevice::PauseOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ == PLAYING) {
-    TRACE_EVENT_ASYNC_END0(
-        "audio", "StartingPlayback", audio_callback_.get());
+    TRACE_EVENT_ASYNC_END0("audio", "StartingPlayback",
+                           audio_thread_callback_.get());
     ipc_->PauseStream();
     state_ = PAUSED;
   }
   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_);
+  // TODO BEFORE COMMIT: Do we need this?

[DaleCurtis, 2016/06/09 22:37:27]

I think you still do unless we're okay spinning up an unused thread when the
call finally comes through; a simple solution is the WeakPtr one mentioned
below. Just invalidate the weakptr used for create such that the return is
dropped.

[Henrik Grunell, 2016/06/13 13:43:43]

Kept as is.

   base::ThreadRestrictions::ScopedAllowIO allow_io;
-  audio_thread_.Stop(NULL);
-  audio_callback_.reset();
-  stopping_hack_ = false;
+  audio_thread_.Stop(nullptr);
+  audio_thread_callback_.reset();
+  play_on_start_ = true;
+
+  // We need to re-authorize after stopping.
+  did_receive_auth_.Reset();
 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
@@ -265,14 +313,7 @@ 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();
+      OnRenderError();
       break;
     default:
       NOTREACHED();
@@ -318,14 +359,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();
-    if (callback_)
-      callback_->OnRenderError();
+    OnRenderError();
   }
 }
 
@@ -346,34 +386,28 @@ void AudioOutputDevice::OnStreamCreated(
     return;
 
   // We can receive OnStreamCreated() on the IO thread after the client has

[DaleCurtis, 2016/06/09 22:37:27]

But aren't you still spinning up a new thread in this case? Instead of stopping
hack it sounds like we want to just add an internal WeakPtrFactory and cancel
vended pointers when Stop() is called? I think if you don't want to do that it
might be worth keeping the stopping_hack_ for now.

[Henrik Grunell, 2016/06/13 13:43:43]

Yes, a thread will start without needing it, and later it will be stopped. With
the stopping hack instead, what happens if we do Stop, Initialize, Start before
we get here? It seems like a risk of getting into trouble in this already hacky
implementation. To be safe, we would need a stopping hack/weak ptr per Start,
was that what you meant?

[DaleCurtis, 2016/06/13 18:52:32]

Ah, I forgot this is an interface, we can't use a WeakPtr because of that. Since
you now clear the callback on stop, why don't we just reject all
OnStreamCreated() calls when callback is null?

[Henrik Grunell, 2016/06/13 20:12:20]

That's a good idea, I'll do that.

-  // 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 |render_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_thread_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
+      audio_parameters_, handle, length, this));
+  audio_thread_.Start(audio_thread_callback_.get(), socket_handle,
+                      "AudioOutputDevice", true);
+
+  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() {
@@ -387,7 +421,7 @@ void AudioOutputDevice::OnIPCClosed() {
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback