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

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 7044e9ae770649c0477b9b37af56eca6fff01abb..58a67a748b9efa72015f8d69ba59acf3c7a97c5b 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -81,14 +81,21 @@ 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() {
@@ -100,20 +107,26 @@ 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_));
+  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() {
@@ -165,12 +178,29 @@ 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_)
@@ -181,7 +211,7 @@ void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
       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;
@@ -194,7 +224,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;
 
@@ -230,7 +260,7 @@ void AudioOutputDevice::PauseOnIOThread() {
   play_on_start_ = false;
 }
 
-void AudioOutputDevice::ShutDownOnIOThread() {
+void AudioOutputDevice::StopOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   // Close the stream, if we haven't already.
@@ -240,19 +270,16 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   }
   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) {
@@ -276,13 +303,8 @@ void AudioOutputDevice::OnStateChanged(AudioOutputIPCDelegateState state) {
       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:
@@ -315,7 +337,7 @@ 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.
@@ -343,9 +365,9 @@ void AudioOutputDevice::OnStreamCreated(
     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
@@ -362,8 +384,13 @@ void AudioOutputDevice::OnStreamCreated(
     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
@@ -384,7 +411,7 @@ void AudioOutputDevice::OnIPCClosed() {
 
 void AudioOutputDevice::WillDestroyCurrentMessageLoop() {
   LOG(ERROR) << "IO loop going away before the audio device has been stopped";
-  ShutDownOnIOThread();
+  StopOnIOThread();
 }
 
 // AudioOutputDevice::AudioThreadCallback