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

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

This is required for making AudioOutputDevice restartable. We'll then spin up a new thread each Start(), and we need to deal with thread checks differently in the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render thread has changed.

BUG=607916
Committed: https://crrev.com/2c754510b660c9c13e754e8e01599977e5c41aad
Cr-Commit-Position: refs/heads/master@{#400429}

[Henrik Grunell, 2016-06-03 13:48:40]

Description was changed from

==========
New AudioRendererSink and ARS::Callback functions.

BUG=
==========

to

==========
New AudioRendererSink and ARS::Callback functions.

We'd use either of them, not both.

BUG=
==========

[Henrik Grunell, 2016-06-03 13:51:36]

grunell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, olka@chromium.org, tommi@chromium.org

[Henrik Grunell, 2016-06-03 13:53:42]

Please have a look at these changes, either of the functions will be added.

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: virtual void OnInitialized() = 0;
Only WebRTC will care about this function, and will make sure
MediaStreamAudioProcessor detaches from the thread it's currently bound to for
render calls.

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:78: virtual bool BelongsToRenderingThread() =
0;
If we go for the above OnInitialized(), we don't need this function.

If we keep this function instead, only WebRTC will use it. In that case we can't
detach in MSAP. We have two options: remove thread checks in MSAP or forward
thread checking to elsewhere (i.e. its owner).

[DaleCurtis, 2016-06-03 20:54:13]

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: virtual void OnInitialized() = 0;
On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> Only WebRTC will care about this function, and will make sure
MediaStreamAudioProcessor detaches from the thread it's currently bound to for
render calls.

Why not always detach until the first render call?

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:78: virtual bool BelongsToRenderingThread() =
0;
On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> If we go for the above OnInitialized(), we don't need this function.
> 
> If we keep this function instead, only WebRTC will use it. In that case we
can't detach in MSAP. We have two options: remove thread checks in MSAP or
forward thread checking to elsewhere (i.e. its owner).

What do you mean about detaching? This would replace any threading checks, no?

[Henrik Grunell, 2016-06-08 09:19:07]

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: virtual void OnInitialized() = 0;
On 2016/06/03 20:54:13, DaleCurtis wrote:
> On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> > Only WebRTC will care about this function, and will make sure
> MediaStreamAudioProcessor detaches from the thread it's currently bound to for
> render calls.
> 
> Why not always detach until the first render call?

The mixer in the upcoming mixing changes will restart the AOD (thus starting a
new thread) if needed when a new input is added. We need to be informed when
that happens and this is the means to do that. OnInitialized() is perhaps not an
accurate naming, but I'd like something somewhat generic.

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:78: virtual bool BelongsToRenderingThread() =
0;
On 2016/06/03 20:54:13, DaleCurtis wrote:
> On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> > If we go for the above OnInitialized(), we don't need this function.
> > 
> > If we keep this function instead, only WebRTC will use it. In that case we
> can't detach in MSAP. We have two options: remove thread checks in MSAP or
> forward thread checking to elsewhere (i.e. its owner).
> 
> What do you mean about detaching? This would replace any threading checks, no?

In the case above, when the mixer restarts AOD, with this option we won't know
about that and not be able to detach. Yes, this would replace thread checks.

[DaleCurtis, 2016-06-08 18:10:13]

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:78: virtual bool BelongsToRenderingThread() =
0;
On 2016/06/08 at 09:19:06, Henrik Grunell wrote:
> On 2016/06/03 20:54:13, DaleCurtis wrote:
> > On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> > > If we go for the above OnInitialized(), we don't need this function.
> > > 
> > > If we keep this function instead, only WebRTC will use it. In that case we
> > can't detach in MSAP. We have two options: remove thread checks in MSAP or
> > forward thread checking to elsewhere (i.e. its owner).
> > 
> > What do you mean about detaching? This would replace any threading checks,
no?
> 
> In the case above, when the mixer restarts AOD, with this option we won't know
about that and not be able to detach. Yes, this would replace thread checks.

The corollary to my question is that if this replaces thread checks, why do you
even need to detach? Just always use this instead of a ThreadChecker.

[Henrik Grunell, 2016-06-08 19:46:40]

So, the twist is MSAP. It doesn't know about ASR and this function. I.e. it
has to do something else, maybe a similar function its owner implements. No
one will ever detach with this option.

Den 8 juni 2016 8:10 em skrev <dalecurtis@chromium.org>:

>
>
>
https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
> File media/base/audio_renderer_sink.h (right):
>
>
>
https://codereview.chromium.org/2038053002/diff/1/media/base/audio_renderer_s...
> media/base/audio_renderer_sink.h:78: virtual bool
> BelongsToRenderingThread() = 0;
> On 2016/06/08 at 09:19:06, Henrik Grunell wrote:
> > On 2016/06/03 20:54:13, DaleCurtis wrote:
> > > On 2016/06/03 at 13:53:42, Henrik Grunell wrote:
> > > > If we go for the above OnInitialized(), we don't need this
> function.
> > > >
> > > > If we keep this function instead, only WebRTC will use it. In that
> case we
> > > can't detach in MSAP. We have two options: remove thread checks in
> MSAP or
> > > forward thread checking to elsewhere (i.e. its owner).
> > >
> > > What do you mean about detaching? This would replace any threading
> checks, no?
> >
> > In the case above, when the mixer restarts AOD, with this option we
> won't know about that and not be able to detach. Yes, this would replace
> thread checks.
>
> The corollary to my question is that if this replaces thread checks, why
> do you even need to detach? Just always use this instead of a
> ThreadChecker.
>
> https://codereview.chromium.org/2038053002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2016-06-08 20:39:13]

Is there any bridge between these classes that can be used to force detach of
MSAP? I.e. if your sink can do a check like:

if (ARS::BelongsToCurrentThread() && !ThreadChecker.BelongsToCurrentThread())
  ThreadChecker.detach()

?

[Henrik Grunell, 2016-06-09 12:01:36]

On 2016/06/08 20:39:13, DaleCurtis wrote:
> Is there any bridge between these classes that can be used to force detach of
> MSAP? I.e. if your sink can do a check like:
> 
> if (ARS::BelongsToCurrentThread() && !ThreadChecker.BelongsToCurrentThread())
>   ThreadChecker.detach()
> 
> ?

Ah, of course. Keeping the current thread checker allows to detect a thread
switch nicely - great idea. If we add
MediaStreamAudioRenderer::BelongsToRenderingThread(), then WebRtcAudioDeviceImpl
who owns MSAP can do this detection and tell MSAP to detach.

[Henrik Grunell, 2016-06-10 12:43:41]

New patch set with only the BelongsToRenderingThread() function, and change to
use it instead of ThreadChecker.

[DaleCurtis, 2016-06-10 18:24:49]

Overall this looks good, but the call sites in production code should probably
not be NOTIMPLEMENTED() if there's a simple implementation available.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:109: bool
AudioDeviceThread::BelongsToRenderingThread() {
Instead of hopping through two locks, it seems like AudioDeviceThread should
just store a ref to the thread id directly. see base::Thread for an example of
how they do this.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:163: #if DCHECK_IS_ON()
Only ThreadCheckers disable for DCHECK, typically BelongsToXXXThread do not, so
I think this should always be available. I could be convinced otherwise since we
don't have a message loop though.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
File media/audio/null_audio_sink.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
media/audio/null_audio_sink.cc:83: NOTIMPLEMENTED();
This is used in production, so I think you want to return
message_loop_->BelongsToCurrentThread().

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
media/base/audio_renderer_mixer_input.cc:126: NOTIMPLEMENTED();
Ditto.

[Henrik Grunell, 2016-06-13 12:43:42]

Description was changed from

==========
New AudioRendererSink and ARS::Callback functions.

We'd use either of them, not both.

BUG=
==========

to

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

This is required for making AudioOutputDevice restartable. We'll then spin up a
new thread each Start(), and we need to deal with thread checks differently in
the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also
MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render
thread has changed.

BUG=607916
==========

[Henrik Grunell, 2016-06-13 12:43:42]

grunell@chromium.org changed reviewers:
+ jam@chromium.org

[Henrik Grunell, 2016-06-13 12:46:15]

Addressed Dale's comments.

Adding jam@ for base/threading/thread_restrictions.h. Is it acceptable to add
AudioDeviceThread as friend to use ScopedAllowWait? It's used for the same
reason as in base::Thread. If no, any suggestion?

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:109: bool
AudioDeviceThread::BelongsToRenderingThread() {
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Instead of hopping through two locks, it seems like AudioDeviceThread should
> just store a ref to the thread id directly. see base::Thread for an example of
> how they do this.

base::Thread corresponds to ADT::Thread. We can replace the lock with an event
there.

Here we still need to ask ATD::Thread about the right thread (or get the ID). We
could store the ID here, but that data still needs to be protected by a lock
since we must clear it in Stop(), to be able to get a new after next Start().
(If you still think it's doable, that's great, could you be more specific on
how?)

I changed the first, removing one lock. Note that the change requires approval
by jam@ or brettw@.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:163: #if DCHECK_IS_ON()
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Only ThreadCheckers disable for DCHECK, typically BelongsToXXXThread do not,
so
> I think this should always be available. I could be convinced otherwise since
we
> don't have a message loop though.

OK, that's fine I think. Done.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
File media/audio/null_audio_sink.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
media/audio/null_audio_sink.cc:83: NOTIMPLEMENTED();
On 2016/06/10 18:24:49, DaleCurtis wrote:
> This is used in production, so I think you want to return
> message_loop_->BelongsToCurrentThread().

Done.

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
media/base/audio_renderer_mixer_input.cc:126: NOTIMPLEMENTED();
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Ditto.

There's no as simple implementation as for NullAudioSink. It would have to be
forwarded to the mixer, which is of course not complicated but a new lock needs
to be introduced to protect |mixer_|. And since no one will use this right now,
it seems unnecessary. When WebRTC is added to mixers, it can be implemented in a
suitable way. (The code here may change.)

[Henrik Grunell, 2016-06-13 12:46:15]

Addressed Dale's comments.

Adding jam@ for base/threading/thread_restrictions.h. Is it acceptable to add
AudioDeviceThread as friend to use ScopedAllowWait? It's used for the same
reason as in base::Thread. If no, any suggestion?

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:109: bool
AudioDeviceThread::BelongsToRenderingThread() {
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Instead of hopping through two locks, it seems like AudioDeviceThread should
> just store a ref to the thread id directly. see base::Thread for an example of
> how they do this.

base::Thread corresponds to ADT::Thread. We can replace the lock with an event
there.

Here we still need to ask ATD::Thread about the right thread (or get the ID). We
could store the ID here, but that data still needs to be protected by a lock
since we must clear it in Stop(), to be able to get a new after next Start().
(If you still think it's doable, that's great, could you be more specific on
how?)

I changed the first, removing one lock. Note that the change requires approval
by jam@ or brettw@.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:163: #if DCHECK_IS_ON()
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Only ThreadCheckers disable for DCHECK, typically BelongsToXXXThread do not,
so
> I think this should always be available. I could be convinced otherwise since
we
> don't have a message loop though.

OK, that's fine I think. Done.

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
File media/audio/null_audio_sink.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/audio/null_audio_...
media/audio/null_audio_sink.cc:83: NOTIMPLEMENTED();
On 2016/06/10 18:24:49, DaleCurtis wrote:
> This is used in production, so I think you want to return
> message_loop_->BelongsToCurrentThread().

Done.

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/2038053002/diff/20001/media/base/audio_render...
media/base/audio_renderer_mixer_input.cc:126: NOTIMPLEMENTED();
On 2016/06/10 18:24:49, DaleCurtis wrote:
> Ditto.

There's no as simple implementation as for NullAudioSink. It would have to be
forwarded to the mixer, which is of course not complicated but a new lock needs
to be introduced to protect |mixer_|. And since no one will use this right now,
it seems unnecessary. When WebRTC is added to mixers, it can be implemented in a
suitable way. (The code here may change.)

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

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK(thread_checker_.CalledOnValidThread());
nit: The DCHECK() part here isn't needed.  Just call
thread_checker_.CalledOnValidThread().

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:67: base::AutoLock
auto_lock(lock_);
we grab this lock right after this block.  Can we just grab it once and have
this code + #ifdefs inside the below scope?

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:69:
DCHECK(on_rendering_thread);
doesn't look like you need this variable.  Just do
DCHECK(renderer_->BelongsToRenderingThread());

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:71: for
(PlayoutDataSinkList::const_iterator it = playout_sinks_.begin();
range based loop?

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.h:113: bool
BelongsToRenderingThread();
CurrentThreadIsRenderingThread.

"belongs to rendering thread" suggests to me that the object belongs to the
rendering thread and not necessarily that we're checking if the current thread
is the rendering thread.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:72: base::PlatformThreadId id_;
thread_id_

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:73: mutable base::WaitableEvent id_event_; 
// Protects |id_|.
how does the event protect the thread id variable?  Maybe further clarification
is in order.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:110: base::AutoLock auto_lock(thread_lock_);
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?

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:111: return thread_.get() ?
thread_->BelongsToRenderingThread() : false;
nit:

return thread_ && thread_->CurrentThreadIsRenderingThread();

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:167: id_event_.Wait();
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 :)

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:187: callback_->InitializeOnAudioThread();
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.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.h:101: bool BelongsToRenderingThread();
similarly, CurrentThreadIsDeviceThread()

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:154: return
audio_thread_.BelongsToRenderingThread();
here you could check the callback instead.

[jam, 2016-06-13 18:22:22]

lgtm

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
base/threading/thread_restrictions.h:220: friend class media::AudioDeviceThread;
nit: order

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

Replying to some comments. I'll fix the rest of the comments tomorrow as
suggested.

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
base/threading/thread_restrictions.h:220: friend class media::AudioDeviceThread;
On 2016/06/13 18:22:22, jam wrote:
> nit: order

I actually added it last because there isn't a clear order. I can order all
friend statements above if you like? Otherwise, I'll put it after android::...

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:67: base::AutoLock
auto_lock(lock_);
On 2016/06/13 14:15:20, tommi-chrömium wrote:
> we grab this lock right after this block.  Can we just grab it once and have
> this code + #ifdefs inside the below scope?

Yeah, that's better locking wise (in builds with DCHECKs enabled), this is
slightly more readable imo. Let's minimize locking, I'll change that.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.h:113: bool
BelongsToRenderingThread();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> CurrentThreadIsRenderingThread.
> 
> "belongs to rendering thread" suggests to me that the object belongs to the
> rendering thread and not necessarily that we're checking if the current thread
> is the rendering thread.

Agree, will change.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:73: mutable base::WaitableEvent id_event_; 
// Protects |id_|.
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> how does the event protect the thread id variable?  Maybe further
clarification
> is in order.

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

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:167: id_event_.Wait();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> 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 :)

So I shall.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:187: callback_->InitializeOnAudioThread();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> 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.

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

[Henrik Grunell, 2016-06-14 11:29:48]

Patchset #5 (id:80001) has been deleted

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

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/2038053002/diff/40001/base/threading/thread_r...
base/threading/thread_restrictions.h:220: friend class media::AudioDeviceThread;
On 2016/06/13 20:38:35, Henrik Grunell wrote:
> On 2016/06/13 18:22:22, jam wrote:
> > nit: order
> 
> I actually added it last because there isn't a clear order. I can order all
> friend statements above if you like? Otherwise, I'll put it after android::...

I put it after android::.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/media_stream_audio_processor.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/media_stream_audio_processor.cc:141:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/06/13 14:15:20, tommi-chrömium wrote:
> nit: The DCHECK() part here isn't needed.  Just call
> thread_checker_.CalledOnValidThread().

Well. There could be a context switch after detach, and if
thread_checker_.CalledOnValidThread() is called on another (wrong) thread...
unlikely but still. I'd prefer to have a guarantee.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:67: base::AutoLock
auto_lock(lock_);
On 2016/06/13 20:38:35, Henrik Grunell wrote:
> On 2016/06/13 14:15:20, tommi-chrömium wrote:
> > we grab this lock right after this block.  Can we just grab it once and have
> > this code + #ifdefs inside the below scope?
> 
> Yeah, that's better locking wise (in builds with DCHECKs enabled), this is
> slightly more readable imo. Let's minimize locking, I'll change that.

Done.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:69:
DCHECK(on_rendering_thread);
On 2016/06/13 14:15:20, tommi-chrömium wrote:
> doesn't look like you need this variable.  Just do
> DCHECK(renderer_->BelongsToRenderingThread());

Oops, left by mistake. Done.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_device_impl.cc:71: for
(PlayoutDataSinkList::const_iterator it = playout_sinks_.begin();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> range based loop?

Done.

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc_audio_renderer.h (right):

https://codereview.chromium.org/2038053002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc_audio_renderer.h:113: bool
BelongsToRenderingThread();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> CurrentThreadIsRenderingThread.
> 
> "belongs to rendering thread" suggests to me that the object belongs to the
> rendering thread and not necessarily that we're checking if the current thread
> is the rendering thread.

Done.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:72: base::PlatformThreadId id_;
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> thread_id_

Removed.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:73: mutable base::WaitableEvent id_event_; 
// Protects |id_|.
On 2016/06/13 20:38:35, Henrik Grunell wrote:
> On 2016/06/13 14:15:21, tommi-chrömium wrote:
> > how does the event protect the thread id variable?  Maybe further
> clarification
> > is in order.
> 
> I'll clarify that. (It's set once only, and when done the event is signaled.)

Removed.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:110: base::AutoLock auto_lock(thread_lock_);
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> 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?

This has been removed altogether.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:111: return thread_.get() ?
thread_->BelongsToRenderingThread() : false;
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> nit:
> 
> return thread_ && thread_->CurrentThreadIsRenderingThread();

Removed.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:167: id_event_.Wait();
On 2016/06/13 20:38:35, Henrik Grunell wrote:
> On 2016/06/13 14:15:21, tommi-chrömium wrote:
> > 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 :)
> 
> So I shall.

This has been removed.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.cc:187: callback_->InitializeOnAudioThread();
On 2016/06/13 20:38:35, Henrik Grunell wrote:
> On 2016/06/13 14:15:21, tommi-chrömium wrote:
> > 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.
> 
> Well, that could perhaps work. And is perhaps better. I'll take a look
tomorrow.

This should be fine, good suggestion. Done.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_devic...
media/audio/audio_device_thread.h:101: bool BelongsToRenderingThread();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> similarly, CurrentThreadIsDeviceThread()

Removed.

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/40001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:154: return
audio_thread_.BelongsToRenderingThread();
On 2016/06/13 14:15:21, tommi-chrömium wrote:
> here you could check the callback instead.

Done.

[DaleCurtis, 2016-06-14 17:07:51]

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:158: // Since this function is supposed to be
called on the rendering thread,
Hmm, seems like this should have a DCHECK(CurrentThreadIsAUdioDeviceThread()) :)
Note: calling this will actually bind the thread checker to the calling
thread... so failures will be confusing if the caller binds to a non-render
thread and then the render thread dcheck fails on the first render() call.

https://codereview.chromium.org/2038053002/diff/60001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/60001/media/base/audio_render...
media/base/audio_renderer_sink.h:73: // Returns true if called on rendering
thread, otherwise false.
Since you've implemented this with a thread checker it will only work in debug
mode.

[Henrik Grunell, 2016-06-14 19:33:41]

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:158: // Since this function is supposed to be
called on the rendering thread,
On 2016/06/14 17:07:49, DaleCurtis wrote:
> Hmm, seems like this should have a DCHECK(CurrentThreadIsAUdioDeviceThread())
:)
> Note: calling this will actually bind the thread checker to the calling
> thread... so failures will be confusing if the caller binds to a non-render
> thread and then the render thread dcheck fails on the first render() call.

