Caching AudioOutputDevice instances in mixer manager

Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to get output device info or to pass to a mixer; helps to reuse thinks to get output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it means that the cache will work as AudioDeviceFactory, and they both probably should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843, 586161, 587461
TESTSING=manual testing on several sample pages with media elements + media_unittests + content_unittests + media_blink_unittests (all updated)

Committed: https://crrev.com/7a4679394c01834175e451c7983747eab8603b82
Cr-Commit-Position: refs/heads/master@{#396471}

[o1ka, 2016-05-02 13:24:54]

Patchset #1 (id:1) has been deleted

[o1ka, 2016-05-02 13:28:18]

Description was changed from

==========
precommit


fix


dvlog


comments


format


cl format


cl format


compiles

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

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.
==========

[o1ka, 2016-05-02 13:29:35]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.

BUG=536843,586161
==========

[o1ka, 2016-05-02 16:27:00]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.

BUG=536843,586161
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.

BUG=536843,586161,587461
==========

[o1ka, 2016-05-02 16:28:22]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

*AudioRendererSinkCache introduced - wons sinks and manages their lifetime.
* AudioRendererMixer does not own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added for easier unit tests creation.

BUG=536843,586161,587461
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

* AudioRendererSinkCache introduced - onws sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.

BUG=536843,586161,587461
==========

[o1ka, 2016-05-03 12:08:29]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft documenting the approach for caching.

* AudioRendererSinkCache introduced - onws sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.

BUG=536843,586161,587461
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft - no unit tests yet. If the reviewers are fine with the
approach, I'll proceed with unit tests.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
*HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.

BUG=536843,586161,587461
==========

[o1ka, 2016-05-03 12:08:30]

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

[o1ka, 2016-05-03 12:15:09]

PTAL (no unit tests yet, will proceed with them if the approach is fine).

[o1ka, 2016-05-03 13:34:37]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft - no unit tests yet. If the reviewers are fine with the
approach, I'll proceed with unit tests.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
*HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.

BUG=536843,586161,587461
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft - no unit tests yet. If the reviewers are fine with the
approach, I'll proceed with unit tests.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
*HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements
==========

[DaleCurtis, 2016-05-03 20:55:51]

This looks good. Thanks! It's a bit heavier weight than I envisioned, but I
think that's mostly because my vision was incomplete. Do you have any concerns
about parameters of the actual device changing on the browser side relative to
the renderer side cache?

[Henrik Grunell, 2016-05-04 06:29:42]

Looks good. Nice! Let me know when I should review.

[o1ka, 2016-05-04 10:15:13]

On 2016/05/03 20:55:51, DaleCurtis wrote:
> This looks good. Thanks! It's a bit heavier weight than I envisioned, but I
> think that's mostly because my vision was incomplete. Do you have any concerns
> about parameters of the actual device changing on the browser side relative to
> the renderer side cache?

Heavier in which sense? (I tried to keep it simple and did not want to mess up
with passing sink ownership, especially because it's going to be non ref-counted
some day;
but there is always a place for improvement.)

If we've got one client rendering though an AOD1 to a device1, and browser side
parameters suddenly change, we just continue to render with stale parameters,
and this does not differ from what it used to be before caching. The difference
is that if a new client comes and asks for output parameters of device1, we will
return the stale ones cached in AOD1 (until AOD1 is stopped). I think it should
be tolerable*, since anyways we've already got an output (through AOD1) using
now-stale parameters.
(*at least if OS does not change output parameters _because_ we try to create a
new AOD2, which we are about to do after we get output parameters)

+guidou@ - what do you think of this stale AOD parameters issue?

[o1ka, 2016-05-04 10:16:30]

olka@chromium.org changed reviewers:
+ guidou@chromium.org

[o1ka, 2016-05-04 10:16:30]

+guidou@ (please see my last comment)

[DaleCurtis, 2016-05-04 21:33:45]

On 2016/05/04 at 10:15:13, olka wrote:
> On 2016/05/03 20:55:51, DaleCurtis wrote:
> > This looks good. Thanks! It's a bit heavier weight than I envisioned, but I
> > think that's mostly because my vision was incomplete. Do you have any
concerns
> > about parameters of the actual device changing on the browser side relative
to
> > the renderer side cache?
> 
> Heavier in which sense? (I tried to keep it simple and did not want to mess up
with passing sink ownership, especially because it's going to be non ref-counted
some day;
> but there is always a place for improvement.)
> 

Heavier in terms of the amount of code, but I tend to underestimate :)

> If we've got one client rendering though an AOD1 to a device1, and browser
side parameters suddenly change, we just continue to render with stale
parameters, and this does not differ from what it used to be before caching. The
difference is that if a new client comes and asks for output parameters of
device1, we will return the stale ones cached in AOD1 (until AOD1 is stopped). I
think it should be tolerable*, since anyways we've already got an output
(through AOD1) using now-stale parameters.
> (*at least if OS does not change output parameters _because_ we try to create
a new AOD2, which we are about to do after we get output parameters)
> 
> +guidou@ - what do you think of this stale AOD parameters issue?

[Guido Urdaneta, 2016-05-05 09:37:11]

On 2016/05/04 10:15:13, o1ka wrote:
> On 2016/05/03 20:55:51, DaleCurtis wrote:
> > This looks good. Thanks! It's a bit heavier weight than I envisioned, but I
> > think that's mostly because my vision was incomplete. Do you have any
concerns
> > about parameters of the actual device changing on the browser side relative
to
> > the renderer side cache?
> 
> Heavier in which sense? (I tried to keep it simple and did not want to mess up
> with passing sink ownership, especially because it's going to be non
ref-counted
> some day;
> but there is always a place for improvement.)
> 
> If we've got one client rendering though an AOD1 to a device1, and browser
side
> parameters suddenly change, we just continue to render with stale parameters,
> and this does not differ from what it used to be before caching. The
difference
> is that if a new client comes and asks for output parameters of device1, we
will
> return the stale ones cached in AOD1 (until AOD1 is stopped). I think it
should
> be tolerable*, since anyways we've already got an output (through AOD1) using
> now-stale parameters.
> (*at least if OS does not change output parameters _because_ we try to create
a
> new AOD2, which we are about to do after we get output parameters)
> 
> +guidou@ - what do you think of this stale AOD parameters issue?

I think the issue of stale parameters is an acceptable one.
First, there is no way to have perfectly strong consistency here because the
hardware parameters can change at any time. 
The model proposed here trades a little bit of consistency for better
performance, cleaner interfaces and more stability (there should be some
reduction in hangs caused by crbug.com/422522). I think this is an improvement
over the current state.
In fact, before we started supporting nondefault devices, the default device
parameters were always cached in the renderer. See
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

So this work allows us to return to a consistency model that had been supported
by Chromium for a long time, but with extra advantages (support for multiple
devices, proper support for session IDs, no sync IPC, etc.)
AFAIK, there were no important issues reported in the past due to parameter
caching in the renderer, so I think this it is a net improvement.

[o1ka, 2016-05-10 11:25:28]

> > +guidou@ - what do you think of this stale AOD parameters issue?
> 
> I think the issue of stale parameters is an acceptable one.
> First, there is no way to have perfectly strong consistency here because the
> hardware parameters can change at any time. 
> The model proposed here trades a little bit of consistency for better
> performance, cleaner interfaces and more stability (there should be some
> reduction in hangs caused by crbug.com/422522). I think this is an improvement
> over the current state.
> In fact, before we started supporting nondefault devices, the default device
> parameters were always cached in the renderer. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 
> So this work allows us to return to a consistency model that had been
supported
> by Chromium for a long time, but with extra advantages (support for multiple
> devices, proper support for session IDs, no sync IPC, etc.)
> AFAIK, there were no important issues reported in the past due to parameter
> caching in the renderer, so I think this it is a net improvement.

Thanks guidou@!

[o1ka, 2016-05-10 11:40:35]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

This is a draft - no unit tests yet. If the reviewers are fine with the
approach, I'll proceed with unit tests.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
*HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests (both updated)
==========

[o1ka, 2016-05-10 11:51:57]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests (both updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[o1ka, 2016-05-10 14:42:38]

Patchset #3 (id:60001) has been deleted

[o1ka, 2016-05-10 14:42:52]

Patchset #3 (id:80001) has been deleted

[o1ka, 2016-05-10 15:51:14]

PTAL - added unit tests, did some cleanup and bugfix.

[o1ka, 2016-05-11 09:14:38]

Patchset #4 (id:120001) has been deleted

[o1ka, 2016-05-11 09:52:42]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[o1ka, 2016-05-11 10:07:58]

olka@chromium.org changed reviewers:
+ chcunningham@chromium.org, miu@chromium.org

[o1ka, 2016-05-11 10:07:58]

Updating the reviewers since dalecurtis@ is OOO:
miu@ - PTAL at content and media/audio (and preferably the rest),
chcunningham@ - PTAL at media,
guidou@ - PTAL at renderer/pipeline change (removing HW config for bug 536843),
grunell@ - fyi.

Thanks!

[o1ka, 2016-05-11 10:17:19]

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Need to fix the copyright

[Guido Urdaneta, 2016-05-11 10:49:29]

https://codereview.chromium.org/1942803002/diff/140001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/140001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:347: if (!expecting_config_changes_ ||
!hw_params.IsValid() ||
Should we also check if GetOutputDeviceInfo().device_status() != OK?

[o1ka, 2016-05-11 11:47:10]

https://codereview.chromium.org/1942803002/diff/140001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/140001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:347: if (!expecting_config_changes_ ||
!hw_params.IsValid() ||
On 2016/05/11 10:49:29, Guido Urdaneta wrote:
> Should we also check if GetOutputDeviceInfo().device_status() != OK?

Good question. If the status is not OK, we won't get any render callbacks, so
the fact that we don't check it is not a problem; it makes sense to skip the
initialization though. I'll leave that for a follow-up CL which will remove HW
config usage everywhere, as we discussed.

[o1ka, 2016-05-11 11:51:29]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The plan is to remove the rest of AudioHardwareConfig usage across the code
in a follow-up CL.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[chcunningham, 2016-05-12 19:56:32]

Nice change :). First pass of comments - I need to spend a little more time
looking at this still.

