Enable renderer side mixing behind a flag.

Enable renderer side mixing behind a flag.

Lands the final bits required to turn on renderer side mixing behind
a flag!  Switching <audio> over to the PCM_LOW_LATENCY track to boot.
Specifically the flag is --enable-renderer-side-mixing.

The big picture: RenderThreadImpl owns an AudioRendererMixerManager
which RenderAudioSourceProvider uses to create AudioRenderMixerInput,
which extensions of AudioRendererSink.  When ARMM creates ARMI it
passes in callbacks which allow ARMI to retrieve an AudioRendererMixer
from ARMM once ARMI::Initialize() has been called.

When ARMM gets a request for an ARM from ARMI::Initialize() it will
first check if one exists and if so, hand out a reference.  If not, it
will create a new ARM instance (which it owns) and initialize it with
the proper hardware output AudioParameters and an AudioDevice based
AudioRenderSink.

ARM will immediately call Initialize() and Start() on AudioDevice which
will then begin requesting data via RenderCallback from ARM.

If the hardware sampling rate is not equal to the input sampling rate,
ARM will instantiate a MultiChannelResampler instance to resample the
audio as required for use by a PCM_LOW_LATENCY AudioDevice.

After ARMI::Initialize() nothing really happens until RASP() eventually
calls ARMI::Start() which will then register the ARMI instance with ARM.
At which point ARM will start mixing it into the output stream it's
providing to AudioDevice.

Everything works as far as I can tell, but an optimization pass and more
careful testing is necessary on Windows before the feature comes out from
behind the flag.

BUG=133637
TEST=New unit tests. Manual testing.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146935

[DaleCurtis, 2012-07-13 01:24:44]

scherkus: please review everything.

jam: non-media content/ changes. Alternatively, please suggest a different
reviewer.

Thanks!

[jochen (gone - plz use gerrit), 2012-07-13 08:51:03]

