Making AudioThread::Thread restartable *** DRAFT ***

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 61 matching lines @@
   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?";

[Guido Urdaneta, 2016/04/01 10:17:02]

Why should Initialize by async?

A common idiom is
AOD::Initialize();
AOD::Start();

That will no longer work if Initialize is asynchronous since the AOD might not
be inizialized yet when AOD::Start() is called.

Also, the DCHECK(!callback_) will not work if the intent was to prevent dual
calls to Initialize(). And you would need a lock to protect callback_

[Henrik Grunell, 2016/04/04 08:08:59]

Initialize() and Start() are serialized, so we're fine. They should only post on
IO thread, nothing else. The DCHECK here and in Start() must be removed. (Or
moved to IO thread function counterpart.)

   DCHECK(params.IsValid());
-  audio_parameters_ = params;
-  callback_ = callback;
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::InitializeOnIOThread, this,
+                            params, callback));
 }
 
 AudioOutputDevice::~AudioOutputDevice() {
+  // I'm not sure if Stop() should remain  mandatory. The purpose of that was
+  // to fix object lifetime issues. Probably we can do it in a cleaner way.

[Guido Urdaneta, 2016/04/01 10:17:01]

The purpose of mandatory Stop() is to make sure you clean up resources on the
browser side. For example, if you never call AOD::Start(), the browser will keep
authorization data related to the AOD. That data is removed with Stop() (also
with Start(), but if you call Start() then you have to call Stop() to remove the
stream on the browser).

[Henrik Grunell, 2016/04/04 08:08:59]

I wonder if we can post a task on the IO thread in the dtor to clean this up.
This should work if the data is guaranteed to be cleaned up when the IPC channel
goes away. (Needed since the IO thread may have gone away when the dtor is run.)

   // The current design requires that the user calls Stop() before deleting
-  // this class.
-  DCHECK(audio_thread_.IsStopped());
+  // this class. Since this object is reference counted, we're the only one
+  // accessing data when we destruct.
+  DCHECK(state_ == IDLE || state_ == IPC_CLOSED);
+  // We need to ensure all the object lifetime stuff still works.
+  // Or even better, fix it to be more clear.
+  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_));
+  task_runner()->PostTask(
+      FROM_HERE, base::Bind(&AudioOutputDevice::CreateStreamOnIOThread, this));
 }
 
 void AudioOutputDevice::Stop() {
+  // TODO(olka): We need to clean this up; audio_thread_.Pause() must be called
+  // on IO thread only.

[Henrik Grunell, 2016/04/04 08:08:59]

The problem here is that after Stop() returns, the callback can go away, this is
the reason for stopping the audio thread right away in Stop(), and having
|stopping_hack_|. We must ensure the callback isn't called or that it outlives
AOD (see comment in OnStreamCreated()).

olka@ and I discussed offline to remove ref counting on AOD, similar work is
currently done for AudioInputDevice.

[Henrik Grunell, 2016/05/09 12:13:37]

This has been addressed in the other CL.

   {
     base::AutoLock auto_lock(audio_thread_lock_);
-    audio_thread_.Stop(base::MessageLoop::current());
+    // Play() is always called on IO thread. It means it can arrive after this
+    // Pause was executed, even if it was issued first. But we are safe, since
+    // Pause() will be called again in StopOnIOThread, so it's guaranteed to be
+    // called again after that sneaky Play(). What a hack.
+    // I'm not sure if we haven't broken this or about to break.
+    audio_thread_.Pause();
     stopping_hack_ = true;
   }
 
   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 31 matching lines @@
 OutputDeviceStatus AudioOutputDevice::GetDeviceStatus() {
   CHECK(!task_runner()->BelongsToCurrentThread());
   did_receive_auth_.Wait();
   return device_status_;
 }
 
 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 re-starting. This involves mostly changes on the browser side:
+  // associate authorization by a new id (currently the stream id), don't forget
+  // authorization away after start or stop. Here we would have to explicitly
+  // make the browser side forget authorization in the dtor with a new
+  // AudioOutputIPC function.
   ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
                                    security_origin_);
 }
 
-void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
+void AudioOutputDevice::InitializeOnIOThread(const AudioParameters& params,
+                                             RenderCallback* callback) {
   DCHECK(task_runner()->BelongsToCurrentThread());
+  DCHECK(!callback_);
+  audio_parameters_ = params;
+  callback_ = callback;
+}
+
+void AudioOutputDevice::CreateStreamOnIOThread() {
+  DCHECK(task_runner()->BelongsToCurrentThread());
+  DCHECK(callback_ || state_ == IPC_CLOSED);
+
   switch (state_) {
     case IPC_CLOSED:
       if (callback_)
         callback_->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;
   }
 }
@@ 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.
   if (state_ >= AUTHORIZING) {
     ipc_->CloseStream();
     state_ = IDLE;
   }
   start_on_authorized_ = false;
 
-  // We can run into an issue where ShutDownOnIOThread is called right after
+  // We can run into an issue where StopOnIOThread 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.

[Guido Urdaneta, 2016/04/01 10:17:01]

now you call Pause() instead of Stop()

[Henrik Grunell, 2016/05/09 12:13:37]

Done in other CL.

-  //
-  // 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;
+  callback_ = nullptr;
 }
 
 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())
+      // The callback object is cleared in Stop().
+      if (callback_)
         callback_->OnRenderError();
       break;
     default:
       NOTREACHED();
       break;
   }
 }
 
 void AudioOutputDevice::OnDeviceAuthorized(
     OutputDeviceStatus device_status,
@@ skipping 12 matching lines @@
   if (!did_receive_auth_.IsSignaled())
     device_status_ = device_status;
 
   if (device_status == OUTPUT_DEVICE_STATUS_OK) {
     state_ = AUTHORIZED;
     if (!did_receive_auth_.IsSignaled()) {
       output_params_ = output_params;
       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();
   }
 }
 
 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
+  // called Stop() but before StopOnIOThread() 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.
+  // |audio_thread_| do nothing and wait for StopOnIOThread() 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.
   {
     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);
+
+    // If the audio thread hasn't been started before, start it.
+    if (audio_thread_.IsStopped()) {
+      audio_thread_.Start("AudioOutputDevice", true);
+    }
+    audio_thread_.Play(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