The cl comment says the cache has the sink returned for re-use, but in practice
it seems the cache just deletes the returned sink immediately. Is there a plan
for re-use down the road? If not, it may be simpler just to use the cache for
getting sinks and let sink reference holders manage deletion themselves. This
would also simplify the concerns around mixers_ lifetime vs sink_cache_
lifetime.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:53: ? sink_cache_
Can you make this call AudioRendererMixerManager::GetOutputDeviceInfo?

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:121: // |sink_cache_|
will outlive any mixers, so it's fine to use Unretained().
I'm not sure sink_cache_ really will outlive mixers_. sink_cache_ gets deleted
with this class, but the destructor calls out that mixers_ may leak (be non
empty) as not all reference holders will have released their reference.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:17:
AudioRendererSinkCache::CreateDefault() {
You named MixerManager's method ::Create() (no Default) - should these names
match?

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:187: // scheduled for
deletion get aquired and released before scheduled deletion -
s/get/got/

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:192:
media::AudioRendererSink* AudioRendererSinkCacheImpl::InsertNewSink(
I'm used to seeing such methods named *_Locked - not sure if this is a chrome
thing or just a media thing. I like that style if you're ok with it.

[o1ka, 2016-05-12 21:51:13]

On 2016/05/12 19:56:32, chcunningham wrote:
> Nice change :). First pass of comments - I need to spend a little more time
> looking at this still.
> 
> The cl comment says the cache has the sink returned for re-use, but in
practice
> it seems the cache just deletes the returned sink immediately. Is there a plan
> for re-use down the road? If not, it may be simpler just to use the cache for
> getting sinks and let sink reference holders manage deletion themselves. This
> would also simplify the concerns around mixers_ lifetime vs sink_cache_
> lifetime.
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> File content/renderer/media/audio_renderer_mixer_manager.cc (right):
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> content/renderer/media/audio_renderer_mixer_manager.cc:53: ? sink_cache_
> Can you make this call AudioRendererMixerManager::GetOutputDeviceInfo?
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> content/renderer/media/audio_renderer_mixer_manager.cc:121: // |sink_cache_|
> will outlive any mixers, so it's fine to use Unretained().
> I'm not sure sink_cache_ really will outlive mixers_. sink_cache_ gets deleted
> with this class, but the destructor calls out that mixers_ may leak (be non
> empty) as not all reference holders will have released their reference.
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> File content/renderer/media/audio_renderer_sink_cache.cc (right):
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache.cc:17:
> AudioRendererSinkCache::CreateDefault() {
> You named MixerManager's method ::Create() (no Default) - should these names
> match?
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache.cc:187: // scheduled for
> deletion get aquired and released before scheduled deletion -
> s/get/got/
> 
>
https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache.cc:192:
> media::AudioRendererSink* AudioRendererSinkCacheImpl::InsertNewSink(
> I'm used to seeing such methods named *_Locked - not sure if this is a chrome
> thing or just a media thing. I like that style if you're ok with it.

Thank you, a quick reply to your question so far:
Re-use of a sink happens if it was created to obtain output device parameters
(it lives for 5 seconds after that and is reused in quite a lot of cases: it's
very common for audio rendering to first request output parameters and then
create a sink, it happens for a number of reasons). At this point I don't want
to reuse sinks returned after a real usage by clients, since their health is
questionable :) (at this point a stopped sink can't be reinitialized) - we'll
probably look into it later when sinks become reinitializable. I intentionally
wanted to avoid passing sink ownership to mixers, because cache uses all the
existing sinks to look up output device info (and then creates a new sink only
if no existing match is found), and dual ownership makes it a bit awkward.
Thanks for pointing out the mixer lifetime issue, I need to take a look at it
and probably revisit sink ownership (or mixer lifetime, we'll see).

[miu, 2016-05-12 21:53:07]

First round of comments (on Patch Set 4):

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:143: RenderThreadImpl*
render_thread = RenderThreadImpl::current();
Not sure if this happens in any unit tests, but what if
RenderThreadImpl::current() returns nullptr? Perhaps a DCHECK(render_thread)
should go here (which indicates to any unit test authors that they need to make
sure a RenderThreadImpl is instantiated).

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:29: :
sink_cache_(std::move(sink_cache)) {}
nit: Looks like all the code assumes |sink_cache_| is never nullptr. Please add
a DCHECK(sink_cache_) in the ctor body (self-documenting code FTW!).

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:149:
std::unique_ptr<AudioRendererSinkCache> sink_cache_;
Please make this const, since it is not mutated any time after construction.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:45: typedef
base::Callback<media::AudioRendererSink*(
nit: Consider using the more-readable C++11 "using" syntax instead of typedefs:

  using GetSinkCallback = base::Callback<...>(...);
  using ReleaseSinkCallback = ...;

...and elsewhere throughout this change.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
To be consistent with how this is done for other impl classes in Chromium code,
this file should be named audio_renderer_sink_cache_impl.cc. It contains the
entire Impl class implementation (and it's okay for it to also contain
AudioRendererSinkCache::CreateDefault()). This is similar to how RenderFrame vs.
RenderFrameImpl code is organized.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:31: for (auto& iter :
sinks_)
Consider (for safety):

  sinks_lock_.AssertNotHeld();

Actually, is a lock necessary? Should this class be thread-safe? I see that
AudioRenderMixerManager does some locking too, but its public methods all seem
to be invoked on the main renderer thread as well.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:58:
std::make_pair(SinkKey(source_render_frame_id, device_info.device_id(),
Does session_id need to be in the SinkKey?

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:123:
media::AudioRendererSink* sink = InsertNewSink(key, true);
Should there be an upper-bound, where we decide too many sinks have been created
without enough of them being released?

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:158: if
(iter->second.sink.get() == sink) {  // Sink is found.
nit: Invert the if-statement expression so the ~25 LOC don't need to be
indented.

Meaning:

    if (it->second.sink != sink)
      continue;
    ...rest of code as-is...

There are a few other places in this change where you might consider doing this
too...

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:192:
media::AudioRendererSink* AudioRendererSinkCacheImpl::InsertNewSink(
On 2016/05/12 19:56:31, chcunningham wrote:
> I'm used to seeing such methods named *_Locked - not sure if this is a chrome
> thing or just a media thing. I like that style if you're ok with it. 

I've also seen people name methods like this: InsertNewSinkWhileLockHeld()

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:10: #include
"base/memory/weak_ptr.h"
This include doesn't seem to be used.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:13: #include "url/origin.h"
nit: Chromium code takes the opposite stance from the Google C++ style guide in
that url::Origin should just be forward-declared, rather than its header file
#included. (Then, #include the header in the .cc file.)

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:24: class CONTENT_EXPORT
AudioRendererSinkCache {
naming nit: This feels more like a "pool" than a "cache" since this class is
managing the lifecycle [including delayed deletion] of the ARS objects.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:100: typedef
std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>
1. If the number of sinks is small, consider using std::vector<SinkEntry>
instead, where SinkEntry is just the members of three structs merged together:
SinkKey, SinkKeyCompare (a.k.a. operator<()), and AudioRendererSinkReference.
This would be more memory-efficient and reduce indirection. FWIW, the multimap
is only being used for the convenience of an equal_range() iterator in
GetSink(), but then DeleteSink() performs a full linear scan anyway.

2. These structs are only used as template parameters for the STL containers,
and in the .cc file. Therefore, they should just be forward-declared here, with
the full class declaration in the .cc file. In other words, try to put as much
of the implementation in the .cc file as possible. For example, in the .h file:

  class AudioRendererSinkCacheImpl {
    ...
   private:
    struct SinkEntry;
    ...
  };

...and in the .cc file:

  struct AudioRendererSinkCacheImpl::SinkEntry {
    int source_render_frame_id;
    std::string device_id;
    url::Origin security_origin;
    scoped_refptr<media::AudioRendererSink> sink;
    bool used;

    bool operator<(const SinkEntry& other) const {...}
  };

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:119:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Please make this const.

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

https://codereview.chromium.org/1942803002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:70: AudioRendererSink* audio_sink_;
Please make this const: AudioRendererSink* const audio_sink_;

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

https://codereview.chromium.org/1942803002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:50: AudioRendererMixerInput(const
GetMixerCB& get_mixer_cb,
Hmm...This is now the third callback being injected, which is starting to abuse
the main purpose of Callbacks (to invoke unknown external implementation after
an asynchronous operation completes). Consider replacing these with an interface
like:

  class MixerPool {
    virtual AudioRendererMixer* AcquireMixer(...) = 0;
    virtual void ReleaseMixer(...) = 0;
    virtual OutputDeviceInfo GetDeviceInfo(...) = 0;
  };

...and the constructor becomes:

  AudioRendererMixerInput(MixerPool* pool,
                          const std::string& device_id,
                          const url::Origin& security_origin);

The reason having all these callbacks is messy is because it becomes harder to
reason about object lifetimes and ownership/encapsulation of functionality.
You'd have to look for where each Callback object is being created and how
base::Bind() was used, and whether it calls via a raw, ref-counted, or weak
pointer. Also, there are subtle "surprises" in how copies of base::Callback
objects can invalidate the original (e.g., when bound with move types like
std::unique_ptr<>).

A teammate of mine recently did a big clean-up like this (reducing an
11-argument constructor down to 4 arguments!), and we actually resolved a few
bugs once the object graph became clearer:
https://codereview.chromium.org/1878883003/ and
https://codereview.chromium.org/1515023002/

[o1ka, 2016-05-17 16:52:41]

Patchset #5 (id:160001) has been deleted

[o1ka, 2016-05-17 17:06:22]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - owns sinks and manages their lifetime.
* AudioRendererMixer does NOT own a sink any more, it borrows a sink from the
cache and then returns it back.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The plan is to remove the rest of AudioHardwareConfig usage across the code
in a follow-up CL.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code in a follow-up
CL.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[o1ka, 2016-05-17 17:06:52]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code in a follow-up
CL.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[o1ka, 2016-05-17 17:07:46]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[o1ka, 2016-05-17 17:17:24]

Thanks everyone for the comments.

I've changed things a little: now the cache does not actually own sinks, but
holds references to them instead. This will allow to give out sinks from it to
other rendering clients, not only mixer manager (this is now a TODO, see the CL
description). It also keeps lifetime requirements for the sinks/mixers/mixer
inputs unchanged from as it is now, which is a poor excuse, but I want to be on
the safe side here.

I've got also a couple of questions to you as well.
Thanks!

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:143: RenderThreadImpl*
render_thread = RenderThreadImpl::current();
On 2016/05/12 21:53:06, miu wrote:
> Not sure if this happens in any unit tests, but what if
> RenderThreadImpl::current() returns nullptr? Perhaps a DCHECK(render_thread)
> should go here (which indicates to any unit test authors that they need to
make
> sure a RenderThreadImpl is instantiated).

Done.
Now it does not happen in unit tests (and I hope it will never happen because
will be properly mocked).

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:29: :
sink_cache_(std::move(sink_cache)) {}
On 2016/05/12 21:53:06, miu wrote:
> nit: Looks like all the code assumes |sink_cache_| is never nullptr. Please
add
> a DCHECK(sink_cache_) in the ctor body (self-documenting code FTW!).

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:53: ? sink_cache_
On 2016/05/12 19:56:31, chcunningham wrote:
> Can you make this call AudioRendererMixerManager::GetOutputDeviceInfo?

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:121: // |sink_cache_|
will outlive any mixers, so it's fine to use Unretained().
On 2016/05/12 19:56:31, chcunningham wrote:
> I'm not sure sink_cache_ really will outlive mixers_. sink_cache_ gets deleted
> with this class, but the destructor calls out that mixers_ may leak (be non
> empty) as not all reference holders will have released their reference. 

I made some changes around that, mostly because if cache returns scoped_refptr
instead of a raw pointer, we can use it to get sinks for all the rendering
clients, not only mixers. I also made mixer manager to notify the cache to
release mixer's sink, because mixer manager is the only one who destroys mixers.
Now object lifetime requirements are the same as in the original code.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:149:
std::unique_ptr<AudioRendererSinkCache> sink_cache_;
On 2016/05/12 21:53:06, miu wrote:
> Please make this const, since it is not mutated any time after construction.

Done.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:45: typedef
base::Callback<media::AudioRendererSink*(
On 2016/05/12 21:53:06, miu wrote:
> nit: Consider using the more-readable C++11 "using" syntax instead of
typedefs:
> 
>   using GetSinkCallback = base::Callback<...>(...);
>   using ReleaseSinkCallback = ...;
> 
> ...and elsewhere throughout this change.

Done.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/05/12 21:53:06, miu wrote:
> To be consistent with how this is done for other impl classes in Chromium
code,
> this file should be named audio_renderer_sink_cache_impl.cc. It contains the
> entire Impl class implementation (and it's okay for it to also contain
> AudioRendererSinkCache::CreateDefault()). This is similar to how RenderFrame
vs.
> RenderFrameImpl code is organized.

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:17:
AudioRendererSinkCache::CreateDefault() {
On 2016/05/12 19:56:31, chcunningham wrote:
> You named MixerManager's method ::Create() (no Default) - should these names
> match?

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:31: for (auto& iter :
sinks_)
On 2016/05/12 21:53:06, miu wrote:
> Consider (for safety):
> 
>   sinks_lock_.AssertNotHeld();
> 
> Actually, is a lock necessary? Should this class be thread-safe? I see that
> AudioRenderMixerManager does some locking too, but its public methods all seem
> to be invoked on the main renderer thread as well.

AFAIK there is no AssertNotHeld() in chromium, only Try(), which behavior is
undefined if called on a thread holding the lock (
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...)

Re: thread-safety: Unfortunately not, as well as mixers. Mixer inputs (and
mixers and their sinks, as a result) are accessed from
WebAudioSourceProviderImpl, which methods run on both media thread (invoked by
audio renderer pipeline) and main thread (invoked by HTMLElement for output
client switch). This is something which needs fixing for sure, but is a bit out
of scope of our "mix all" effort.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:58:
std::make_pair(SinkKey(source_render_frame_id, device_info.device_id(),
On 2016/05/12 21:53:06, miu wrote:
> Does session_id need to be in the SinkKey?

No. Session_id is a mapping between an open input device to an associated output
device and is unique for an input device session. The actual device id is
returned by browser side on AOD creation, and we use that as a part of the key.
This allows for better reuse of sinks.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:123:
media::AudioRendererSink* sink = InsertNewSink(key, true);
On 2016/05/12 21:53:06, miu wrote:
> Should there be an upper-bound, where we decide too many sinks have been
created
> without enough of them being released?

No, not for now at least. Temporary sinks created for GetOutputDeviceInfo() are
deleted if not reused after a timeout; and we can't prevent creating new sinks
(+AudioOutputDevices) requested by clients. There is no any limit for a number
of AOD now (before caching), and we don't want to introduce it at this point.
But agree that caching does enable a possibility to track and limit that number
and we may want to introduce a limit on some platforms at some point.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:158: if
(iter->second.sink.get() == sink) {  // Sink is found.
On 2016/05/12 21:53:06, miu wrote:
> nit: Invert the if-statement expression so the ~25 LOC don't need to be
> indented.
> 
> Meaning:
> 
>     if (it->second.sink != sink)
>       continue;
>     ...rest of code as-is...
> 
> There are a few other places in this change where you might consider doing
this
> too...

Done. (I hope I did not miss anything)

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:187: // scheduled for
deletion get aquired and released before scheduled deletion -
On 2016/05/12 19:56:31, chcunningham wrote:
> s/get/got/

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.cc:192:
media::AudioRendererSink* AudioRendererSinkCacheImpl::InsertNewSink(
On 2016/05/12 21:53:06, miu wrote:
> On 2016/05/12 19:56:31, chcunningham wrote:
> > I'm used to seeing such methods named *_Locked - not sure if this is a
chrome
> > thing or just a media thing. I like that style if you're ok with it. 
> 
> I've also seen people name methods like this: InsertNewSinkWhileLockHeld()

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:10: #include
"base/memory/weak_ptr.h"
On 2016/05/12 21:53:06, miu wrote:
> This include doesn't seem to be used.

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:13: #include "url/origin.h"
On 2016/05/12 21:53:06, miu wrote:
> nit: Chromium code takes the opposite stance from the Google C++ style guide
in
> that url::Origin should just be forward-declared, rather than its header file
> #included. (Then, #include the header in the .cc file.)

Done.

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:24: class CONTENT_EXPORT
AudioRendererSinkCache {
On 2016/05/12 21:53:06, miu wrote:
> naming nit: This feels more like a "pool" than a "cache" since this class is
> managing the lifecycle [including delayed deletion] of the ARS objects.

Now it does not quite manage the lifetime any more. I also thought of it as a
cache in terms of hits/misses when getting a sink/OutputDeviceInfo

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:100: typedef
std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>
On 2016/05/12 21:53:06, miu wrote:
> 1. If the number of sinks is small, consider using std::vector<SinkEntry>
> instead, where SinkEntry is just the members of three structs merged together:
> SinkKey, SinkKeyCompare (a.k.a. operator<()), and AudioRendererSinkReference.
> This would be more memory-efficient and reduce indirection. FWIW, the multimap
> is only being used for the convenience of an equal_range() iterator in
> GetSink(), but then DeleteSink() performs a full linear scan anyway.
> 
> 2. These structs are only used as template parameters for the STL containers,
> and in the .cc file. Therefore, they should just be forward-declared here,
with
> the full class declaration in the .cc file. In other words, try to put as much
> of the implementation in the .cc file as possible. For example, in the .h
file:
> 
>   class AudioRendererSinkCacheImpl {
>     ...
>    private:
>     struct SinkEntry;
>     ...
>   };
> 
> ...and in the .cc file:
> 
>   struct AudioRendererSinkCacheImpl::SinkEntry {
>     int source_render_frame_id;
>     std::string device_id;
>     url::Origin security_origin;
>     scoped_refptr<media::AudioRendererSink> sink;
>     bool used;
> 
>     bool operator<(const SinkEntry& other) const {...}
>   };

1. multimap is also used for find() in GetSinkInfo, which can happen multiple
times per sink (opposite to DeleteSink which happens only once). Overall, if the
number of elements is small, neither performance no memory matter much, and I do
not have good enough vision of how big it can be in some extreme cases. Also,
map interface looks clearer to me. I modified DeleteSink to accept the key as
well. WDYT?
2. My understanding is that incomplete types are not allowed in STL containers
by the standard (and the code does not compile), and I tried but haven't found a
proof of the opposite. Could you point me to the info you have on that?

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:119:
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2016/05/12 21:53:06, miu wrote:
> Please make this const.

Done.

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

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2016/05/11 10:17:19, o1ka wrote:
> Need to fix the copyright

Done.

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

https://codereview.chromium.org/1942803002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:70: AudioRendererSink* audio_sink_;
On 2016/05/12 21:53:06, miu wrote:
> Please make this const: AudioRendererSink* const audio_sink_;

Done.

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

https://codereview.chromium.org/1942803002/diff/140001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:50: AudioRendererMixerInput(const
GetMixerCB& get_mixer_cb,
On 2016/05/12 21:53:06, miu wrote:
> Hmm...This is now the third callback being injected, which is starting to
abuse
> the main purpose of Callbacks (to invoke unknown external implementation after
> an asynchronous operation completes). Consider replacing these with an
interface
> like:
> 
>   class MixerPool {
>     virtual AudioRendererMixer* AcquireMixer(...) = 0;
>     virtual void ReleaseMixer(...) = 0;
>     virtual OutputDeviceInfo GetDeviceInfo(...) = 0;
>   };
> 
> ...and the constructor becomes:
> 
>   AudioRendererMixerInput(MixerPool* pool,
>                           const std::string& device_id,
>                           const url::Origin& security_origin);
> 
> The reason having all these callbacks is messy is because it becomes harder to
> reason about object lifetimes and ownership/encapsulation of functionality.
> You'd have to look for where each Callback object is being created and how
> base::Bind() was used, and whether it calls via a raw, ref-counted, or weak
> pointer. Also, there are subtle "surprises" in how copies of base::Callback
> objects can invalidate the original (e.g., when bound with move types like
> std::unique_ptr<>).
> 
> A teammate of mine recently did a big clean-up like this (reducing an
> 11-argument constructor down to 4 arguments!), and we actually resolved a few
> bugs once the object graph became clearer:
> https://codereview.chromium.org/1878883003/ and
> https://codereview.chromium.org/1515023002/

Done. (I actually was going to do that at some point, but then forgot about it
:) )

[o1ka, 2016-05-17 17:37:57]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Switch other clients to get sinks through the cache (basically it means that
the cache will work as AudioDeviceFactory, and they both probably should be
merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it
means that the cache will work as AudioDeviceFactory, and they both probably
should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[DaleCurtis, 2016-05-17 19:13:48]

Why is there both a sink pool and a cache? I'm also not a fan of all the virtual
method usage. Instead of adding fakes, what is preventing you from just using
the actual cache in various places?

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:140: // The mixer will be
deleted now, so releasing the sink.
s/releasing/release/

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1509:
audio_renderer_mixer_manager_.reset(
just == ?

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:20: scoped_refptr<media::AudioRendererSink>
sink)
Ditto.

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:30: scoped_refptr<media::AudioRendererSink>
sink);
Remove media:: this is already in media. Ditto below.

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: const AudioRendererSink* sink_ptr() {
return audio_sink_.get(); };
Needed? for_testing?

[o1ka, 2016-05-17 19:28:23]

On 2016/05/17 19:13:48, DaleCurtis wrote:
> Why is there both a sink pool and a cache? I'm also not a fan of all the
virtual
> method usage. Instead of adding fakes, what is preventing you from just using
> the actual cache in various places?
> 
It's a mixer pool, not a sink pool. It's a substitution of a set of callbacks
bound to ARMM.
(ARMM is in context, and ARMIs are in media). 
And sink cache interface is convenient for mocking. And mocking is convenient
since the code is distributed between content and media.
And I'm a fan of virtual methods :)

[o1ka, 2016-05-18 11:55:36]

Patchset #6 (id:190001) has been deleted

[o1ka, 2016-05-18 13:15:12]

Addressed Dale's comments.

More specific answer to the question:

> Why is there both a sink pool and a cache? I'm also not a fan of all the
virtual
> method usage. Instead of adding fakes, what is preventing you from just using
> the actual cache in various places?

ARSinkCache interface provides a possibility for mocking in ARMM unit test, for
example. Thus we can have mock sinks provided to mixers and set expectations on
them. Using ARSinkCacheImpl directly won't allow that without having "for
testing" logic in the cache, which generally looks wrong.
Overall, my opinion is that we should mock as much as possible for a unit test,
because mocks allow to verify side effects of operations.

ARMixerPool is just a substitution of a set of callbacks
bound to ARMM, those used to be passed to ARMI constructor. Having it makes the
fact that ARMI uses the pointer to ARMM more obvious and interface more clean
and less error-prone (and media::ARMI can't use context::ARMM directly).

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:140: // The mixer will be
deleted now, so releasing the sink.
On 2016/05/17 19:13:48, DaleCurtis wrote:
> s/releasing/release/

Done.

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:1509:
audio_renderer_mixer_manager_.reset(
On 2016/05/17 19:13:48, DaleCurtis wrote:
> just == ?

Of cause, thank you!

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:20: scoped_refptr<media::AudioRendererSink>
sink)
On 2016/05/17 19:13:48, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:30: scoped_refptr<media::AudioRendererSink>
sink);
On 2016/05/17 19:13:48, DaleCurtis wrote:
> Remove media:: this is already in media. Ditto below.

Done.

https://codereview.chromium.org/1942803002/diff/180001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: const AudioRendererSink* sink_ptr() {
return audio_sink_.get(); };
On 2016/05/17 19:13:48, DaleCurtis wrote:
> Needed? for_testing?

Needed, I put it into the comment.
Maybe you have an idea on how to make it cleaner? (Having mixer to call a
callback to release a sink or storing mixer sink pointers in ARMM does not look
good to me)

[miu, 2016-05-18 22:37:01]

FYI--Will get to this first thing Thursday (demo'ing at IO today).

[miu, 2016-05-19 22:27:16]

Comments on Patch Set 6:

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:100: typedef
std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>
On 2016/05/17 17:17:24, o1ka wrote:
> 1. multimap is also used for find() in GetSinkInfo, which can happen multiple
> times per sink (opposite to DeleteSink which happens only once). Overall, if
the
> number of elements is small, neither performance no memory matter much, and I
do
> not have good enough vision of how big it can be in some extreme cases. Also,
> map interface looks clearer to me. I modified DeleteSink to accept the key as
> well. WDYT?

Well, okay. I'll admit I'm "bike-shedding" a little here. If you prefer the
multimap, I'm fine with that.

As for requiring the key for DeleteSink() method: IMHO, it's just extra data
having to be stored and passed around; and it isn't really necessary to execute
a simple, efficient delete operation. So, I like the solution you had before
better. ;-)

> 2. My understanding is that incomplete types are not allowed in STL containers
> by the standard (and the code does not compile), and I tried but haven't found
a
> proof of the opposite. Could you point me to the info you have on that?

IIRC, the type only needs to be complete where the template would be
instantiated. There's probably something in the header file that is causing the
template to be instantiated (i.e., when compiling other modules that include
this header file). It's not a huge deal, so don't go to any trouble here if it's
not obvious what's causing the compile error.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:44: // base::Unretained()
is safe since AudioRendererMixerManager lives on the
This comment is obsolete now.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:88:
AudioRendererMixerManager(std::unique_ptr<AudioRendererSinkCache> sink_cache);
Need 'explicit' keyword for 1-arg ctors.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:54: virtual
~AudioRendererSinkCache() {}
style nit: dtors are usually near the top. This one should probably go  after
the Create() function decl.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:15: enum {
kDeleteTimeoutMs = 5000 };
As of a few weeks ago, they've just started allowing the use of constexpr in
Chromium code. This can now become:

  constexpr int kDeleteTimeoutMs = 5000;

(and no need for anonymous namespace here since constexpr's are not visible
outside the module)

BTW--In the near future, we're going to try to make base::TimeDelta and friends
constexpr literals so eventually this LOC could be simply:

  constexpr base::TimeDelta kDeleteTimeout = ...;

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:32: :
delete_timeout_ms_(delete_timeout_ms),
nit: For consistency/readability, try to maintain the same order of ctor args
with data members.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:64: base::AutoLock
auto_lock(sinks_lock_);
Try to minimize how long this lock is held (here and elsewhere in this file).
For example, it seems the code here should be:

  {
    base::AutoLock auto_lock(sinks_lock_);
    sinks_.insert(...);
  }
  DeleteLaterIfUnused(key, sink.get());
  return device_info;

Actually, the lines above are almost identical to the other code path for this
method. Suggested refactoring for this method:

{
  SinkKey key;
  scoped_refptr<media::AudioRendererSink> sink;
  if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(...)) {
    const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
    key = SinkKey(source_render_frame_id, device_info.device_id(),
security_origin);
    sink = create_sink_cb_.Run(...);
  } else {
    key = SinkKey(source_render_frame_id, device_id, security_origin);
    {
      base::AutoLock auto_lock(sinks_lock_);
      auto sink_iter = sinks_.find(key);
      if (sink_iter != sinks_.end())
        return sink_iter->second.sink->GetOutputDeviceInfo();
    }

    // No matching sink is found, create one and cache it.
    sink = create_sink_cb_.Run(...);
  }

  {
    base::AutoLock auto_lock(sinks_lock_);
    sinks_.insert(std::make_pair(
        key, AudioRendererSinkReference(sink, false /*not in use*/)));
  }
  DeleteLaterIfUnused(key, sink.get());
  return sink->GetOutputDeviceInfo();
}

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:147: // We don't know
the sink state, so won't reused it. Delete it immediately.
typo: s/reused/reuse/

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:5: #include
"content/renderer/media/audio_renderer_sink_cache.h"
Need to surround the contents of this header with:

#ifndef CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_
#define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_

...

#endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:32: int
delete_timeout_ms);
Prefer using base::TimeDelta to pass time durations, rather than POD integer
values.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:68: bool
operator()(const SinkKey& a, const SinkKey& b) const {
Along the lines of my comment from last round: You might get rid of this struct,
and instead declare an operator<() method in SinkKey, and then define the method
in the .cc file. This would put more of the implementation in the .cc file
instead of here in the header file.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:119: // For example: (1)
sink was create and cached in GetSinkInfo(), and then (2)
typo: s/create/created/

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:124: const int
delete_timeout_ms_;
ditto: Prefer using base::TimeDelta for time durations, rather than POD integer
values.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:264: // Check that
a sink created on one thread in responce to GetSinkInfo can be
s/responce/response/

https://codereview.chromium.org/1942803002/diff/210001/media/audio/clockless_...
File media/audio/clockless_audio_sink.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/audio/clockless_...
media/audio/clockless_audio_sink.h:54: OutputDeviceInfo device_info_;
const OutputDeviceInfo, please.

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:18:
AudioRendererMixer::AudioRendererMixer(const AudioParameters& output_params,
nit: The order of args is opposite the order of the initialized data members. If
this is easy to fix (i.e., there aren't a ton of ctor calls to fix all over the
codebase), then please do.

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:20: : audio_sink_(sink),
std::move(sink)

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_input.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:82: const int source_render_frame_id_;
Dale may have already mentioned this, but "render frame ID" is a concept that
should not be known to src/media code (since src/media code cannot depend on
src/content code). I'm afraid you'll have to find another way to plumb this
through. Suggestion: Can the object that implements AudioRendererMixerPool store
the |source_render_frame_id_| instead?

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:35: virtual void ReturnMixer(int
source_render_frame_id,
Are all these arguments necessary to return a mixer? Could the client instead
just provide the pointer? Meaning:

  AudioRendererMixer* mixer = pool->GetMixer(...);
  ...use mixer...
  pool->ReturnMixer(mixer);
  mixer = nullptr;

[o1ka, 2016-05-20 10:40:33]

Thanks!
Before going through another round, I'd like us to reach an agreement on a
couple of things (see the comments).

**Dale**:

Which version of void AudioRendererSinkCache::ReleaseSink() you prefer:

a) AudioRendererSinkCache::ReleaseSink(const media::AudioRendererSink* sink)
-iterates through all the sinks to find a matching pointer.

b)AudioRendererSinkCache::ReleaseSink(
     int source_render_frame_id,
     const std::string& device_id,
     const url::Origin& security_origin,
     const media::AudioRendererSink* sink)
- gets a range of sinks with the same frame id, device id, origin from the map,
and looks up a matching pointer in that range.

a) looks like a cleaner interface,
b) is faster if we are at ~20+ unique combinations of (frame id, device id,
origin).

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/140001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:100: typedef
std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>
On 2016/05/19 22:27:14, miu wrote:
> On 2016/05/17 17:17:24, o1ka wrote:
> > 1. multimap is also used for find() in GetSinkInfo, which can happen
multiple
> > times per sink (opposite to DeleteSink which happens only once). Overall, if
> the
> > number of elements is small, neither performance no memory matter much, and
I
> do
> > not have good enough vision of how big it can be in some extreme cases.
Also,
> > map interface looks clearer to me. I modified DeleteSink to accept the key
as
> > well. WDYT?
> 
> Well, okay. I'll admit I'm "bike-shedding" a little here. If you prefer the
> multimap, I'm fine with that.
> 
> As for requiring the key for DeleteSink() method: IMHO, it's just extra data
> having to be stored and passed around; and it isn't really necessary to
execute
> a simple, efficient delete operation. So, I like the solution you had before
> better. ;-)
> 

Ok. But before reverting this to the first version I'd like to make sure Dale is
also happy with that.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:5: #include
"content/renderer/media/audio_renderer_sink_cache.h"
On 2016/05/19 22:27:15, miu wrote:
> Need to surround the contents of this header with:
> 
> #ifndef CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_
> #define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_
> 
> ...
> 
> #endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_

Ooops

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:32: int
delete_timeout_ms);
On 2016/05/19 22:27:15, miu wrote:
> Prefer using base::TimeDelta to pass time durations, rather than POD integer
> values.

Could you suggest why it's more preferable?

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_input.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:82: const int source_render_frame_id_;
On 2016/05/19 22:27:16, miu wrote:
> Dale may have already mentioned this, but "render frame ID" is a concept that
> should not be known to src/media code (since src/media code cannot depend on
> src/content code). I'm afraid you'll have to find another way to plumb this
> through. Suggestion: Can the object that implements AudioRendererMixerPool
store
> the |source_render_frame_id_| instead?

...Well, "render frame ID" hadn't been known to mixer inputs util a colleague of
mine proposed passing it in the constructor. :)

"Hmm...This is now the third callback being injected, which is starting to abuse
the main purpose of Callbacks (to invoke unknown external implementation after
an asynchronous operation completes). Consider replacing these with an interface
like:

  class MixerPool {
    virtual AudioRendererMixer* AcquireMixer(...) = 0;
    virtual void ReleaseMixer(...) = 0;
    virtual OutputDeviceInfo GetDeviceInfo(...) = 0;
  };

...and the constructor becomes:

  AudioRendererMixerInput(MixerPool* pool,
                          const std::string& device_id,
                          const url::Origin& security_origin);"


1) Why an innocent int named "render frame ID" means introducing a whole new
dependency of media to content? Maybe it's just a naming problem? We can call it
"owner id" or whatever (open to suggestions, I'm bad at naming).

2) No, MixerPool as it is now cannot hold render frame id, because it serves all
the mixer inputs for the renderer process and it does not keep track of them.

3) If you insist that we need to remove that int, I can, for example:
a) Store it in the mixer input as an "unnamed blob".
b) Instead of passing frame id into constructor, pass a callback returning frame
id or an "unamed blob". 
(both of the above still expose frame id in pool interface)
c) : b) + pass that callback as a parameter to MixerPool interface instead of
frame id.
d) Stand on my head (well, actually no, I cannot).
e) Return back to binding (which looks like a more natural solution to me).
g) Have a wrapper class that implements MixerPool interface, holds frame id and
redirects all calls to mixer manager (this is an extra layer of indirection
which does not add much value).
h) ?

