Cleanup AudioDeviceThread to reduce locking and unused features.

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..03987d90d8ef7bf836d5273cee335a28cfe0b8cc 100644
--- a/media/audio/audio_device_thread.cc
+++ b/media/audio/audio_device_thread.cc
@@ -4,170 +4,68 @@
 
 #include "media/audio/audio_device_thread.h"
 
-#include <stddef.h>
-#include <stdint.h>
-
-#include <algorithm>
 #include <limits>
 
-#include "base/bind.h"
 #include "base/logging.h"
-#include "base/macros.h"
-#include "base/memory/aligned_memory.h"
-#include "base/message_loop/message_loop.h"
-#include "base/numerics/safe_conversions.h"
-#include "base/threading/platform_thread.h"
 #include "base/threading/thread_restrictions.h"
-#include "media/base/audio_bus.h"
-
-using base::PlatformThread;
 
 namespace media {
 
-// The actual worker thread implementation.  It's very bare bones and much
-// simpler than SimpleThread (no synchronization in Start, etc) and supports
-// joining the thread handle asynchronously via a provided message loop even
-// after the Thread object itself has been deleted.
-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);
-
-  void Start();
-
-  // Stops 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.
-  void Stop(base::MessageLoop* loop_for_join);
-
- private:
-  friend class base::RefCountedThreadSafe<AudioDeviceThread::Thread>;
-  ~Thread() override;
-
-  // Overrides from PlatformThread::Delegate.
-  void ThreadMain() override;
-
-  // Runs the loop that reads from the socket.
-  void Run();
-
- private:
-  base::PlatformThreadHandle thread_;
-  AudioDeviceThread::Callback* callback_;
-  base::CancelableSyncSocket socket_;
-  base::Lock callback_lock_;
-  const char* thread_name_;
-  const bool synchronized_buffers_;
-
-  DISALLOW_COPY_AND_ASSIGN(Thread);
-};
-
-// AudioDeviceThread implementation
-
-AudioDeviceThread::AudioDeviceThread() {
-}
-
-AudioDeviceThread::~AudioDeviceThread() { DCHECK(!thread_.get()); }
-
-void AudioDeviceThread::Start(AudioDeviceThread::Callback* callback,
-                              base::SyncSocket::Handle socket,
-                              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_->Start();
-}
-
-void AudioDeviceThread::Stop(base::MessageLoop* loop_for_join) {
-  base::AutoLock auto_lock(thread_lock_);
-  if (thread_.get()) {
-    thread_->Stop(loop_for_join);
-    thread_ = NULL;
-  }
-}
+// AudioDeviceThread::Callback implementation
 
