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),
+      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_(true, false),
       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

[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() {
   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(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() {
   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(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;
       }
       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;
   }
 }
@@ skipping 15 matching lines @@
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ == PLAYING) {
     TRACE_EVENT_ASYNC_END0(
         "audio", "StartingPlayback", audio_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_);
-  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) {
   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();
+      {
+        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();
       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();
+    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();
   }
 }
 
 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
-  // 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;
+  // 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_callback_.reset(new AudioOutputDevice::AudioThreadCallback(
+      audio_parameters_, handle, length, this));
 
-    // We handle the case where Play() and/or Pause() may have been called
-    // multiple times before OnStreamCreated() gets called.
-    if (play_on_start_)
-      PlayOnIOThread();
+  // 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() {
   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