I hope that just renaming will do though.
What would you suggest?

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:35: virtual void ReturnMixer(int
source_render_frame_id,
On 2016/05/19 22:27:16, miu wrote:
> Are all these arguments necessary to return a mixer? Could the client instead
> just provide the pointer? Meaning:
> 
>   AudioRendererMixer* mixer = pool->GetMixer(...);
>   ...use mixer...
>   pool->ReturnMixer(mixer);
>   mixer = nullptr;

Those are the key used to look-up the mixer. Just a pointer can be used as well,
but this is how it was originally done and I don't think it makes sense to
change it in this CL (and I'm not sure it's beneficial overall
performance-wise).

[o1ka, 2016-05-20 11:59:04]

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:68: bool
operator()(const SinkKey& a, const SinkKey& b) const {
On 2016/05/19 22:27:15, miu wrote:
> Along the lines of my comment from last round: You might get rid of this
struct,
> and instead declare an operator<() method in SinkKey, and then define the
method
> in the .cc file. This would put more of the implementation in the .cc file
> instead of here in the header file.

That would probably look neater, but according to Google style guide: "don't
define operator overloads just because other libraries expect them. For example,
if your type doesn't have a natural ordering, but you want to store it in a
std::set, use a custom comparator rather than overloading <".

[DaleCurtis, 2016-05-20 17:41:41]

Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for used
and one for unused, then you don't need a comparator or structure; then you can
use option a and it'll be just as fast.

[o1ka, 2016-05-20 17:58:38]

On 2016/05/20 17:41:41, DaleCurtis wrote:
> Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for used
> and one for unused, then you don't need a comparator or structure; then you
can
> use option a and it'll be just as fast.

How would I get a sink by (frame id, device id, origin) from that?

[o1ka, 2016-05-20 18:02:14]

On 2016/05/20 17:58:38, o1ka wrote:
> On 2016/05/20 17:41:41, DaleCurtis wrote:
> > Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for
used
> > and one for unused, then you don't need a comparator or structure; then you
> can
> > use option a and it'll be just as fast.
> 
> How would I get a sink by (frame id, device id, origin) from that?

Also, moving between used to unused is (log n) complexity, isn't it?

[o1ka, 2016-05-20 18:04:39]

On 2016/05/20 18:02:14, o1ka wrote:
> On 2016/05/20 17:58:38, o1ka wrote:
> > On 2016/05/20 17:41:41, DaleCurtis wrote:
> > > Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for
> used
> > > and one for unused, then you don't need a comparator or structure; then
you
> > can
> > > use option a and it'll be just as fast.
> > 
> > How would I get a sink by (frame id, device id, origin) from that?
> 
> Also, moving between used to unused is (log n) complexity, isn't it?

Overall, the fact that everybody(including me) has an opinion and advice on that
structure gives me a hit that it's really not that important :)

[DaleCurtis, 2016-05-20 18:17:37]

On 2016/05/20 at 17:58:38, olka wrote:
> On 2016/05/20 17:41:41, DaleCurtis wrote:
> > Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for
used
> > and one for unused, then you don't need a comparator or structure; then you
can
> > use option a and it'll be just as fast.
> 
> How would I get a sink by (frame id, device id, origin) from that?

Oh, sorry I was confused by the fact that you have a map and a multimap, this is
more complicated than necessary and going for speed for < 100 items is a moot
point. If you really care about speed you'll just use a vector and iterate with
sizes like that :) As a general rule if you have O(100) items always choose the
simplest structure; a vector will almost always be fastest. Here are numbers
from a recent comparison I did with 250 elements:

https://paste.googleplex.com/6454185432186880

elapsed map find/erase/insert: 15058 us
elapsed vector find/erase/insert: 8863 us

For the number of items we're talking about just using a single std::vector<>
which now includes a used bool would be simplest. You can then have the API take
whatever you want. I don't really have a preference, aesthetically option b) has
symmetry with your other methods, but a) is certainly simpler.

