Switch to pcm_low_latency and enable resampling.

Switch to pcm_low_latency and enable resampling.

Plumbs all the relevant bits around.  Updates the unit tests to do a
basic resampling sanity check.

Still requires http://codereview.chromium.org/10636036/ before
anything will work on ToT.  I'll land that behind a flag later.

BUG=133637
TEST=These unit tests + CL above for manual testing.

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

[DaleCurtis, 2012-06-30 03:00:08]

PTAL Chris. Lots of bug squashing still to be done, but let me know if there are
any glaring design issues.

[Chris Rogers, 2012-07-02 22:53:50]

Overall design looks good.  Hopefully, using GetAudioHardwareBufferSize() below
will start yielding some good results.

To really test this, we'll need to try on win/mac/linux.  For now, we have no
"low-latency" patch on Linux, so the smaller buffer sizes really will come into
play on win/mac

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer.cc (right):

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:24: output_sample_rate, 16,
output_sample_rate / 100);
Instead of output_sample_rate / 100, better to use:

GetAudioHardwareBufferSize()

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:27: if (params.sample_rate() !=
output_sample_rate)
Maybe some basic sanity checking on params.sample_rate() to protect against wild
values (here and/or internal to MultiChannelResampler)

I thought we had pre-defined constants for min/max

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:81: // if we couldn't get enough data.
nit: fix up grammar here

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:110: int frames_filled =
input->callback()->Render(
small nit: this isn't new to this patch, but it still strikes me as odd that we
have a callback() method on the input.  Why don't we just implement a
ARMI::ProvideInput() method which internally accesses the callback.  Up to you,
but this seems a little round-about

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:127: // TODO(dalecurtis): Does the resampler
need values betwen [-1, 1] ?
The resampler will be fine.  I'd just entirely remove the comments on 125:127

[DaleCurtis, 2012-07-12 00:40:43]

crogers to cc since he's on vacation, scherkus please review.

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer.cc (right):

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:24: output_sample_rate, 16,
output_sample_rate / 100);
On 2012/07/02 22:53:50, Chris Rogers wrote:
> Instead of output_sample_rate / 100, better to use:
> 
> GetAudioHardwareBufferSize()

Done.

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:27: if (params.sample_rate() !=
output_sample_rate)
On 2012/07/02 22:53:50, Chris Rogers wrote:
> Maybe some basic sanity checking on params.sample_rate() to protect against
wild
> values (here and/or internal to MultiChannelResampler)
> 
> I thought we had pre-defined constants for min/max

Done.

http://codereview.chromium.org/10698066/diff/2001/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:81: // if we couldn't get enough data.
On 2012/07/02 22:53:50, Chris Rogers wrote:
> nit: fix up grammar here

Done.

