Make AudioOutputDevice restartable and reinitializable

media/audio/audio_output_device.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_output_device.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <cmath>
@@ skipping 36 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(AudioThreadCallback);
 };
 
 AudioOutputDevice::AudioOutputDevice(
     std::unique_ptr<AudioOutputIPC> ipc,
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
     int session_id,
     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),
       play_on_start_(true),
       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) {
   CHECK(ipc_);
 
   // The correctness of the code depends on the relative values assigned in the
   // State enum.
   static_assert(IPC_CLOSED < IDLE, "invalid enum value assignment 0");
   static_assert(IDLE < AUTHORIZING, "invalid enum value assignment 1");
   static_assert(AUTHORIZING < AUTHORIZED, "invalid enum value assignment 2");
   static_assert(AUTHORIZED < CREATING_STREAM,
                 "invalid enum value assignment 3");
   static_assert(CREATING_STREAM < PAUSED, "invalid enum value assignment 4");
   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.

 }
 
 void AudioOutputDevice::RequestDeviceAuthorization() {
   task_runner()->PostTask(
       FROM_HERE,
       base::Bind(&AudioOutputDevice::RequestDeviceAuthorizationOnIOThread,
                  this));
 }
 
 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() {
   task_runner()->PostTask(FROM_HERE,
       base::Bind(&AudioOutputDevice::PlayOnIOThread, this));
 }
 
 void AudioOutputDevice::Pause() {
   task_runner()->PostTask(FROM_HERE,
       base::Bind(&AudioOutputDevice::PauseOnIOThread, this));
@@ skipping 14 matching lines @@
 OutputDeviceInfo AudioOutputDevice::GetOutputDeviceInfo() {
   CHECK(!task_runner()->BelongsToCurrentThread());
   did_receive_auth_.Wait();
   return OutputDeviceInfo(AudioDeviceDescription::UseSessionIdToSelectDevice(
                               session_id_, device_id_)
                               ? matched_device_id_
                               : device_id_,
                           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;
       }
       break;
 
     case AUTHORIZING:
       start_on_authorized_ = true;
       break;
 
     case AUTHORIZED:
       state_ = CREATING_STREAM;
-      ipc_->CreateStream(this, params);
+      ipc_->CreateStream(this, audio_parameters_);
       start_on_authorized_ = false;
       break;
 
     case CREATING_STREAM:
     case PAUSED:
     case PLAYING:
       NOTREACHED();
       break;
   }
 }
 
 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;
   } else {
     play_on_start_ = true;
   }
 }
 
 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;
+  // 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(nullptr);
+  audio_thread_callback_.reset();
+  play_on_start_ = true;
 
-  // 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_callback_.reset();
-  stopping_hack_ = false;
+  // We need to re-authorize after stopping.
+  did_receive_auth_.Reset();
 }
 
 void AudioOutputDevice::SetVolumeOnIOThread(double volume) {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ >= CREATING_STREAM)
     ipc_->SetVolume(volume);
 }
 
 void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Do nothing if the stream has been closed.
   if (state_ < CREATING_STREAM)
     return;
 
   // TODO(miu): Clean-up inconsistent and incomplete handling here.
   // http://crbug.com/180640
   switch (state) {
     case AUDIO_OUTPUT_IPC_DELEGATE_STATE_PLAYING:
     case AUDIO_OUTPUT_IPC_DELEGATE_STATE_PAUSED:
       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();
       break;
   }
 }
 
 void AudioOutputDevice::OnDeviceAuthorized(
     OutputDeviceStatus device_status,
     const media::AudioParameters& output_params,
@@ skipping 25 matching lines @@
              matched_device_id_.empty());
       matched_device_id_ = matched_device_id;
 
       DVLOG(1) << "AudioOutputDevice authorized, session_id: " << session_id_
                << ", device_id: " << device_id_
                << ", matched_device_id: " << matched_device_id_;
 
       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();
   }
 }
 
 void AudioOutputDevice::OnStreamCreated(
     base::SharedMemoryHandle handle,
     base::SyncSocket::Handle socket_handle,
     int length) {
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK(base::SharedMemory::IsHandleValid(handle));
 #if defined(OS_WIN)
   DCHECK(socket_handle);
 #else
   DCHECK_GE(socket_handle, 0);
 #endif
   DCHECK_GT(length, 0);
 
   if (state_ != CREATING_STREAM)
     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;
+  // 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.
 
-    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;
+  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);
 
-    // We handle the case where Play() and/or Pause() may have been called
-    // multiple times before OnStreamCreated() gets called.
-    if (play_on_start_)
-      PlayOnIOThread();
-  }
+  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() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   state_ = IPC_CLOSED;
   ipc_.reset();
 
   // Signal to unblock any blocked threads waiting for parameters
   did_receive_auth_.Signal();
 }
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback
 
 AudioOutputDevice::AudioThreadCallback::AudioThreadCallback(
     const AudioParameters& audio_parameters,
     base::SharedMemoryHandle memory,
     int memory_length,
     AudioRendererSink::RenderCallback* render_callback)
     : AudioDeviceThread::Callback(audio_parameters, memory, memory_length, 1),
@@ skipping 43 matching lines @@
 
   // Update the audio-delay measurement, inform about the number of skipped
   // frames, and ask client to render audio.  Since |output_bus_| is wrapping
   // the shared memory the Render() call is writing directly into the shared
   // memory.
   render_callback_->Render(output_bus_.get(), std::round(frames_delayed),
                            frames_skipped);
 }
 
 }  // namespace media