drive-by content comments

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:14: using
content::AudioDeviceFactory;
after you've moved the class into namespace content, this is no longer required

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:20:
AudioRendererMixerManager::~AudioRendererMixerManager() {}
maybe DCHECK that all mixers have been removed?

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:24: return new
media::AudioRendererMixerInput(
please add a comment in the header that the caller must ensure that the
AudioRenderMixerManager outlives the return value

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:63: // Remove the
reference.
that comment doesn't add any value

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:34: class CONTENT_EXPORT
AudioRendererMixerManager {
all code in content/ should go into namespace content

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:40:
scoped_refptr<media::AudioRendererMixerInput> CreateInput();
why not just return a raw pointer? that way, the mixer isn't unnecessarily
reffed up and down during return

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:43: friend class
AudioRendererMixerManagerTest;
please use FRIEND_TEST_ALL_PREFIXES

[jam, 2012-07-13 15:19:42]

my parents lgtm with nit, and after Jochen's comments are addressed

http://codereview.chromium.org/10636036/diff/3010/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/10636036/diff/3010/content/renderer/render_thr...
content/renderer/render_thread_impl.h:227: // Return an
AudioRendererMixerManager instance which manages renderer side
nit: for simple getters like this, the documentation of what the variable does
goes by the member variable (i.e. line 313) and the getter isn't documented

[jam, 2012-07-13 15:19:42]

my parents lgtm with nit, and after Jochen's comments are addressed

[jam, 2012-07-13 15:35:31]

On 2012/07/13 15:19:42, John Abd-El-Malek wrote:
> my parents lgtm with nit, and after Jochen's comments are addressed

parents->parts, don't know how that happened :)

[DaleCurtis, 2012-07-13 18:00:53]

On 2012/07/13 15:35:31, John Abd-El-Malek wrote:
> On 2012/07/13 15:19:42, John Abd-El-Malek wrote:
> > my parents lgtm with nit, and after Jochen's comments are addressed
> 
> parents->parts, don't know how that happened :)

jam: Glad your parents look good. :) Q to you though, since you blessed the
AudioDeviceFactory pattern:  I realized this morning that
AudioRendererMixerManager could actually be an AudioRendererMixerInputFactory
now which allows removal of the RenderThreadImpl changes. Would you prefer this
to the current approach?

[scherkus (not reviewing), 2012-07-13 18:40:15]

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:17: // DCHECK something
static?
wha?

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:20:
AudioRendererMixerManager::~AudioRendererMixerManager() {}
On 2012/07/13 08:51:03, jochen wrote:
> maybe DCHECK that all mixers have been removed?

yeah DCHECK(mixer_map_.empty()) wouldn't hurt

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.cc:37: // Increment the
reference count.
comment ain't useful

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:5: // Manages sharing of
an AudioRendererMixer among AudioRendererMixerInputs based
nit: we (media code) has typically put these alongside class definitions as
opposed to top of file

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:7: // configuration will
share a mixer.  While a new AudioRendererMixer will be
s/.  While/ while/

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:37: virtual
~AudioRendererMixerManager();
remove virtual -- I'm not seeing any virtualness in this class

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:58:
std::pair<media::AudioRendererMixer*,int>,
s/,int/, int/

Alternatively can use a struct so code is self documenting. Something like:
struct MixerReferences {
  ARM* mixer;
  int count;
};

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/audio_renderer_mixer_manager.h:61: base::Lock
mixer_map_lock_;
which threads interact with this class requiring a lock?

CreateInput() should be on render thread -- what thread is input removal
happening on?

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
File content/renderer/media/render_audiosourceprovider.cc (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/content/renderer/me...
content/renderer/media/render_audiosourceprovider.cc:30: static int
enable_render_side_mixing = -1;
don't worry about optimizing the switch lookup on subsequent object creation --
just do the HasSwitch() every time

https://chromiumcodereview.appspot.com/10636036/diff/3010/media/base/media_sw...
File media/base/media_switches.h (right):

https://chromiumcodereview.appspot.com/10636036/diff/3010/media/base/media_sw...
media/base/media_switches.h:32: MEDIA_EXPORT extern const char
kEnableRendererSideMixing[];
this should be a content switch, not a media switch

[jam, 2012-07-13 19:41:11]

On 2012/07/13 18:00:53, DaleCurtis wrote:
> On 2012/07/13 15:35:31, John Abd-El-Malek wrote:
> > On 2012/07/13 15:19:42, John Abd-El-Malek wrote:
> > > my parents lgtm with nit, and after Jochen's comments are addressed
> > 
> > parents->parts, don't know how that happened :)
> 
> jam: Glad your parents look good. :) Q to you though, since you blessed the
> AudioDeviceFactory pattern:  I realized this morning that
> AudioRendererMixerManager could actually be an AudioRendererMixerInputFactory
> now which allows removal of the RenderThreadImpl changes. Would you prefer
this
> to the current approach?

would that be done because you're going to swap in different implementations of
AudioRendererMixerManager (i.e. like AudioDevice), or is that just to avoid
adding a member variable to RenderThread? If the former, then sure add a
factory. If the latter, no need to make the code more complicated.

[DaleCurtis, 2012-07-13 21:28:33]

Just to avoid a member variable in RenderThread; I'll keep the current design
then.

All comments addressed.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:14: using
content::AudioDeviceFactory;
On 2012/07/13 08:51:03, jochen wrote:
> after you've moved the class into namespace content, this is no longer
required

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:17: // DCHECK something
static?
On 2012/07/13 18:40:16, scherkus wrote:
> wha?

Leftover from an earlier thought process about ensuring only one ARMM is ever in
existence.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:20:
AudioRendererMixerManager::~AudioRendererMixerManager() {}
On 2012/07/13 18:40:16, scherkus wrote:
> On 2012/07/13 08:51:03, jochen wrote:
> > maybe DCHECK that all mixers have been removed?
> 
> yeah DCHECK(mixer_map_.empty()) wouldn't hurt

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:24: return new
media::AudioRendererMixerInput(
On 2012/07/13 08:51:03, jochen wrote:
> please add a comment in the header that the caller must ensure that the
> AudioRenderMixerManager outlives the return value

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:37: // Increment the
reference count.
On 2012/07/13 18:40:16, scherkus wrote:
> comment ain't useful

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:63: // Remove the
reference.
On 2012/07/13 08:51:03, jochen wrote:
> that comment doesn't add any value

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:5: // Manages sharing of
an AudioRendererMixer among AudioRendererMixerInputs based
On 2012/07/13 18:40:16, scherkus wrote:
> nit: we (media code) has typically put these alongside class definitions as
> opposed to top of file

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:7: // configuration will
share a mixer.  While a new AudioRendererMixer will be
On 2012/07/13 18:40:16, scherkus wrote:
> s/.  While/ while/

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:34: class CONTENT_EXPORT
AudioRendererMixerManager {
On 2012/07/13 08:51:03, jochen wrote:
> all code in content/ should go into namespace content

Done.  I noticed the classes I was interacting with didn't have this
(AudioDevice, RASP), so I left it off.  Should those classes have it too?

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:37: virtual
~AudioRendererMixerManager();
On 2012/07/13 18:40:16, scherkus wrote:
> remove virtual -- I'm not seeing any virtualness in this class

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:40:
scoped_refptr<media::AudioRendererMixerInput> CreateInput();
On 2012/07/13 08:51:03, jochen wrote:
> why not just return a raw pointer? that way, the mixer isn't unnecessarily
> reffed up and down during return

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:43: friend class
AudioRendererMixerManagerTest;
On 2012/07/13 08:51:03, jochen wrote:
> please use FRIEND_TEST_ALL_PREFIXES

I don't think that's necessary here? I only access the private methods/members
through the base AudioRendererMixerManagerTest class and delegate for the
instantiated test cases.  As such the test names can be tagged with whatever and
the code will compile fine.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:58:
std::pair<media::AudioRendererMixer*,int>,
On 2012/07/13 18:40:16, scherkus wrote:
> s/,int/, int/
> 
> Alternatively can use a struct so code is self documenting. Something like:
> struct MixerReferences {
>   ARM* mixer;
>   int count;
> };

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:61: base::Lock
mixer_map_lock_;
On 2012/07/13 18:40:16, scherkus wrote:
> which threads interact with this class requiring a lock?
> 
> CreateInput() should be on render thread -- what thread is input removal
> happening on?

ARMM is constructed on the render thread, GetMixer and RemoveMixer happen on the
pipeline thread for each <audio> source.  Specifically GetMixer ends up being
called from media::Pipeline::InitializeTask() and RemoveMixer from
media::Pipeline::TeardownStateTransitionTask().

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/rend...
File content/renderer/media/render_audiosourceprovider.cc (right):

http://codereview.chromium.org/10636036/diff/3010/content/renderer/media/rend...
content/renderer/media/render_audiosourceprovider.cc:30: static int
enable_render_side_mixing = -1;
On 2012/07/13 18:40:16, scherkus wrote:
> don't worry about optimizing the switch lookup on subsequent object creation
--
> just do the HasSwitch() every time

Done.

http://codereview.chromium.org/10636036/diff/3010/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/10636036/diff/3010/content/renderer/render_thr...
content/renderer/render_thread_impl.h:227: // Return an
AudioRendererMixerManager instance which manages renderer side
On 2012/07/13 15:19:42, John Abd-El-Malek wrote:
> nit: for simple getters like this, the documentation of what the variable does
> goes by the member variable (i.e. line 313) and the getter isn't documented

Done.

http://codereview.chromium.org/10636036/diff/3010/media/base/media_switches.h
File media/base/media_switches.h (right):

http://codereview.chromium.org/10636036/diff/3010/media/base/media_switches.h...
media/base/media_switches.h:32: MEDIA_EXPORT extern const char
kEnableRendererSideMixing[];
On 2012/07/13 18:40:16, scherkus wrote:
> this should be a content switch, not a media switch

Done.

[scherkus (not reviewing), 2012-07-13 23:17:38]

LGTM w/ nits

Ddon't forget to update CL description in case anything changed. For example you
can remove "Alternate 
naming suggestions welcome!" because your switch name is a-ok.

http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
content/public/common/content_switches.cc:315: // Enable renderer side mixing
and the low latency output path for <audio>.
s/<audio>/media elements/

I hope this works for <video> too :)

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:43: // is it okay for
unit tests to call these methods?
as discussed offline you can change this to reference http://crbug.com/137326

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:45: new
media::AudioRendererMixer(
nit for code clarity -- can you break out these objects into separate
statements?

media::AudioParameters output_params = ...;
media::ARM mixer = ...;
reference = { mixer, 1 };

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:65: AudioRendererMixerMap
mixer_map_;
nit: don't bother with _map / _list to variable names -- it's a form of
hungarian notation and doesn't add much value so go with |mixers_|

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:66: base::Lock
mixer_map_lock_;
ditto here with mixers_lock_

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:58: };
DISALLOW etc ... yes, even for testing code :)

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:63: // There
should be no mixers outstanding to start with.
nice tests!

[DaleCurtis, 2012-07-14 00:39:26]

Done.

scherkus: Still need you to finish reviewing
http://codereview.chromium.org/10698066/ before this can land :)

jochen: Anything else?

http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
File content/public/common/content_switches.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
content/public/common/content_switches.cc:315: // Enable renderer side mixing
and the low latency output path for <audio>.
On 2012/07/13 23:17:38, scherkus wrote:
> s/<audio>/media elements/
> 
> I hope this works for <video> too :)

Done.

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:43: // is it okay for
unit tests to call these methods?
On 2012/07/13 23:17:38, scherkus wrote:
> as discussed offline you can change this to reference http://crbug.com/137326

Done.

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:45: new
media::AudioRendererMixer(
On 2012/07/13 23:17:38, scherkus wrote:
> nit for code clarity -- can you break out these objects into separate
> statements?
> 
> media::AudioParameters output_params = ...;
> media::ARM mixer = ...;
> reference = { mixer, 1 };

Done.

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:65: AudioRendererMixerMap
mixer_map_;
On 2012/07/13 23:17:38, scherkus wrote:
> nit: don't bother with _map / _list to variable names -- it's a form of
> hungarian notation and doesn't add much value so go with |mixers_|

Done.

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:66: base::Lock
mixer_map_lock_;
On 2012/07/13 23:17:38, scherkus wrote:
> ditto here with mixers_lock_

Done.

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:58: };
On 2012/07/13 23:17:38, scherkus wrote:
> DISALLOW etc ... yes, even for testing code :)

Done.

[DaleCurtis, 2012-07-16 17:55:52]

On 2012/07/14 00:39:26, DaleCurtis wrote:
> Done.
> 
> scherkus: Still need you to finish reviewing
> http://codereview.chromium.org/10698066/ before this can land :)
> 
> jochen: Anything else?
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
> File content/public/common/content_switches.cc (right):
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/public/common/cont...
> content/public/common/content_switches.cc:315: // Enable renderer side mixing
> and the low latency output path for <audio>.
> On 2012/07/13 23:17:38, scherkus wrote:
> > s/<audio>/media elements/
> > 
> > I hope this works for <video> too :)
> 
> Done.
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> File content/renderer/media/audio_renderer_mixer_manager.cc (right):
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.cc:43: // is it okay for
> unit tests to call these methods?
> On 2012/07/13 23:17:38, scherkus wrote:
> > as discussed offline you can change this to reference
http://crbug.com/137326
> 
> Done.
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.cc:45: new
> media::AudioRendererMixer(
> On 2012/07/13 23:17:38, scherkus wrote:
> > nit for code clarity -- can you break out these objects into separate
> > statements?
> > 
> > media::AudioParameters output_params = ...;
> > media::ARM mixer = ...;
> > reference = { mixer, 1 };
> 
> Done.
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> File content/renderer/media/audio_renderer_mixer_manager.h (right):
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.h:65:
AudioRendererMixerMap
> mixer_map_;
> On 2012/07/13 23:17:38, scherkus wrote:
> > nit: don't bother with _map / _list to variable names -- it's a form of
> > hungarian notation and doesn't add much value so go with |mixers_|
> 
> Done.
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.h:66: base::Lock
> mixer_map_lock_;
> On 2012/07/13 23:17:38, scherkus wrote:
> > ditto here with mixers_lock_
> 
> Done.
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):
> 
>
http://codereview.chromium.org/10636036/diff/27001/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager_unittest.cc:58: };
> On 2012/07/13 23:17:38, scherkus wrote:
> > DISALLOW etc ... yes, even for testing code :)
> 
> Done.

Talked to jochen on chat; no more comments. CQ'ing. Thanks everyone!

[commit-bot: I haz the power, 2012-07-16 17:56:03]

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

[commit-bot: I haz the power, 2012-07-16 18:18:25]

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

[DaleCurtis, 2012-07-16 20:22:35]

scherkus: PTAL. The GetAudioHardware*() methods weren't working properly on bots
w/o audio hardware. I've added a virtual GetAudioHardwareParameters() which the
unit test now-subclasses to avoid a hardware dependency.

[scherkus (not reviewing), 2012-07-16 20:52:58]

http://codereview.chromium.org/10636036/diff/17017/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

http://codereview.chromium.org/10636036/diff/17017/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:36:
media::GetAudioHardwareBufferSize());
Two things:
  - This is kinda ugly
  - I just remembered you can't reliably call these methods inside the sandbox
anyway and should be using content/renderer/media/audio_hardware.cc

Reason why it works on linux because those methods return hardcoded numbers. I'd
expect this to fail on Mac/Windows where it calls OS APIs to get the actual
values.

Of course this presents some issues:
  - audio_hardware methods need to be called on the render thread
  - as discussed GetMixer/RemoveMixer is called on the pipeline thread
  - we could call them in the ctor...
  - ...but they are synchronous IPCs that really shouldn't be called during