[scherkus (not reviewing), 2012-07-12 23:02:52]

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:35: Initialize(params, AudioParameters(
should the test-only ARM(in, out, sink) ctor be "the only" ctor?

What's the rationale for baking in the output params for ARM clients? Should ARM
clients be able to choose their output params?

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:84: current_audio_delay_milliseconds_ =
audio_delay_milliseconds;
quick check: are we still planning on removing delay at some point?

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:86: if (resampler_ != NULL)
s/resampler_ != NULL/resampler_.get()/

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer.h (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.h:49: virtual void ProvideInput(const
std::vector<float*>& audio_data,
does this need to be virtual?

I didn't see any overrides elsewhere

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input_unittest.cc:28: EXPECT_CALL(*sink,
Stop());
ideally you should try to move this closer to where Stop() will get called
(dtor?)

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_unittest.cc:33: typedef std::tr1::tuple<int,
int, double> AudioRendererMixerTestData;
AFAIK a struct *should* work here instead, which makes for easier-to-read code
(std::tr1::get<1> vs output_sample_rate)

struct ARMTD {
  int input_sample_rate;
  int output_sample_rate;
  double epsilon;
};

http://codereview.chromium.org/10698066/diff/5006/media/base/fake_audio_rende...
File media/base/fake_audio_render_callback.h (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.h:19: explicit
FakeAudioRenderCallback(double step);
docs for step

http://codereview.chromium.org/10698066/diff/5006/media/base/mock_audio_rende...
File media/base/mock_audio_renderer_sink.h (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.h:16: MockAudioRendererSink() {}
don't inline ctor/dtor for any gmock-containing class inside a .h! in fact it's
easier to just never inline ctor+dtors for any class containing virtual methods

(all the MOCK_XXX stuff generates a ton of crud that ends up getting included in
each translation unit due to inlined ctor/dtor --> increased compile+link times)

http://codereview.chromium.org/10698066/diff/5006/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.h:27: void Initialize(const AudioParameters&
params,
add virtual + don't inline

[DaleCurtis, 2012-07-12 23:16:29]

No changes, just replies.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer.cc:35: Initialize(params, AudioParameters(
On 2012/07/12 23:02:52, scherkus wrote:
> should the test-only ARM(in, out, sink) ctor be "the only" ctor?
> 
> What's the rationale for baking in the output params for ARM clients? Should
ARM
> clients be able to choose their output params?

We could bake it into AudioRenderMixerManager (next CL) instead.  Then
media/base doesn't need to care about the audio hardware stuff, only
content/renderer/media.

We have to bake them in at least here because AudioRendererImpl needs to keep
percolating up the AudioParameters of the source sampling rate etc.  As such ARM
clients can't set their own output parameters.

Having them live in ARMM sounds fine with me. WDYT?

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer.cc:84: current_audio_delay_milliseconds_ =
audio_delay_milliseconds;
On 2012/07/12 23:02:52, scherkus wrote:
> quick check: are we still planning on removing delay at some point?

Yes, as far as I know. We can't do that until the rest of the pipeline is in
place though.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer_input_unittest.cc:28: EXPECT_CALL(*sink,
Stop());
On 2012/07/12 23:02:52, scherkus wrote:
> ideally you should try to move this closer to where Stop() will get called
> (dtor?)

Would need to make sink a class variable then, it used to be there though if
that's what you'd prefer.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer_unittest.cc:33: typedef std::tr1::tuple<int,
int, double> AudioRendererMixerTestData;
On 2012/07/12 23:02:52, scherkus wrote:
> AFAIK a struct *should* work here instead, which makes for easier-to-read code
> (std::tr1::get<1> vs output_sample_rate)
> 
> struct ARMTD {
>   int input_sample_rate;
>   int output_sample_rate;
>   double epsilon;
> };

I don't think we'd get nice error messages then. It'd be "where GetParam() = 12
byte<00-00-00-00 ..." etc.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
File media/base/mock_audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
media/base/mock_audio_renderer_sink.h:16: MockAudioRendererSink() {}
On 2012/07/12 23:02:52, scherkus wrote:
> don't inline ctor/dtor for any gmock-containing class inside a .h! in fact
it's
> easier to just never inline ctor+dtors for any class containing virtual
methods
> 
> (all the MOCK_XXX stuff generates a ton of crud that ends up getting included
in
> each translation unit due to inlined ctor/dtor --> increased compile+link
times)

Yeah this is all wrong, I recompiled today and clang yelled at me too. Not sure
why it passed yesterday.

[DaleCurtis, 2012-07-13 00:01:25]

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer.cc:35: Initialize(params, AudioParameters(
On 2012/07/12 23:16:29, DaleCurtis wrote:
> On 2012/07/12 23:02:52, scherkus wrote:
> > should the test-only ARM(in, out, sink) ctor be "the only" ctor?
> > 
> > What's the rationale for baking in the output params for ARM clients? Should
> ARM
> > clients be able to choose their output params?
> 
> We could bake it into AudioRenderMixerManager (next CL) instead.  Then
> media/base doesn't need to care about the audio hardware stuff, only
> content/renderer/media.
> 
> We have to bake them in at least here because AudioRendererImpl needs to keep
> percolating up the AudioParameters of the source sampling rate etc.  As such
ARM
> clients can't set their own output parameters.
> 
> Having them live in ARMM sounds fine with me. WDYT?

Next patch set does this.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer.cc:86: if (resampler_ != NULL)
On 2012/07/12 23:02:52, scherkus wrote:
> s/resampler_ != NULL/resampler_.get()/

Done.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer.h (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer.h:49: virtual void ProvideInput(const
std::vector<float*>& audio_data,
On 2012/07/12 23:02:52, scherkus wrote:
> does this need to be virtual?
> 
> I didn't see any overrides elsewhere

Done.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
media/base/audio_renderer_mixer_unittest.cc:33: typedef std::tr1::tuple<int,
int, double> AudioRendererMixerTestData;
On 2012/07/12 23:16:29, DaleCurtis wrote:
> On 2012/07/12 23:02:52, scherkus wrote:
> > AFAIK a struct *should* work here instead, which makes for easier-to-read
code
> > (std::tr1::get<1> vs output_sample_rate)
> > 
> > struct ARMTD {
> >   int input_sample_rate;
> >   int output_sample_rate;
> >   double epsilon;
> > };
> 
> I don't think we'd get nice error messages then. It'd be "where GetParam() =
12
> byte<00-00-00-00 ..." etc.

Confirmed. I'll need to add an ostream() operator override for the struct to go
this route.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/fake_aud...
File media/base/fake_audio_render_callback.h (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/fake_aud...
media/base/fake_audio_render_callback.h:19: explicit
FakeAudioRenderCallback(double step);
On 2012/07/12 23:02:52, scherkus wrote:
> docs for step

Done.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
File media/base/mock_audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
media/base/mock_audio_renderer_sink.h:16: MockAudioRendererSink() {}
On 2012/07/12 23:16:29, DaleCurtis wrote:
> On 2012/07/12 23:02:52, scherkus wrote:
> > don't inline ctor/dtor for any gmock-containing class inside a .h! in fact
> it's
> > easier to just never inline ctor+dtors for any class containing virtual
> methods
> > 
> > (all the MOCK_XXX stuff generates a ton of crud that ends up getting
included
> in
> > each translation unit due to inlined ctor/dtor --> increased compile+link
> times)
> 
> Yeah this is all wrong, I recompiled today and clang yelled at me too. Not
sure
> why it passed yesterday.

Done.

https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
media/base/mock_audio_renderer_sink.h:27: void Initialize(const AudioParameters&
params,
On 2012/07/12 23:02:52, scherkus wrote:
> add virtual + don't inline

Done.

[DaleCurtis, 2012-07-13 00:16:58]

On 2012/07/13 00:01:25, DaleCurtis wrote:
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> File media/base/audio_renderer_mixer.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> media/base/audio_renderer_mixer.cc:35: Initialize(params, AudioParameters(
> On 2012/07/12 23:16:29, DaleCurtis wrote:
> > On 2012/07/12 23:02:52, scherkus wrote:
> > > should the test-only ARM(in, out, sink) ctor be "the only" ctor?
> > > 
> > > What's the rationale for baking in the output params for ARM clients?
Should
> > ARM
> > > clients be able to choose their output params?
> > 
> > We could bake it into AudioRenderMixerManager (next CL) instead.  Then
> > media/base doesn't need to care about the audio hardware stuff, only
> > content/renderer/media.
> > 
> > We have to bake them in at least here because AudioRendererImpl needs to
keep
> > percolating up the AudioParameters of the source sampling rate etc.  As such
> ARM
> > clients can't set their own output parameters.
> > 
> > Having them live in ARMM sounds fine with me. WDYT?
> 
> Next patch set does this.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> media/base/audio_renderer_mixer.cc:86: if (resampler_ != NULL)
> On 2012/07/12 23:02:52, scherkus wrote:
> > s/resampler_ != NULL/resampler_.get()/
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> File media/base/audio_renderer_mixer.h (right):
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> media/base/audio_renderer_mixer.h:49: virtual void ProvideInput(const
> std::vector<float*>& audio_data,
> On 2012/07/12 23:02:52, scherkus wrote:
> > does this need to be virtual?
> > 
> > I didn't see any overrides elsewhere
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> File media/base/audio_renderer_mixer_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/audio_re...
> media/base/audio_renderer_mixer_unittest.cc:33: typedef std::tr1::tuple<int,
> int, double> AudioRendererMixerTestData;
> On 2012/07/12 23:16:29, DaleCurtis wrote:
> > On 2012/07/12 23:02:52, scherkus wrote:
> > > AFAIK a struct *should* work here instead, which makes for easier-to-read
> code
> > > (std::tr1::get<1> vs output_sample_rate)
> > > 
> > > struct ARMTD {
> > >   int input_sample_rate;
> > >   int output_sample_rate;
> > >   double epsilon;
> > > };
> > 
> > I don't think we'd get nice error messages then. It'd be "where GetParam() =
> 12
> > byte<00-00-00-00 ..." etc.
> 
> Confirmed. I'll need to add an ostream() operator override for the struct to
go
> this route.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/fake_aud...
> File media/base/fake_audio_render_callback.h (right):
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/fake_aud...
> media/base/fake_audio_render_callback.h:19: explicit
> FakeAudioRenderCallback(double step);
> On 2012/07/12 23:02:52, scherkus wrote:
> > docs for step
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
> File media/base/mock_audio_renderer_sink.h (right):
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
> media/base/mock_audio_renderer_sink.h:16: MockAudioRendererSink() {}
> On 2012/07/12 23:16:29, DaleCurtis wrote:
> > On 2012/07/12 23:02:52, scherkus wrote:
> > > don't inline ctor/dtor for any gmock-containing class inside a .h! in fact
> > it's
> > > easier to just never inline ctor+dtors for any class containing virtual
> > methods
> > > 
> > > (all the MOCK_XXX stuff generates a ton of crud that ends up getting
> included
> > in
> > > each translation unit due to inlined ctor/dtor --> increased compile+link
> > times)
> > 
> > Yeah this is all wrong, I recompiled today and clang yelled at me too. Not
> sure
> > why it passed yesterday.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10698066/diff/5006/media/base/mock_aud...
> media/base/mock_audio_renderer_sink.h:27: void Initialize(const
AudioParameters&
> params,
> On 2012/07/12 23:02:52, scherkus wrote:
> > add virtual + don't inline
> 
> Done.

Newest patch set also removes RefCounted<AudioRenderMixer> and the callback
changes we discussed earlier.

[scherkus (not reviewing), 2012-07-14 01:45:02]

LGTM w/ nits

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer.cc:84: current_audio_delay_milliseconds_ =
audio_delay_milliseconds;
On 2012/07/12 23:16:29, DaleCurtis wrote:
> On 2012/07/12 23:02:52, scherkus wrote:
> > quick check: are we still planning on removing delay at some point?
> 
> Yes, as far as I know. We can't do that until the rest of the pipeline is in
> place though.

SGTM

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input_unittest.cc:28: EXPECT_CALL(*sink,
Stop());
On 2012/07/12 23:16:29, DaleCurtis wrote:
> On 2012/07/12 23:02:52, scherkus wrote:
> > ideally you should try to move this closer to where Stop() will get called
> > (dtor?)
> 
> Would need to make sink a class variable then, it used to be there though if
> that's what you'd prefer.

ah didn't notice sink wasn't saved anywhere -- what you have here is fine

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/5006/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_unittest.cc:33: typedef std::tr1::tuple<int,
int, double> AudioRendererMixerTestData;
On 2012/07/13 00:01:25, DaleCurtis wrote:
> On 2012/07/12 23:16:29, DaleCurtis wrote:
> > On 2012/07/12 23:02:52, scherkus wrote:
> > > AFAIK a struct *should* work here instead, which makes for easier-to-read
> code
> > > (std::tr1::get<1> vs output_sample_rate)
> > > 
> > > struct ARMTD {
> > >   int input_sample_rate;
> > >   int output_sample_rate;
> > >   double epsilon;
> > > };
> > 
> > I don't think we'd get nice error messages then. It'd be "where GetParam() =
> 12
> > byte<00-00-00-00 ..." etc.
> 
> Confirmed. I'll need to add an ostream() operator override for the struct to
go
> this route.

yeah that's what I figured -- your call

I wonder if tr1 usage will compile everywhere but I guess we'll find out! I only
see 2 instances in all of chrome (!), which suggests to me it isn't too common
(both seem to be for parameterized tests, so I guess it's not too foreign of a
concept):
http://code.google.com/p/chromium/source/search?q=tr1%3A%3Atuple+-file%3Athir...

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_input.h (right):

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input.h:23: AudioRendererMixerInput(
nit: blank line before here

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input.h:56: AudioRendererMixer* mixer_;
add docs on ownership / lifetime (i.e., expected to live for duration of ARMI)

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input_unittest.cc:35: EXPECT_CALL(*this,
RemoveMixer(testing::_));
nit: can you use audio_parameters_ instead of testing::_ for a stronger
expectation?

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input_unittest.cc:39: if (mixer_ == NULL) {
s/mixer_ == NULL/!mixer_.get()/

(general comment: prefer boolean checks w/ pointers vs comparison to NULL --
this extends to DCHECK(), EXPECT_TRUE(), etc... it's pithier)

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_unittest.cc (right):

http://codereview.chromium.org/10698066/diff/1029/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_unittest.cc:37: class AudioRendererMixerTestCase
this should be ARMTest, not ARMTestCase

the only other instances of XXXTestCase for TEST_F/TEST_P tests in all of chrome
appear to be MultiChannelResamplerTestCase and SincResamplerTestCase -- you
should change those too :)

(can be follow up CL / TBR / etc)

[DaleCurtis, 2012-07-14 02:10:03]

Thanks!

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
File media/base/audio_renderer_mixer_input.h (right):

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_input.h:23: AudioRendererMixerInput(
On 2012/07/14 01:45:02, scherkus wrote:
> nit: blank line before here

Done.

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_input.h:56: AudioRendererMixer* mixer_;
On 2012/07/14 01:45:02, scherkus wrote:
> add docs on ownership / lifetime (i.e., expected to live for duration of ARMI)

Done.

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_input_unittest.cc:35: EXPECT_CALL(*this,
RemoveMixer(testing::_));
On 2012/07/14 01:45:02, scherkus wrote:
> nit: can you use audio_parameters_ instead of testing::_ for a stronger
> expectation?

Doesn't seem to like that:
error: invalid operands to binary expression ('const media::AudioParameters' and
'const media::AudioParameters')

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_input_unittest.cc:39: if (mixer_ == NULL) {
On 2012/07/14 01:45:02, scherkus wrote:
> s/mixer_ == NULL/!mixer_.get()/
> 
> (general comment: prefer boolean checks w/ pointers vs comparison to NULL --
> this extends to DCHECK(), EXPECT_TRUE(), etc... it's pithier)

Done.

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_unittest.cc:37: class AudioRendererMixerTestCase
On 2012/07/14 01:45:02, scherkus wrote:
> this should be ARMTest, not ARMTestCase
> 
> the only other instances of XXXTestCase for TEST_F/TEST_P tests in all of
chrome
> appear to be MultiChannelResamplerTestCase and SincResamplerTestCase -- you
> should change those too :)
> 
> (can be follow up CL / TBR / etc)

Done.

[commit-bot: I haz the power, 2012-07-14 02:13:25]

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

[scherkus (not reviewing), 2012-07-14 02:15:31]

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
File media/base/audio_renderer_mixer_input_unittest.cc (right):

https://chromiumcodereview.appspot.com/10698066/diff/1029/media/base/audio_re...
media/base/audio_renderer_mixer_input_unittest.cc:35: EXPECT_CALL(*this,
RemoveMixer(testing::_));
On 2012/07/14 02:10:03, DaleCurtis wrote:
> On 2012/07/14 01:45:02, scherkus wrote:
> > nit: can you use audio_parameters_ instead of testing::_ for a stronger
> > expectation?
> 
> Doesn't seem to like that:
> error: invalid operands to binary expression ('const media::AudioParameters'
and
> 'const media::AudioParameters')

I guess AP needs to implement operator==() -- I wouldn't worry about it then :)

[commit-bot: I haz the power, 2012-07-14 03:42:03]

Change committed as 146717