Yes, a failure could be confusing if that happens. You mean a DCHECK would avoid
that? How? Or why have a DCHECK?

https://codereview.chromium.org/2038053002/diff/60001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/60001/media/base/audio_render...
media/base/audio_renderer_sink.h:73: // Returns true if called on rendering
thread, otherwise false.
On 2016/06/14 17:07:49, DaleCurtis wrote:
> Since you've implemented this with a thread checker it will only work in debug
> mode.

Updated comment.

[DaleCurtis, 2016-06-14 20:07:16]

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:158: // Since this function is supposed to be
called on the rendering thread,
On 2016/06/14 at 19:33:41, Henrik Grunell wrote:
> On 2016/06/14 17:07:49, DaleCurtis wrote:
> > Hmm, seems like this should have a
DCHECK(CurrentThreadIsAUdioDeviceThread()) :)
> > Note: calling this will actually bind the thread checker to the calling
> > thread... so failures will be confusing if the caller binds to a non-render
> > thread and then the render thread dcheck fails on the first render() call.
> 
> Yes, a failure could be confusing if that happens. You mean a DCHECK would
avoid that? How? Or why have a DCHECK?

I'd just add some text to the DCHECK() on the audio thread that says this will
only fail if the thread checker was attached on the wrong thread.

[Henrik Grunell, 2016-06-15 07:17:53]

