Make AudioOutputDevice restartable and reinitializable

media/audio/audio_device_thread.cc

Index: media/audio/audio_device_thread.cc
diff --git a/media/audio/audio_device_thread.cc b/media/audio/audio_device_thread.cc
index 20874599e68f5f7cfddeff0022291e64aa718b6c..acc6148a72625f4db3fdab762770fdf76249fc73 100644
--- a/media/audio/audio_device_thread.cc
+++ b/media/audio/audio_device_thread.cc
@@ -16,6 +16,7 @@
 #include "base/memory/aligned_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/synchronization/condition_variable.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "media/base/audio_bus.h"
@@ -32,55 +33,75 @@ class AudioDeviceThread::Thread
     : public PlatformThread::Delegate,
       public base::RefCountedThreadSafe<AudioDeviceThread::Thread> {
  public:
-  Thread(AudioDeviceThread::Callback* callback,
-         base::SyncSocket::Handle socket,
-         const char* thread_name,
-         bool synchronized_buffers);
+  Thread(const char* thread_name, bool synchronized_buffers);
 
+  // Starts the thread which will wait in ThreadMain(). Resume() must then be
+  // called to actually start running it.
   void Start();
 
-  // Stops the thread.  If |loop_for_join| is non-NULL, the function posts
+  // Shuts down the thread.  If |loop_for_join| is non-NULL, the function posts
   // a task to join (close) the thread handle later instead of waiting for
   // the thread.  If loop_for_join is NULL, then the function waits
   // synchronously for the thread to terminate.
+  // Must be called.
   void Stop(base::MessageLoop* loop_for_join);
 
+  // Releases the thread from waiting and starts running it with the given
+  // |callback| and |socket|.
+  void Resume(AudioDeviceThread::Callback* callback,
+              base::SyncSocket::Handle socket);
+
+  // Pauses the thread, it will wait in ThreadMain(). Shuts down the
+  // socket. Resume() can then be called to restart it.
+  void Pause();
+
  private:
+  enum State { kStatePaused, kStatePlaying, kStateStopped };

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

Intentionally not using MACRO_STYLE?

[Henrik Grunell, 2016/05/12 12:00:08]

No. Actually Olga's code and I missed that. Done.

+
   friend class base::RefCountedThreadSafe<AudioDeviceThread::Thread>;
+
   ~Thread() override;
 
   // Overrides from PlatformThread::Delegate.
   void ThreadMain() override;
 
-  // Runs the loop that reads from the socket.
+  // Runs the loop that reads from the socket, called in ThreadMain().
   void Run();
 
- private:
-  base::PlatformThreadHandle thread_;
-  AudioDeviceThread::Callback* callback_;
-  base::CancelableSyncSocket socket_;
-  base::Lock callback_lock_;
+  // Start(), Resume(), Pause() must be serialized; Stop() can be called any

[Guido Urdaneta, 2016/05/09 13:15:49]

It's not clear (to me at least) why this comment is directly related to this
variable.

[Henrik Grunell, 2016/05/09 13:46:20]

Clarified.

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

serialized and required to be called on the same thread aren't exactly the same
things (serialized can still execute on multiple threads by using locking). 
Should this be "must be called on the same thread"?

And then "any moment" can still mean on one thread.  I think you mean "on any
thread".

hmm.. actually, maybe I'm misunderstanding the comment completely... does this
refer to the calling sequence perhaps?

[Henrik Grunell, 2016/05/12 12:00:09]

This comment is very much a draft. :) I already clarified it so your comments
have been addressed.

+  // moment.
+  base::ThreadChecker thread_checker_;
+
   const char* thread_name_;
   const bool synchronized_buffers_;
 
+  base::Lock state_lock_;  // Protects everything below.

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

nit: Could just call it lock_ now that it doesn't only protect the state.

[Henrik Grunell, 2016/05/12 12:00:08]

Agree, done.

I also changed the name of the |state_condition_| to "exit_paused_state_".

+  base::ConditionVariable state_condition_;
+  State state_;
+  AudioDeviceThread::Callback* callback_;
+
+  base::PlatformThreadHandle thread_;

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

nit: do you mind renaming this variable to thread_handle_ ?  It can be confusing
to read code in the file and look for where this variable is used and then see
the code in AudioDeviceThread that uses a variable with the same name.

[Henrik Grunell, 2016/05/12 12:00:08]

Sure, done. Also changed name of the local variable in Stop() below.

+
+  // Reset in ThreadMain(). Shutdown on client thread.
+  std::unique_ptr<base::CancelableSyncSocket> socket_;
+
+  // Set on client thead.
+  base::SyncSocket::Handle new_socket_handle_;
+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
-// AudioDeviceThread implementation
+AudioDeviceThread::AudioDeviceThread() {}
 