[chromium-reviews, 2016-05-20 19:38:08]

Thank you! Why mixer manager uses a map? (Number of sinks and mixers is
about the same, so I'd like to understand the motivation for ARMM)
On May 20, 2016 8:17 PM, <dalecurtis@chromium.org> wrote:

> On 2016/05/20 at 17:58:38, olka wrote:
> > On 2016/05/20 17:41:41, DaleCurtis wrote:
> > > Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One
> for
> used
> > > and one for unused, then you don't need a comparator or structure;
> then you
> can
> > > use option a and it'll be just as fast.
> >
> > How would I get a sink by (frame id, device id, origin) from that?
>
> Oh, sorry I was confused by the fact that you have a map and a multimap,
> this is
> more complicated than necessary and going for speed for < 100 items is a
> moot
> point. If you really care about speed you'll just use a vector and iterate
> with
> sizes like that :) As a general rule if you have O(100) items always
> choose the
> simplest structure; a vector will almost always be fastest. Here are
> numbers
> from a recent comparison I did with 250 elements:
>
> https://paste.googleplex.com/6454185432186880
>
> elapsed map find/erase/insert: 15058 us
> elapsed vector find/erase/insert: 8863 us
>
> For the number of items we're talking about just using a single
> std::vector<>
> which now includes a used bool would be simplest. You can then have the
> API take
> whatever you want. I don't really have a preference, aesthetically option
> b) has
> symmetry with your other methods, but a) is certainly simpler.
>
> https://codereview.chromium.org/1942803002/
>

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

[DaleCurtis, 2016-05-20 19:59:32]

I didn't know this when I wrote ARMM :) Also sometimes the map makes things much
simpler and your code isn't that hot, so it doesn't matter. In this case the
maps are making things more complicated, so it seems reasonable to just use a
single vector for both speed and simplicity.

[o1ka, 2016-05-23 11:12:14]

On 2016/05/20 18:17:37, DaleCurtis wrote:
> On 2016/05/20 at 17:58:38, olka wrote:
> > On 2016/05/20 17:41:41, DaleCurtis wrote:
> > > Why not just have two std::set<scoped_refptr<AudioRendererSink>>? One for
> used
> > > and one for unused, then you don't need a comparator or structure; then
you
> can
> > > use option a and it'll be just as fast.
> > 
> > How would I get a sink by (frame id, device id, origin) from that?
> 
> Oh, sorry I was confused by the fact that you have a map and a multimap, this
is
> more complicated than necessary and going for speed for < 100 items is a moot
> point. If you really care about speed you'll just use a vector and iterate
with
> sizes like that :) As a general rule if you have O(100) items always choose
the
> simplest structure; a vector will almost always be fastest. Here are numbers
> from a recent comparison I did with 250 elements:
> 
> https://paste.googleplex.com/6454185432186880
> 
> elapsed map find/erase/insert: 15058 us
> elapsed vector find/erase/insert: 8863 us
> 


For the sake of accuracy: this benchmark test uses int as a key, and key
comparison may be quite a heavy operation in real life.
Also, operations on int keys may be easier to optimize (and map is faster that
vector if optimization is off for the test).
This version https://paste.googleplex.com/5409556909785088 works ~3 times faster
on maps that on vectors for 250 elements with optimization enabled (and they
show about the same performance at ~50 elements).

[o1ka, 2016-05-23 16:16:55]

Review comments addressed to the best of my knowledge (did not get an answer to
some questions), map is replaced with a vector (score 2:1).

****miu@ - could you please answer the questions from the previous post?

****miu@, dalecurtis@ - I know you both are very busy and time difference
complicates the things as well. I appreciate you efforts reviewing this CL.
Unfortunately, it's still taking quite a lot of time (non-draft has been up for
2 weeks already), and it's blocking our further work. Let's wrap it up quickly?
Or let me know if you have no time for it, so I could adjust the reviewers.
Thank you for your help!

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:44: // base::Unretained()
is safe since AudioRendererMixerManager lives on the
On 2016/05/19 22:27:15, miu wrote:
> This comment is obsolete now.

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:88:
AudioRendererMixerManager(std::unique_ptr<AudioRendererSinkCache> sink_cache);
On 2016/05/19 22:27:15, miu wrote:
> Need 'explicit' keyword for 1-arg ctors.

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:54: virtual
~AudioRendererSinkCache() {}
On 2016/05/19 22:27:15, miu wrote:
> style nit: dtors are usually near the top. This one should probably go  after
> the Create() function decl.

I check with the style guide, and it should be the first, before any methods,
including static ones.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:15: enum {
kDeleteTimeoutMs = 5000 };
On 2016/05/19 22:27:15, miu wrote:
> As of a few weeks ago, they've just started allowing the use of constexpr in
> Chromium code. This can now become:
> 
>   constexpr int kDeleteTimeoutMs = 5000;
> 
> (and no need for anonymous namespace here since constexpr's are not visible
> outside the module)
> 
> BTW--In the near future, we're going to try to make base::TimeDelta and
friends
> constexpr literals so eventually this LOC could be simply:
> 
>   constexpr base::TimeDelta kDeleteTimeout = ...;

Thanks. Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:32: :
delete_timeout_ms_(delete_timeout_ms),
On 2016/05/19 22:27:15, miu wrote:
> nit: For consistency/readability, try to maintain the same order of ctor args
> with data members.

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:64: base::AutoLock
auto_lock(sinks_lock_);
On 2016/05/19 22:27:15, miu wrote:
> Try to minimize how long this lock is held (here and elsewhere in this file).
> For example, it seems the code here should be:
> 
>   {
>     base::AutoLock auto_lock(sinks_lock_);
>     sinks_.insert(...);
>   }
>   DeleteLaterIfUnused(key, sink.get());
>   return device_info;
> 
> Actually, the lines above are almost identical to the other code path for this
> method. Suggested refactoring for this method:
> 
> {
>   SinkKey key;
>   scoped_refptr<media::AudioRendererSink> sink;
>   if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(...)) {
>     const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
>     key = SinkKey(source_render_frame_id, device_info.device_id(),
> security_origin);
>     sink = create_sink_cb_.Run(...);
>   } else {
>     key = SinkKey(source_render_frame_id, device_id, security_origin);
>     {
>       base::AutoLock auto_lock(sinks_lock_);
>       auto sink_iter = sinks_.find(key);
>       if (sink_iter != sinks_.end())
>         return sink_iter->second.sink->GetOutputDeviceInfo();
>     }
> 
>     // No matching sink is found, create one and cache it.
>     sink = create_sink_cb_.Run(...);
>   }
> 
>   {
>     base::AutoLock auto_lock(sinks_lock_);
>     sinks_.insert(std::make_pair(
>         key, AudioRendererSinkReference(sink, false /*not in use*/)));
>   }
>   DeleteLaterIfUnused(key, sink.get());
>   return sink->GetOutputDeviceInfo();
> }

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:147: // We don't know
the sink state, so won't reused it. Delete it immediately.
On 2016/05/19 22:27:15, miu wrote:
> typo: s/reused/reuse/

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:119: // For example: (1)
sink was create and cached in GetSinkInfo(), and then (2)
On 2016/05/19 22:27:15, miu wrote:
> typo: s/create/created/

Done.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:264: // Check that
a sink created on one thread in responce to GetSinkInfo can be
On 2016/05/19 22:27:15, miu wrote:
> s/responce/response/

Done.

https://codereview.chromium.org/1942803002/diff/210001/media/audio/clockless_...
File media/audio/clockless_audio_sink.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/audio/clockless_...
media/audio/clockless_audio_sink.h:54: OutputDeviceInfo device_info_;
On 2016/05/19 22:27:15, miu wrote:
> const OutputDeviceInfo, please.

Done.

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:18:
AudioRendererMixer::AudioRendererMixer(const AudioParameters& output_params,
On 2016/05/19 22:27:15, miu wrote:
> nit: The order of args is opposite the order of the initialized data members.
If
> this is easy to fix (i.e., there aren't a ton of ctor calls to fix all over
the
> codebase), then please do.

Done (another way around)

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer.cc:20: : audio_sink_(sink),
On 2016/05/19 22:27:15, miu wrote:
> std::move(sink)

Done.

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_input.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:82: const int source_render_frame_id_;
On 2016/05/20 10:40:32, o1ka wrote:
> On 2016/05/19 22:27:16, miu wrote:
> > Dale may have already mentioned this, but "render frame ID" is a concept
that
> > should not be known to src/media code (since src/media code cannot depend on
> > src/content code). I'm afraid you'll have to find another way to plumb this
> > through. Suggestion: Can the object that implements AudioRendererMixerPool
> store
> > the |source_render_frame_id_| instead?
> 
> ...Well, "render frame ID" hadn't been known to mixer inputs util a colleague
of
> mine proposed passing it in the constructor. :)
> 
> "Hmm...This is now the third callback being injected, which is starting to
abuse
> the main purpose of Callbacks (to invoke unknown external implementation after
> an asynchronous operation completes). Consider replacing these with an
interface
> like:
> 
>   class MixerPool {
>     virtual AudioRendererMixer* AcquireMixer(...) = 0;
>     virtual void ReleaseMixer(...) = 0;
>     virtual OutputDeviceInfo GetDeviceInfo(...) = 0;
>   };
> 
> ...and the constructor becomes:
> 
>   AudioRendererMixerInput(MixerPool* pool,
>                           const std::string& device_id,
>                           const url::Origin& security_origin);"
> 
> 
> 1) Why an innocent int named "render frame ID" means introducing a whole new
> dependency of media to content? Maybe it's just a naming problem? We can call
it
> "owner id" or whatever (open to suggestions, I'm bad at naming).
> 
> 2) No, MixerPool as it is now cannot hold render frame id, because it serves
all
> the mixer inputs for the renderer process and it does not keep track of them.
> 
> 3) If you insist that we need to remove that int, I can, for example:
> a) Store it in the mixer input as an "unnamed blob".
> b) Instead of passing frame id into constructor, pass a callback returning
frame
> id or an "unamed blob". 
> (both of the above still expose frame id in pool interface)
> c) : b) + pass that callback as a parameter to MixerPool interface instead of
> frame id.
> d) Stand on my head (well, actually no, I cannot).
> e) Return back to binding (which looks like a more natural solution to me).
> g) Have a wrapper class that implements MixerPool interface, holds frame id
and
> redirects all calls to mixer manager (this is an extra layer of indirection
> which does not add much value).
> h) ?
> 
> I hope that just renaming will do though.
> What would you suggest? 

Renamed.

[DaleCurtis, 2016-05-23 18:29:09]

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:64: base::AutoLock
auto_lock(sinks_lock_);
On 2016/05/23 at 16:16:54, o1ka wrote:
> On 2016/05/19 22:27:15, miu wrote:
> > Try to minimize how long this lock is held (here and elsewhere in this
file).
> > For example, it seems the code here should be:
> > 
> >   {
> >     base::AutoLock auto_lock(sinks_lock_);
> >     sinks_.insert(...);
> >   }
> >   DeleteLaterIfUnused(key, sink.get());
> >   return device_info;
> > 
> > Actually, the lines above are almost identical to the other code path for
this
> > method. Suggested refactoring for this method:
> > 
> > {
> >   SinkKey key;
> >   scoped_refptr<media::AudioRendererSink> sink;
> >   if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(...)) {
> >     const media::OutputDeviceInfo& device_info =
sink->GetOutputDeviceInfo();
> >     key = SinkKey(source_render_frame_id, device_info.device_id(),
> > security_origin);
> >     sink = create_sink_cb_.Run(...);
> >   } else {
> >     key = SinkKey(source_render_frame_id, device_id, security_origin);
> >     {
> >       base::AutoLock auto_lock(sinks_lock_);
> >       auto sink_iter = sinks_.find(key);
> >       if (sink_iter != sinks_.end())
> >         return sink_iter->second.sink->GetOutputDeviceInfo();
> >     }
> > 
> >     // No matching sink is found, create one and cache it.
> >     sink = create_sink_cb_.Run(...);
> >   }
> > 
> >   {
> >     base::AutoLock auto_lock(sinks_lock_);
> >     sinks_.insert(std::make_pair(
> >         key, AudioRendererSinkReference(sink, false /*not in use*/)));
> >   }
> >   DeleteLaterIfUnused(key, sink.get());
> >   return sink->GetOutputDeviceInfo();
> > }
> 
> Done.