grunell@chromium.org changed reviewers:
- jam@chromium.org

[Henrik Grunell, 2016-06-15 07:17:54]

Any more comments now or are we good to go?

Also removed jam@ as reviewer since thread_restrictions.h is no longer changed.

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/2038053002/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:158: // Since this function is supposed to be
called on the rendering thread,
On 2016/06/14 20:07:16, DaleCurtis wrote:
> On 2016/06/14 at 19:33:41, Henrik Grunell wrote:
> > On 2016/06/14 17:07:49, DaleCurtis wrote:
> > > Hmm, seems like this should have a
> DCHECK(CurrentThreadIsAUdioDeviceThread()) :)
> > > Note: calling this will actually bind the thread checker to the calling
> > > thread... so failures will be confusing if the caller binds to a
non-render
> > > thread and then the render thread dcheck fails on the first render() call.
> > 
> > Yes, a failure could be confusing if that happens. You mean a DCHECK would
> avoid that? How? Or why have a DCHECK?
> 
> I'd just add some text to the DCHECK() on the audio thread that says this will
> only fail if the thread checker was attached on the wrong thread.

Done.

[Henrik Grunell, 2016-06-15 07:19:07]

The CQ bit was checked by grunell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 07:19:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038053002/120001

[commit-bot: I haz the power, 2016-06-15 07:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 07:33:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Henrik Grunell, 2016-06-15 11:29:41]