-AudioDeviceThread::AudioDeviceThread() {
+AudioDeviceThread::~AudioDeviceThread() {
+  DCHECK(!thread_.get());
 }
 
-AudioDeviceThread::~AudioDeviceThread() { DCHECK(!thread_.get()); }
-
-void AudioDeviceThread::Start(AudioDeviceThread::Callback* callback,
-                              base::SyncSocket::Handle socket,
-                              const char* thread_name,
+void AudioDeviceThread::Start(const char* thread_name,
                               bool synchronized_buffers) {
   base::AutoLock auto_lock(thread_lock_);
   CHECK(!thread_.get());
-  thread_ = new AudioDeviceThread::Thread(
-      callback, socket, thread_name, synchronized_buffers);
+  thread_ = new AudioDeviceThread::Thread(thread_name, synchronized_buffers);
   thread_->Start();
 }
 
@@ -88,34 +109,53 @@ void AudioDeviceThread::Stop(base::MessageLoop* loop_for_join) {
   base::AutoLock auto_lock(thread_lock_);
   if (thread_.get()) {
     thread_->Stop(loop_for_join);
-    thread_ = NULL;
+    thread_ = nullptr;
   }
 }
 
+void AudioDeviceThread::Resume(AudioDeviceThread::Callback* callback,
+                               base::SyncSocket::Handle socket) {
+  base::AutoLock auto_lock(thread_lock_);
+  if (thread_.get())
+    thread_->Resume(callback, socket);
+}
+
+void AudioDeviceThread::Pause() {
+  base::AutoLock auto_lock(thread_lock_);
+  if (thread_.get())
+    thread_->Pause();
+}
+
 bool AudioDeviceThread::IsStopped() {
   base::AutoLock auto_lock(thread_lock_);
   return !thread_.get();
 }
 
 // AudioDeviceThread::Thread implementation
-AudioDeviceThread::Thread::Thread(AudioDeviceThread::Callback* callback,
-                                  base::SyncSocket::Handle socket,
-                                  const char* thread_name,
+AudioDeviceThread::Thread::Thread(const char* thread_name,
                                   bool synchronized_buffers)
-    : thread_(),
-      callback_(callback),
-      socket_(socket),
-      thread_name_(thread_name),
-      synchronized_buffers_(synchronized_buffers) {
+    : thread_name_(thread_name),
+      synchronized_buffers_(synchronized_buffers),
+      state_condition_(&state_lock_),
+      state_(kStatePaused),
+      callback_(nullptr),
+      new_socket_handle_(base::CancelableSyncSocket::kInvalidHandle) {
+  thread_checker_.DetachFromThread();
 }
 
 AudioDeviceThread::Thread::~Thread() {
   DCHECK(thread_.is_null());
+  DCHECK_EQ(state_, kStateStopped);
 }
 
 void AudioDeviceThread::Thread::Start() {
-  base::AutoLock auto_lock(callback_lock_);
-  DCHECK(thread_.is_null());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  base::AutoLock auto_lock(state_lock_);
+  DCHECK(thread_.is_null()) << "Calling Start() on a started thread?";
+  if (state_ == kStateStopped)
+    return;  // Stop has already been executed on another thread.
+
   // This reference will be released when the thread exists.

[Guido Urdaneta, 2016/05/09 13:15:49]

drive-by comment: s/exists/exits ?

[Henrik Grunell, 2016/05/09 13:46:20]

Yep. Done.

   AddRef();
 
@@ -125,15 +165,26 @@ void AudioDeviceThread::Thread::Start() {
 }
 
 void AudioDeviceThread::Thread::Stop(base::MessageLoop* loop_for_join) {
-  socket_.Shutdown();
+  // Note: Stop() can be called any time, even before Start(), because it's

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

nit: called at any time

[Henrik Grunell, 2016/05/12 12:00:08]

Done.

+  // allowed to be executed on a different thread.
 
   base::PlatformThreadHandle thread = base::PlatformThreadHandle();
+  {
+    base::AutoLock auto_lock(state_lock_);
+    DCHECK(state_ != kStateStopped) << "Calling Stop() on a stopped thread?";
+    callback_ = nullptr;
+
+    // |socket_| is only set/reset under the |state_lock_|, so it's safe to
+    // access under that lock here.
+    if (socket_)
+      socket_->Shutdown();
 
-  {  // NOLINT
-    base::AutoLock auto_lock(callback_lock_);
-    callback_ = NULL;
+    // Must be done under lock to avoid race wuth Start().

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

with

[Henrik Grunell, 2016/05/12 12:00:08]

Done.

     std::swap(thread, thread_);
-  }
+
+    state_ = kStateStopped;
+    state_condition_.Signal();
+  }  // auto_lock(state_lock_)
 
   if (!thread.is_null()) {
     if (loop_for_join) {
@@ -145,6 +196,42 @@ void AudioDeviceThread::Thread::Stop(base::MessageLoop* loop_for_join) {
   }
 }
 
+void AudioDeviceThread::Thread::Resume(AudioDeviceThread::Callback* callback,
+                                       base::SyncSocket::Handle socket_handle) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  base::AutoLock auto_lock(state_lock_);
+  if (state_ == kStateStopped)
+    return;  // Stop is the last operation to be executed.
+
+  DCHECK(state_ == kStatePaused) << "Resume a non-paused thread?";
+
+  DCHECK_EQ(new_socket_handle_, base::CancelableSyncSocket::kInvalidHandle);
+
+  callback_ = callback;
+  new_socket_handle_ = socket_handle;
+
+  state_ = kStatePlaying;
+  state_condition_.Signal();
+}
+
+void AudioDeviceThread::Thread::Pause() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  base::AutoLock auto_lock(state_lock_);
+  if (state_ == kStateStopped)
+    return;  // Stop is the last operation to be executed.
+
+  DCHECK(state_ == kStatePlaying) << "Pause a paused thread?";
+  callback_ = nullptr;
+
+  // |socket_| is only set/reset under the |state_lock_|, so it's safe to
+  // access under that lock here.
+  if (socket_)
+    socket_->Shutdown();
+  state_ = kStatePaused;
+}
+
 void AudioDeviceThread::Thread::ThreadMain() {
   PlatformThread::SetName(thread_name_);
 
@@ -154,13 +241,33 @@ void AudioDeviceThread::Thread::ThreadMain() {
   // by another thread on which singleton access is OK as well.
   base::ThreadRestrictions::SetSingletonAllowed(true);
 
-  {  // NOLINT
-    base::AutoLock auto_lock(callback_lock_);
-    if (callback_)
-      callback_->InitializeOnAudioThread();
-  }
+  while (true) {
+    {
+      base::AutoLock auto_lock(state_lock_);
+      while (state_ == kStatePaused)
+        state_condition_.Wait();

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

Can you add a comment that this releases the state_lock_ while waiting?

[Henrik Grunell, 2016/05/12 12:00:08]

Done.

+
+      if (state_ == kStateStopped)
+        break;
+      // Else reinitialize and resume.
+
+      socket_.reset(new base::CancelableSyncSocket(new_socket_handle_));
+
+      if (callback_)
+        callback_->InitializeOnAudioThread();
 
-  Run();
+      new_socket_handle_ = base::CancelableSyncSocket::kInvalidHandle;
+    }  // auto_lock(state_lock_)
+
+    Run();
+
+    // We are here if and only if socket operation failed. The possible reasons
+    // are:
+    // 1. Shutdown() has been called on the socket, i.e. Stop() or Pause() has
+    //    been called, or
+    // 2. Socket failed.
+    // In both cases go back and wait for stop or resume.
+  }  // while

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

nit: remove the comment.  It looks like it's a part of the previous comment.

[Henrik Grunell, 2016/05/12 12:00:09]

Done.

 
   // Release the reference for the thread. Note that after this, the Thread
   // instance will most likely be deleted.
@@ -169,9 +276,12 @@ void AudioDeviceThread::Thread::ThreadMain() {
 
 void AudioDeviceThread::Thread::Run() {
   uint32_t buffer_index = 0;
+

[Guido Urdaneta, 2016/05/09 13:15:49]

Should there be a thread_checker or similar here?

[Henrik Grunell, 2016/05/09 13:46:20]

That's not necessary, it's the audio thread run function only called from
ThreadMain.

   while (true) {
     uint32_t pending_data = 0;
-    size_t bytes_read = socket_.Receive(&pending_data, sizeof(pending_data));
+    // |socket_| is only modified in ThreadMain, so it is safe to access here.

[tommi (sloooow) - chröme, 2016/05/11 14:12:34]

nit: // |socket_| is only modified on this thread, so we don't need a lock to
access it here.

[Henrik Grunell, 2016/05/12 12:00:08]

Done.

+    size_t bytes_read = socket_->Receive(&pending_data, sizeof(pending_data));
+
     if (bytes_read != sizeof(pending_data))
       break;
 
@@ -184,7 +294,7 @@ void AudioDeviceThread::Thread::Run() {
     //
     // See comments in AudioOutputController::DoPause() for details on why.
     if (pending_data != std::numeric_limits<uint32_t>::max()) {
-      base::AutoLock auto_lock(callback_lock_);
+      base::AutoLock auto_lock(state_lock_);
       if (callback_)
         callback_->Process(pending_data);
     }
@@ -201,11 +311,12 @@ void AudioDeviceThread::Thread::Run() {
     // details on how this works see AudioSyncReader::WaitUntilDataIsReady().
     if (synchronized_buffers_) {
       ++buffer_index;
-      size_t bytes_sent = socket_.Send(&buffer_index, sizeof(buffer_index));
+      size_t bytes_sent = socket_->Send(&buffer_index, sizeof(buffer_index));
+
       if (bytes_sent != sizeof(buffer_index))
         break;
     }
-  }
+  }  // while
 }
 
 // AudioDeviceThread::Callback implementation