I don't think the original comment is good advice. You shouldn't cycle the lock
within a single function unless you like unexpected behavior. :) I'm fine with
scoping the lock when you only have one thing to do, but you shouldn't take the
lock multiple times within a single function without a good reason.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:33: // Helper class for
CacheEntry lookup.
Why choose a helper class vs a lambda?

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:83: :
task_runner_(task_runner),
Isn't std::move necessary now whenever you pass-by-value?

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:85:
delete_timeout_ms_(delete_timeout_ms),
store as timedelta, FromMilliseconds(...)

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:86:
weak_ptr_factory_(this) {}
If you take my suggestion below to use a RepeatingTimer instead for scrubbing,
you can drop the weak factory since it'll internally handle that.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:115: base::AutoLock
auto_lock(cache_lock_);
Instead of dropping in and out of lock state (something we know can be
expensive), why not grab it once at the top of the function and hold until
completion? None of these methods should be expensive and cycling the lock like
this makes me nervous about what other threads might be doing.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
Why schedule them individually for deletion vs having a repeating task that
scrubs them every x seconds based on some last_used_time_ inside the CacheEntry?

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:168: base::AutoLock
auto_lock(cache_lock_);
Ditto on lock once.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:79: const int
delete_timeout_ms_;
Just use a TimeDelta?

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: // Used by AudioRendererMixerManager to
remove mixer sink from the sink cache.
This isn't necessary, just have the ARMM store the sink as part of the MixerKey.

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:20: class MEDIA_EXPORT
AudioRendererMixerPool {
Needs class comment.

[o1ka, 2016-05-23 19:21:35]

Thanks! Quick reply/questions (to catch up on time difference)

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:33: // Helper class for
CacheEntry lookup.
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Why choose a helper class vs a lambda?
It's used in two places and requires a quite lot of capture.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:115: base::AutoLock
auto_lock(cache_lock_);
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Instead of dropping in and out of lock state (something we know can be
> expensive), why not grab it once at the top of the function and hold until
> completion? None of these methods should be expensive and cycling the lock
like
> this makes me nervous about what other threads might be doing.
Ok will do.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Why schedule them individually for deletion vs having a repeating task that
> scrubs them every x seconds based on some last_used_time_ inside the
CacheEntry?

Because it's quite a rare occasion when output parameters are requested and we
schedule the deletion. Why to have the timer always running if it may be needed
once on page load, for example? WDYT?

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: // Used by AudioRendererMixerManager to
remove mixer sink from the sink cache.
On 2016/05/23 18:29:08, DaleCurtis wrote:
> This isn't necessary, just have the ARMM store the sink as part of the
MixerKey.

Well, I don feel like it's any better to store it in one more place. Do you
insist?

[o1ka, 2016-05-23 19:26:27]

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/23 19:21:34, o1ka wrote:
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Why schedule them individually for deletion vs having a repeating task that
> > scrubs them every x seconds based on some last_used_time_ inside the
> CacheEntry?
> 
> Because it's quite a rare occasion when output parameters are requested and we
> schedule the deletion. Why to have the timer always running if it may be
needed
> once on page load, for example? WDYT?

(It was like that in the original draft version which you approved...)

[DaleCurtis, 2016-05-23 20:20:27]

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:33: // Helper class for
CacheEntry lookup.
On 2016/05/23 at 19:21:34, o1ka wrote:
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Why choose a helper class vs a lambda?
> It's used in two places and requires a quite lot of capture.

Hmm, looks like my original comment got eaten. I changed this to be: "Why not a
helper function like CacheEntry::iterator FindCacheEntry_Locked(...) { return
std::find_if(..., [] {...} }" I think it'll end up being a lot less boiler plate
as well as make the the calling functions clearer.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/23 at 19:26:27, o1ka wrote:
> On 2016/05/23 19:21:34, o1ka wrote:
> > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > Why schedule them individually for deletion vs having a repeating task
that
> > > scrubs them every x seconds based on some last_used_time_ inside the
> > CacheEntry?
> > 
> > Because it's quite a rare occasion when output parameters are requested and
we
> > schedule the deletion. Why to have the timer always running if it may be
needed
> > once on page load, for example? WDYT?
> 
> (It was like that in the original draft version which you approved...)

You wouldn't run the timer the whole time. You'd only Start() it when sinks need
to be deleted. If you prefer the current approach you should at least use a
base::CancelableClosure() instead of the weak factory.

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: // Used by AudioRendererMixerManager to
remove mixer sink from the sink cache.
On 2016/05/23 at 19:21:35, o1ka wrote:
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > This isn't necessary, just have the ARMM store the sink as part of the
MixerKey.
> 
> Well, I don feel like it's any better to store it in one more place. Do you
insist?

I do in this case. Can you explain why you think it's equivalent? In one
instance you're adding a new public API where as the other is a private detail
to ARMM.

[o1ka, 2016-05-24 15:00:41]

Thanks, Dale: most of the comments addressed, there are 2 left to reach an
agreement on.

miu@, chcunningham@ - friendly ping (It happened this CL has so many reviewers
because of Dale's vacation in the middle of the process - sorry for that. But I
feel like I need everybody's approval now)

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:33: // Helper class for
CacheEntry lookup.
On 2016/05/23 20:20:26, DaleCurtis wrote:
> On 2016/05/23 at 19:21:34, o1ka wrote:
> > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > Why choose a helper class vs a lambda?
> > It's used in two places and requires a quite lot of capture.
> 
> Hmm, looks like my original comment got eaten. I changed this to be: "Why not
a
> helper function like CacheEntry::iterator FindCacheEntry_Locked(...) { return
> std::find_if(..., [] {...} }" I think it'll end up being a lot less boiler
plate
> as well as make the the calling functions clearer.

Yes, this is another way to implement it. I don't sink that 17-line lambda in
std::find_if(..., [] {...}) is better than the current version, it's just
different. And I don't see a big difference in
std::find_if(cache_.begin(), cache_.end(),
          CacheEntryFinder(source_render_frame_id, device_id, security_origin,
                           false /* unused_only */));
vs
FindCacheEntry_Locked(source_render_frame_id, device_id, security_origin,
                           false /* unused_only */)
To me they are equally verbose.
I would not mind any of them when reading such a code (though I would feel like
lambdas are a bit overused :)).

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:83: :
task_runner_(task_runner),
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Isn't std::move necessary now whenever you pass-by-value?

Done.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:85:
delete_timeout_ms_(delete_timeout_ms),
On 2016/05/23 18:29:08, DaleCurtis wrote:
> store as timedelta, FromMilliseconds(...) 

Done.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:86:
weak_ptr_factory_(this) {}
On 2016/05/23 18:29:08, DaleCurtis wrote:
> If you take my suggestion below to use a RepeatingTimer instead for scrubbing,
> you can drop the weak factory since it'll internally handle that.

This is a nice suggestion. The advantage of the current code is that it's
straightforward => less opportunities to have bugs. I would prefer to leave it
as it is now, but keep in mind this approach as a possible optimization to be
done if required.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:115: base::AutoLock
auto_lock(cache_lock_);
On 2016/05/23 19:21:34, o1ka wrote:
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Instead of dropping in and out of lock state (something we know can be
> > expensive), why not grab it once at the top of the function and hold until
> > completion? None of these methods should be expensive and cycling the lock
> like
> > this makes me nervous about what other threads might be doing.
> Ok will do.

Done.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/23 20:20:26, DaleCurtis wrote:
> On 2016/05/23 at 19:26:27, o1ka wrote:
> > On 2016/05/23 19:21:34, o1ka wrote:
> > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > Why schedule them individually for deletion vs having a repeating task
> that
> > > > scrubs them every x seconds based on some last_used_time_ inside the
> > > CacheEntry?
> > > 
> > > Because it's quite a rare occasion when output parameters are requested
and
> we
> > > schedule the deletion. Why to have the timer always running if it may be
> needed
> > > once on page load, for example? WDYT?
> > 
> > (It was like that in the original draft version which you approved...)
> 
> You wouldn't run the timer the whole time. You'd only Start() it when sinks
need
> to be deleted. If you prefer the current approach you should at least use a
> base::CancelableClosure() instead of the weak factory.

I looked into CancelableCallback before implementing this. The problem is that
it can be created only on the same thread it's posted to/deleted on
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
wheres for weak pointer only dereference and invalidation are thread-bound
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
Mixer input(=>mixer) interfaces can be accessed from both media (pipeline) and
main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one of
them. And we can't pre-create all those CancelableCallback instances for the
sinks.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:168: base::AutoLock
auto_lock(cache_lock_);
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Ditto on lock once.

Done.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:79: const int
delete_timeout_ms_;
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Just use a TimeDelta?

Done.

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:51: // Used by AudioRendererMixerManager to
remove mixer sink from the sink cache.
On 2016/05/23 20:20:26, DaleCurtis wrote:
> On 2016/05/23 at 19:21:35, o1ka wrote:
> > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > This isn't necessary, just have the ARMM store the sink as part of the
> MixerKey.
> > 
> > Well, I don feel like it's any better to store it in one more place. Do you
> insist?
> 
> I do in this case. Can you explain why you think it's equivalent? In one
> instance you're adding a new public API where as the other is a private detail
> to ARMM.

Ok, the one without an interface is nicer. (added pointer to the mixer
reference)

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:20: class MEDIA_EXPORT
AudioRendererMixerPool {
On 2016/05/23 18:29:08, DaleCurtis wrote:
> Needs class comment.

Done.

[chcunningham, 2016-05-24 17:14:52]

On 2016/05/24 15:00:41, o1ka wrote:
> Thanks, Dale: most of the comments addressed, there are 2 left to reach an
> agreement on.
> 
> miu@, chcunningham@ - friendly ping (It happened this CL has so many reviewers
> because of Dale's vacation in the middle of the process - sorry for that. But
I
> feel like I need everybody's approval now)
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:33: // Helper class
for
> CacheEntry lookup.
> On 2016/05/23 20:20:26, DaleCurtis wrote:
> > On 2016/05/23 at 19:21:34, o1ka wrote:
> > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > Why choose a helper class vs a lambda?
> > > It's used in two places and requires a quite lot of capture.
> > 
> > Hmm, looks like my original comment got eaten. I changed this to be: "Why
not
> a
> > helper function like CacheEntry::iterator FindCacheEntry_Locked(...) {
return
> > std::find_if(..., [] {...} }" I think it'll end up being a lot less boiler
> plate
> > as well as make the the calling functions clearer.
> 
> Yes, this is another way to implement it. I don't sink that 17-line lambda in
> std::find_if(..., [] {...}) is better than the current version, it's just
> different. And I don't see a big difference in
> std::find_if(cache_.begin(), cache_.end(),
>           CacheEntryFinder(source_render_frame_id, device_id, security_origin,
>                            false /* unused_only */));
> vs
> FindCacheEntry_Locked(source_render_frame_id, device_id, security_origin,
>                            false /* unused_only */)
> To me they are equally verbose.
> I would not mind any of them when reading such a code (though I would feel
like
> lambdas are a bit overused :)).
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:83: :
> task_runner_(task_runner),
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Isn't std::move necessary now whenever you pass-by-value?
> 
> Done.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:85:
> delete_timeout_ms_(delete_timeout_ms),
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > store as timedelta, FromMilliseconds(...) 
> 
> Done.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:86:
> weak_ptr_factory_(this) {}
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > If you take my suggestion below to use a RepeatingTimer instead for
scrubbing,
> > you can drop the weak factory since it'll internally handle that.
> 
> This is a nice suggestion. The advantage of the current code is that it's
> straightforward => less opportunities to have bugs. I would prefer to leave it
> as it is now, but keep in mind this approach as a possible optimization to be
> done if required.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:115: base::AutoLock
> auto_lock(cache_lock_);
> On 2016/05/23 19:21:34, o1ka wrote:
> > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > Instead of dropping in and out of lock state (something we know can be
> > > expensive), why not grab it once at the top of the function and hold until
> > > completion? None of these methods should be expensive and cycling the lock
> > like
> > > this makes me nervous about what other threads might be doing.
> > Ok will do.
> 
> Done.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it
for
> deletion.
> On 2016/05/23 20:20:26, DaleCurtis wrote:
> > On 2016/05/23 at 19:26:27, o1ka wrote:
> > > On 2016/05/23 19:21:34, o1ka wrote:
> > > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > > Why schedule them individually for deletion vs having a repeating task
> > that
> > > > > scrubs them every x seconds based on some last_used_time_ inside the
> > > > CacheEntry?
> > > > 
> > > > Because it's quite a rare occasion when output parameters are requested
> and
> > we
> > > > schedule the deletion. Why to have the timer always running if it may be
> > needed
> > > > once on page load, for example? WDYT?
> > > 
> > > (It was like that in the original draft version which you approved...)
> > 
> > You wouldn't run the timer the whole time. You'd only Start() it when sinks
> need
> > to be deleted. If you prefer the current approach you should at least use a
> > base::CancelableClosure() instead of the weak factory.
> 
> I looked into CancelableCallback before implementing this. The problem is that
> it can be created only on the same thread it's posted to/deleted on
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
> wheres for weak pointer only dereference and invalidation are thread-bound
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> Mixer input(=>mixer) interfaces can be accessed from both media (pipeline) and
> main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one of
> them. And we can't pre-create all those CancelableCallback instances for the
> sinks.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:168: base::AutoLock
> auto_lock(cache_lock_);
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Ditto on lock once.
> 
> Done.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> File content/renderer/media/audio_renderer_sink_cache_impl.h (right):
> 
>
https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
> content/renderer/media/audio_renderer_sink_cache_impl.h:79: const int
> delete_timeout_ms_;
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Just use a TimeDelta?
> 
> Done.
> 
>
https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
> File media/base/audio_renderer_mixer.h (right):
> 
>
https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
> media/base/audio_renderer_mixer.h:51: // Used by AudioRendererMixerManager to
> remove mixer sink from the sink cache.
> On 2016/05/23 20:20:26, DaleCurtis wrote:
> > On 2016/05/23 at 19:21:35, o1ka wrote:
> > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > This isn't necessary, just have the ARMM store the sink as part of the
> > MixerKey.
> > > 
> > > Well, I don feel like it's any better to store it in one more place. Do
you
> > insist?
> > 
> > I do in this case. Can you explain why you think it's equivalent? In one
> > instance you're adding a new public API where as the other is a private
detail
> > to ARMM.
> 
> Ok, the one without an interface is nicer. (added pointer to the mixer
> reference)
> 
>
https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
> File media/base/audio_renderer_mixer_pool.h (right):
> 
>
https://codereview.chromium.org/1942803002/diff/230001/media/base/audio_rende...
> media/base/audio_renderer_mixer_pool.h:20: class MEDIA_EXPORT
> AudioRendererMixerPool {
> On 2016/05/23 18:29:08, DaleCurtis wrote:
> > Needs class comment.
> 
> Done.

[chcunningham, 2016-05-24 17:16:06]

Sorry for the empty message above. Meant to say: Dale's LGTM is sufficient for
me. You've resolved my initial feedback. Feel free to move me to CC

[DaleCurtis, 2016-05-24 20:41:42]

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/24 at 15:00:41, o1ka wrote:
> On 2016/05/23 20:20:26, DaleCurtis wrote:
> > On 2016/05/23 at 19:26:27, o1ka wrote:
> > > On 2016/05/23 19:21:34, o1ka wrote:
> > > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > > Why schedule them individually for deletion vs having a repeating task
> > that
> > > > > scrubs them every x seconds based on some last_used_time_ inside the
> > > > CacheEntry?
> > > > 
> > > > Because it's quite a rare occasion when output parameters are requested
and
> > we
> > > > schedule the deletion. Why to have the timer always running if it may be
> > needed
> > > > once on page load, for example? WDYT?
> > > 
> > > (It was like that in the original draft version which you approved...)
> > 
> > You wouldn't run the timer the whole time. You'd only Start() it when sinks
need
> > to be deleted. If you prefer the current approach you should at least use a
> > base::CancelableClosure() instead of the weak factory.
> 
> I looked into CancelableCallback before implementing this. The problem is that
it can be created only on the same thread it's posted to/deleted on
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
wheres for weak pointer only dereference and invalidation are thread-bound
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
Mixer input(=>mixer) interfaces can be accessed from both media (pipeline) and
main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one of
them. And we can't pre-create all those CancelableCallback instances for the
sinks.

This isn't true of WeakPtr either. You have a race right now where if there are
no WeakPtr refs outstanding and multiple threads try to create one they will
stomp on eachother. If you look at WeakPtr::GetRef() you can see that it is not
thread safe. The simplest fix is instead pre-create a weak_this_ on the right
thread that you hand out instead of using GetWeakptr(). Alternatively, make
RepeatingTimer or CancelableClosure work as suggested previously as the current
approach isn't as clear as you might think :)

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:58: return (s.device_id
== device_id_) &&
Avoid unnecessary parens.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:63: int
source_render_frame_id_;
const all these

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:67: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:80: const
base::TimeDelta& delete_timeout)
Pass base::TimeDelta by value (see time.h)

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:95: CacheEntry
cache_entry;
Worth adding a constructor to CacheEntry with some default values?

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:136: }  // if
(cache_iter != cache_.end())
It's not chrome style to annotate trailing } in conditional blocks.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:198: DVLOG(1) <<
"GetSink: address: " << cache_entry.sink.get()
I'd drop some of these DVLOGs from the production version, but up to you.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:208: // We don't know
the sink state, so won't reuse it. Delete it immediately.
Aren't we always taking this path when tearing down a sink? Is it worth any
effort to try and reuse these?

[miu, 2016-05-25 01:23:21]

Sorry for the delay. I was fighting fires and sheriffing. ;)

lgtm % one or two nits:

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:32: int
delete_timeout_ms);
On 2016/05/20 10:40:32, o1ka wrote:
> On 2016/05/19 22:27:15, miu wrote:
> > Prefer using base::TimeDelta to pass time durations, rather than POD integer
> > values.
> 
> Could you suggest why it's more preferable?

Because it let's the compiler confirm, via static type-checking, that developers
are not mishandling time values or doing incorrect time math. Also, the Chromium
time types make it easy for developers to write self-documenting code (e.g.,
TimeDelta::FromMilliseconds(...) is unmistakable).

This may sound like a small thing, but I myself have fixed a number of weird,
very time-consuming bugs in Chromium code where people used POD ints instead of
a strong time type. For example, in one case someone renamed a value from
|foo_us| to |foo_ms| and forgot to change the interpretation of the value in one
spot; and impossible-to-solve crbugs piled in for several months before the root
cause was ascertained.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:68: bool
operator()(const SinkKey& a, const SinkKey& b) const {
On 2016/05/20 11:59:03, o1ka wrote:
> On 2016/05/19 22:27:15, miu wrote:
> > Along the lines of my comment from last round: You might get rid of this
> struct,
> > and instead declare an operator<() method in SinkKey, and then define the
> method
> > in the .cc file. This would put more of the implementation in the .cc file
> > instead of here in the header file.
> 
> That would probably look neater, but according to Google style guide: "don't
> define operator overloads just because other libraries expect them. For
example,
> if your type doesn't have a natural ordering, but you want to store it in a
> std::set, use a custom comparator rather than overloading <".

Looks like this got resolved via the switch-over to std::vector.

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
File media/base/audio_renderer_mixer_input.h (right):

https://codereview.chromium.org/1942803002/diff/210001/media/base/audio_rende...
media/base/audio_renderer_mixer_input.h:82: const int source_render_frame_id_;
On 2016/05/20 10:40:32, o1ka wrote:
> 1) Why an innocent int named "render frame ID" means introducing a whole new
> dependency of media to content? Maybe it's just a naming problem? We can call
it
> "owner id" or whatever (open to suggestions, I'm bad at naming).

It's not that it is introducing a dependency. It is that "render frame" is a
concept unknown to src/media code. It's an abstraction layer violation, since
src/media code is completely independent of src/content concepts.

I'm fine with just calling it "owner_id" to mitigate this.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:95: CacheEntry
cache_entry;
On 2016/05/24 20:41:42, DaleCurtis wrote:
> Worth adding a constructor to CacheEntry with some default values?

IIRC, the uniform initializer syntax is now approved for Chromium code. Instead
of coding a constructor, you can initialize structs directly. For example:

  CacheEntry cache_entry{ source_render_frame_id, std::string(),
security_origin, nullptr, false };

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:187: CacheEntry
cache_entry;
ditto here: You could just use the uniform initializer syntax.

https://codereview.chromium.org/1942803002/diff/250001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/250001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:38: virtual void ReturnMixer(int
owner_id,
Not sure if this was already addressed in other review comments, but I'm still a
fan of just returning by a single-arg AudioRendererMixer*. Your call here.

[o1ka, 2016-05-25 12:08:06]

On 2016/05/24 17:16:06, chcunningham wrote:
> Sorry for the empty message above. Meant to say: Dale's LGTM is sufficient for
> me. You've resolved my initial feedback. Feel free to move me to CC

Thank you! Your input was really valuable.

[o1ka, 2016-05-25 12:08:44]

olka@chromium.org changed reviewers:
- chcunningham@chromium.org, grunell@chromium.org

[o1ka, 2016-05-25 12:20:52]

Thanks Yuri and Dale; new version uploaded.

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:32: int
delete_timeout_ms);
On 2016/05/25 01:23:20, miu wrote:
> On 2016/05/20 10:40:32, o1ka wrote:
> > On 2016/05/19 22:27:15, miu wrote:
> > > Prefer using base::TimeDelta to pass time durations, rather than POD
integer
> > > values.
> > 
> > Could you suggest why it's more preferable?
> 
> Because it let's the compiler confirm, via static type-checking, that
developers
> are not mishandling time values or doing incorrect time math. Also, the
Chromium
> time types make it easy for developers to write self-documenting code (e.g.,
> TimeDelta::FromMilliseconds(...) is unmistakable).
> 
> This may sound like a small thing, but I myself have fixed a number of weird,
> very time-consuming bugs in Chromium code where people used POD ints instead
of
> a strong time type. For example, in one case someone renamed a value from
> |foo_us| to |foo_ms| and forgot to change the interpretation of the value in
one
> spot; and impossible-to-solve crbugs piled in for several months before the
root
> cause was ascertained.

Aha, thanks for the explanation! Actually, I recently fixed just the same type
of bug when ms were renamed to frames. Now the profit becomes obvious :)

https://codereview.chromium.org/1942803002/diff/210001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:124: const int
delete_timeout_ms_;
On 2016/05/19 22:27:15, miu wrote:
> ditto: Prefer using base::TimeDelta for time durations, rather than POD
integer
> values.

Done.

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/24 20:41:42, DaleCurtis wrote:
> On 2016/05/24 at 15:00:41, o1ka wrote:
> > On 2016/05/23 20:20:26, DaleCurtis wrote:
> > > On 2016/05/23 at 19:26:27, o1ka wrote:
> > > > On 2016/05/23 19:21:34, o1ka wrote:
> > > > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > > > Why schedule them individually for deletion vs having a repeating
task
> > > that
> > > > > > scrubs them every x seconds based on some last_used_time_ inside the
> > > > > CacheEntry?
> > > > > 
> > > > > Because it's quite a rare occasion when output parameters are
requested
> and
> > > we
> > > > > schedule the deletion. Why to have the timer always running if it may
be
> > > needed
> > > > > once on page load, for example? WDYT?
> > > > 
> > > > (It was like that in the original draft version which you approved...)
> > > 
> > > You wouldn't run the timer the whole time. You'd only Start() it when
sinks
> need
> > > to be deleted. If you prefer the current approach you should at least use
a
> > > base::CancelableClosure() instead of the weak factory.
> > 
> > I looked into CancelableCallback before implementing this. The problem is
that
> it can be created only on the same thread it's posted to/deleted on
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
> wheres for weak pointer only dereference and invalidation are thread-bound
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> Mixer input(=>mixer) interfaces can be accessed from both media (pipeline) and
> main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one of
> them. And we can't pre-create all those CancelableCallback instances for the
> sinks.
> 
> This isn't true of WeakPtr either. You have a race right now where if there
are
> no WeakPtr refs outstanding and multiple threads try to create one they will
> stomp on eachother. If you look at WeakPtr::GetRef() you can see that it is
not
> thread safe. The simplest fix is instead pre-create a weak_this_ on the right
> thread that you hand out instead of using GetWeakptr(). Alternatively, make
> RepeatingTimer or CancelableClosure work as suggested previously as the
current
> approach isn't as clear as you might think :)

Oh, I've had a feeling that I might be missing something here. Actually, I
should have asked you about it. Thanks for catching!

If I use RepeatingTimer of CancelableClosure => I'm not passing sink pointer to
the callback. So I mark each sink with timestamp when it is added, and in the
callback I'll go through all of them and check the expiration. When a new unused
sink is added to the cache, I set its time stamp, and then I need to reset the
timer only if it's not running. But timer API is not thread safe
(https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...)
which is a problem here, since the code works on at least two threads.
If CancelableClosure is used, than I need to know if it's already been posted,
otherwise I'll post it for each sink (and it will be going through all sinks to
check their expiration) => I need a flag +  a lock.
So both options do not look very clean to me. Or do you have some specific idea
how to use them in a more elegant way?

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:58: return (s.device_id
== device_id_) &&
On 2016/05/24 20:41:42, DaleCurtis wrote:
> Avoid unnecessary parens.