The CQ bit was checked by grunell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 11:29:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038053002/140001

[commit-bot: I haz the power, 2016-06-15 13:38:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-15 13:38:09]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2016-06-15 16:45:11]

dalecurtis@chromium.org changed reviewers:
+ jam@chromium.org

[DaleCurtis, 2016-06-15 16:45:12]

lgtm

[tommi (sloooow) - chröme, 2016-06-15 19:29:11]

lgtm.  One question below but I don't feel strongly about it.

https://codereview.chromium.org/2038053002/diff/140001/content/renderer/media...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/2038053002/diff/140001/content/renderer/media...
content/renderer/media/webrtc_audio_device_impl.cc:72:
sink->OnRenderThreadChanged();
should this method only exist if DCHECK_IS_ON()?

https://codereview.chromium.org/2038053002/diff/140001/media/base/audio_rende...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_sink.h:75: // is intended to be used for DCHECKing.
should we make this method only available in builds where DCHECK is enabled?

[Henrik Grunell, 2016-06-16 12:19:16]

https://codereview.chromium.org/2038053002/diff/140001/content/renderer/media...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/2038053002/diff/140001/content/renderer/media...
content/renderer/media/webrtc_audio_device_impl.cc:72:
sink->OnRenderThreadChanged();
On 2016/06/15 19:29:11, tommi-chrömium wrote:
> should this method only exist if DCHECK_IS_ON()?

