Collapse AudioRendererMixer and OnMoreDataResampler into AudioTransform.

Collapse AudioRendererMixer and OnMoreDataResampler into AudioTransform.

Currently we have roughly equivalent functionality in two places, and
the CloudView project will add a third.  As such there's a need for a
single super class which can handle mixing, resampling, and general
conversion from one set of AudioParameters to another.

This change introduces the AudioTransform object which collapses the
key functionality from AudioRendererMixer and OnMoreDataResampler into
a single AudioTransform class which can do everything and is oblivious
to the peculiars of RenderCallback vs AudioSourceCallback.

It also introduces output_frames_ready() methods to the AudioPullFifo
and MultiChannelResampler classes so that buffer delay can be measured
accurately without resorting to input vs output byte counting. 

Due to the bulk of AudioRendererMixer's functionality moving into the
new AudioTransform, it made sense to move some decisions into the
AudioRendererMixerInput class as well.

On my Z600, benchmarking 50000 iterations:
Convert() w/ FIFO took 7030.11ms.
Convert() w/o FIFO took 5218.83ms.

BUG=none
TEST=AudioTransform* unittests.
TBR=sergeyu

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

[DaleCurtis, 2012-11-10 05:23:22]

crogers, scherkus: Please review.  For ease of review the first patch set is a
copy of the original OnMoreDataResampler code, so you can just diff against that
to see the major changes in AudioTransform.

justinlin: FYI, this is the work I mentioned on your preliminary cloud view CL.

[DaleCurtis, 2012-11-12 20:15:23]