Done.
This is something interesting though - I haven't found any recommendations in
Google/Chromium guidelines for this, and in other style guides I've worked with
parentheses are usually recommended.
Should it probably be added to the style guide?

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:63: int
source_render_frame_id_;
On 2016/05/24 20:41:42, DaleCurtis wrote:
> const all these

Done. (And then I removed the code)

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:67: };
On 2016/05/24 20:41:42, DaleCurtis wrote:
> DISALLOW_COPY_AND_ASSIGN

It's copyable (and needs to be so for find_if), so I should have declared the
default copy constructor. But now when I think of all that copying happening
here I don't like it any more :) lambda it will be!

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:80: const
base::TimeDelta& delete_timeout)
On 2016/05/24 20:41:42, DaleCurtis wrote:
> Pass base::TimeDelta by value (see time.h)

Done.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:95: CacheEntry
cache_entry;
On 2016/05/25 01:23:20, miu wrote:
> On 2016/05/24 20:41:42, DaleCurtis wrote:
> > Worth adding a constructor to CacheEntry with some default values?
> 
> IIRC, the uniform initializer syntax is now approved for Chromium code.
Instead
> of coding a constructor, you can initialize structs directly. For example:
> 
>   CacheEntry cache_entry{ source_render_frame_id, std::string(),
> security_origin, nullptr, false };

True, but assignment syntax is recommended for this case, and uniform
initializer is to be used only if neither this nor constructor works
(https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...).
So I went for assignment.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:136: }  // if
(cache_iter != cache_.end())
On 2016/05/24 20:41:42, DaleCurtis wrote:
> It's not chrome style to annotate trailing } in conditional blocks.

Done.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:187: CacheEntry
cache_entry;
On 2016/05/25 01:23:20, miu wrote:
> ditto here: You could just use the uniform initializer syntax.

Done, same as above.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:198: DVLOG(1) <<
"GetSink: address: " << cache_entry.sink.get()
On 2016/05/24 20:41:42, DaleCurtis wrote:
> I'd drop some of these DVLOGs from the production version, but up to you.

Yes, they hurt readability. Reduced a bit.

https://codereview.chromium.org/1942803002/diff/250001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:208: // We don't know
the sink state, so won't reuse it. Delete it immediately.
On 2016/05/24 20:41:42, DaleCurtis wrote:
> Aren't we always taking this path when tearing down a sink? Is it worth any
> effort to try and reuse these?

