Pass task runners to AudioManager constructor.

Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
Committed: https://crrev.com/e05981bd06b4694b426990acd41be8ab6d9a1659
Cr-Commit-Position: refs/heads/master@{#388048}

[alokp, 2016-03-17 23:09:00]

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

[alokp, 2016-03-17 23:09:00]

Dale: This is not ready to commit.

I have updated the AudioManager interface to pass the task runners in
constructor and made GetTask[Worker]Runner non-virtual. I will rename them in a
follow-up patch.

I have added a AudioManager::Destroy method to destroy the AM instance on audio
thread.

I have created a utility AudioThread class that various platforms can use to
create a thread outside of AudioManagerBase.

With the current patch, I can compile and successfully run media_unittests on
linux. If you like the direction of the patch, I will make it ready to commit.

[DaleCurtis, 2016-03-18 00:19:10]

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.h:53: static AudioManager* Create(AudioLogFactory*
audio_log_factory);
Hmm, I thought you were changing this to Create(..., |task_runner|)? Then you
can just say that AudioManager must be deleted on |task_runner|.

[alokp, 2016-03-18 00:35:19]

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.h:53: static AudioManager* Create(AudioLogFactory*
audio_log_factory);
On 2016/03/18 00:19:10, DaleCurtis wrote:
> Hmm, I thought you were changing this to Create(..., |task_runner|)? Then you
> can just say that AudioManager must be deleted on |task_runner|.

I wanted to keep task_runner an internal detail of AudioManager implementation.
As a client of AudioManager I do not care which task_runner is used. I should be
able to just call AudioManager::Create and rely on the platform implementation
to choose an appropriate thread.

Otherwise how do you propose BrowserMainLoop get a task_runner to pass into
AudioManager::Create?

We could add a static AudioManager::GetRecommendedTaskRunner to be implemented
by various platforms. Is this what you had in mind?

[DaleCurtis, 2016-03-18 00:40:41]

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.h:53: static AudioManager* Create(AudioLogFactory*
audio_log_factory);
On 2016/03/18 at 00:35:19, alokp wrote:
> On 2016/03/18 00:19:10, DaleCurtis wrote:
> > Hmm, I thought you were changing this to Create(..., |task_runner|)? Then
you
> > can just say that AudioManager must be deleted on |task_runner|.
> 
> I wanted to keep task_runner an internal detail of AudioManager
implementation. As a client of AudioManager I do not care which task_runner is
used. I should be able to just call AudioManager::Create and rely on the
platform implementation to choose an appropriate thread.
> 
> Otherwise how do you propose BrowserMainLoop get a task_runner to pass into
AudioManager::Create?
> 
> We could add a static AudioManager::GetRecommendedTaskRunner to be implemented
by various platforms. Is this what you had in mind?

No, I was expecting that you would require the task runner to be passed in
always or at the very least that you'd have one method which took no task runner
and used a default and a second which allowed one to be passed in.
Alternatively, BML would have it's own base::Thread it starts and passes the
task runner in for on non-OSX, non-Cast platforms.

This would help both your use case and our test situation immensely since we
have a ton of tests which have to hop to the platform default thread instead of
just using a test task runner.

[alokp, 2016-03-18 06:46:52]

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/60001/media/audio/audio_manag...
media/audio/audio_manager.h:53: static AudioManager* Create(AudioLogFactory*
audio_log_factory);
On 2016/03/18 00:40:41, DaleCurtis wrote:
> On 2016/03/18 at 00:35:19, alokp wrote:
> > On 2016/03/18 00:19:10, DaleCurtis wrote:
> > > Hmm, I thought you were changing this to Create(..., |task_runner|)? Then
> you
> > > can just say that AudioManager must be deleted on |task_runner|.
> > 
> > I wanted to keep task_runner an internal detail of AudioManager
> implementation. As a client of AudioManager I do not care which task_runner is
> used. I should be able to just call AudioManager::Create and rely on the
> platform implementation to choose an appropriate thread.
> > 
> > Otherwise how do you propose BrowserMainLoop get a task_runner to pass into
> AudioManager::Create?
> > 
> > We could add a static AudioManager::GetRecommendedTaskRunner to be
implemented
> by various platforms. Is this what you had in mind?
> 
> No, I was expecting that you would require the task runner to be passed in
> always or at the very least that you'd have one method which took no task
runner
> and used a default and a second which allowed one to be passed in.
> Alternatively, BML would have it's own base::Thread it starts and passes the
> task runner in for on non-OSX, non-Cast platforms.
> 
> This would help both your use case and our test situation immensely since we
> have a ton of tests which have to hop to the platform default thread instead
of
> just using a test task runner.
> 

I was under the impression that those tests were intentionally written that way
to test AM in multi-threaded environment :D

Passing the task runners into Create SGTM. PTAL at the latest patch that
combines the various Create functions into one and also passes the task runners.
The task runners are optional to allow the platform to choose a default one if
not specified.

It should be now trivial to update those tests to use a test task runner. I can
do that in follow-up patch.

[alokp, 2016-03-18 16:27:26]

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:55: // The manager will use |task_runner| for audio
IO. If the given |task_runner|
We could make the task_runner arguments mandatory by creating the audio thread
in BrowserMainLoop. But then Cast needs a way to override it. One way to do that
would be to add ContentBrowserClient::GetAudioTaskRunner. The default
implementation will return NULL, in which case BML will create the audio thread.

ContentBrowserClient::GetAudioTaskRunner will work better for us and we will
also avoid the singleton AudioThread class but I am not sure if this function
belongs in ContentBrowserClient. We do have a
ContentBrowserClient::CreateCdmFactory, so maybe?

[DaleCurtis, 2016-03-18 17:48:17]

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

[DaleCurtis, 2016-03-18 17:48:19]

Looking good! I'd like a second set of eyes on this one so +tommi to review or
forward.

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:1049: // Destroy audio_manager_ on audio
thread.
Be careful here, this changes the destruction order of AudioManager relative to
it's current position.

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:1262:
scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner;
What will this code look like for cast?

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_input...
File media/audio/audio_input_controller_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_input...
media/audio/audio_input_controller_unittest.cc:72: :
audio_manager_(AudioManager::Create(nullptr,
I think it might be worth keeping the CreateForTesting(task_runner) method which
handles creation + fake log factory. It's a bit ugly to have to specify this
everywhere.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (left):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:62: static AudioManager* CreateForTesting();
As mentioned elsewhere this might be worth keeping, but perhaps with the
addition of a task_runner.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:63: // If |monitor_task_runner| is not NULL, a
monitor will be scheduled on
This should just say that a UMA will be logged now. We don't crash anymore since
it's a third party driver / OS bug that isn't being addressed readily by
Microsoft :( Really you can just say that |monitor_task_runner| will be used to
monitor |task_runner|.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:66: static AudioManager* Create(
Could we just have this return a scoped_ptr w/ custom deleter that calls
Destroy? I'm fine with doing that in a follow up patch if it proves to be churn
in this CL.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:67: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
Per recent chromium-dev discussion, now that we have movable types we should be
passing scoped_refptr by value.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_thread.h
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_threa...
media/audio/audio_thread.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
What is this class getting us? It seems to be pretty much just a typedef.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/linux/audio...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/linux/audio...
media/audio/linux/audio_manager_linux.cc:31: AudioManager* CreateAudioManager(
It'd be nice if the default Create() function handled sorting this out instead
of copy/pasting the same logic for all classes.

[alokp, 2016-03-18 18:40:07]

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:1049: // Destroy audio_manager_ on audio
thread.
On 2016/03/18 17:48:18, DaleCurtis wrote:
> Be careful here, this changes the destruction order of AudioManager relative
to
> it's current position.

Yeah I need to cycle this through the trybots.

https://codereview.chromium.org/1806313003/diff/80001/content/browser/browser...
content/browser/browser_main_loop.cc:1262:
scoped_refptr<base::SingleThreadTaskRunner> audio_task_runner;
On 2016/03/18 17:48:18, DaleCurtis wrote:
> What will this code look like for cast? 

Since BML is common for all platforms/content-embedders, we cannot add any
cast-specific override here. I can think of two options:
1. [Not Good] Rely on BML passing NULL task_runner, which can be overridden in
CastAudioManagerFactory.

2. [Good] Add ContentBrowserClient::GetAudio[Worker]TaskRunner functions that we
can override. This approach is much cleaner. I think you missed one my comments
in AudioManager about making task runners non-optional. Copying it here:

We could make the task_runner arguments mandatory by creating the audio thread
in BrowserMainLoop. But then Cast needs a way to override it. One way to do that
would be to add ContentBrowserClient::GetAudioTaskRunner. The default
implementation will return NULL, in which case BML will create the audio thread.

ContentBrowserClient::GetAudioTaskRunner will work better for us and we will
also avoid the singleton AudioThread class but I am not sure if this function
belongs in ContentBrowserClient. We do have a
ContentBrowserClient::CreateCdmFactory, so maybe?

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_input...
File media/audio/audio_input_controller_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_input...
media/audio/audio_input_controller_unittest.cc:72: :
audio_manager_(AudioManager::Create(nullptr,
On 2016/03/18 17:48:18, DaleCurtis wrote:
> I think it might be worth keeping the CreateForTesting(task_runner) method
which
> handles creation + fake log factory. It's a bit ugly to have to specify this
> everywhere.

OK. Should it take only one task_runner?

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (left):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:62: static AudioManager* CreateForTesting();
On 2016/03/18 17:48:18, DaleCurtis wrote:
> As mentioned elsewhere this might be worth keeping, but perhaps with the
> addition of a task_runner.

Just one task_runner should be sufficient - no worker_task_runner?

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:63: // If |monitor_task_runner| is not NULL, a
monitor will be scheduled on
On 2016/03/18 17:48:18, DaleCurtis wrote:
> This should just say that a UMA will be logged now. We don't crash anymore
since
> it's a third party driver / OS bug that isn't being addressed readily by
> Microsoft :( Really you can just say that |monitor_task_runner| will be used
to
> monitor |task_runner|.

Done.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:66: static AudioManager* Create(
On 2016/03/18 17:48:18, DaleCurtis wrote:
> Could we just have this return a scoped_ptr w/ custom deleter that calls
> Destroy? I'm fine with doing that in a follow up patch if it proves to be
churn
> in this CL. 

Acknowledged.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:67: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
On 2016/03/18 17:48:18, DaleCurtis wrote:
> Per recent chromium-dev discussion, now that we have movable types we should
be
> passing scoped_refptr by value.

Done.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_thread.h
File media/audio/audio_thread.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_threa...
media/audio/audio_thread.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2016/03/18 17:48:19, DaleCurtis wrote:
> What is this class getting us? It seems to be pretty much just a typedef.

It lazily creates the thread. See AudioThread::Get and the lazyinstance in cc
file.

Do see my comment in audio_manager.h and browser_main_loop.cc about how we can
avoid this class by creating a local thread in BML and adding
ContentBrowserClient::GetAudioTaskRunner.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/linux/audio...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/linux/audio...
media/audio/linux/audio_manager_linux.cc:31: AudioManager* CreateAudioManager(
On 2016/03/18 17:48:19, DaleCurtis wrote:
> It'd be nice if the default Create() function handled sorting this out instead
> of copy/pasting the same logic for all classes.

I did not not do that because I wanted to use a different default task runner
for Cast. But we can avoid that by adding
ContentBrowserClient::GetAudioTaskRunner.

[DaleCurtis, 2016-03-18 20:11:49]

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (left):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:62: static AudioManager* CreateForTesting();
On 2016/03/18 at 18:40:07, alokp wrote:
> On 2016/03/18 17:48:18, DaleCurtis wrote:
> > As mentioned elsewhere this might be worth keeping, but perhaps with the
> > addition of a task_runner.
> 
> Just one task_runner should be sufficient - no worker_task_runner?

Correct.

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/80001/media/audio/audio_manag...
media/audio/audio_manager.h:55: // The manager will use |task_runner| for audio
IO. If the given |task_runner|
On 2016/03/18 at 16:27:25, alokp wrote:
> We could make the task_runner arguments mandatory by creating the audio thread
in BrowserMainLoop. But then Cast needs a way to override it. One way to do that
would be to add ContentBrowserClient::GetAudioTaskRunner. The default
implementation will return NULL, in which case BML will create the audio thread.
> 
> ContentBrowserClient::GetAudioTaskRunner will work better for us and we will
also avoid the singleton AudioThread class but I am not sure if this function
belongs in ContentBrowserClient. We do have a
ContentBrowserClient::CreateCdmFactory, so maybe?

Hmm, if you take the scoped_ptr w/ custom deleter approach I suggested below, we
actually don't need to have BrowserMainLoop pass in the threads anymore; I think
we still want that to happen under the hood. The deleter can just put it on the
right thread naturally.

So something like:

public ScopedAudioManager Create(factory, monitor_task_runner);
public ScopedAudioManager CreateForTesting(task_runner);

protected static CreateDefaultAudioTaskRunner();
private static CreateAudioManager(factory, task_runner);
private AudioManager(factory, task_runner),

Within most of the platform specific CreateAudioManager() methods, you would
just do something like:

return <Platform>AudioManager(factory, task_runner ? task_runner :
CreateDefaultAudioTaskRunner());

For Cast and OSX you would instead do something like:

return <Cast|OSX>AudioManager>(factory, task_runner ? task_runner :
<custom_task_runner>);

Internally the OSX and Cast ones can either use CreateDefaultAudioTaskRunner()
or spin up their own thread for the worker task runner. I don't think any other
platform needs to worry about specifying this.

During destruction ScopedAudioManager will do pretty much what your destroy
method does now, but without needing the explicit addition in BML.

I think this minimizes the changes in BML and elsewhere while getting all the
gains we're looking for.

[tommi (sloooow) - chröme, 2016-03-19 02:32:42]

Can you update the description with a little more information on why the change
is needed?
My comments are mostly around adding extra calls to AddRef/Release which I don't
think are needed. There might be some misunderstanding there (possibly by me).

https://codereview.chromium.org/1806313003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1274:
media::AudioManager::Create(audio_task_runner, worker_task_runner,
nit: since these are going out of scope and Create() accepts reptrs by value,
you should use std::move().  Alternatively, if ownership passing is not
intended, change the Create() signature to accept by const&.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/alsa/audio...
File media/audio/alsa/audio_manager_alsa.h (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/alsa/audio...
media/audio/alsa/audio_manager_alsa.h:22:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
sure you don't want const&?

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_inpu...
File media/audio/audio_input_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_inpu...
media/audio/audio_input_unittest.cc:54: public:
this should be indented by 1 space, but since it isn't the whole indent below is
now off too.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:78: scoped_refptr<base::SingleThreadTaskRunner>
monitor_task_runner) {
does the caller use std::move?

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:80: monitor_task_runner_ = monitor_task_runner;
if the above is intended, then you should use std::move here.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:264: scoped_refptr<base::SingleThreadTaskRunner>
task_runner =
won't we always be on the main thread when we get here?  If we're on a random
thread, then we might have a (theoratical) problem with changing g_last_created.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:324:
g_helper.Pointer()->StartHangTimer(monitor_task_runner);
std::move() (or see other comment)

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.h:55: // Construct the audio manager; only one
instance is allowed.
since it has come up a few times in the past and been the source of confusion,
can you call out explicitly which one of the below threads will be the UI thread
on Mac?

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.h:72: scoped_refptr<base::SingleThreadTaskRunner>
task_runner,
nit: passing by value implies that the caller might want to pass ownership. I'm
not sure that's true though since at least some of these task runners are held
elsewhere.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_thre...
File media/audio/audio_thread.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_thre...
media/audio/audio_thread.cc:16: thread_.init_com_with_mta(true);
just checking - is this how things were before?  using mta has been known to
cause crashes if there might be 3rd party COM components in the picture.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:70: bool AudioManagerPulse::Init() {
I'm assuming this is all moved code.

[DaleCurtis, 2016-03-19 17:15:38]

A recent chromium thread decided that scoped_refptr should be passed by value
now that we have movable types:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86...
-- IIRC the reasoning is that they will be automatically moved when possible.

[tommi (sloooow) - chröme, 2016-03-19 17:48:25]

On 2016/03/19 17:15:38, DaleCurtis wrote:
> A recent chromium thread decided that scoped_refptr should be passed by value
> now that we have movable types:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86...
> -- IIRC the reasoning is that they will be automatically moved when possible.

Ah, I'm aware of the discussion but I was under the impression that explicit
moves would have to be done at the call sites. I hadn't followed all of it. 
Great if that's not the case and my comments around that can be ignored.

[tommi (sloooow) - chröme, 2016-03-19 17:51:31]

On 2016/03/19 17:48:25, tommi-chromium wrote:
> On 2016/03/19 17:15:38, DaleCurtis wrote:
> > A recent chromium thread decided that scoped_refptr should be passed by
value
> > now that we have movable types:
> >
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86...
> > -- IIRC the reasoning is that they will be automatically moved when
possible.
> 
> Ah, I'm aware of the discussion but I was under the impression that explicit
> moves would have to be done at the call sites. I hadn't followed all of it. 
> Great if that's not the case and my comments around that can be ignored.

Looking a bit closer, I guess we still need to do that if the variable being
passed, isn't implicitly an rvalue.

[DaleCurtis, 2016-03-21 16:54:18]

On 2016/03/19 at 17:51:31, tommi wrote:
> On 2016/03/19 17:48:25, tommi-chromium wrote:
> > On 2016/03/19 17:15:38, DaleCurtis wrote:
> > > A recent chromium thread decided that scoped_refptr should be passed by
value
> > > now that we have movable types:
> > >
> >
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86...
> > > -- IIRC the reasoning is that they will be automatically moved when
possible.
> > 
> > Ah, I'm aware of the discussion but I was under the impression that explicit
> > moves would have to be done at the call sites. I hadn't followed all of it. 
> > Great if that's not the case and my comments around that can be ignored.
> 
> Looking a bit closer, I guess we still need to do that if the variable being
passed, isn't implicitly an rvalue.

Ah, that seems subtle and prone to mistakes. Might be worth leaving as const&
then until a rewriter+clang check comes along.

[alokp, 2016-03-22 21:38:36]

Description was changed from

==========
Pass task runners to AudioManager constructor.
==========

to

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.
==========

[alokp, 2016-03-22 21:47:07]

I think I have addressed all the comments. I still need to update non-linux
implementations. I will do that once we are happy with the AudioManager
interface.

I decided to add an audio thread to BML instead of a singleton thread in
previous patches. This is nicer because:

1. It allows us to consolidate the task-runner logic in one place. On mac for
example, we can explicitly specify the ui thread instead of the mac
implementation for checking whether it is on the UI thread or not. It also
allows us to make the decision about hang monitor at the same place instead of
keeping this logic spread between BML and AudioManagerMac.

2. It lets us make AudioThread non-singleton, which always nice IMO.

My current thinking about overriding the default AudioThread is to add
ContentBrowserClient::GetAudioTaskRunner. The default implementation will return
null. Please let me know if you see issues with this plan.

https://codereview.chromium.org/1806313003/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1274:
media::AudioManager::Create(audio_task_runner, worker_task_runner,
On 2016/03/19 02:32:41, tommi-chromium wrote:
> nit: since these are going out of scope and Create() accepts reptrs by value,
> you should use std::move().  Alternatively, if ownership passing is not
> intended, change the Create() signature to accept by const&.

I think we need to pass the ownership to match the signature of
AudioManager::GetTaskRunner(). I have a TODO to rename it to
AudioManager::task_runner(). At that time I will evaluate the various callsites
and if none of them assume ownership, I will convert it to const reference
everywhere. Although to be honest I really do not understand the rationale
behind passing a const reference. Shouldn't we pass raw pointers instead when
ownership is not being transferred?

I have used std::move here for now.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/alsa/audio...
File media/audio/alsa/audio_manager_alsa.h (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/alsa/audio...
media/audio/alsa/audio_manager_alsa.h:22:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
On 2016/03/19 02:32:41, tommi-chromium wrote:
> sure you don't want const&?

Please see my response in BrowserMainLoop.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_inpu...
File media/audio/audio_input_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_inpu...
media/audio/audio_input_unittest.cc:54: public:
On 2016/03/19 02:32:41, tommi-chromium wrote:
> this should be indented by 1 space, but since it isn't the whole indent below
is
> now off too.

Done.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:78: scoped_refptr<base::SingleThreadTaskRunner>
monitor_task_runner) {
On 2016/03/19 02:32:41, tommi-chromium wrote:
> does the caller use std::move?

Done.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:80: monitor_task_runner_ = monitor_task_runner;
On 2016/03/19 02:32:41, tommi-chromium wrote:
> if the above is intended, then you should use std::move here.

Done.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:264: scoped_refptr<base::SingleThreadTaskRunner>
task_runner =
On 2016/03/19 02:32:41, tommi-chromium wrote:
> won't we always be on the main thread when we get here?  If we're on a random
> thread, then we might have a (theoratical) problem with changing
g_last_created.

Are you suggesting always posting the task to delete the instance?

Yes we will always be on the main thread. This check avoids posting the delete
task when audio task runner belongs to the main thread (mac and unittests).

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.cc:324:
g_helper.Pointer()->StartHangTimer(monitor_task_runner);
On 2016/03/19 02:32:41, tommi-chromium wrote:
> std::move() (or see other comment)

Done.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.h:55: // Construct the audio manager; only one
instance is allowed.
On 2016/03/19 02:32:41, tommi-chromium wrote:
> since it has come up a few times in the past and been the source of confusion,
> can you call out explicitly which one of the below threads will be the UI
thread
> on Mac?

Done.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_mana...
media/audio/audio_manager.h:72: scoped_refptr<base::SingleThreadTaskRunner>
task_runner,
On 2016/03/19 02:32:41, tommi-chromium wrote:
> nit: passing by value implies that the caller might want to pass ownership.
I'm
> not sure that's true though since at least some of these task runners are held
> elsewhere.

I agree. I plan to look into it in a later patch. Please see my response in BML.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_thre...
File media/audio/audio_thread.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/audio_thre...
media/audio/audio_thread.cc:16: thread_.init_com_with_mta(true);
On 2016/03/19 02:32:41, tommi-chromium wrote:
> just checking - is this how things were before?  using mta has been known to
> cause crashes if there might be 3rd party COM components in the picture.

yes this code is moved from elsewhere. I can remove it although I am not too
familiar with issues here. FYI this piece of code has now moved to BML.

https://codereview.chromium.org/1806313003/diff/120001/media/audio/fake_audio...
File media/audio/fake_audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/fake_audio...
media/audio/fake_audio_manager.cc:20: : AudioManagerBase(task_runner,
worker_task_runner, audio_log_factory) {}
Do all these constructors require std::move too?

https://codereview.chromium.org/1806313003/diff/120001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/120001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:70: bool AudioManagerPulse::Init() {
On 2016/03/19 02:32:42, tommi-chromium wrote:
> I'm assuming this is all moved code.

YES.

[DaleCurtis, 2016-03-22 21:58:27]

Nice cleanup. It's a little unfortunate some logic moves into BML, but it does
simplify AM a lot.

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
content/browser/browser_main_loop.cc:1234: CHECK(audio_thread_->Start());
It's unfortunate that we'll always start this now, previously OSX didn't unless
the worker task runner was used. It's probably a minor thing though if it's
never used.

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
content/browser/browser_main_loop.cc:1235: media::AudioLogFactory*
audio_log_factory = MediaInternals::GetInstance();
Seems unnecessary to break this out. I'd just inline.

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
media/audio/audio_manager.cc:319: DCHECK(task_runner && worker_task_runner);
split dcheck so failures are easy to understand.

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
File media/audio/audio_manager_factory.h (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
media/audio/audio_manager_factory.h:24: virtual scoped_ptr<AudioManager,
AudioManagerDeleter> CreateInstance(
ScopedPtrAudioManager?

https://codereview.chromium.org/1806313003/diff/180001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:43: scoped_ptr<AudioManagerPulse,
AudioManagerDeleter> manager(
ScopedPtrAudioManager?

[alokp, 2016-03-22 22:23:39]

OK to update mac and win?

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
content/browser/browser_main_loop.cc:1234: CHECK(audio_thread_->Start());
On 2016/03/22 21:58:27, DaleCurtis wrote:
> It's unfortunate that we'll always start this now, previously OSX didn't
unless
> the worker task runner was used. It's probably a minor thing though if it's
> never used.

Yeah I could not think of a way to avoid this.

https://codereview.chromium.org/1806313003/diff/180001/content/browser/browse...
content/browser/browser_main_loop.cc:1235: media::AudioLogFactory*
audio_log_factory = MediaInternals::GetInstance();
On 2016/03/22 21:58:27, DaleCurtis wrote:
> Seems unnecessary to break this out. I'd just inline.

Done.

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
media/audio/audio_manager.cc:319: DCHECK(task_runner && worker_task_runner);
On 2016/03/22 21:58:27, DaleCurtis wrote:
> split dcheck so failures are easy to understand.

Done.

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
File media/audio/audio_manager_factory.h (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/audio_mana...
media/audio/audio_manager_factory.h:24: virtual scoped_ptr<AudioManager,
AudioManagerDeleter> CreateInstance(
On 2016/03/22 21:58:27, DaleCurtis wrote:
> ScopedPtrAudioManager?

I would then need to include audio_manager.h here. Is that OK? Or I could define
ScopedAudioManagerPtr in a different .h file?

https://codereview.chromium.org/1806313003/diff/180001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/180001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:43: scoped_ptr<AudioManagerPulse,
AudioManagerDeleter> manager(
On 2016/03/22 21:58:27, DaleCurtis wrote:
> ScopedPtrAudioManager?

I need a pointer for AudioManagerPulse to be able to call
AudioManagerPulse::Init.

[alokp, 2016-03-24 17:42:58]

Ping!

If the interface looks reasonable, I would like to go ahead and update various
AudioManager implmentations. Please let me know. Thanks!

[DaleCurtis, 2016-03-24 17:45:08]

Whoops, thought I posted already, yes go ahead to update. Though I'm still not
sold on switching all our scoped_refptr away from const&. I think we may want to
wait until that's done via a rewriter + clang plugin since it's easy to mess up
now.

[alokp, 2016-03-25 16:54:25]

On 2016/03/24 17:45:08, DaleCurtis wrote:
> Whoops, thought I posted already, yes go ahead to update. Though I'm still not
> sold on switching all our scoped_refptr away from const&. I think we may want
to
> wait until that's done via a rewriter + clang plugin since it's easy to mess
up
> now.

AFAICT this patch does not switch any scoped_refptr away from const&. The
function signature of AudioManager::Get[Worker]TaskRunner() is exactly the same.

We added task runner params to AudioManager::Create where we decided to match
the signature of AudioManager::Get[Worker]TaskRunner() as per the new
guidelines. And even there this patch consistently uses std::move to avoid any
refcounting overhead. Do you see anything different?

[DaleCurtis, 2016-03-25 22:18:16]

On 2016/03/25 at 16:54:25, alokp wrote:
> On 2016/03/24 17:45:08, DaleCurtis wrote:
> > Whoops, thought I posted already, yes go ahead to update. Though I'm still
not
> > sold on switching all our scoped_refptr away from const&. I think we may
want to
> > wait until that's done via a rewriter + clang plugin since it's easy to mess
up
> > now.
> 
> AFAICT this patch does not switch any scoped_refptr away from const&. The
function signature of AudioManager::Get[Worker]TaskRunner() is exactly the same.
> 
> We added task runner params to AudioManager::Create where we decided to match
the signature of AudioManager::Get[Worker]TaskRunner() as per the new
guidelines. And even there this patch consistently uses std::move to avoid any
refcounting overhead. Do you see anything different?

I'm just saying there's no presubmit to verify you use the new behavior
correctly yet :) Either is fine for me since it's a 1 time thing though.

[alokp, 2016-03-28 23:35:05]

Dale: I have updated all unittests and they pass on all platforms.

I do not have a good handle on two failures that you may help with:

1. webkit_tests
(inspector/elements/shadow/elements-panel-shadow-selection-on-refresh.html) It
does not seem related and did not fail on one run but failed consistently on the
last two. Nothing in the log jumps out.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oi...

2. telemetry_perf_unittests: mse_cases is timing out due to a crash during
browser launch?
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

[DaleCurtis, 2016-03-29 20:07:55]

Hmm, nothing stands out in the crashes. You'll have to try and get a local repro
going to investigate more. Sorry.

[DaleCurtis, 2016-03-30 02:03:32]

On 2016/03/29 at 20:07:55, DaleCurtis wrote:
> Hmm, nothing stands out in the crashes. You'll have to try and get a local
repro going to investigate more. Sorry.

The first one definitely seems like flake. I just hit it on one of my own CLs.

[DaleCurtis, 2016-03-30 02:04:06]

On 2016/03/30 at 02:03:32, DaleCurtis wrote:
> On 2016/03/29 at 20:07:55, DaleCurtis wrote:
> > Hmm, nothing stands out in the crashes. You'll have to try and get a local
repro going to investigate more. Sorry.
> 
> The first one definitely seems like flake. I just hit it on one of my own CLs.

And there's already a bug
https://bugs.chromium.org/p/chromium/issues/detail?id=577870

[alokp, 2016-03-31 20:22:19]

phew, a green try run finally!

Dale: PTAL

The unittests can be further simplified with the assumption that AudioManager is
now running on the main thread, but I chose to make most of them just pass for
now. I will make another pass at simplifying them in another patch.

[miu, 2016-03-31 22:42:57]

On 2016/03/31 20:22:19, alokp wrote:
> The unittests can be further simplified with the assumption that AudioManager
is
> now running on the main thread, but I chose to make most of them just pass for
> now. I will make another pass at simplifying them in another patch.

FYI--This was attempted before, but it's unclear why it bounced back out of the
tree.  Seems nobody bothered to come back and try again.

Attempt: https://codereview.chromium.org/1376313002/
Revert: https://codereview.chromium.org/1384983002/

[DaleCurtis, 2016-03-31 22:55:54]

Overall this lg2m but want to take another look over it tomorrow with fresher
eyes.

https://codereview.chromium.org/1806313003/diff/460001/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:156: TEST(WinAudioTest,
PCMWaveStreamGetAndClose) {
Might be easier to make these TEST_F with an audio_manager_

[alokp, 2016-04-01 02:45:09]

On 2016/03/31 22:42:57, miu wrote:
> On 2016/03/31 20:22:19, alokp wrote:
> > The unittests can be further simplified with the assumption that
AudioManager
> is
> > now running on the main thread, but I chose to make most of them just pass
for
> > now. I will make another pass at simplifying them in another patch.
> 
> FYI--This was attempted before, but it's unclear why it bounced back out of
the
> tree.  Seems nobody bothered to come back and try again.
> 
> Attempt: https://codereview.chromium.org/1376313002/
> Revert: https://codereview.chromium.org/1384983002/

miu@: This patch does not make the change you referenced above.

[alokp, 2016-04-01 03:40:53]

https://codereview.chromium.org/1806313003/diff/460001/media/audio/win/audio_...
File media/audio/win/audio_output_win_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:156: TEST(WinAudioTest,
PCMWaveStreamGetAndClose) {
On 2016/03/31 22:55:54, DaleCurtis wrote:
> Might be easier to make these TEST_F with an audio_manager_

Right. It would prefer to do this in a separate patch though because I do not a
windows machine. I need to rely on trybots to catch any compile errors, which
will easier with a smaller patch.

[alokp, 2016-04-04 18:06:47]

friendly ping!

[DaleCurtis, 2016-04-04 18:17:45]

Sorry will get to this tomorrow. No fresh eyes today :(

[DaleCurtis, 2016-04-05 21:55:36]

Seems unnecessary to even have a Shutdown() method now. Instead should we just
move the contents into each respective destructor? This ensures destruction of
concepts is always in the right order -- notably you've flipped this in some
cases currently. Which I'm not sure is correct (even if it does work right now).

https://codereview.chromium.org/1806313003/diff/460001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:84: ShutdownOnAudioThread();
Why the reordering?

https://codereview.chromium.org/1806313003/diff/460001/media/audio/mac/audio_...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/mac/audio_...
media/audio/mac/audio_manager_mac.cc:391: ShutdownOnAudioThread();
Ditto. Also these methods are confusing now. Maybe they should just be merged
into the destructor?

[alokp, 2016-04-05 23:57:40]

I think we can get rid of AudioManagerBase::Shutdown except in the case of
AudioManagerPulse.

https://codereview.chromium.org/1806313003/diff/460001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:84: ShutdownOnAudioThread();
On 2016/04/05 21:55:36, DaleCurtis wrote:
> Why the reordering?

You are right about this one. I read the code wrong and assumed that you need to
AudioManagerBase::ShutdownOnAudioThread which destroys all output streams before
AudioManagerAndroid::ShutdownOnAudioThread.

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:69: DestroyPulse();
I think we need to Shutdown AudioManagerBase, which releases all output stream
dispatchers before we can destroy pulse.

[DaleCurtis, 2016-04-06 01:22:29]

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:69: DestroyPulse();
On 2016/04/05 at 23:57:40, alokp wrote:
> I think we need to Shutdown AudioManagerBase, which releases all output stream
dispatchers before we can destroy pulse.

Can we just post a task to a static DestroyPulse method on the current thread?

[alokp, 2016-04-06 02:03:44]

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/460001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:69: DestroyPulse();
On 2016/04/06 01:22:29, DaleCurtis wrote:
> On 2016/04/05 at 23:57:40, alokp wrote:
> > I think we need to Shutdown AudioManagerBase, which releases all output
stream
> dispatchers before we can destroy pulse.
> 
> Can we just post a task to a static DestroyPulse method on the current thread?

Yup.. thats exactly what I am doing..

[alokp, 2016-04-06 05:32:29]

Description was changed from

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.
==========

to

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

[alokp, 2016-04-06 06:14:53]

AudioManagerBase::Shutdown is gone. PTAL.

[DaleCurtis, 2016-04-06 22:07:50]

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:81:
DCHECK(GetTaskRunner()->BelongsToCurrentThread());
Hmm, this might be a bit dicy in the future since j_audio_manager_ is used by
ReleaseInputStream(), but I suppose at this point there should be zero input
streams which is okay for now.

https://codereview.chromium.org/1806313003/diff/500001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/audio_mana...
media/audio/audio_manager.cc:356: return Create(task_runner, task_runner,
nullptr,
std::move?

https://codereview.chromium.org/1806313003/diff/500001/media/audio/mac/audio_...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/mac/audio_...
media/audio/mac/audio_manager_mac.cc:392: // Since CoreAudio calls have to run
on the UI thread and browser shutdown
These should be removable if you change the scoped deleter to always use
DeleteSoon. Fine to do in another patch set.

https://codereview.chromium.org/1806313003/diff/520001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/520001/media/audio/audio_mana...
media/audio/audio_manager.cc:279: base::RunLoop().RunUntilIdle();
I don't think this is allowed in production code. I also think that you need to
always post here to avoid the same issues that the OSX manager works around by
shutting down streams within the destructor.

https://codereview.chromium.org/1806313003/diff/520001/media/audio/audio_mana...
media/audio/audio_manager.cc:281: task_runner->DeleteSoon(FROM_HERE, instance);
This can fail FYI, but should only do so if other objects are already dead.

[alokp, 2016-04-07 00:38:10]

I am now always posting a task to delete audio manager. I am sure it will take a
few iterations to make all the tests pass, so please hold off the review until
then.

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:81:
DCHECK(GetTaskRunner()->BelongsToCurrentThread());
On 2016/04/06 22:07:49, DaleCurtis wrote:
> Hmm, this might be a bit dicy in the future since j_audio_manager_ is used by
> ReleaseInputStream(), but I suppose at this point there should be zero input
> streams which is okay for now.

We have a DCHECK_EQ(0, num_input_streams_) in AudioManagerBase destructor, which
should catch this, although I agree it does look dicy. If this is a problem, we
will need to post a task to destroy j_audio_manager_ similar to what we are
doing in Pulse.

https://codereview.chromium.org/1806313003/diff/500001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/audio_mana...
media/audio/audio_manager.cc:356: return Create(task_runner, task_runner,
nullptr,
On 2016/04/06 22:07:49, DaleCurtis wrote:
> std::move?

Cant do because the same task_runner is being passed for both audio task runner
and worker task runner.

https://codereview.chromium.org/1806313003/diff/500001/media/audio/mac/audio_...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/mac/audio_...
media/audio/mac/audio_manager_mac.cc:392: // Since CoreAudio calls have to run
on the UI thread and browser shutdown
On 2016/04/06 22:07:49, DaleCurtis wrote:
> These should be removable if you change the scoped deleter to always use
> DeleteSoon. Fine to do in another patch set.

Done.

https://codereview.chromium.org/1806313003/diff/520001/media/audio/audio_mana...
File media/audio/audio_manager.cc (right):

https://codereview.chromium.org/1806313003/diff/520001/media/audio/audio_mana...
media/audio/audio_manager.cc:279: base::RunLoop().RunUntilIdle();
On 2016/04/06 22:07:49, DaleCurtis wrote:
> I don't think this is allowed in production code. I also think that you need
to
> always post here to avoid the same issues that the OSX manager works around by
> shutting down streams within the destructor.

Done.

[tommi (sloooow) - chröme, 2016-04-07 14:33:40]

thanks for the update.  one comment to consider while you're iterating

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:81:
DCHECK(GetTaskRunner()->BelongsToCurrentThread());
On 2016/04/07 00:38:10, alokp wrote:
> On 2016/04/06 22:07:49, DaleCurtis wrote:
> > Hmm, this might be a bit dicy in the future since j_audio_manager_ is used
by
> > ReleaseInputStream(), but I suppose at this point there should be zero input
> > streams which is okay for now.
> 
> We have a DCHECK_EQ(0, num_input_streams_) in AudioManagerBase destructor,
which
> should catch this, although I agree it does look dicy. If this is a problem,
we
> will need to post a task to destroy j_audio_manager_ similar to what we are
> doing in Pulse.

What do you think about upgrading that to CHECK_EQ?  I'd like to do that for all
platforms actually, so if we're confident with this requirement being met, maybe
we should do that.  I think it's an important enough design assertion and if we
don't meet it, there's a chance of a use-after-free.

[DaleCurtis, 2016-04-07 17:23:09]

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/1806313003/diff/500001/media/audio/android/au...
media/audio/android/audio_manager_android.cc:81:
DCHECK(GetTaskRunner()->BelongsToCurrentThread());
On 2016/04/07 at 14:33:40, tommi-sloooow wrote:
> On 2016/04/07 00:38:10, alokp wrote:
> > On 2016/04/06 22:07:49, DaleCurtis wrote:
> > > Hmm, this might be a bit dicy in the future since j_audio_manager_ is used
by
> > > ReleaseInputStream(), but I suppose at this point there should be zero
input
> > > streams which is okay for now.
> > 
> > We have a DCHECK_EQ(0, num_input_streams_) in AudioManagerBase destructor,
which
> > should catch this, although I agree it does look dicy. If this is a problem,
we
> > will need to post a task to destroy j_audio_manager_ similar to what we are
> > doing in Pulse.
> 
> What do you think about upgrading that to CHECK_EQ?  I'd like to do that for
all platforms actually, so if we're confident with this requirement being met,
maybe we should do that.  I think it's an important enough design assertion and
if we don't meet it, there's a chance of a use-after-free.

+1 I think we should do this, though it'll be hard to be actionable given the
amount of information available at time of destruction. It won't catch all
problem cases either. The risk case is: previously the thread stopped during
destruction w/ a join, now it will stop after destruction. If anything during
destruction posts another task to the task runner thread doing things with an
audio manager we'll have problems.

[alokp, 2016-04-12 23:02:14]

I have addressed the remaining comments.

- Always DeleteSoon to destroy AudioManager instance. Used TestMessageLoop to
ensure that the instance is actually destroyed.

- Changed DCHECK_EQ to CHECK_EQ for num_input/output_streams

- I had to restore AudioManagerBase::Shutdown because multiple AudioManager
implementations have members that need to outlive the AudioOutputProxy streams
that are cleared in AudioManagerBase::Shutdown. AudioManagerPulse cannot destroy
pulse. AudioManagerAlsa has AlsaWrapper. These could be solved by posting task
to the message queue but a few implementation also override ReleaseOutputStream
that get called when AudioOutputStream is closed. So we need to keep the
implementation alive until Shutdown has completed.

PTAL

[tommi (sloooow) - chröme, 2016-04-13 07:31:51]

lgtm.  Some minor suggestions below.

https://codereview.chromium.org/1806313003/diff/940001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_manager_factory.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/chromecast/media/audio...
chromecast/media/audio/cast_audio_manager_factory.cc:20:
scoped_ptr<::media::AudioManager, ::media::AudioManagerDeleter>
nit: if you feel inclined to make further cosmetic changes, I'd like to get rid
of the global namespace qualifier.  I.e. ::media::AudioManager ->
media::AudioManager etc.

https://codereview.chromium.org/1806313003/diff/940001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/content/browser/browse...
content/browser/browser_main_loop.cc:1230: // TODO(alokp): Allow content
embedders to override the default
nit: in order to keep the size of BrowserThreadsStarted() down, it could be good
to extract this scope out to a separate function now.

https://codereview.chromium.org/1806313003/diff/940001/media/audio/audio_mana...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/audio_mana...
media/audio/audio_manager_unittest.cc:45: };
remove semicolon (here and below)

https://codereview.chromium.org/1806313003/diff/940001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:53: } else {
nit: no need for this scope since there's an early return above.  Since you
don't want to use the |manager| variable in the following lines, you could also
scope it:

  {
    // Do not move task runners when creating AudioManagerPulse.
    // If the creation fails, we need to use the task runners to create other
    // AudioManager implementations.
    std::unique_ptr<AudioManagerPulse, AudioManagerDeleter> manager(
        new AudioManagerPulse(task_runner, worker_task_runner,
                              audio_log_factory));
    if (manager->Init() {
      UMA_HISTOGRAM_ENUMERATION("Media.LinuxAudioIO", kPulse, kAudioIOMax + 1);
      return manager;
    }
  }

  DVLOG(1) << "PulseAudio is not available on the OS";
  ...

[DaleCurtis, 2016-04-14 00:00:32]

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

[DaleCurtis, 2016-04-14 00:00:35]

+xhwang for media/mojo

lgtm otherwise, good luck with the tree. You might want to wait until a quiet
period to land this and alert sheriffs.

https://codereview.chromium.org/1806313003/diff/940001/media/audio/mac/audio_...
File media/audio/mac/audio_manager_mac.h (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/mac/audio_...
media/audio/mac/audio_manager_mac.h:142: // TODO(alokp): We used to track these
streams to close before destruction.
Delete these? You already removed the stop streams code. If you can't remove
them then you can't land this CL as is right? I.e. it would imply that all other
platforms will now have the same issue as OSX.

https://codereview.chromium.org/1806313003/diff/940001/media/audio/pulse/audi...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/pulse/audi...
media/audio/pulse/audio_manager_pulse.cc:71: bool AudioManagerPulse::Init() {
What's the point of this change? Why not just merge this with the method below?

[DaleCurtis, 2016-04-14 00:01:04]

Also, thanks again for this refactoring. It cleans up a lot of cruft. I'm sad we
couldn't remove shutdown, but this is still great to have.

[xhwang, 2016-04-14 00:05:59]

mojo/media lgtm

[xhwang, 2016-04-14 00:06:30]

sorry, I meant media/mojo lgtm :)

[alokp, 2016-04-14 13:30:03]

https://codereview.chromium.org/1806313003/diff/940001/media/audio/mac/audio_...
File media/audio/mac/audio_manager_mac.h (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/mac/audio_...
media/audio/mac/audio_manager_mac.h:142: // TODO(alokp): We used to track these
streams to close before destruction.
On 2016/04/14 00:00:35, DaleCurtis wrote:
> Delete these? You already removed the stop streams code. If you can't remove
> them then you can't land this CL as is right? I.e. it would imply that all
other
> platforms will now have the same issue as OSX.

Sorry I do not understand your comment. Remove the TODO or the member variables?
The member variables are still being used by MaybeChangeBufferSize().

[DaleCurtis, 2016-04-14 15:35:48]

Ah, didn't realize they had other uses these days. Lgtm

[DaleCurtis, 2016-04-14 15:35:52]

Ah, didn't realize they had other uses these days. Lgtm

[alokp, 2016-04-17 16:54:43]

Description was changed from

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

to

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

[alokp, 2016-04-17 16:54:43]

alokp@chromium.org changed reviewers:
+ jam@chromium.org, rkc@chromium.org

[alokp, 2016-04-17 16:55:19]

jam: content/
rkc: components/audio_modem/

https://codereview.chromium.org/1806313003/diff/940001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_manager_factory.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/chromecast/media/audio...
chromecast/media/audio/cast_audio_manager_factory.cc:20:
scoped_ptr<::media::AudioManager, ::media::AudioManagerDeleter>
On 2016/04/13 07:31:51, tommi-chrömium wrote:
> nit: if you feel inclined to make further cosmetic changes, I'd like to get
rid
> of the global namespace qualifier.  I.e. ::media::AudioManager ->
> media::AudioManager etc.

Global qualifier is needed here because CastAudioManagerFactory lives in
chromecast::media. Just using media:: confuses the compiler.

https://codereview.chromium.org/1806313003/diff/940001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/content/browser/browse...
content/browser/browser_main_loop.cc:1230: // TODO(alokp): Allow content
embedders to override the default
On 2016/04/13 07:31:51, tommi-chrömium wrote:
> nit: in order to keep the size of BrowserThreadsStarted() down, it could be
good
> to extract this scope out to a separate function now.

Done.

https://codereview.chromium.org/1806313003/diff/940001/media/audio/audio_mana...
File media/audio/audio_manager_unittest.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/audio_mana...
media/audio/audio_manager_unittest.cc:45: };
On 2016/04/13 07:31:51, tommi-chrömium wrote:
> remove semicolon (here and below)

Done.

https://codereview.chromium.org/1806313003/diff/940001/media/audio/linux/audi...
File media/audio/linux/audio_manager_linux.cc (right):

https://codereview.chromium.org/1806313003/diff/940001/media/audio/linux/audi...
media/audio/linux/audio_manager_linux.cc:53: } else {
On 2016/04/13 07:31:51, tommi-chrömium wrote:
> nit: no need for this scope since there's an early return above.  Since you
> don't want to use the |manager| variable in the following lines, you could
also
> scope it:
> 
>   {
>     // Do not move task runners when creating AudioManagerPulse.
>     // If the creation fails, we need to use the task runners to create other
>     // AudioManager implementations.
>     std::unique_ptr<AudioManagerPulse, AudioManagerDeleter> manager(
>         new AudioManagerPulse(task_runner, worker_task_runner,
>                               audio_log_factory));
>     if (manager->Init() {
>       UMA_HISTOGRAM_ENUMERATION("Media.LinuxAudioIO", kPulse, kAudioIOMax +
1);
>       return manager;
>     }
>   }
> 
>   DVLOG(1) << "PulseAudio is not available on the OS";
>   ...

Done.

[rkc, 2016-04-17 20:20:04]

rkc@chromium.org changed reviewers:
+ ckehoe@chromium.org

[rkc, 2016-04-17 20:20:06]

components/audio_modem lgtm
+ckehoe fyi

[jam, 2016-04-18 19:04:07]

lgtm

[alokp, 2016-04-18 20:53:18]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-04-18 20:53:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
xhwang@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1806313003/#ps980001
(title: "address comments from patch 48")

[commit-bot: I haz the power, 2016-04-18 20:54:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1806313003/980001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1806313003/980001

[commit-bot: I haz the power, 2016-04-18 22:24:16]

Description was changed from

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

to

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

[commit-bot: I haz the power, 2016-04-18 22:24:19]

Committed patchset #50 (id:980001)

[commit-bot: I haz the power, 2016-04-18 22:25:39]

Description was changed from

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234
==========

to

==========
Pass task runners to AudioManager constructor.

This patch replaces the internal AudioManagerBase::audio_thread with
an external task runner. The old implementation made it harder to test
and override the internal audio thread.

AudioManagerBase::Shutdown had to complete on the audio thread before
the AudioManager instance could be deleted. It relied on Thread::Stop
on the main thread to wait for the audio thread to shutdown. A subclass
using a different thread shared with other modules could not do that.

Passing a task runner into AudioManager and making GetTaskRunner
non-virtual ensures that the task runner will not change for the
lifetime of AudioManager instance.

BUG=594234

Committed: https://crrev.com/e05981bd06b4694b426990acd41be8ab6d9a1659
Cr-Commit-Position: refs/heads/master@{#388048}
==========

[commit-bot: I haz the power, 2016-04-18 22:25:41]

Patchset 50 (id:??) landed as
https://crrev.com/e05981bd06b4694b426990acd41be8ab6d9a1659
Cr-Commit-Position: refs/heads/master@{#388048}

[Jeffrey Yasskin, 2016-04-19 00:38:48]

A revert of this CL (patchset #50 id:980001) has been created in
https://codereview.chromium.org/1901583005/ by jyasskin@chromium.org.

The reason for reverting is: Broke TSan:
https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test...

WARNING: ThreadSanitizer: data race (pid=25863)
  Write of size 8 at 0x0000081b8060 by main thread:
    #0 media::AudioManagerDeleter::operator()(media::AudioManager const*) const
media/audio/audio_manager.cc:264:20 (content_browsertests+0x000001e77078)
    #1 reset buildtools/third_party/libc++/trunk/include/memory:2735:13
(content_browsertests+0x000004e5f2af)
    #2 ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2703
(content_browsertests+0x000004e5f2af)
    #3 content::BrowserMainLoop::~BrowserMainLoop()
content/browser/browser_main_loop.cc:428 (content_browsertests+0x000004e5f2af)
    #4 content::BrowserMainLoop::~BrowserMainLoop()
content/browser/browser_main_loop.cc:424:37
(content_browsertests+0x000004e5f779)
    #5 operator() buildtools/third_party/libc++/trunk/include/memory:2529:13
(content_browsertests+0x000004e68120)
    #6 reset buildtools/third_party/libc++/trunk/include/memory:2735
(content_browsertests+0x000004e68120)
    #7 content::BrowserMainRunnerImpl::Shutdown()
content/browser/browser_main_runner.cc:223 (content_browsertests+0x000004e68120)
    #8 ShellBrowserMain(content::MainFunctionParams const&,
std::__1::unique_ptr<content::BrowserMainRunner,
std::__1::default_delete<content::BrowserMainRunner> > const&)
content/shell/browser/shell_browser_main.cc:33:3
(content_browsertests+0x000000c97ce7)
    #9 content::ShellMainDelegate::RunProcess(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&)
content/shell/app/shell_main_delegate.cc:288:16
(content_browsertests+0x000000c8bec4)
    #10 content::RunNamedProcessTypeMain(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&, content::ContentMainDelegate*)
content/app/content_main_runner.cc:368:25 (content_browsertests+0x000005558812)
    #11 content::ContentMainRunnerImpl::Run()
content/app/content_main_runner.cc:742:12 (content_browsertests+0x000005559511)
    #12 content::ContentMain(content::ContentMainParams const&)
content/app/content_main.cc:20:15 (content_browsertests+0x00000555797e)
    #13 content::BrowserTestBase::SetUp()
content/public/test/browser_test_base.cc:287:3
(content_browsertests+0x000000c55601)
    #14 content::ContentBrowserTest::SetUp()
content/public/test/content_browser_test.cc:92:3
(content_browsertests+0x000000c5e8e6)
    #15 SetUp
content/browser/renderer_host/render_widget_host_view_browsertest.cc:221:5
(content_browsertests+0x000000739618)
    #16 content::(anonymous
namespace)::CompositingRenderWidgetHostViewBrowserTestTabCapture::SetUp()
content/browser/renderer_host/render_widget_host_view_browsertest.cc:403
(content_browsertests+0x000000739618)
    #17 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3168d)
    #18 testing::Test::Run() testing/gtest/src/gtest.cc:2470
(content_browsertests+0x000000f3168d)
    #19 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5
(content_browsertests+0x000000f32833)
    #20 testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5
(content_browsertests+0x000000f33118)
    #21 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4647:11 (content_browsertests+0x000000f3c562)
    #22 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3bfc6)
    #23 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255
(content_browsertests+0x000000f3bfc6)
    #24 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10
(content_browsertests+0x000000d209eb)
    #25 base::TestSuite::Run() base/test/test_suite.cc:230
(content_browsertests+0x000000d209eb)
    #26 content::ContentTestLauncherDelegate::RunTestSuite(int, char**)
content/test/content_test_launcher.cc:105:12
(content_browsertests+0x000000c677db)
    #27 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**)
content/public/test/test_launcher.cc:517:12
(content_browsertests+0x000000cfb2fc)
    #28 main content/test/content_test_launcher.cc:131:10
(content_browsertests+0x000000c67762)

  Previous read of size 8 at 0x0000081b8060 by thread T15:
    #0 media::(anonymous
namespace)::AudioManagerHelper::UpdateLastAudioThreadTimeTick()
media/audio/audio_manager.cc:179:5 (content_browsertests+0x000001e77986)
    #1 Run<> base/bind_internal.h:181:12 (content_browsertests+0x000001e77df2)
    #2 MakeItSo<media::(anonymous namespace)::AudioManagerHelper *>
base/bind_internal.h:321 (content_browsertests+0x000001e77df2)
    #3 base::internal::Invoker<base::IndexSequence<0ul>,
base::internal::BindState<base::internal::RunnableAdapter<void
(media::(anonymous namespace)::AudioManagerHelper::*)()>, void
(media::(anonymous namespace)::AudioManagerHelper*),
base::internal::UnretainedWrapper<media::(anonymous
namespace)::AudioManagerHelper> >, base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void (media::(anonymous
namespace)::AudioManagerHelper::*)()> >, void
()>::Run(base::internal::BindStateBase*) base/bind_internal.h:372
(content_browsertests+0x000001e77df2)
    #4 Run base/callback.h:397:12 (content_browsertests+0x00000111aa7d)
    #5 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask
const&) base/debug/task_annotator.cc:51 (content_browsertests+0x00000111aa7d)
    #6 base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:479:3 (content_browsertests+0x0000010958e6)
    #7 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
base/message_loop/message_loop.cc:488:5 (content_browsertests+0x000001095ecd)
    #8 base::MessageLoop::DoDelayedWork(base::TimeTicks*)
base/message_loop/message_loop.cc:638:10 (content_browsertests+0x00000109662a)
    #9 base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
base/message_loop/message_pump_default.cc:37:17
(content_browsertests+0x000001099875)
    #10 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:443:3
(content_browsertests+0x00000109528b)
    #11 base::RunLoop::Run() base/run_loop.cc:35:3
(content_browsertests+0x0000010b5cc6)
    #12 base::MessageLoop::Run() base/message_loop/message_loop.cc:295:3
(content_browsertests+0x000001094a95)
    #13 base::Thread::Run(base::MessageLoop*) base/threading/thread.cc:202:3
(content_browsertests+0x0000010de279)
    #14 base::Thread::ThreadMain() base/threading/thread.cc:254:3
(content_browsertests+0x0000010de449)
    #15 base::(anonymous namespace)::ThreadFunc(void*)
base/threading/platform_thread_posix.cc:70:3
(content_browsertests+0x0000010d88bd)

  Location is global 'media::(anonymous namespace)::g_last_created' of size 8 at
0x0000081b8060 (content_browsertests+0x0000081b8060)

  Thread T15 'AudioThread' (tid=25884, running) created by main thread at:
    #0 pthread_create <null> (content_browsertests+0x0000004a15c5)
    #1 base::(anonymous namespace)::CreateThread(unsigned long, bool,
base::PlatformThread::Delegate*, base::PlatformThreadHandle*,
base::ThreadPriority) base/threading/platform_thread_posix.cc:109:13
(content_browsertests+0x0000010d860a)
    #2 base::PlatformThread::CreateWithPriority(unsigned long,
base::PlatformThread::Delegate*, base::PlatformThreadHandle*,
base::ThreadPriority) base/threading/platform_thread_posix.cc:190:10
(content_browsertests+0x0000010d8515)
    #3 base::Thread::StartWithOptions(base::Thread::Options const&)
base/threading/thread.cc:116:10 (content_browsertests+0x0000010ddfa0)
    #4 base::Thread::Start() base/threading/thread.cc:86:10
(content_browsertests+0x0000010ddde4)
    #5 content::BrowserMainLoop::CreateAudioManager()
content/browser/browser_main_loop.cc:1468:3
(content_browsertests+0x000004e665c6)
    #6 content::BrowserMainLoop::BrowserThreadsStarted()
content/browser/browser_main_loop.cc:1228:5
(content_browsertests+0x000004e633b5)
    #7 Run<> base/bind_internal.h:181:12 (content_browsertests+0x000004e67612)
    #8 MakeItSo<content::BrowserMainLoop *> base/bind_internal.h:313
(content_browsertests+0x000004e67612)
    #9 base::internal::Invoker<base::IndexSequence<0ul>,
base::internal::BindState<base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()>, int (content::BrowserMainLoop*),
base::internal::UnretainedWrapper<content::BrowserMainLoop> >,
base::internal::InvokeHelper<false, int, base::internal::RunnableAdapter<int
(content::BrowserMainLoop::*)()> >, int ()>::Run(base::internal::BindStateBase*)
base/bind_internal.h:372 (content_browsertests+0x000004e67612)
    #10 Run base/callback.h:397:12 (content_browsertests+0x0000050dd941)
    #11 content::StartupTaskRunner::RunAllTasksNow()
content/browser/startup_task_runner.cc:45 (content_browsertests+0x0000050dd941)
    #12 content::BrowserMainLoop::CreateStartupTasks()
content/browser/browser_main_loop.cc:803:3 (content_browsertests+0x000004e622db)
    #13 content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams
const&) content/browser/browser_main_runner.cc:139:5
(content_browsertests+0x000004e67c63)
    #14 ShellBrowserMain(content::MainFunctionParams const&,
std::__1::unique_ptr<content::BrowserMainRunner,
std::__1::default_delete<content::BrowserMainRunner> > const&)
content/shell/browser/shell_browser_main.cc:23:19
(content_browsertests+0x000000c97ca7)
    #15 content::ShellMainDelegate::RunProcess(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&)
content/shell/app/shell_main_delegate.cc:288:16
(content_browsertests+0x000000c8bec4)
    #16 content::RunNamedProcessTypeMain(std::__1::basic_string<char,
std::__1::char_traits<char>, std::__1::allocator<char> > const&,
content::MainFunctionParams const&, content::ContentMainDelegate*)
content/app/content_main_runner.cc:368:25 (content_browsertests+0x000005558812)
    #17 content::ContentMainRunnerImpl::Run()
content/app/content_main_runner.cc:742:12 (content_browsertests+0x000005559511)
    #18 content::ContentMain(content::ContentMainParams const&)
content/app/content_main.cc:20:15 (content_browsertests+0x00000555797e)
    #19 content::BrowserTestBase::SetUp()
content/public/test/browser_test_base.cc:287:3
(content_browsertests+0x000000c55601)
    #20 content::ContentBrowserTest::SetUp()
content/public/test/content_browser_test.cc:92:3
(content_browsertests+0x000000c5e8e6)
    #21 SetUp
content/browser/renderer_host/render_widget_host_view_browsertest.cc:221:5
(content_browsertests+0x000000739618)
    #22 content::(anonymous
namespace)::CompositingRenderWidgetHostViewBrowserTestTabCapture::SetUp()
content/browser/renderer_host/render_widget_host_view_browsertest.cc:403
(content_browsertests+0x000000739618)
    #23 HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3168d)
    #24 testing::Test::Run() testing/gtest/src/gtest.cc:2470
(content_browsertests+0x000000f3168d)
    #25 testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5
(content_browsertests+0x000000f32833)
    #26 testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5
(content_browsertests+0x000000f33118)
    #27 testing::internal::UnitTestImpl::RunAllTests()
testing/gtest/src/gtest.cc:4647:11 (content_browsertests+0x000000f3c562)
    #28 HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
bool> testing/gtest/src/gtest.cc:2458:12 (content_browsertests+0x000000f3bfc6)
    #29 testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255
(content_browsertests+0x000000f3bfc6)
    #30 RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10
(content_browsertests+0x000000d209eb)
    #31 base::TestSuite::Run() base/test/test_suite.cc:230
(content_browsertests+0x000000d209eb)
    #32 content::ContentTestLauncherDelegate::RunTestSuite(int, char**)
content/test/content_test_launcher.cc:105:12
(content_browsertests+0x000000c677db)
    #33 content::LaunchTests(content::TestLauncherDelegate*, int, int, char**)
content/public/test/test_launcher.cc:517:12
(content_browsertests+0x000000cfb2fc)
    #34 main content/test/content_test_launcher.cc:131:10
(content_browsertests+0x000000c67762)

SUMMARY: ThreadSanitizer: data race media/audio/audio_manager.cc:264:20 in
media::AudioManagerDeleter::operator()(media::AudioManager const*) const
.