Some thoughts I had over the weekend:

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

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_unittest.cc:75: void InitializeInputs(int count)
{
Some more tests can probably be removed now that they're covered in
AudioTransfromTest.

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.cc
File media/base/audio_transform.cc (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.cc:18: AudioTransform::AudioTransform(const
AudioParameters& input_params,
I'll add micro-benchmark and run it against the old ARM code, to see how this
new AudioTransform performs.  I'll attempt to isolate into something like:
1. x fake render() calls.
2. x Transforms() with n input unresampled, unfifo'd.
4. x Transforms() with n input channel mixed.
5. x Transforms() with n input rebuffered.
6. x Transforms() with n input resampled.
7. x Transforms() with n input resampled, channel mixed.
8. x Transforms() with n input resampled, channel mixed, refifo'd.

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.cc:92: void
AudioTransform::AddInput(AudioTransformInput* input) {
What do you think about making AudioTransform thread safe w/ a lock and
providing a Map(<callback>) operation that can be applied to all
AudioTransformInputs.

This would allow us to remove the lock and list from AudioRendererMixer with
something like AudioTransform::Map(OnRenderErrorCallback) to issue down stream
errors for each input.

I suspect CloudView may need to do similar thread safe operations on the list.

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.cc:138: AudioBus* temp_dest = needs_downmix ?
unmixed_audio_.get() : dest;
Next CL Optimization: Make FIFO optional for clients, by automatically sizing
the mixer_input_audio_bus_ and unmixed_audio_ buses on the fly.

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.cc:162: float volume =
input->ProvideAudioTransformInput(
Next CL Optimization: Write directly to output bus for the first input and using
vector_math::FMUL / memset for volume < 1 and volume == 0 respectively.

[justinlin, 2012-11-12 20:29:38]

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.h:52: void AddInput(AudioTransformInput* input);
Purely from an interface point of view, would it make sense to instead pass the
input_params here? This saves from having to maintain maps of <AudioParams,
AudioTransforms> if we want to resample/mix multiple different input streams.

[DaleCurtis, 2012-11-12 20:48:18]

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.h:52: void AddInput(AudioTransformInput* input);
On 2012/11/12 20:29:38, justinlin wrote:
> Purely from an interface point of view, would it make sense to instead pass
the
> input_params here? This saves from having to maintain maps of <AudioParams,
> AudioTransforms> if we want to resample/mix multiple different input streams.

Currently AudioRendererMixerManager does a similar function, but abstracting
that here would be pretty difficult since we'd need to manage multiple
resamplers, fifos, etc and then feed them into a post-mixer. I'll give it some
thought, but I don't think we'd do that in this CL at least.

For now, I'd just transform all the audio into one set of parameters, and then
have one more AudioTransform which mixes all those together. I think you can
probably even do this without having to manage any maps or whatnot. just a list
of AudioTransforms and then a custom callback you provide to the n+1
AudioTransform which pulls data from the other AudioTransforms.

[miu, 2012-11-12 20:51:59]

You didn't ask, but here's my two cents.  ;-)

-Yuri

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:69: int input_bytes_per_second_;
nit: could be "const int"

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:284: io_ratio_ = static_cast<double>(
The indentation here makes it look like you're casting the entire RHS to double.
 Also, instead of multiplying, why not use params.GetBytesPerSecond()?

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer_input.cc:22:
current_audio_delay_milliseconds_(0) {
This new member is never updated anywhere.  Do you still need it?

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.cc
File media/base/audio_transform.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.cc:92: void
AudioTransform::AddInput(AudioTransformInput* input) {
On 2012/11/12 20:15:23, DaleCurtis wrote:
> What do you think about making AudioTransform thread safe w/ a lock and

I'm not familiar with how this all works at the higher level (i.e., glue between
inputs and single output, and also the threading scheme), but I'd like to be
educated.  :)  Can we chat on Wed?

https://codereview.chromium.org/11410012/diff/1004/media/base/fake_audio_rend...
File media/base/fake_audio_render_callback.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/fake_audio_rend...
media/base/fake_audio_render_callback.h:19: public
AudioTransform::AudioTransformInput {
I wonder whether it makes sense for AudioRendererSink::RenderCallback inherit
the AudioTransform::AudioTransformInput interface, and provide it's own default
implementation of ProvideAudioTransformInput() (which just calls Render())?

Is there a use case where a implementer of AudioRendererSink::RenderCallback
would do something different for ProvideAudioTransformInput()?

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
File media/base/multi_channel_resampler.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.h:42: int output_frames_ready() { return
output_frames_ready_; }
This appears to be monotonically increasing.  The word "ready" makes it sound
like some kind of temporary "what's currently available" thing.  Perhaps the
naming could be changed to something like output_frames_processed() or even
cumulative_output_frames_processed()?

[DaleCurtis, 2012-11-12 21:23:53]

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_input.cc (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_input.cc:22:
current_audio_delay_milliseconds_(0) {
On 2012/11/12 20:51:59, Yuri wrote:
> This new member is never updated anywhere.  Do you still need it?

Set in header file via hacker_style() setter.

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.cc
File media/base/audio_transform.cc (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform....
media/base/audio_transform.cc:92: void
AudioTransform::AddInput(AudioTransformInput* input) {
On 2012/11/12 20:51:59, Yuri wrote:
> On 2012/11/12 20:15:23, DaleCurtis wrote:
> > What do you think about making AudioTransform thread safe w/ a lock and
> 
> I'm not familiar with how this all works at the higher level (i.e., glue
between
> inputs and single output, and also the threading scheme), but I'd like to be
> educated.  :)  Can we chat on Wed?

I'm not sure I'll be in KIR this Wednesday, but we can chat whenever, send out a
mtg invite.

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

http://codereview.chromium.org/11410012/diff/1004/media/base/fake_audio_rende...
media/base/fake_audio_render_callback.h:19: public
AudioTransform::AudioTransformInput {
On 2012/11/12 20:51:59, Yuri wrote:
> I wonder whether it makes sense for AudioRendererSink::RenderCallback inherit
> the AudioTransform::AudioTransformInput interface, and provide it's own
default
> implementation of ProvideAudioTransformInput() (which just calls Render())?
> 
> Is there a use case where a implementer of AudioRendererSink::RenderCallback
> would do something different for ProvideAudioTransformInput()?

I considered that, but at present it doesn't seem worth the yak shave since most
callers will never interact with an AudioTransform directly.

On differences: one returns a float volume() while the other returns the number
of frames written to the bus. They also use an integer delay mechanism vs
TimeDelta.

Long term I could see us migrating to having the interfaces nearly the same, but
not presently.

http://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_re...
File media/base/multi_channel_resampler.h (right):

http://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_re...
media/base/multi_channel_resampler.h:42: int output_frames_ready() { return
output_frames_ready_; }
On 2012/11/12 20:51:59, Yuri wrote:
> This appears to be monotonically increasing.  The word "ready" makes it sound
> like some kind of temporary "what's currently available" thing.  Perhaps the
> naming could be changed to something like output_frames_processed() or even
> cumulative_output_frames_processed()?

It is not monotonically increasing. It is reset to 0 on each Resample() call.

[scherkus (not reviewing), 2012-11-14 22:43:50]

neato -- few q's

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_pull_fifo.cc
File media/base/audio_pull_fifo.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.cc:36: output_frames_ready_ = write_pos;
do we need to update output_frames_ready_ anywhere else? e.g., Clear()?

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer.h:43: typedef std::list<
scoped_refptr<AudioRendererMixerInput> >
nit: remove space after first <

that's how roll 'round here

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer_input.cc:83: current_audio_delay_milliseconds_ +
buffer_delay.InMilliseconds());
this mixing 'n matching between TimeDelta and integer milliseconds is a bit
whack

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:29: class MEDIA_EXPORT AudioTransformInput {
nit: this can be "Input" because its name is already scoped to AudioTransform

also... any motivation for interface vs. callback?

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
File media/base/multi_channel_resampler.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.h:67: };
missing DISALLOW_...

[Chris Rogers, 2012-11-14 23:50:49]

Dale, this looks really great overall - very nice cleanup!

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:50: //
AudioTransform::ProvideAudioTransformInput implementation.
class name is wrong

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:71: // Handles resampling, rebuffering,
and channel mixing between input and
rebuffering?  How about just "buffering"

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:238: // the
OnMoreDataTransform::ProvideInput_Locked() call.
Not that we need to change anything for this CL, but don't we want to get rid of
SetVolume?  I really hoped we could get rid of the volume and pause stuff on the
browser-side, since that was just an artifact of older <audio> implementation.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:286: (output_params.sample_rate() *
output_params.GetBytesPerFrame());
Not that we have to change this right now, but can we at least add a TODO here
that we want to get rid of relying on |input_bytes_per_second_|
(bytes-per-frame) at this level of the audio stack?  After all, both the input
and output parameters are dealing with AudioBus's with float32 data, regardless
of any wacky legacy info we are still propagating about bit-depth in the
AudioParameters

Instead we should always be dealing with number of frames at canonical float32

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:296: audio_transform_.AddInput(this);
It seems like AudioTransform is capable of doing mixing (multiple inputs) but
we're just using it in a simple way right now with one single input.  It's
probably worth a comment right here about this.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:342: 
If ProvideAudioTransformInput() is called multiple times (if there's a FIFO or
sample-rate conversion) then we'll call OnMoreIOData() multiple times with the
same |source_bus|.  We don't have to solve this problem right now, but I think
we need to add a warning comment here that the "synchronized" I/O needs more
work and that our use of |source_bus_| here is not quite fully baked.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.h:20: class OnMoreDataTransform;
OnMoreDataTransform sounds a bit awkward, how about OnMoreDataConverter?

In general, I recommend avoiding the word "transform" since this often implies
audio effects processing.  Instead, I'd recommend "converter".

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer.cc:56: }
This is a topic for another conversation and need not hold up this CL, but I'm
very convinced we're doing very much the wrong thing for audio/video sync in
using this information.  <audio>/<video> is not the same as WebRTC with AEC, and
there are much cleaner and smoother ways to get video sync.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:27: class MEDIA_EXPORT AudioTransform {
This is just a naming nit, but always good to get the names right:
I recommend avoiding "transform" and calling this class "AudioConverter".  This
is because "transform" has a lot of connotations in audio such as FFT, DCT,
wavelet transform, and effects processing.  "converter" is more often used here.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:34: // volume level of the provided audio data.  If
a volume level of zero is
can you elaborate a little more in this comment on exactly how volume is used

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
File media/base/multi_channel_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.cc:37: output_frames_ready_ = 0;
As we discussed, I think we need much better comments (perhaps in the .h file)
about output_frames_ready_, exactly what it is and how it's useful

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.cc:56: output_frames_ready_ +=
frames_this_time;
I don't understand how at the end of this loop that |output_frames_ready_| can
be anything other than |frames|.  Isn't it required that Resampler() process all
and exactly all of the |frames| requested?  Or is it that |output_frames_ready_|
is queried upstream during/in-the-middle of this loop?

[DaleCurtis, 2012-11-15 00:30:59]

(Just comments, next patch set later)

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:238: // the
OnMoreDataTransform::ProvideInput_Locked() call.
On 2012/11/14 23:50:49, Chris Rogers wrote:
> Not that we need to change anything for this CL, but don't we want to get rid
of
> SetVolume?  I really hoped we could get rid of the volume and pause stuff on
the
> browser-side, since that was just an artifact of older <audio> implementation.

Hmmm, is <audio> really the only person using stream volume adjustment? I guess
I thought WebAudio/WebRTC were passing on renderer side volume information too.

If that's the case, this TODO can just be dropped.  I'll also add a
vector_math::FAC() method in my optimization pass to skip the multiply step if
we're not going to be handling volume adjustment in most cases.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:286: (output_params.sample_rate() *
output_params.GetBytesPerFrame());
On 2012/11/14 23:50:49, Chris Rogers wrote:
> Not that we have to change this right now, but can we at least add a TODO here
> that we want to get rid of relying on |input_bytes_per_second_|
> (bytes-per-frame) at this level of the audio stack?  After all, both the input
> and output parameters are dealing with AudioBus's with float32 data,
regardless
> of any wacky legacy info we are still propagating about bit-depth in the
> AudioParameters
> 
> Instead we should always be dealing with number of frames at canonical float32

Yes I think that's a good idea, we need to stop passing AudioBufferState() in
bytes before that can happen though.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:342: 
On 2012/11/14 23:50:49, Chris Rogers wrote:
> If ProvideAudioTransformInput() is called multiple times (if there's a FIFO or
> sample-rate conversion) then we'll call OnMoreIOData() multiple times with the
> same |source_bus|.  We don't have to solve this problem right now, but I think
> we need to add a warning comment here that the "synchronized" I/O needs more
> work and that our use of |source_bus_| here is not quite fully baked.

Hmmm, should I just NULL the source_bus_ after the first call?

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_pull_fifo.cc
File media/base/audio_pull_fifo.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.cc:36: output_frames_ready_ = write_pos;
On 2012/11/14 22:43:50, scherkus wrote:
> do we need to update output_frames_ready_ anywhere else? e.g., Clear()?

No, it's not valid to call Clear() during Consume(), the class is not thread
safe.

I'm thinking it might be better to provide this value to the callback directly
instead of this weird side-channel value which is only valid at certain times.
There are only two users (opus and AudioTransform), so it shouldn't be a hard
change.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:29: class MEDIA_EXPORT AudioTransformInput {
On 2012/11/14 22:43:50, scherkus wrote:
> nit: this can be "Input" because its name is already scoped to AudioTransform
> 
> also... any motivation for interface vs. callback?

Just what we discussed at dinner the other day. You can stick callbacks in a
list :-/  Even providing an operator == for the Equals() method doesn't work if
you have construct two bound callbacks for the same call + params.

If we use a callback then we have to accept Callback* and have the client hold
onto it in order to remove it later. We could also take a void* key plus
callback and use an std::map<> which loses the ordering benefits of std::list.

Neither of those sounded appealing to me, so interface it is.

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
File media/base/multi_channel_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.cc:56: output_frames_ready_ +=
frames_this_time;
On 2012/11/14 23:50:49, Chris Rogers wrote:
> I don't understand how at the end of this loop that |output_frames_ready_| can
> be anything other than |frames|.  Isn't it required that Resampler() process
all
> and exactly all of the |frames| requested?  Or is it that
|output_frames_ready_|
> is queried upstream during/in-the-middle of this loop?

Queried upstream only in the middle of this loop. Which is why I think changing
this to be provided via the callback mechanism makes more sense. I'll change
this in the next patch set.

[DaleCurtis, 2012-11-16 23:51:04]

PTAL.

Added benchmark + numbers. We could squeeze ~4% more out by:
1. Writing directly to the output bus for a single input. 
2. Adding a vector_math::FAC() for volume == 1.0

But the code gets hairier than I think it's worth for such a small gain.  The
FIFO stuff for 20% was well worth it though.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:50: //
AudioTransform::ProvideAudioTransformInput implementation.
On 2012/11/14 23:50:49, Chris Rogers wrote:
> class name is wrong

Done.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:69: int input_bytes_per_second_;
On 2012/11/12 20:51:59, Yuri wrote:
> nit: could be "const int"

Done.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:71: // Handles resampling, rebuffering,
and channel mixing between input and
On 2012/11/14 23:50:49, Chris Rogers wrote:
> rebuffering?  How about just "buffering"

Done.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:284: io_ratio_ = static_cast<double>(
On 2012/11/12 20:51:59, Yuri wrote:
> The indentation here makes it look like you're casting the entire RHS to
double.
>  Also, instead of multiplying, why not use params.GetBytesPerSecond()?

Done.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.cc:296: audio_transform_.AddInput(this);
On 2012/11/14 23:50:49, Chris Rogers wrote:
> It seems like AudioTransform is capable of doing mixing (multiple inputs) but
> we're just using it in a simple way right now with one single input.  It's
> probably worth a comment right here about this.

Done.

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
File media/audio/audio_output_resampler.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/audio/audio_output_r...
media/audio/audio_output_resampler.h:20: class OnMoreDataTransform;
On 2012/11/14 23:50:49, Chris Rogers wrote:
> OnMoreDataTransform sounds a bit awkward, how about OnMoreDataConverter?
> 
> In general, I recommend avoiding the word "transform" since this often implies
> audio effects processing.  Instead, I'd recommend "converter".

Done.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer.h:43: typedef std::list<
scoped_refptr<AudioRendererMixerInput> >
On 2012/11/14 22:43:50, scherkus wrote:
> nit: remove space after first <
> 
> that's how roll 'round here

Hideous!!!

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer_input.cc:83: current_audio_delay_milliseconds_ +
buffer_delay.InMilliseconds());
On 2012/11/14 22:43:50, scherkus wrote:
> this mixing 'n matching between TimeDelta and integer milliseconds is a bit
> whack

Yeah, the end goal is everyone using TimeDelta everywhere, but I'm not
yak-shaving that in this CL.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.cc
File media/base/audio_transform.cc (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.cc:92: void
AudioTransform::AddInput(AudioTransformInput* input) {
On 2012/11/12 21:23:53, DaleCurtis wrote:
> On 2012/11/12 20:51:59, Yuri wrote:
> > On 2012/11/12 20:15:23, DaleCurtis wrote:
> > > What do you think about making AudioTransform thread safe w/ a lock and
> > 
> > I'm not familiar with how this all works at the higher level (i.e., glue
> between
> > inputs and single output, and also the threading scheme), but I'd like to be
> > educated.  :)  Can we chat on Wed?
> 
> I'm not sure I'll be in KIR this Wednesday, but we can chat whenever, send out
a
> mtg invite.

I'll add the Map() operation in a follow up CL and we can discuss there.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.cc:162: float volume =
input->ProvideAudioTransformInput(
On 2012/11/12 20:15:23, DaleCurtis wrote:
> Next CL Optimization: Write directly to output bus for the first input and
using
> vector_math::FMUL / memset for volume < 1 and volume == 0 respectively.

Tried this and it didn't make a huge impact. ~100ms over 50000 iterations over 3
inputs.  I suspect it would matter more with larger channel counts.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform.h
File media/base/audio_transform.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:27: class MEDIA_EXPORT AudioTransform {
On 2012/11/14 23:50:49, Chris Rogers wrote:
> This is just a naming nit, but always good to get the names right:
> I recommend avoiding "transform" and calling this class "AudioConverter". 
This
> is because "transform" has a lot of connotations in audio such as FFT, DCT,
> wavelet transform, and effects processing.  "converter" is more often used
here.

Done. I'll rename the file in the last patch set to ease reviewing.

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:29: class MEDIA_EXPORT AudioTransformInput {
On 2012/11/15 00:30:59, DaleCurtis wrote:
> On 2012/11/14 22:43:50, scherkus wrote:
> > nit: this can be "Input" because its name is already scoped to
AudioTransform
> > 
> > also... any motivation for interface vs. callback?
> 
> Just what we discussed at dinner the other day. You can stick callbacks in a
> list :-/  Even providing an operator == for the Equals() method doesn't work
if
> you have construct two bound callbacks for the same call + params.
> 
> If we use a callback then we have to accept Callback* and have the client hold
> onto it in order to remove it later. We could also take a void* key plus
> callback and use an std::map<> which loses the ordering benefits of std::list.
> 
> Neither of those sounded appealing to me, so interface it is.

Changed to InputCallback a la RenderCallback

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_transform...
media/base/audio_transform.h:34: // volume level of the provided audio data.  If
a volume level of zero is
On 2012/11/14 23:50:49, Chris Rogers wrote:
> can you elaborate a little more in this comment on exactly how volume is used

Done.

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
File media/base/multi_channel_resampler.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/multi_channel_r...
media/base/multi_channel_resampler.h:67: };
On 2012/11/14 22:43:50, scherkus wrote:
> missing DISALLOW_...

Done.

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transform.h
File media/base/audio_transform.h (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
media/base/audio_transform.h:52: bool disable_fifo);
Added this after benchmark shows a 20% perf hit when the FIFO is invoked.  This
is because using the fifo results in 3x the memcpy. Once from the callback, once
into the fifo, and once into the output.

[scherkus (not reviewing), 2012-11-17 01:04:37]

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/11410012/diff/1004/media/base/audio_renderer_...
media/base/audio_renderer_mixer.h:43: typedef std::list<
scoped_refptr<AudioRendererMixerInput> >
On 2012/11/16 23:51:05, DaleCurtis wrote:
> On 2012/11/14 22:43:50, scherkus wrote:
> > nit: remove space after first <
> > 
> > that's how roll 'round here
> 
> Hideous!!!

blame c++

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:281: audio_converter_(input_params,
output_params, false) {
who calls disable_fifo w/ true?

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
File media/base/audio_transform_unittest.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
media/base/audio_transform_unittest.cc:210: TEST(AudioConverterTest,
ConvertBenchmark) {
is this going to run every time and logspam to console?

it can be DISABLED_ and combined with --gtest_also_run_disabled_tests
--gtest_filter=AudioConvererTest.ConvertBenchmark

[scherkus (not reviewing), 2012-11-17 01:06:05]

don't forget to rename files

[DaleCurtis, 2012-11-17 01:07:14]

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:281: audio_converter_(input_params,
output_params, false) {
On 2012/11/17 01:04:37, scherkus wrote:
> who calls disable_fifo w/ true?

AudioRendererMixer.

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
File media/base/audio_transform_unittest.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
media/base/audio_transform_unittest.cc:210: TEST(AudioConverterTest,
ConvertBenchmark) {
On 2012/11/17 01:04:37, scherkus wrote:
> is this going to run every time and logspam to console?
> 
> it can be DISABLED_ and combined with --gtest_also_run_disabled_tests
> --gtest_filter=AudioConvererTest.ConvertBenchmark

Yes and yes it could be. fischman and I discussed this some time back and
settled on leaving it enabled with a small number of iterations (see
SincResampler and VectorMath unittests). Two printfs() in one test aren't going
to be even noticed by anyone.

[scherkus (not reviewing), 2012-11-20 00:51:10]

lgtm assuming you rename files

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
File media/base/audio_transform_unittest.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_transfor...
media/base/audio_transform_unittest.cc:210: TEST(AudioConverterTest,
ConvertBenchmark) {
On 2012/11/17 01:07:14, DaleCurtis wrote:
> On 2012/11/17 01:04:37, scherkus wrote:
> > is this going to run every time and logspam to console?
> > 
> > it can be DISABLED_ and combined with --gtest_also_run_disabled_tests
> > --gtest_filter=AudioConvererTest.ConvertBenchmark
> 
> Yes and yes it could be. fischman and I discussed this some time back and
> settled on leaving it enabled with a small number of iterations (see
> SincResampler and VectorMath unittests). Two printfs() in one test aren't
going
> to be even noticed by anyone.

I'll let it slide but this is some slippery-slope reasonsing :)

[DaleCurtis, 2012-11-20 00:52:55]

Chris, any last comments? Yuri, did you want to review?

[miu, 2012-11-20 00:58:01]

lgtm

Thanks for doing this so quickly.  It really moves us forward a lot (to get tab
mirroring working).

[Chris Rogers, 2012-11-20 20:01:24]

lgtm if nits are addressed.

Don't forget to rename audio_transform -> audio_converter :)

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:336: AudioBuffersState new_buffers_state;
TODO to stop dealing in calculations in units of bytes and switch to frames

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:345: // |source_bus_| should only be
provided once.
Setting to NULL doesn't really solve the problem (which we don't need to solve
in this CL).  We should add a TODO describing that *if* we're called back more
than once then we currently don't handle |source_bus_| correctly.  That's ok for
now, but just need to make a note...

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_pull_fif...
media/base/audio_pull_fifo.h:25: typedef base::Callback<void(int frame_delay,
AudioBus* audio_bus)> ReadCB;
can we switch the order of these arguments -- the most important arguments
should always go first in functions/methods, which in this case is |audio_bus|

|frame_delay| is only of specialized interest and would often be ignored in
general purpose uses of this class

[DaleCurtis, 2012-11-21 00:33:30]

Thanks for review guys. Rename completed.

TBR=sergeyu for remoting/ function prototype update.

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:336: AudioBuffersState new_buffers_state;
On 2012/11/20 20:01:24, Chris Rogers wrote:
> TODO to stop dealing in calculations in units of bytes and switch to frames

Done.

https://codereview.chromium.org/11410012/diff/18001/media/audio/audio_output_...
media/audio/audio_output_resampler.cc:345: // |source_bus_| should only be
provided once.
On 2012/11/20 20:01:24, Chris Rogers wrote:
> Setting to NULL doesn't really solve the problem (which we don't need to solve
> in this CL).  We should add a TODO describing that *if* we're called back more
> than once then we currently don't handle |source_bus_| correctly.  That's ok
for
> now, but just need to make a note...

Done.

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

https://codereview.chromium.org/11410012/diff/18001/media/base/audio_pull_fif...
media/base/audio_pull_fifo.h:25: typedef base::Callback<void(int frame_delay,
AudioBus* audio_bus)> ReadCB;
On 2012/11/20 20:01:24, Chris Rogers wrote:
> can we switch the order of these arguments -- the most important arguments
> should always go first in functions/methods, which in this case is |audio_bus|
> 
> |frame_delay| is only of specialized interest and would often be ignored in
> general purpose uses of this class

Sorry, Chrome style is the opposite: input values first and output values last.

[commit-bot: I haz the power, 2012-11-21 00:34:27]

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

[commit-bot: I haz the power, 2012-11-21 04:24:21]

Change committed as 168976