I don't have strong opinions on this either. I slightly prefer not to clutter
too much with ifdefs.

https://codereview.chromium.org/2038053002/diff/140001/media/base/audio_rende...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/2038053002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_sink.h:75: // is intended to be used for DCHECKing.
On 2016/06/15 19:29:11, tommi-chrömium wrote:
> should we make this method only available in builds where DCHECK is enabled?

Same here, I slightly prefer not to clutter with ifdefs. No strong opinion.

[Henrik Grunell, 2016-06-16 12:20:04]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2016-06-16 12:20:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2038053002/#ps140001
(title: "Build fix. Rebase.")

[commit-bot: I haz the power, 2016-06-16 12:20:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038053002/140001

[commit-bot: I haz the power, 2016-06-16 17:10:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 17:10:35]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Henrik Grunell, 2016-06-16 20:07:10]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2016-06-16 20:07:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038053002/140001

[commit-bot: I haz the power, 2016-06-17 00:05:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 00:05:04]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Henrik Grunell, 2016-06-17 11:22:59]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2016-06-17 11:23:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038053002/140001

[commit-bot: I haz the power, 2016-06-17 15:10:44]

Description was changed from

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

This is required for making AudioOutputDevice restartable. We'll then spin up a
new thread each Start(), and we need to deal with thread checks differently in
the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also
MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render
thread has changed.

