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 12d102c4ad196911c9ddfb7430e60da7ca99093b..634f055b20caf5f2c06ae6c352d123a6c395d936 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -57,7 +57,6 @@ AudioOutputDevice::AudioOutputDevice(
     const url::Origin& security_origin,
     base::TimeDelta authorization_timeout)
     : ScopedTaskRunnerObserver(io_task_runner),
-      callback_(NULL),
       ipc_(std::move(ipc)),
       state_(IDLE),
       start_on_authorized_(false),
@@ -65,6 +64,7 @@ AudioOutputDevice::AudioOutputDevice(
       session_id_(session_id),
       device_id_(device_id),
       security_origin_(security_origin),
+      render_callback_(nullptr),
       stopping_hack_(false),
       did_receive_auth_(base::WaitableEvent::ResetPolicy::MANUAL,
                         base::WaitableEvent::InitialState::NOT_SIGNALED),
@@ -81,19 +81,29 @@ AudioOutputDevice::AudioOutputDevice(
                 "invalid enum value assignment 3");
   static_assert(CREATING_STREAM < PAUSED, "invalid enum value assignment 4");
   static_assert(PAUSED < PLAYING, "invalid enum value assignment 5");
+
+  control_thread_checker_.DetachFromThread();
 }
 
-void AudioOutputDevice::Initialize(const AudioParameters& params,
-                                   RenderCallback* callback) {
-  DCHECK(!callback_) << "Calling Initialize() twice?";
+void AudioOutputDevice::Initialize(
+    const AudioParameters& params,
+    AudioRendererSink::RenderCallback* callback) {
+  DCHECK(control_thread_checker_.CalledOnValidThread());
+  DCHECK(!render_callback_) << "Already initialized.";
   DCHECK(params.IsValid());
   audio_parameters_ = params;
-  callback_ = callback;
+  {

[DaleCurtis, 2016/06/16 16:39:14]

Drop the lock? DCHECK(!thread.started()) ?

[Henrik Grunell, 2016/06/16 20:23:00]

SG, I'll do that tomorrow.

+    base::AutoLock auto_lock(lock_);
+    render_callback_ = callback;
+  }
 }
 
 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(audio_thread_.IsStopped());
 }
 
@@ -105,21 +115,23 @@ 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_));
+  DCHECK(control_thread_checker_.CalledOnValidThread());
+  DCHECK(render_callback_) << "Not initialized.";
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this));
 }
 
 void AudioOutputDevice::Stop() {
+  DCHECK(control_thread_checker_.CalledOnValidThread());
   {
-    base::AutoLock auto_lock(audio_thread_lock_);
+    base::AutoLock auto_lock(lock_);
     audio_thread_.Stop(base::MessageLoop::current());
     stopping_hack_ = true;
+    render_callback_ = nullptr;
   }
 
   task_runner()->PostTask(FROM_HERE,
-      base::Bind(&AudioOutputDevice::ShutDownOnIOThread, this));
+                          base::Bind(&AudioOutputDevice::StopOnIOThread, this));
 }
 
 void AudioOutputDevice::Play() {
@@ -154,10 +166,19 @@ OutputDeviceInfo AudioOutputDevice::GetOutputDeviceInfo() {
                           device_status_, output_params_);
 }
 
+void AudioOutputDevice::ReportRenderError() {
+  base::AutoLock auto_lock(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_);
 
@@ -172,19 +193,19 @@ void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() {
                  std::string()));
 }
 
-void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+void AudioOutputDevice::CreateStreamOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
+
   switch (state_) {
     case IPC_CLOSED:
-      if (callback_)
-        callback_->OnRenderError();
+      ReportRenderError();
       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;
@@ -197,7 +218,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;
 
@@ -212,8 +233,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;
@@ -225,24 +246,30 @@ 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 need to re-authorize after stopping.
+  did_receive_auth_.Reset();
+
   // Destoy the timer on the thread it's used on.
   auth_timeout_action_.reset();
 
@@ -254,11 +281,12 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   // 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::AutoLock auto_lock_(lock_);

[DaleCurtis, 2016/06/16 16:39:14]

I'm worried this can deadlock now. You're waiting for the render thread to stop,
but something on the render thread might be waiting for the lock.

[Henrik Grunell, 2016/06/16 20:23:00]

This lock is never grabbed on the audio render thread. Only the control
(typically main render) thread and IO thread. Do you find a concrete case? I'll
add a thread check in ReportRenderError, and maybe change name. It should only
be called on IO thread.

[DaleCurtis, 2016/06/16 22:00:53]

No, I had forgotten we're not using your old patch that added a lock there.

   base::ThreadRestrictions::ScopedAllowIO allow_io;
   audio_thread_.Stop(NULL);
-  audio_callback_.reset();
+  audio_thread_callback_.reset();
   stopping_hack_ = false;
+  play_on_start_ = true;
 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
@@ -289,7 +317,7 @@ void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
       // object.  Possibly require calling Initialize again or provide
       // a callback object via Start() and clear it in Stop().
       if (!audio_thread_.IsStopped())
-        callback_->OnRenderError();
+        ReportRenderError();
       break;
     default:
       NOTREACHED();
@@ -345,14 +373,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();
+    ReportRenderError();
   }
 }
 
@@ -377,22 +404,22 @@ void AudioOutputDevice::OnStreamCreated(
   // situation |callback_| might point to freed memory. Instead of starting
   // |audio_thread_| do nothing and wait for ShutDownOnIOThread() 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
-  // 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.
+  // 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.
+
   {
-    base::AutoLock auto_lock(audio_thread_lock_);
+    base::AutoLock auto_lock(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,
+    audio_thread_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
+        audio_parameters_, handle, length, render_callback_));
+    audio_thread_.Start(audio_thread_callback_.get(), socket_handle,
                         "AudioOutputDevice", true);
     state_ = PAUSED;
 
@@ -414,7 +441,7 @@ void AudioOutputDevice::OnIPCClosed() {
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback