Use the browser UI thread for audio on OSX.

Use the browser UI thread for audio on OSX.

After months of attempted workarounds, there appears no avenue
left except to move CoreAudio calls onto the main thread.  Sadly
that turns out to be the UI thread.

This CL swaps the message loop normally used by AudioManagerBase
with a pointer to the message loop on which the audio manager
is created (the UI loop in Chrome).

The audio thread used by Chrome is spun up on demand if a class
asks for it.  Currently only Tab Audio capture will use this
thread.

BUG=158170
TEST=No more crashes.  miu manually tested tab capture.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204130

[DaleCurtis, 2013-05-31 01:32:14]

+miu for WebContents changes.  I don't think that particular code section is
covered by any tests, so ideas or manual test direction welcome :)

[jam, 2013-05-31 07:18:03]

lgtm

[scherkus (not reviewing), 2013-05-31 19:30:34]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
when would this be NULL?

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
does it make sense to lazily start this thread?

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:62: worker_loop_ = message_loop_ =
audio_thread_->message_loop_proxy();
nit: can you split the assignments?

multiple assignments are pretty subtle

alternatively ... do we need worker_loop_? couldn't we inspect audio_thread_ as
to whether it's been started and just return audio_thread_->m_l_p()?

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:297: return audio_thread->Stop();
split return to next line -- it's too subtle and makes it appear like
Thread::Stop() returns a value (it doesn't)

https://codereview.chromium.org/14273018/diff/20001/media/base/media_switches.h
File media/base/media_switches.h (right):

https://codereview.chromium.org/14273018/diff/20001/media/base/media_switches...
media/base/media_switches.h:15: MEDIA_EXPORT extern const char
kAudioBufferSize[];
what's up with the big diff? alphabetizing?

[DaleCurtis, 2013-06-03 19:51:10]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/05/31 19:30:35, scherkus wrote:
> when would this be NULL?

Many unittests create an AudioManager without owning a message loop.

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
On 2013/05/31 19:30:35, scherkus wrote:
> does it make sense to lazily start this thread?

It is lazily started? See if (!worker_loop_) on line 86.

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:62: worker_loop_ = message_loop_ =
audio_thread_->message_loop_proxy();
On 2013/05/31 19:30:35, scherkus wrote:
> nit: can you split the assignments?
> 
> multiple assignments are pretty subtle
> 
> alternatively ... do we need worker_loop_? couldn't we inspect audio_thread_
as
> to whether it's been started and just return audio_thread_->m_l_p()?

Done.  It shouldn't be possible to call GetWorkerLoop() when audio_thread is
NULL.

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:297: return audio_thread->Stop();
On 2013/05/31 19:30:35, scherkus wrote:
> split return to next line -- it's too subtle and makes it appear like
> Thread::Stop() returns a value (it doesn't)

Done.

https://codereview.chromium.org/14273018/diff/20001/media/base/media_switches.h
File media/base/media_switches.h (right):

https://codereview.chromium.org/14273018/diff/20001/media/base/media_switches...
media/base/media_switches.h:15: MEDIA_EXPORT extern const char
kAudioBufferSize[];
On 2013/05/31 19:30:35, scherkus wrote:
> what's up with the big diff? alphabetizing?

Yeah, this has bugged me for a long time :)

[miu, 2013-06-03 21:48:41]

lgtm

Thanks for taking care of making sure tab audio capture had its own thread.

[scherkus (not reviewing), 2013-06-03 22:04:28]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/03 19:51:10, DaleCurtis wrote:
> On 2013/05/31 19:30:35, scherkus wrote:
> > when would this be NULL?
> 
> Many unittests create an AudioManager without owning a message loop.

Hmm... it's unfortunate that production code is altered for tests. How big of a
change would it be to fix the tests instead of having this check?

Also ... does this mean the new production code path isn't being covered w/
tests?

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
On 2013/06/03 19:51:10, DaleCurtis wrote:
> On 2013/05/31 19:30:35, scherkus wrote:
> > does it make sense to lazily start this thread?
> 
> It is lazily started? See if (!worker_loop_) on line 86.

I was referring to the Linux/Win/CrOS/disable-main-thread-audio case where this
thread gets launched immediately when an AudioManager is created.

You don't have to change this in this CL -- mostly curious.

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
media/audio/audio_manager_base.cc:332: if (!audio_thread)
OOC does Shutdown() get called multiple times, hence this guard?

https://codereview.chromium.org/14273018/diff/29016/media/audio/mock_audio_ma...
File media/audio/mock_audio_manager.cc (right):

https://codereview.chromium.org/14273018/diff/29016/media/audio/mock_audio_ma...
media/audio/mock_audio_manager.cc:36: media::AudioDeviceNames* device_names) {
fix indent

https://codereview.chromium.org/14273018/diff/29016/media/audio/mock_audio_ma...
media/audio/mock_audio_manager.cc:40: const media::AudioParameters& params,
fix indent

[DaleCurtis, 2013-06-03 22:26:35]

No changes, just comments.

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/03 22:04:28, scherkus wrote:
> On 2013/06/03 19:51:10, DaleCurtis wrote:
> > On 2013/05/31 19:30:35, scherkus wrote:
> > > when would this be NULL?
> > 
> > Many unittests create an AudioManager without owning a message loop.
> 
> Hmm... it's unfortunate that production code is altered for tests. How big of
a
> change would it be to fix the tests instead of having this check?
> 
> Also ... does this mean the new production code path isn't being covered w/
> tests?

"Fixing" means giving those tests a MessageLoop and possibly a thread to resolve
a hidden implicit dependency.  Not good.  Ideally everything should just work or
the dependency should be explicit.

Making it explicit means having BrowserMainLoop check the
--disable-main-thread-audio flag and choose to inject its UI message loop or not
and allows all existing tests to continue to function. The downside is we'll
only have test coverage via browser tests then; the upside is the dependency is
a lot clearer.

WDYT?

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
On 2013/06/03 22:04:28, scherkus wrote:
> On 2013/06/03 19:51:10, DaleCurtis wrote:
> > On 2013/05/31 19:30:35, scherkus wrote:
> > > does it make sense to lazily start this thread?
> > 
> > It is lazily started? See if (!worker_loop_) on line 86.
> 
> I was referring to the Linux/Win/CrOS/disable-main-thread-audio case where
this
> thread gets launched immediately when an AudioManager is created.
> 
> You don't have to change this in this CL -- mostly curious.

Probably? I don't see any reason why not. We would need to be careful with the
GetMessageLoop() semantics and avoid using message_loop_ directly.

[DaleCurtis, 2013-06-03 22:59:28]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/03 22:26:35, DaleCurtis wrote:
> On 2013/06/03 22:04:28, scherkus wrote:
> > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > when would this be NULL?
> > > 
> > > Many unittests create an AudioManager without owning a message loop.
> > 
> > Hmm... it's unfortunate that production code is altered for tests. How big
of
> a
> > change would it be to fix the tests instead of having this check?
> > 
> > Also ... does this mean the new production code path isn't being covered w/
> > tests?
> 
> "Fixing" means giving those tests a MessageLoop and possibly a thread to
resolve
> a hidden implicit dependency.  Not good.  Ideally everything should just work
or
> the dependency should be explicit.
> 
> Making it explicit means having BrowserMainLoop check the
> --disable-main-thread-audio flag and choose to inject its UI message loop or
not
> and allows all existing tests to continue to function. The downside is we'll
> only have test coverage via browser tests then; the upside is the dependency
is
> a lot clearer.
> 
> WDYT?

Actually just changing BrowserMainLoop would means tests will be using the wrong
thread... Which could be a source of occasional flakiness.

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
media/audio/audio_manager_base.cc:332: if (!audio_thread)
On 2013/06/03 22:04:28, scherkus wrote:
> OOC does Shutdown() get called multiple times, hence this guard?

It shouldn't be. Likely this is dead code. I'll remove it and see if we get null
ptrs.

[scherkus (not reviewing), 2013-06-03 23:22:17]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/03 22:26:35, DaleCurtis wrote:
> On 2013/06/03 22:04:28, scherkus wrote:
> > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > when would this be NULL?
> > > 
> > > Many unittests create an AudioManager without owning a message loop.
> > 
> > Hmm... it's unfortunate that production code is altered for tests. How big
of
> a
> > change would it be to fix the tests instead of having this check?
> > 
> > Also ... does this mean the new production code path isn't being covered w/
> > tests?
> 
> "Fixing" means giving those tests a MessageLoop and possibly a thread to
resolve
> a hidden implicit dependency.  Not good.  Ideally everything should just work
or
> the dependency should be explicit.
> 
> Making it explicit means having BrowserMainLoop check the
> --disable-main-thread-audio flag and choose to inject its UI message loop or
not
> and allows all existing tests to continue to function. The downside is we'll
> only have test coverage via browser tests then; the upside is the dependency
is
> a lot clearer.
> 
> WDYT?

I agree that implicit dependencies kinda suck, hence the reason why our code is
pretty explicit about passing in message loops.

While I would say IWBN to change AudioManager::Create() to let clients pass in
an option thread-to-run-on, this case is a bit trickier because it's only used
on one platform and is still controlled via runtime flag. I'd hate to make a big
change if we end up reverting / changing bits of this CL.

I'm fine leaving as is. As you note we should be getting test coverage via
content_browsertests

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
On 2013/06/03 22:26:35, DaleCurtis wrote:
> On 2013/06/03 22:04:28, scherkus wrote:
> > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > does it make sense to lazily start this thread?
> > > 
> > > It is lazily started? See if (!worker_loop_) on line 86.
> > 
> > I was referring to the Linux/Win/CrOS/disable-main-thread-audio case where
> this
> > thread gets launched immediately when an AudioManager is created.
> > 
> > You don't have to change this in this CL -- mostly curious.
> 
> Probably? I don't see any reason why not. We would need to be careful with the
> GetMessageLoop() semantics and avoid using message_loop_ directly.

Yeah I'm wonder if there would be any races as AFAIK AM is accessed on the IO
thread.

Reason why I'm wondering is that this code is in the critical path for browser
launch. Deferring a thread launch if it's easy to do so would be nice.

[scherkus (not reviewing), 2013-06-03 23:22:38]

oh, and lgtm

[scherkus (not reviewing), 2013-06-03 23:59:04]

On 2013/06/03 23:22:38, scherkus wrote:
> oh, and lgtm

food for thought: this is a very timely G+ post :)
http://goto.google.com/jawze