Do you mean reusing a sink after it was returned by a client? We can't do it as
of now, since AODs are not reinitializable until grunell@'s CL is landed. We can
look into it later (that will need some care to make sure we are not reusing
stale sinks, so we'll keep them for some short period of time). I'm not sure if
it will be very beneficial though, because chances that such a sink will be
requested for reuse seem to be low. I'm going to add some UMA stats to see what
sink reuse looks like.

So now we are reusing only sinks created to retrieve OutputDeviceInfo, which is
a typical scenario: the client requests output device parameters (sink created),
initializes rendering pipeline basing on those and then requests and initializes
a sink (sink reused).

https://codereview.chromium.org/1942803002/diff/250001/media/base/audio_rende...
File media/base/audio_renderer_mixer_pool.h (right):

https://codereview.chromium.org/1942803002/diff/250001/media/base/audio_rende...
media/base/audio_renderer_mixer_pool.h:38: virtual void ReturnMixer(int
owner_id,
On 2016/05/25 01:23:20, miu wrote:
> Not sure if this was already addressed in other review comments, but I'm still
a
> fan of just returning by a single-arg AudioRendererMixer*. Your call here.

Yes, that discussion was for SinkCache and I reverted that to use a single
pointer.
And this interface is for AudioRendererMixerManager, it's done this way quite a
long ago and I prefer not to touch it now (This is the interface that was
introduced to avoid binding 3 callbacks).

[o1ka, 2016-05-25 12:24:00]

olka@chromium.org changed reviewers:
+ avi@chromium.org

[o1ka, 2016-05-25 12:24:01]

avi@chromium.org: Please review changes in 
render_frame_impl.cc and render_thread_impl.cc - need your RS. Thanks!

[Guido Urdaneta, 2016-05-25 13:33:14]

lgtm % nits

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:64: // Returns a mixer
instance based on AudioParameters; an existing one if one
Since GetMixer and ReturnMixer are now implementations of an interface, you
should just have a comment saying "AudioRendererMixerPool implementation." and
rely on the documentation of the interface.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:81: // Returns output
device information. This call goes to the sink cache.
Nit: So far, the sink cache has not been mentioned in the docs for this class,
but you are referring to it.
If you choose to refer to it, perhaps you should explain somewhere the existence
of the cache. Alternatively, you may just not mention the cache in this comment
and consider it an implementation detail.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:146: const
media::AudioRendererSink* sink_ptr;
What about adding a sink() getter to ARM, so that you get the sink pointer from
|mixer| instead of storing it here?
Not a blocker.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:28: static const
int kSampleRate = 48000;
static is redundant in all these constants. namespace/global constants are
static in C++ by default.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:447: // We
expect 1 mixers to be created; it should release its sink.
nit: s/mixers/mixer

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:34: // Returnes output device
information for a specified sink.
typo: s/Returnes/Returns

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:43: // deletion, but after
releasing it from the cache.
How come? Why not stop it before releasing it from the cache?

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:71: DVLOG(1) <<
"GetSinkInfo: address: " << cache_entry.sink.get()
Consider removing these DVLOG statements.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:186: // If we got here
and |force_delete_used| is not set it means the sink
remove "we got here and"

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:187: // scheduled for
deletion get aquired and released before scheduled
typo: s/get/got

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:73: int
GetCacheSizeForTesting();
Is it possible to get rid of the ForTesting()?
If the test is a friend, can it just call cache_.size()?

[o1ka, 2016-05-25 14:52:11]

Thanks guiou@. Addressed/answered.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:64: // Returns a mixer
instance based on AudioParameters; an existing one if one
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> Since GetMixer and ReturnMixer are now implementations of an interface, you
> should just have a comment saying "AudioRendererMixerPool implementation." and
> rely on the documentation of the interface.

This comment contains implementation details not exposed in the interface. Is it
ok to have it here?

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:81: // Returns output
device information. This call goes to the sink cache.
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> Nit: So far, the sink cache has not been mentioned in the docs for this class,
> but you are referring to it.
> If you choose to refer to it, perhaps you should explain somewhere the
existence
> of the cache. Alternatively, you may just not mention the cache in this
comment
> and consider it an implementation detail.

changed to |sink_cache_| which is mentioned here.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:146: const
media::AudioRendererSink* sink_ptr;
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> What about adding a sink() getter to ARM, so that you get the sink pointer
from
> |mixer| instead of storing it here?
> Not a blocker.

We've already been there at PS7 :) changed according to Dale's recommendation.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:28: static const
int kSampleRate = 48000;
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> static is redundant in all these constants. namespace/global constants are
> static in C++ by default.

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:447: // We
expect 1 mixers to be created; it should release its sink.
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> nit: s/mixers/mixer

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:34: // Returnes output device
information for a specified sink.
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> typo: s/Returnes/Returns

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache.h:43: // deletion, but after
releasing it from the cache.
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> How come? Why not stop it before releasing it from the cache?

(discussed offline - review comment is obsolete)

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:71: DVLOG(1) <<
"GetSinkInfo: address: " << cache_entry.sink.get()
On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> Consider removing these DVLOG statements.

You mean - ALL??? :) If reviewers insist I'll do it, please let me know.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:186: // If we got here
and |force_delete_used| is not set it means the sink
On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> remove "we got here and"

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:187: // scheduled for
deletion get aquired and released before scheduled
On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> typo: s/get/got

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:73: int
GetCacheSizeForTesting();
On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> Is it possible to get rid of the ForTesting()?
> If the test is a friend, can it just call cache_.size()?

For this I need to publish CacheEntry struct in the header. What would the
reviewers recommend?

[Guido Urdaneta, 2016-05-25 15:44:09]

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:64: // Returns a mixer
instance based on AudioParameters; an existing one if one
On 2016/05/25 14:52:11, o1ka wrote:
> On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> > Since GetMixer and ReturnMixer are now implementations of an interface, you
> > should just have a comment saying "AudioRendererMixerPool implementation."
and
> > rely on the documentation of the interface.
> 
> This comment contains implementation details not exposed in the interface. Is
it
> ok to have it here?

I think the comments at the top of the class together with the interface
comments already contain what these comments say. Perhaps you can complement the
class comment saying that mixers are removed when they are no longer referenced.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:146: const
media::AudioRendererSink* sink_ptr;
On 2016/05/25 14:52:11, o1ka wrote:
> On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> > What about adding a sink() getter to ARM, so that you get the sink pointer
> from
> > |mixer| instead of storing it here?
> > Not a blocker.
> 
> We've already been there at PS7 :) changed according to Dale's recommendation.

Acknowledged.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:71: DVLOG(1) <<
"GetSinkInfo: address: " << cache_entry.sink.get()
On 2016/05/25 14:52:11, o1ka wrote:
> On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> > Consider removing these DVLOG statements.
> 
> You mean - ALL??? :) If reviewers insist I'll do it, please let me know.

No big deal, really. It's up to you :)

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:73: int
GetCacheSizeForTesting();
On 2016/05/25 14:52:11, o1ka wrote:
> On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> > Is it possible to get rid of the ForTesting()?
> > If the test is a friend, can it just call cache_.size()?
> 
> For this I need to publish CacheEntry struct in the header. What would the
> reviewers recommend?

I guess it's better to leave it as it is for now then.

[DaleCurtis, 2016-05-25 16:48:51]

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

[DaleCurtis, 2016-05-25 16:48:52]

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/25 at 12:20:51, o1ka wrote:
> On 2016/05/24 20:41:42, DaleCurtis wrote:
> > On 2016/05/24 at 15:00:41, o1ka wrote:
> > > On 2016/05/23 20:20:26, DaleCurtis wrote:
> > > > On 2016/05/23 at 19:26:27, o1ka wrote:
> > > > > On 2016/05/23 19:21:34, o1ka wrote:
> > > > > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > > > > Why schedule them individually for deletion vs having a repeating
task
> > > > that
> > > > > > > scrubs them every x seconds based on some last_used_time_ inside
the
> > > > > > CacheEntry?
> > > > > > 
> > > > > > Because it's quite a rare occasion when output parameters are
requested
> > and
> > > > we
> > > > > > schedule the deletion. Why to have the timer always running if it
may be
> > > > needed
> > > > > > once on page load, for example? WDYT?
> > > > > 
> > > > > (It was like that in the original draft version which you approved...)
> > > > 
> > > > You wouldn't run the timer the whole time. You'd only Start() it when
sinks
> > need
> > > > to be deleted. If you prefer the current approach you should at least
use a
> > > > base::CancelableClosure() instead of the weak factory.
> > > 
> > > I looked into CancelableCallback before implementing this. The problem is
that
> > it can be created only on the same thread it's posted to/deleted on
> >
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
> > wheres for weak pointer only dereference and invalidation are thread-bound
> >
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> > Mixer input(=>mixer) interfaces can be accessed from both media (pipeline)
and
> > main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one
of
> > them. And we can't pre-create all those CancelableCallback instances for the
> > sinks.
> > 
> > This isn't true of WeakPtr either. You have a race right now where if there
are
> > no WeakPtr refs outstanding and multiple threads try to create one they will
> > stomp on eachother. If you look at WeakPtr::GetRef() you can see that it is
not
> > thread safe. The simplest fix is instead pre-create a weak_this_ on the
right
> > thread that you hand out instead of using GetWeakptr(). Alternatively, make
> > RepeatingTimer or CancelableClosure work as suggested previously as the
current
> > approach isn't as clear as you might think :)
> 
> Oh, I've had a feeling that I might be missing something here. Actually, I
should have asked you about it. Thanks for catching!
> 
> If I use RepeatingTimer of CancelableClosure => I'm not passing sink pointer
to the callback. So I mark each sink with timestamp when it is added, and in the
callback I'll go through all of them and check the expiration. When a new unused
sink is added to the cache, I set its time stamp, and then I need to reset the
timer only if it's not running. But timer API is not thread safe
(https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...)
which is a problem here, since the code works on at least two threads.
> If CancelableClosure is used, than I need to know if it's already been posted,
otherwise I'll post it for each sink (and it will be going through all sinks to
check their expiration) => I need a flag +  a lock.
> So both options do not look very clean to me. Or do you have some specific
idea how to use them in a more elegant way?

The multiple threads thing isn't a problem if you use a timer. You would just
PostTask() to the right thread to start the timer (which would do nothing if
it's already running) and it would automatically stop when no unused sinks are
remaining.

For the closure you're only cancelling tasks during destruction, so you'd just
pre-bind the closure on the right thread and then post it when a deletion needs
to be scheduled.

As you've pointed out in both cases you'd skim the vector looking for unused
sinks, but that's cheap as you've benchmarked :)

[o1ka, 2016-05-25 17:00:56]

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/230001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:150: // Schedule it for
deletion.
On 2016/05/25 16:48:52, DaleCurtis wrote:
> On 2016/05/25 at 12:20:51, o1ka wrote:
> > On 2016/05/24 20:41:42, DaleCurtis wrote:
> > > On 2016/05/24 at 15:00:41, o1ka wrote:
> > > > On 2016/05/23 20:20:26, DaleCurtis wrote:
> > > > > On 2016/05/23 at 19:26:27, o1ka wrote:
> > > > > > On 2016/05/23 19:21:34, o1ka wrote:
> > > > > > > On 2016/05/23 18:29:08, DaleCurtis wrote:
> > > > > > > > Why schedule them individually for deletion vs having a
repeating
> task
> > > > > that
> > > > > > > > scrubs them every x seconds based on some last_used_time_ inside
> the
> > > > > > > CacheEntry?
> > > > > > > 
> > > > > > > Because it's quite a rare occasion when output parameters are
> requested
> > > and
> > > > > we
> > > > > > > schedule the deletion. Why to have the timer always running if it
> may be
> > > > > needed
> > > > > > > once on page load, for example? WDYT?
> > > > > > 
> > > > > > (It was like that in the original draft version which you
approved...)
> > > > > 
> > > > > You wouldn't run the timer the whole time. You'd only Start() it when
> sinks
> > > need
> > > > > to be deleted. If you prefer the current approach you should at least
> use a
> > > > > base::CancelableClosure() instead of the weak factory.
> > > > 
> > > > I looked into CancelableCallback before implementing this. The problem
is
> that
> > > it can be created only on the same thread it's posted to/deleted on
> > >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
> > > wheres for weak pointer only dereference and invalidation are thread-bound
> > >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
> > > Mixer input(=>mixer) interfaces can be accessed from both media (pipeline)
> and
> > > main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on
one
> of
> > > them. And we can't pre-create all those CancelableCallback instances for
the
> > > sinks.
> > > 
> > > This isn't true of WeakPtr either. You have a race right now where if
there
> are
> > > no WeakPtr refs outstanding and multiple threads try to create one they
will
> > > stomp on eachother. If you look at WeakPtr::GetRef() you can see that it
is
> not
> > > thread safe. The simplest fix is instead pre-create a weak_this_ on the
> right
> > > thread that you hand out instead of using GetWeakptr(). Alternatively,
make
> > > RepeatingTimer or CancelableClosure work as suggested previously as the
> current
> > > approach isn't as clear as you might think :)
> > 
> > Oh, I've had a feeling that I might be missing something here. Actually, I
> should have asked you about it. Thanks for catching!
> > 
> > If I use RepeatingTimer of CancelableClosure => I'm not passing sink pointer
> to the callback. So I mark each sink with timestamp when it is added, and in
the
> callback I'll go through all of them and check the expiration. When a new
unused
> sink is added to the cache, I set its time stamp, and then I need to reset the
> timer only if it's not running. But timer API is not thread safe
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...)
> which is a problem here, since the code works on at least two threads.
> > If CancelableClosure is used, than I need to know if it's already been
posted,
> otherwise I'll post it for each sink (and it will be going through all sinks
to
> check their expiration) => I need a flag +  a lock.
> > So both options do not look very clean to me. Or do you have some specific
> idea how to use them in a more elegant way?
> 
> The multiple threads thing isn't a problem if you use a timer. You would just
> PostTask() to the right thread to start the timer (which would do nothing if
> it's already running) and it would automatically stop when no unused sinks are
> remaining.

Would't I need to use a weak pointer to post that task? Am I missing something?
> 
> For the closure you're only cancelling tasks during destruction, so you'd just
> pre-bind the closure on the right thread and then post it when a deletion
needs
> to be scheduled.

What is the advantage comparing to posting tasks with weak_ptr? (I think that's
in the end what CancelableClosure does).
(And disadvantage is that we need to deal with time stamps)

> 
> As you've pointed out in both cases you'd skim the vector looking for unused
> sinks, but that's cheap as you've benchmarked :)

Yes, but it will cost a bit more since in both cases we need to do maths on time
stamps.

I still do not quite see obvious benefits this logic complication brings.

[DaleCurtis, 2016-05-25 19:12:38]

On 2016/05/25 at 17:00:56, olka wrote:
> > 
> > The multiple threads thing isn't a problem if you use a timer. You would
just
> > PostTask() to the right thread to start the timer (which would do nothing if
> > it's already running) and it would automatically stop when no unused sinks
are
> > remaining.
> 
> Would't I need to use a weak pointer to post that task? Am I missing
something?

No, you just PostTask(FROM_HERE, CancelableClosure.callback());

> > 
> > For the closure you're only cancelling tasks during destruction, so you'd
just
> > pre-bind the closure on the right thread and then post it when a deletion
needs
> > to be scheduled.
> 
> What is the advantage comparing to posting tasks with weak_ptr? (I think
that's in the end what CancelableClosure does).
> (And disadvantage is that we need to deal with time stamps)

The disadvantage is essentially the issue you just hit. It's non-obvious that
your usage of WeakPtr is correct and your code is one minor change away from
having a race that your tests can't catch. You now have both a WeakPtrFactory
and WeakPtr that you've constructed to let you post cleanup tasks. At the very
least if you don't want to switch to the safer alternative, you need to document
why you're binding |weak_this_| during construction and try to find a way to add
a DCHECK to catch this.

[o1ka, 2016-05-25 19:37:48]

On 2016/05/25 19:12:38, DaleCurtis wrote:
> On 2016/05/25 at 17:00:56, olka wrote:
> > > 
> > > The multiple threads thing isn't a problem if you use a timer. You would
> just
> > > PostTask() to the right thread to start the timer (which would do nothing
if
> > > it's already running) and it would automatically stop when no unused sinks
> are
> > > remaining.
> > 
> > Would't I need to use a weak pointer to post that task? Am I missing
> something?
> 
> No, you just PostTask(FROM_HERE, CancelableClosure.callback());

Ah, so do you mean I should use both CancelableClosure and timer?

> 
> > > 
> > > For the closure you're only cancelling tasks during destruction, so you'd
> just
> > > pre-bind the closure on the right thread and then post it when a deletion
> needs
> > > to be scheduled.
> > 
> > What is the advantage comparing to posting tasks with weak_ptr? (I think
> that's in the end what CancelableClosure does).
> > (And disadvantage is that we need to deal with time stamps)
> 
> The disadvantage is essentially the issue you just hit. It's non-obvious that
> your usage of WeakPtr is correct and your code is one minor change away from
> having a race that your tests can't catch. You now have both a WeakPtrFactory
> and WeakPtr that you've constructed to let you post cleanup tasks. At the very
> least if you don't want to switch to the safer alternative, you need to
document
> why you're binding |weak_this_| during construction and try to find a way to
add
> a DCHECK to catch this.

Agree, will document it.
Still combination of Closure, Timer and timestamps (which also require
documentation and correct usage to be safe; posting a closure to reset a timer
is not a straightforward thing) does not look more attractive/safer than
weak_factory + weak_ptr (which seems to be a pattern I see in several places). I
do agree though that a closure or a timer would look just beautiful here if not
multithreaded access.
It's not that I don't want to change things - I just want to see a reason to do
so.

[DaleCurtis, 2016-05-25 19:55:09]

I think there's some confusion here. The CancelableClosure and Timer ideas are
separate. With a cancelable you'd just do the following during construction:

cancelable_closure.Reset(base::Bind(&CleanupTask));

Then during DeleteLaterIfUnused() you'd do:

PostTask(FROM_HERE, cancelable_closure.callback());

CleanupTask would do something like:
for (sink : sinks) { if (unused && unused_time_ > acceptable_delta) delete };

You're right that if we want to go with a timer based solution that only runs
when there are sinks to delete, it's not as clear.

P.S. Do we need the used flag? I just realized that since the sinks are
RefCounted you could just check sink_->HasOneRef(); though that's not quite as
clear :)

[o1ka, 2016-05-25 20:12:52]

On 2016/05/25 19:55:09, DaleCurtis wrote:
> I think there's some confusion here. The CancelableClosure and Timer ideas are
> separate. With a cancelable you'd just do the following during construction:
> 
> cancelable_closure.Reset(base::Bind(&CleanupTask));
> 
> Then during DeleteLaterIfUnused() you'd do:
> 
> PostTask(FROM_HERE, cancelable_closure.callback());
> 
> CleanupTask would do something like:
> for (sink : sinks) { if (unused && unused_time_ > acceptable_delta) delete };
> 

Yes, this is how I understood CancelableClosure-only solution. It's both a bit
safer and a bit messier/less effective than the current version.
I hope that proper documentation will compensate lower safety.

> You're right that if we want to go with a timer based solution that only runs
> when there are sinks to delete, it's not as clear.
> 
> P.S. Do we need the used flag? I just realized that since the sinks are
> RefCounted you could just check sink_->HasOneRef(); though that's not quite as
> clear :)

Yes we do (or if using closure than zero time stamp can be treated as a "used"
marker), because the client can (and most likely will) tell the cache to release
a sink and still hold a reference to it.

[o1ka, 2016-05-25 20:19:07]

On 2016/05/25 20:12:52, o1ka wrote:
> On 2016/05/25 19:55:09, DaleCurtis wrote:
> > I think there's some confusion here. The CancelableClosure and Timer ideas
are
> > separate. With a cancelable you'd just do the following during construction:
> > 
> > cancelable_closure.Reset(base::Bind(&CleanupTask));
> > 
> > Then during DeleteLaterIfUnused() you'd do:
> > 
> > PostTask(FROM_HERE, cancelable_closure.callback());
> > 
> > CleanupTask would do something like:
> > for (sink : sinks) { if (unused && unused_time_ > acceptable_delta) delete
};
> > 
> 
> Yes, this is how I understood CancelableClosure-only solution. It's both a bit
> safer and a bit messier/less effective than the current version.
> I hope that proper documentation will compensate lower safety.
> 
> > You're right that if we want to go with a timer based solution that only
runs
> > when there are sinks to delete, it's not as clear.
> > 
> > P.S. Do we need the used flag? I just realized that since the sinks are
> > RefCounted you could just check sink_->HasOneRef(); though that's not quite
as
> > clear :)
> 
> Yes we do (or if using closure than zero time stamp can be treated as a "used"
> marker), because the client can (and most likely will) tell the cache to
release
> a sink and still hold a reference to it.

BTW, If we could use CancelableCallback with a sink pointer as parameter, then I
would go for it without discussions. But that multithreading..

[o1ka, 2016-05-25 20:25:50]

On 2016/05/25 20:12:52, o1ka wrote:
> On 2016/05/25 19:55:09, DaleCurtis wrote:
> > I think there's some confusion here. The CancelableClosure and Timer ideas
are
> > separate. With a cancelable you'd just do the following during construction:
> > 
> > cancelable_closure.Reset(base::Bind(&CleanupTask));
> > 
> > Then during DeleteLaterIfUnused() you'd do:
> > 
> > PostTask(FROM_HERE, cancelable_closure.callback());
> > 
> > CleanupTask would do something like:
> > for (sink : sinks) { if (unused && unused_time_ > acceptable_delta) delete
};
> > 
> 
> Yes, this is how I understood CancelableClosure-only solution. It's both a bit
> safer and a bit messier/less effective than the current version.
> I hope that proper documentation will compensate lower safety.
> 
> > You're right that if we want to go with a timer based solution that only
runs
> > when there are sinks to delete, it's not as clear.
> > 
> > P.S. Do we need the used flag? I just realized that since the sinks are
> > RefCounted you could just check sink_->HasOneRef(); though that's not quite
as
> > clear :)
> 
> Yes we do (or if using closure than zero time stamp can be treated as a "used"
> marker), because the client can (and most likely will) tell the cache to
release
> a sink and still hold a reference to it.

Actually, probably we don't. We did need it before for some reason which
probably went away. I'll think about it after I have some sleep :) sorry

[o1ka, 2016-05-26 09:00:27]

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:64: // Returns a mixer
instance based on AudioParameters; an existing one if one
On 2016/05/25 15:44:09, Guido Urdaneta wrote:
> On 2016/05/25 14:52:11, o1ka wrote:
> > On 2016/05/25 13:33:13, Guido Urdaneta wrote:
> > > Since GetMixer and ReturnMixer are now implementations of an interface,
you
> > > should just have a comment saying "AudioRendererMixerPool implementation."
> and
> > > rely on the documentation of the interface.
> > 
> > This comment contains implementation details not exposed in the interface.
Is
> it
> > ok to have it here?
> 
> I think the comments at the top of the class together with the interface
> comments already contain what these comments say. Perhaps you can complement
the
> class comment saying that mixers are removed when they are no longer
referenced.

Done.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:71: DVLOG(1) <<
"GetSinkInfo: address: " << cache_entry.sink.get()
On 2016/05/25 15:44:09, Guido Urdaneta wrote:
> On 2016/05/25 14:52:11, o1ka wrote:
> > On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> > > Consider removing these DVLOG statements.
> > 
> > You mean - ALL??? :) If reviewers insist I'll do it, please let me know.
> 
> No big deal, really. It's up to you :)

Acknowledged.

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/270001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:73: int
GetCacheSizeForTesting();
On 2016/05/25 15:44:09, Guido Urdaneta wrote:
> On 2016/05/25 14:52:11, o1ka wrote:
> > On 2016/05/25 13:33:14, Guido Urdaneta wrote:
> > > Is it possible to get rid of the ForTesting()?
> > > If the test is a friend, can it just call cache_.size()?
> > 
> > For this I need to publish CacheEntry struct in the header. What would the
> > reviewers recommend?
> 
> I guess it's better to leave it as it is for now then.

Acknowledged.

https://codereview.chromium.org/1942803002/diff/310001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/310001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:98: // racy access to
the factory.
Another alternative would be to post the task under cache_lock_ as it was done
initially. (If I understand the code correctly, weak_factory is bound to a
thread on WeakPtr dereference/invalidation only.)

[o1ka, 2016-05-26 09:15:45]

On 2016/05/25 20:25:50, o1ka wrote:
> On 2016/05/25 20:12:52, o1ka wrote:
> > On 2016/05/25 19:55:09, DaleCurtis wrote:
> > > I think there's some confusion here. The CancelableClosure and Timer ideas
> are
> > > separate. With a cancelable you'd just do the following during
construction:
> > > 
> > > cancelable_closure.Reset(base::Bind(&CleanupTask));
> > > 
> > > Then during DeleteLaterIfUnused() you'd do:
> > > 
> > > PostTask(FROM_HERE, cancelable_closure.callback());
> > > 
> > > CleanupTask would do something like:
> > > for (sink : sinks) { if (unused && unused_time_ > acceptable_delta) delete
> };
> > > 
> > 
> > Yes, this is how I understood CancelableClosure-only solution. It's both a
bit
> > safer and a bit messier/less effective than the current version.
> > I hope that proper documentation will compensate lower safety.
> > 
> > > You're right that if we want to go with a timer based solution that only
> runs
> > > when there are sinks to delete, it's not as clear.
> > > 
> > > P.S. Do we need the used flag? I just realized that since the sinks are
> > > RefCounted you could just check sink_->HasOneRef(); though that's not
quite
> as
> > > clear :)
> > 
> > Yes we do (or if using closure than zero time stamp can be treated as a
"used"
> > marker), because the client can (and most likely will) tell the cache to
> release
> > a sink and still hold a reference to it.
> 
> Actually, probably we don't. We did need it before for some reason which
> probably went away. I'll think about it after I have some sleep :) sorry

Ok, now I remember: I did not want to use HasOneRef() as an indication of
"unused" because ref incrementing/decrementing is an implicit operation
occurring, for example, when passing refs by value/copying them on the stack, so
relying on that makes the code quite fragile regarding possible future
modifications and may introduce quite hard-to-catch bugs. So - yes, "that's not
quite as clear", seems better to use an explicit flag.

[o1ka, 2016-05-26 16:29:51]

dalecurtis@ - PTAL: updated comments, added a "smoke" unit test which hopefully
will catch threading issues if some future change will break something.

[o1ka, 2016-05-26 16:31:27]

avi@, friendly ping: render_frame_impl.cc and render_thread_impl.cc - need your
RS. Thanks!

[DaleCurtis, 2016-05-26 19:18:10]

lgtm

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:35:
base::Bind(AudioDeviceFactory::NewAudioRendererMixerSink),
Hmm, should this have a & ? I'm surprised it compiles without it.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:249: int
AudioRendererSinkCacheImpl::GetCacheSizeForTesting() {
This can be an inline get_cache_size_for_testing() instead if you like.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:34: const
base::TimeDelta delete_timeout);
Remove const.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:22: namespace {
If you convert to an anonymous namespace, drop the static.

https://codereview.chromium.org/1942803002/diff/330001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:334: const AudioParameters& hw_params =
Do you know what this returns when the element is attached to WebAudio? If it
returns invalid parameters, that's actually good since we'll try to output at
input parameters which is best for WebAudio.

[Avi (use Gerrit), 2016-05-26 20:16:58]

On 2016/05/26 16:31:27, o1ka wrote:
> avi@, friendly ping: render_frame_impl.cc and render_thread_impl.cc - need
your
> RS. Thanks!

LGTM stampity stamp

[o1ka, 2016-05-27 13:48:52]

Thank you, Dale. Please see my comment to
media/renderers/audio_renderer_impl.cc:334: I modified WeAudioSourceProviderImpl
to always return sink parameters, to keep the current behavior. Let me know if
you think we need to change that.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:35:
base::Bind(AudioDeviceFactory::NewAudioRendererMixerSink),
On 2016/05/26 19:18:09, DaleCurtis wrote:
> Hmm, should this have a & ? I'm surprised it compiles without it.

It's a static method, so & is optional (just like with regular functions). But I
had no intention to omit it - it's a misprint. Thanks for catching!

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.cc:249: int
AudioRendererSinkCacheImpl::GetCacheSizeForTesting() {
On 2016/05/26 19:18:09, DaleCurtis wrote:
> This can be an inline get_cache_size_for_testing() instead if you like.

Unfortunately not, I need to publish CacheEntry struct in the header to do that
(and then I would not need a method, because test fixture is a friend). I added
a comment.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_impl.h:34: const
base::TimeDelta delete_timeout);
On 2016/05/26 19:18:09, DaleCurtis wrote:
> Remove const.

Done.

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
File content/renderer/media/audio_renderer_sink_cache_unittest.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/content/renderer/media...
content/renderer/media/audio_renderer_sink_cache_unittest.cc:22: namespace {
On 2016/05/26 19:18:09, DaleCurtis wrote:
> If you convert to an anonymous namespace, drop the static.

Done.

https://codereview.chromium.org/1942803002/diff/330001/media/renderers/audio_...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/1942803002/diff/330001/media/renderers/audio_...
media/renderers/audio_renderer_impl.cc:334: const AudioParameters& hw_params =
On 2016/05/26 19:18:09, DaleCurtis wrote:
> Do you know what this returns when the element is attached to WebAudio? If it
> returns invalid parameters, that's actually good since we'll try to output at
> input parameters which is best for WebAudio.

This is a good question.

Here
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webaud...

it returns device info with OUTPUT_DEVICE_STATUS_ERROR_INTERNAL and
UnavailableDeviceParams(defined here
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_p...)
which are valid, but AUDIO_FAKE, so we do fail back to input parameters here.

But in fact I'm not sure if it's correct. As far as I understand,
WebAudioSourceProviderImpl client can be reset by HTMLMediaElement to nullptr
later, which means rendering will switch to the real sink (ARMI). But pipeline
will continue to output at input parameters, which is a new behavior, and I'm
not sure that it's desired.
So, in order to keep the things as they are, I modified
WebAudioSourceProviderImpl to always return actual sink parameters no matter if
the client is set or not.

I can change that in a follow-up CL if you confirm that the new behavior is
valid.

(Note that if WebAudioSourceProvider is initialized with NullAudioSink  we do
output on input params here, because NullAudioSink params are AUDIO_FAKE)

[o1ka, 2016-05-27 13:51:22]

The CQ bit was checked by olka@chromium.org

[o1ka, 2016-05-27 13:51:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, miu@chromium.org, guidou@chromium.org,
avi@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1942803002/#ps370001
(title: "Make WebAudioSourceProvider to always return real sink info reguardless
the client - to avoid behavior change.")

[commit-bot: I haz the power, 2016-05-27 13:51:42]

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

[commit-bot: I haz the power, 2016-05-27 15:33:14]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it
means that the cache will work as AudioDeviceFactory, and they both probably
should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it
means that the cache will work as AudioDeviceFactory, and they both probably
should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

[commit-bot: I haz the power, 2016-05-27 15:33:17]

Committed patchset #14 (id:370001)

[commit-bot: I haz the power, 2016-05-27 15:34:31]

Description was changed from

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it
means that the cache will work as AudioDeviceFactory, and they both probably
should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)
==========

to

==========
Caching AudioOutputDevice instances in mixer manager.

* AudioRendererSinkCache introduced - keeps references to the sinks created to
get output device info or to pass to a mixer; helps to reuse thinks to get
output device info.
* Some factory methods are added to simplify mocking/unit tests creation.
* HardwareConfig usage is removed from the renderer (except
GetHighlatencyBufferSize() call); sink is used to obtain output parameters.
* Unit tests and some mocks updated.

- see bugs for more info.

*** The follow-up plan is:
1) Add UMA stats on cache usage?
2) Probably switch other clients to get sinks through the cache (basically it
means that the cache will work as AudioDeviceFactory, and they both probably
should be merged, except factory part which handles input devices)
3) Remove the rest of AudioHardwareConfig usage across the code.

BUG=536843,586161,587461

TESTSING=manual testing on several sample pages with media elements +
media_unittests + content_unittests + media_blink_unittests (all updated)

Committed: https://crrev.com/7a4679394c01834175e451c7983747eab8603b82
Cr-Commit-Position: refs/heads/master@{#396471}
==========

[commit-bot: I haz the power, 2016-05-27 15:34:33]

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