BUG=607916
==========

to

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

This is required for making AudioOutputDevice restartable. We'll then spin up a
new thread each Start(), and we need to deal with thread checks differently in
the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also
MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render
thread has changed.

BUG=607916
==========

[commit-bot: I haz the power, 2016-06-17 15:10:46]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-17 15:12:24]

Description was changed from

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

This is required for making AudioOutputDevice restartable. We'll then spin up a
new thread each Start(), and we need to deal with thread checks differently in
the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also
MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render
thread has changed.

BUG=607916
==========

to

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

This is required for making AudioOutputDevice restartable. We'll then spin up a
new thread each Start(), and we need to deal with thread checks differently in
the WebRTC audio rendering path.

This change introduces AudioRendererSink::BelongsToRendererThread(). Also
MediaStreamAudioProcessor::OnRenderThreadChanged() to notify that the render
thread has changed.

BUG=607916

Committed: https://crrev.com/2c754510b660c9c13e754e8e01599977e5c41aad
Cr-Commit-Position: refs/heads/master@{#400429}
==========

[commit-bot: I haz the power, 2016-06-17 15:12:26]

Patchset 7 (id:??) landed as
https://crrev.com/2c754510b660c9c13e754e8e01599977e5c41aad
Cr-Commit-Position: refs/heads/master@{#400429}