-bool AudioDeviceThread::IsStopped() {
-  base::AutoLock auto_lock(thread_lock_);
-  return !thread_.get();
+AudioDeviceThread::Callback::Callback(const AudioParameters& audio_parameters,
+                                      base::SharedMemoryHandle memory,
+                                      int memory_length,
+                                      int total_segments)
+    : audio_parameters_(audio_parameters),
+      samples_per_ms_(static_cast<double>(audio_parameters.sample_rate()) /
+                      base::Time::kMillisecondsPerSecond),
+      bytes_per_ms_(audio_parameters.channels() *
+                    (audio_parameters_.bits_per_sample() / 8) *
+                    samples_per_ms_),
+      bytes_per_frame_(audio_parameters.GetBytesPerFrame()),
+      shared_memory_(memory, false),
+      memory_length_(memory_length),
+      total_segments_(total_segments) {
+  CHECK_NE(bytes_per_ms_, 0.0);  // Catch division by zero early.
+  CHECK_NE(samples_per_ms_, 0.0);
+  CHECK_NE(bytes_per_frame_, 0u);
+  CHECK_GT(total_segments_, 0);
+  CHECK_EQ(memory_length_ % total_segments_, 0);
+  segment_length_ = memory_length_ / total_segments_;
 }
 
-// AudioDeviceThread::Thread implementation
-AudioDeviceThread::Thread::Thread(AudioDeviceThread::Callback* callback,
-                                  base::SyncSocket::Handle socket,
-                                  const char* thread_name,
-                                  bool synchronized_buffers)
-    : thread_(),
-      callback_(callback),
-      socket_(socket),
-      thread_name_(thread_name),
-      synchronized_buffers_(synchronized_buffers) {
-}
+AudioDeviceThread::Callback::~Callback() {}
 
-AudioDeviceThread::Thread::~Thread() {
-  DCHECK(thread_.is_null());
+void AudioDeviceThread::Callback::InitializeOnAudioThread() {
+  MapSharedMemory();
+  CHECK(shared_memory_.memory());

[tommi (sloooow) - chröme, 2016/06/16 22:19:22]

should there be a [D]CHECK for memory() being nullptr at the beginning of the
function?
Henrik had a dcheck + comment that InitializeOnAudioThread wasn't guaranteed to
be called on the same thread always, but I think it should be called exactly
once (right?).

[DaleCurtis, 2016/06/16 22:24:59]

It's called in the ThreadMain() below so it must be exactly once. I'll add the
DCHECK.

 }
 
-void AudioDeviceThread::Thread::Start() {
-  base::AutoLock auto_lock(callback_lock_);
-  DCHECK(thread_.is_null());
-  // This reference will be released when the thread exists.
-  AddRef();
+// AudioDeviceThread implementation
 
-  PlatformThread::CreateWithPriority(0, this, &thread_,
-                                     base::ThreadPriority::REALTIME_AUDIO);
-  CHECK(!thread_.is_null());
+AudioDeviceThread::AudioDeviceThread(Callback* callback,
+                                     base::SyncSocket::Handle socket,
+                                     const char* thread_name)
+    : callback_(callback), thread_name_(thread_name), socket_(socket) {
+  CHECK(base::PlatformThread::CreateWithPriority(
+      0, this, &thread_handle_, base::ThreadPriority::REALTIME_AUDIO));
+  DCHECK(!thread_handle_.is_null());
 }
 
-void AudioDeviceThread::Thread::Stop(base::MessageLoop* loop_for_join) {
+AudioDeviceThread::~AudioDeviceThread() {
   socket_.Shutdown();
-
-  base::PlatformThreadHandle thread = base::PlatformThreadHandle();
-
-  {  // NOLINT
-    base::AutoLock auto_lock(callback_lock_);
-    callback_ = NULL;
-    std::swap(thread, thread_);
-  }
-
-  if (!thread.is_null()) {
-    if (loop_for_join) {
-      loop_for_join->PostTask(FROM_HERE,
-          base::Bind(&base::PlatformThread::Join, thread));
-    } else {
-      base::PlatformThread::Join(thread);
-    }
-  }
+  if (thread_handle_.is_null())
+    return;
+  base::PlatformThread::Join(thread_handle_);
+  DCHECK(thread_handle_.is_null());
 }
 
-void AudioDeviceThread::Thread::ThreadMain() {
-  PlatformThread::SetName(thread_name_);
-
-  // Singleton access is safe from this thread as long as callback is non-NULL.
-  // The callback is the only point where the thread calls out to 'unknown' code
-  // that might touch singletons and the lifetime of the callback is controlled
-  // by another thread on which singleton access is OK as well.
+void AudioDeviceThread::ThreadMain() {
+  base::PlatformThread::SetName(thread_name_);
   base::ThreadRestrictions::SetSingletonAllowed(true);
+  callback_->InitializeOnAudioThread();
 
-  {  // NOLINT
-    base::AutoLock auto_lock(callback_lock_);
-    if (callback_)
-      callback_->InitializeOnAudioThread();
-  }
-
-  Run();
-
-  // Release the reference for the thread. Note that after this, the Thread
-  // instance will most likely be deleted.
-  Release();
-}
-
-void AudioDeviceThread::Thread::Run() {
   uint32_t buffer_index = 0;
   while (true) {
     uint32_t pending_data = 0;
@@ -180,63 +78,32 @@ void AudioDeviceThread::Thread::Run() {
     // renderer side request.
     //
     // Avoid running Process() for the paused signal, we still need to update
-    // the buffer index if |synchronized_buffers_| is true though.
+    // the buffer index for synchronized buffers though.
     //
     // See comments in AudioOutputController::DoPause() for details on why.
-    if (pending_data != std::numeric_limits<uint32_t>::max()) {
-      base::AutoLock auto_lock(callback_lock_);
-      if (callback_)
-        callback_->Process(pending_data);
-    }
-
-    // The usage of |synchronized_buffers_| differs between input and output
-    // cases.
-    // Input:
-    // Let the other end know that we have read data, so that it can verify
-    // it doesn't overwrite any data before read. The |buffer_index| value is
-    // not used. For more details, see AudioInputSyncWriter::Write().
-    // Output:
-    // Let the other end know which buffer we just filled. The |buffer_index| is
-    // used to ensure the other end is getting the buffer it expects. For more
-    // details on how this works see AudioSyncReader::WaitUntilDataIsReady().
-    if (synchronized_buffers_) {
-      ++buffer_index;
-      size_t bytes_sent = socket_.Send(&buffer_index, sizeof(buffer_index));
-      if (bytes_sent != sizeof(buffer_index))
-        break;
-    }
-  }
-}
-
-// AudioDeviceThread::Callback implementation
+    if (pending_data != std::numeric_limits<uint32_t>::max())
+      callback_->Process(pending_data);
 
-AudioDeviceThread::Callback::Callback(const AudioParameters& audio_parameters,
-                                      base::SharedMemoryHandle memory,
-                                      int memory_length,
-                                      int total_segments)
-    : audio_parameters_(audio_parameters),
-      samples_per_ms_(static_cast<double>(audio_parameters.sample_rate()) /
-                      base::Time::kMillisecondsPerSecond),
-      bytes_per_ms_(audio_parameters.channels() *
-                    (audio_parameters_.bits_per_sample() / 8) *
-                    samples_per_ms_),
-      bytes_per_frame_(audio_parameters.GetBytesPerFrame()),
-      shared_memory_(memory, false),
-      memory_length_(memory_length),
-      total_segments_(total_segments) {
-  CHECK_NE(bytes_per_ms_, 0.0);  // Catch division by zero early.
-  CHECK_NE(samples_per_ms_, 0.0);
-  CHECK_NE(bytes_per_frame_, 0u);
-  CHECK_GT(total_segments_, 0);
-  CHECK_EQ(memory_length_ % total_segments_, 0);
-  segment_length_ = memory_length_ / total_segments_;
-}
-
-AudioDeviceThread::Callback::~Callback() {}
+    // The usage of synchronized buffers differs between input and output cases.
+    //
+    // Input: Let the other end know that we have read data, so that it can
+    // verify it doesn't overwrite any data before read. The |buffer_index|
+    // value is not used. For more details, see AudioInputSyncWriter::Write().
+    //
+    // Output: Let the other end know which buffer we just filled. The
+    // |buffer_index| is used to ensure the other end is getting the buffer it
+    // expects. For more details on how this works see
+    // AudioSyncReader::WaitUntilDataIsReady().
+    ++buffer_index;
+    size_t bytes_sent = socket_.Send(&buffer_index, sizeof(buffer_index));
+    if (bytes_sent != sizeof(buffer_index))
+      break;
+  }
 
-void AudioDeviceThread::Callback::InitializeOnAudioThread() {
-  MapSharedMemory();
-  CHECK(shared_memory_.memory());
+  // This can be racy, but we can't hold a lock here and if we don't clear this
+  // on failure, we may join the wrong thread during a later Stop().
+  // TODO(dalecurtis): Is there a better option here?
+  thread_handle_ = base::PlatformThreadHandle();

[tommi (sloooow) - chröme, 2016/06/16 22:19:22]

I think we'll need to fix this before checkin.  Ideally, the handle should
always be closed on the same thread.

[DaleCurtis, 2016/06/16 22:24:59]

What do you think about changing this to a shutdown_signal_.Wait()? And
signaling during destruction before the Join?

[tommi (sloooow) - chröme, 2016/06/16 22:36:18]

Hmm... this is both the normal and abnormal exit path.  How could we join the
wrong thread during a later stop?  Even though it has exited, how could we join
the wrong thread?  (Is it a pthread thing?)

It does seem like we're missing a notification to the callback that the thread
is exiting.  If we had that, the owner of the thread could trigger the teardown
by e.g. posting a task to the thread that actually owns this one.
Possibly, the implementation of such a signal, could be done by moving ownership
of the callback to the AudioDeviceThread and always delete the callback object
before exiting the thread (i.e. the dtor would be the signal).

[DaleCurtis, 2016/06/16 22:55:16]

Derp, I should actually do a bit more research before declaring something a bug.
I was wrong, just because ThreadMain() exits, doesn't mean that the
|thread_handle_| is invalid. Removed this silliness. 

 }
 
 }  // namespace media.