[DaleCurtis, 2013-06-04 00:15:17]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/03 23:22:18, scherkus wrote:
> On 2013/06/03 22:26:35, DaleCurtis wrote:
> > On 2013/06/03 22:04:28, scherkus wrote:
> > > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > > when would this be NULL?
> > > > 
> > > > Many unittests create an AudioManager without owning a message loop.
> > > 
> > > Hmm... it's unfortunate that production code is altered for tests. How big
> of
> > a
> > > change would it be to fix the tests instead of having this check?
> > > 
> > > Also ... does this mean the new production code path isn't being covered
w/
> > > tests?
> > 
> > "Fixing" means giving those tests a MessageLoop and possibly a thread to
> resolve
> > a hidden implicit dependency.  Not good.  Ideally everything should just
work
> or
> > the dependency should be explicit.
> > 
> > Making it explicit means having BrowserMainLoop check the
> > --disable-main-thread-audio flag and choose to inject its UI message loop or
> not
> > and allows all existing tests to continue to function. The downside is we'll
> > only have test coverage via browser tests then; the upside is the dependency
> is
> > a lot clearer.
> > 
> > WDYT?
> 
> I agree that implicit dependencies kinda suck, hence the reason why our code
is
> pretty explicit about passing in message loops.
> 
> While I would say IWBN to change AudioManager::Create() to let clients pass in
> an option thread-to-run-on, this case is a bit trickier because it's only used
> on one platform and is still controlled via runtime flag. I'd hate to make a
big
> change if we end up reverting / changing bits of this CL.
> 
> I'm fine leaving as is. As you note we should be getting test coverage via
> content_browsertests

Lets leave it as is until it's sticky then I'll submit a follow up CL with an
explicit dependency.

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:61: CHECK(audio_thread_->Start());
On 2013/06/03 23:22:18, scherkus wrote:
> On 2013/06/03 22:26:35, DaleCurtis wrote:
> > On 2013/06/03 22:04:28, scherkus wrote:
> > > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > > does it make sense to lazily start this thread?
> > > > 
> > > > It is lazily started? See if (!worker_loop_) on line 86.
> > > 
> > > I was referring to the Linux/Win/CrOS/disable-main-thread-audio case where
> > this
> > > thread gets launched immediately when an AudioManager is created.
> > > 
> > > You don't have to change this in this CL -- mostly curious.
> > 
> > Probably? I don't see any reason why not. We would need to be careful with
the
> > GetMessageLoop() semantics and avoid using message_loop_ directly.
> 
> Yeah I'm wonder if there would be any races as AFAIK AM is accessed on the IO
> thread.
> 
> Reason why I'm wondering is that this code is in the critical path for browser
> launch. Deferring a thread launch if it's easy to do so would be nice.

Shouldn't be too hard, we'd need to make generic InitializeOnAudioThread()
methods though that could be called lazily instead of automatically in each
platform specific constructor.  Listed in the AudioManager cleanup bug.

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/29016/media/audio/audio_manager...
media/audio/audio_manager_base.cc:332: if (!audio_thread)
On 2013/06/03 22:59:28, DaleCurtis wrote:
> On 2013/06/03 22:04:28, scherkus wrote:
> > OOC does Shutdown() get called multiple times, hence this guard?
> 
> It shouldn't be. Likely this is dead code. I'll remove it and see if we get
null
> ptrs.

