Change audio render thread checking to use new AudioRendererSink::BelongsToRendererThread()

media/audio/audio_device_thread.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_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/synchronization/lock.h"
+#include "base/synchronization/waitable_event.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);
 
+  // Returns true if called on the device thread, otherwise false.
+  bool BelongsToRenderingThread();
+
  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_;
 
+  // The thread's id once it has started.
+  base::PlatformThreadId id_;

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

thread_id_

[Henrik Grunell, 2016/06/14 11:30:01]

Removed.

+  mutable base::WaitableEvent id_event_;  // Protects |id_|.

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

how does the event protect the thread id variable?  Maybe further clarification
is in order.

[Henrik Grunell, 2016/06/13 20:38:35]

I'll clarify that. (It's set once only, and when done the event is signaled.)

[Henrik Grunell, 2016/06/14 11:30:01]

Removed.

+
   DISALLOW_COPY_AND_ASSIGN(Thread);
 };
 
 // AudioDeviceThread implementation
 
 AudioDeviceThread::AudioDeviceThread() {
 }
 
 AudioDeviceThread::~AudioDeviceThread() { DCHECK(!thread_.get()); }
 
@@ skipping 14 matching lines @@
     thread_->Stop(loop_for_join);
     thread_ = NULL;
   }
 }
 
 bool AudioDeviceThread::IsStopped() {
   base::AutoLock auto_lock(thread_lock_);
   return !thread_.get();
 }
 
+bool AudioDeviceThread::BelongsToRenderingThread() {
+  base::AutoLock auto_lock(thread_lock_);

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

It seems unfortunate to require a lock for this...

We use this function only for debugging and DCHECKing, right?
Is it safe to assume that whenever this function returns |false|, that we have a
problem?  If so, then maybe it's OK to not need a lock here.  If there's a race,
then would it be because we're trying to test the thread object at a point we
shouldn't be assuming anything?

[Henrik Grunell, 2016/06/14 11:30:01]

This has been removed altogether.

+  return thread_.get() ? thread_->BelongsToRenderingThread() : false;

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

nit:

return thread_ && thread_->CurrentThreadIsRenderingThread();

[Henrik Grunell, 2016/06/14 11:30:01]

Removed.

+}
+
 // 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) {
-}
+      synchronized_buffers_(synchronized_buffers),
+      id_(base::kInvalidThreadId),
+      id_event_(base::WaitableEvent::ResetPolicy::MANUAL,
+                base::WaitableEvent::InitialState::NOT_SIGNALED) {}
 
 AudioDeviceThread::Thread::~Thread() {
   DCHECK(thread_.is_null());
 }
 
 void AudioDeviceThread::Thread::Start() {
   base::AutoLock auto_lock(callback_lock_);
   DCHECK(thread_.is_null());
   // This reference will be released when the thread exists.
   AddRef();
@@ skipping 17 matching lines @@
   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);
     }
   }
 }
 
+bool AudioDeviceThread::Thread::BelongsToRenderingThread() {
+  // If the thread is created but not started yet, wait for |id_| being ready.
+  base::ThreadRestrictions::ScopedAllowWait allow_wait;
+  id_event_.Wait();

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

would be great to not require an event object for this... Can we make sure that
this method is never called until we've created the thread and have the
thread_id?

Hmm... since we do have the PlatformThreadHandle from when we create the thread
(see the thread_ member variable), there is a way to get a 'ref' on the starting
thread without having to synchronize.

I don't think this will work, but just for future reference:

On posix systems:
* PlatformThreadHandle will be the pthread_t for the thread
* ...which is also the same thing as what PlatformThreadRef represents.
* So, on Posix, PlatformThreadHandle == PlatformThreadRef, so comparison between
thread_ and PlatformThread:: CurrentRef(), should be possible.  You probably
need to wrap the Handle() from the thread_ in the correct type though, but that
shouldn't be a problem.

On Windows:
* PlatformThreadHandle will be the HANDLE of the thread - not the pseudo handle.
* From this handle however, you can easily get the thread id by calling the
GetThreadId() API and construct a PlatformThreadRef from the id.

So, by using the above, you should be able to keep id_ of type PlatformThreadRef
and have it available immediately after creating the thread.

However, I don't think this will work for the current needs, so please move on
to the next comment :)

[Henrik Grunell, 2016/06/13 20:38:35]

So I shall.

[Henrik Grunell, 2016/06/14 11:30:01]

This has been removed.

+  return id_ == PlatformThread::CurrentId();
+}
+
 void AudioDeviceThread::Thread::ThreadMain() {
+  id_ = PlatformThread::CurrentId();
+  DCHECK_NE(base::kInvalidThreadId, id_);
+  id_event_.Signal();
+
   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.
   base::ThreadRestrictions::SetSingletonAllowed(true);
 
   {  // NOLINT
     base::AutoLock auto_lock(callback_lock_);
     if (callback_)
       callback_->InitializeOnAudioThread();

[tommi (sloooow) - chröme, 2016/06/13 14:15:21]

It looks like AudioDeviceThread itself doesn't need the BelongsToRenderingThread
function.

What about moving all this thread id stuff to the one that implements
InitializeOnAudioThread()?

If so, then we could have a ThreadChecker in that class, Detach in the ctor and
call CalledOnValidThread inside of InitializeOnAudioThread().  Subclasses could
then have access to that same thread checker and offer a
CurrentThreadIsAudioThread type method.

[Henrik Grunell, 2016/06/13 20:38:35]

Well, that could perhaps work. And is perhaps better. I'll take a look tomorrow.

[Henrik Grunell, 2016/06/14 11:30:01]

This should be fine, good suggestion. Done.

   }
 
   Run();
 
   // Release the reference for the thread. Note that after this, the Thread
   // instance will most likely be deleted.
   Release();
 }
 
 void AudioDeviceThread::Thread::Run() {
@@ skipping 62 matching lines @@
 }
 
 AudioDeviceThread::Callback::~Callback() {}
 
 void AudioDeviceThread::Callback::InitializeOnAudioThread() {
   MapSharedMemory();
   CHECK(shared_memory_.memory());
 }
 
 }  // namespace media.