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 39 matching lines @@
 };
 
 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,
     base::TimeDelta authorization_timeout)
     : ScopedTaskRunnerObserver(io_task_runner),
-      callback_(NULL),
       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),
+      render_callback_(nullptr),
       stopping_hack_(false),
       did_receive_auth_(base::WaitableEvent::ResetPolicy::MANUAL,
                         base::WaitableEvent::InitialState::NOT_SIGNALED),
       device_status_(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL),
       auth_timeout_(authorization_timeout) {
   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");
+
+  thread_checker_.DetachFromThread();

[DaleCurtis, 2016/06/15 16:51:41]

Needs a more specific name in light of your other change for thread checking.

[Henrik Grunell, 2016/06/16 12:39:41]

Done.

 }
 
-void AudioOutputDevice::Initialize(const AudioParameters& params,
-                                   RenderCallback* callback) {
-  DCHECK(!callback_) << "Calling Initialize() twice?";
+void AudioOutputDevice::Initialize(
+    const AudioParameters& params,
+    AudioRendererSink::RenderCallback* callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!render_callback_) << "Already initialized.";
   DCHECK(params.IsValid());
   audio_parameters_ = params;
-  callback_ = callback;
+  {
+    base::AutoLock auto_lock(lock_);

[DaleCurtis, 2016/06/15 16:51:42]

Locking shouldn't be necessary here, right? The thread should be stopped at this
point. I don't think locking the callback is required at all anymore since you
went back to the stopping hack. Ditto for ReportRenderError. The thread is
either running or not, when running it always has a callback.

[Henrik Grunell, 2016/06/16 12:39:42]

|render_callback_| isn't only coupled with the thread.

ReportRenderError is called on the IO thread. If we execute that and get a
Stop() call (where it's set to null) it could dereference null. The old code
worked without locks since we never cleared the callback.

+    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());
 }
 
 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_));
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(render_callback_) << "Not initialized.";
+  {

[DaleCurtis, 2016/06/15 16:51:41]

Again, shouldn't be necessary, the thread is stopped here (and should be
dchecked as such). Also I don't think you should change this value here, the
task will be posted after ShutdownOnIOThread() so by the time it goes through
the right value should be set for stopping_hack.

[Henrik Grunell, 2016/06/16 12:39:41]

Right, it's cleared in StoponIOThread. That should be enough. Removed.

+    base::AutoLock auto_lock(lock_);
+    stopping_hack_ = false;
+  }
+
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this));
 }
 
 void AudioOutputDevice::Stop() {
+  DCHECK(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() {
   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_);
 }
 
+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_);
 
   // Create the timer on the thread it's used on. It's guaranteed to be
   // deleted on the same thread since users must call Stop() before deleting
   // AudioOutputDevice; see ShutDownOnIOThread().
   auth_timeout_action_.reset(new base::OneShotTimer());
   auth_timeout_action_->Start(
       FROM_HERE, auth_timeout_,
       base::Bind(&AudioOutputDevice::OnDeviceAuthorized, this,
                  OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT, media::AudioParameters(),
                  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;
       }
       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;
 
+  // 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();
 
   // 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::AutoLock auto_lock_(lock_);
   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) {
   DCHECK(task_runner()->BelongsToCurrentThread());
   if (state_ >= CREATING_STREAM)
     ipc_->SetVolume(volume);
 }
 
 void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
   DCHECK(task_runner()->BelongsToCurrentThread());
@@ skipping 10 matching lines @@
       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();
+        ReportRenderError();
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
 void AudioOutputDevice::OnDeviceAuthorized(
     OutputDeviceStatus device_status,
     const media::AudioParameters& output_params,
@@ skipping 35 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();
+    ReportRenderError();
   }
 }
 
 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.
   //
-  // 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;
 
     // 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