RenderThreadImpl::Init()

How about this:
  - Modifying ARMM's ctor to accept a sample rate + buffer size parameter
  - Lazily create an ARMM by changing
RenderThreadImpl::audio_renderer_mixer_manager() into
GetAudioRendererMixerManager() that will call the audio_hardware methods +
create the ARMM

That will solve both the testing issue (you can pass in whatever you want) as
well as the sandboxing issue

WDYT?

[DaleCurtis, 2012-07-16 22:25:59]

http://codereview.chromium.org/10636036/diff/17017/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

http://codereview.chromium.org/10636036/diff/17017/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:36:
media::GetAudioHardwareBufferSize());
On 2012/07/16 20:52:59, scherkus wrote:
> Two things:
>   - This is kinda ugly
>   - I just remembered you can't reliably call these methods inside the sandbox
> anyway and should be using content/renderer/media/audio_hardware.cc
> 
> Reason why it works on linux because those methods return hardcoded numbers.
I'd
> expect this to fail on Mac/Windows where it calls OS APIs to get the actual
> values.
> 
> Of course this presents some issues:
>   - audio_hardware methods need to be called on the render thread
>   - as discussed GetMixer/RemoveMixer is called on the pipeline thread
>   - we could call them in the ctor...
>   - ...but they are synchronous IPCs that really shouldn't be called during
> RenderThreadImpl::Init()
> 
> How about this:
>   - Modifying ARMM's ctor to accept a sample rate + buffer size parameter
>   - Lazily create an ARMM by changing
> RenderThreadImpl::audio_renderer_mixer_manager() into
> GetAudioRendererMixerManager() that will call the audio_hardware methods +
> create the ARMM
> 
> That will solve both the testing issue (you can pass in whatever you want) as
> well as the sandboxing issue
> 
> WDYT?

SGTM. Done!

[scherkus (not reviewing), 2012-07-16 22:32:55]

lgtm!

[commit-bot: I haz the power, 2012-07-16 22:35:07]

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

[commit-bot: I haz the power, 2012-07-16 23:15:44]

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

[commit-bot: I haz the power, 2012-07-17 00:25:43]

Change committed as 146935