Actually going to save this for a follow on cleanup CL. http://crbug.com/246424
filed.

[scherkus (not reviewing), 2013-06-04 00:21:30]

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/20001/media/audio/audio_manager...
media/audio/audio_manager_base.cc:55: base::MessageLoopProxy::current()) {
On 2013/06/04 00:15:17, DaleCurtis wrote:
> On 2013/06/03 23:22:18, scherkus wrote:
> > On 2013/06/03 22:26:35, DaleCurtis wrote:
> > > On 2013/06/03 22:04:28, scherkus wrote:
> > > > On 2013/06/03 19:51:10, DaleCurtis wrote:
> > > > > On 2013/05/31 19:30:35, scherkus wrote:
> > > > > > when would this be NULL?
> > > > > 
> > > > > Many unittests create an AudioManager without owning a message loop.
> > > > 
> > > > Hmm... it's unfortunate that production code is altered for tests. How
big
> > of
> > > a
> > > > change would it be to fix the tests instead of having this check?
> > > > 
> > > > Also ... does this mean the new production code path isn't being covered
> w/
> > > > tests?
> > > 
> > > "Fixing" means giving those tests a MessageLoop and possibly a thread to
> > resolve
> > > a hidden implicit dependency.  Not good.  Ideally everything should just
> work
> > or
> > > the dependency should be explicit.
> > > 
> > > Making it explicit means having BrowserMainLoop check the
> > > --disable-main-thread-audio flag and choose to inject its UI message loop
or
> > not
> > > and allows all existing tests to continue to function. The downside is
we'll
> > > only have test coverage via browser tests then; the upside is the
dependency
> > is
> > > a lot clearer.
> > > 
> > > WDYT?
> > 
> > I agree that implicit dependencies kinda suck, hence the reason why our code
> is
> > pretty explicit about passing in message loops.
> > 
> > While I would say IWBN to change AudioManager::Create() to let clients pass
in
> > an option thread-to-run-on, this case is a bit trickier because it's only
used
> > on one platform and is still controlled via runtime flag. I'd hate to make a
> big
> > change if we end up reverting / changing bits of this CL.
> > 
> > I'm fine leaving as is. As you note we should be getting test coverage via
> > content_browsertests
> 
> Lets leave it as is until it's sticky then I'll submit a follow up CL with an
> explicit dependency.

SGTM

[DaleCurtis, 2013-06-04 00:41:19]

crogers: Any comments?

[Chris Rogers, 2013-06-04 19:16:51]

Thanks Dale, lgtm -- you can choose to ignore or incorporate the suggestion
below

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
media/audio/audio_manager_base.cc:89: #elif defined(OS_MACOSX)
It'd be nice to have the ability to run on the main thread be a run-time feature
instead of compile-time.  We could have a method UseMainThread() (or something
like that, which would compile-time conditionally return |true| for OS_MACOSX. 
Then down below of line 339 we could avoid another #if defined(OS_MACOSX) and
instead call UseMainThread()

That way, in the future, if there are additional places in the code that need to
be treated specially, we can avoid a proliferation of #if defined(OS_MACOSX)
each time it's necessary to check

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
media/audio/audio_manager_base.cc:344: // |worker_loop_|.
I see no member variable called |worker_loop_| - can you please update the
comment

[DaleCurtis, 2013-06-04 20:03:14]

Thanks for review everyone!

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
media/audio/audio_manager_base.cc:89: #elif defined(OS_MACOSX)
On 2013/06/04 19:16:52, Chris Rogers wrote:
> It'd be nice to have the ability to run on the main thread be a run-time
feature
> instead of compile-time.  We could have a method UseMainThread() (or something
> like that, which would compile-time conditionally return |true| for OS_MACOSX.

> Then down below of line 339 we could avoid another #if defined(OS_MACOSX) and
> instead call UseMainThread()
> 
> That way, in the future, if there are additional places in the code that need
to
> be treated specially, we can avoid a proliferation of #if defined(OS_MACOSX)
> each time it's necessary to check

I've changed the code so the only OS_MACOSX necessary is here.  Long term the
flag will go away if this sticks since it doesn't make sense to offer a runtime
selection for bad behavior :)

https://codereview.chromium.org/14273018/diff/35007/media/audio/audio_manager...
media/audio/audio_manager_base.cc:344: // |worker_loop_|.
On 2013/06/04 19:16:52, Chris Rogers wrote:
> I see no member variable called |worker_loop_| - can you please update the
> comment

Comment removed. It's not really helpful given the comment above it.

[commit-bot: I haz the power, 2013-06-04 20:03:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/14273018/49016

[commit-bot: I haz the power, 2013-06-05 02:36:55]

Change committed as 204130