Introduce AudioOutputResampler for browser side resampling.

As titled! AudioOutputResampler is a single stream output resampler which uses
the existing AudioOutputDispatcherImpl to handle the heavy lifting. It intercepts 
the AudioSourceCallback provided during OpenStream() and replaces it with a 
call to itself.

An AudioFifo object is used to ensure we interface with the renderer and 
downstream components using the same buffer size they were configured with.

A MultiChannelResampler object interfaces with a provided callback method 
which pulls data from the AudioFifo if available and otherwise retrieves more 
data from the client (using the buffer size the client expects).

The pending bytes value is scaled according to the input / output sample rate 
and bits per channel values. Outstanding data is tracked between OnMoreData() 
calls in order to provide the client with an accurate delay estimate.

Feature is behind the "--enable-audio-output-resampler" for a/b testing. Will be
removed from behind the flag after tuning and functionality can be verified by 
the Pepper Flash team.

BUG=147572
TEST=PepperFlash + 192kHz->44kHz, 48kHz->44kHz, 44kHz->96kHz. Manual
testing by various parties.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155968

[DaleCurtis, 2012-09-06 16:27:45]

PTAL and provide early feedback (don't worry about nits for now). Works locally
on my mac using <audio>. I don't have a windows or flash box to test on
currently. 

Will build and test with NaCl + bastion shortly though.

[Chris Rogers, 2012-09-06 19:18:25]

Thanks Dale, I can see you're hacking into the AudioFifo directly instead of
using the forthcoming AudioPullFifo.

I'll play around with the patch...

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:64: audio_fifo_.reset(new AudioFifo(
Even if we're not doing sample-rate conversion, we still might need the FIFO to
match the client buffer size vs. the hardware buffer size.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:133: return
source_callback_->OnMoreData(audio_bus, buffers_state);
We need to have a conditional here to check if we need to use the FIFO (when
client buffer size does not match hardware buffer size).

[henrika (OOO until Aug 14), 2012-09-06 19:47:35]

I am sure Dale can add usage of the AudioPullFifo tomorrow.


On Thu, Sep 6, 2012 at 9:18 PM, <crogers@google.com> wrote:

> Thanks Dale, I can see you're hacking into the AudioFifo directly instead
> of
> using the forthcoming AudioPullFifo.
>
> I'll play around with the patch...
>
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc>
> File media/audio/audio_output_**resampler.cc (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode64<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode64>
> media/audio/audio_output_**resampler.cc:64: audio_fifo_.reset(new
> AudioFifo(
> Even if we're not doing sample-rate conversion, we still might need the
> FIFO to match the client buffer size vs. the hardware buffer size.
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode133<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode133>
> media/audio/audio_output_**resampler.cc:133: return
> source_callback_->OnMoreData(**audio_bus, buffers_state);
> We need to have a conditional here to check if we need to use the FIFO
> (when client buffer size does not match hardware buffer size).
>
>
https://chromiumcodereview.**appspot.com/10918098/<https://chromiumcodereview...
>

[DaleCurtis, 2012-09-07 13:06:37]

I figured out how to enable PepperFlash on my Mac and resampling / buffer rate
conversion appears to be working fine! \o/

I'll switch over to AudioPullFIFO now and address the other comments. Hopefully
have a new patch set up shortly.

[scherkus (not reviewing), 2012-09-07 13:30:16]

looking good so far -- haven't done a deep inspection of the actual output
resampler bits

going to take a look at henrika's AudioPullFifo CL now

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_m...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_m...
media/audio/audio_manager_base.cc:165: //dispatcher = new
AudioOutputDispatcherImpl(this, params, close_delay);
remove or is this supposed to be a flag?

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:10: #include "base/compiler_specific.h"
a->z ordering

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:87: return dispatcher_->StartStream(this,
stream_proxy);
which thread is calling these methods?

is dispatcher_ also thread safe?

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:131: // TODO(dalecurtis): Copy ASB to
local variable, release lock prior to call?
what's ASB?

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:160: CHECK(audio_fifo_->Consume(audio_bus,
0, frames));
:)

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.h:6: // always output at the configured
hardware parameters.
move to class def instead of top of file

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.h:21: class AudioFifo;
nit: blank line before

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/audio_fi...
File media/base/audio_fifo_unittest.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/audio_fi...
media/base/audio_fifo_unittest.cc:12: #if 0
!

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
File media/base/multi_channel_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
media/base/multi_channel_resampler.h:37: // Flush all buffered data and reset
internal indexes.
s/indexes/indicies/

$ git grep -i "indexes" . | wc -l
     995
$ git grep -i "indices" . | wc -l
    2619

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
media/base/multi_channel_resampler.h:41: friend class MultiChannelResamplerTest;
hrmm.. friend only for last_frame_count_?

I'd either just ignore having a test for that or if you really want to test it
provide a last_frame_count_for_test() method

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
File media/base/sinc_resampler_unittest.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:74: resampler.r4_ + (resampler.r4_ -
resampler.r3_) - resampler.r1_);
instead of friend test + poking around internals, perhaps we can add some
xxx_for_test() / XxxForTest() methods on SinceResampler that does some of this
work for us?

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:75: int buffer_size_bytes =
buffer_size_float * sizeof(*(
looks bizarre -- drop sizeof to next line?

[henrika (OOO until Aug 14), 2012-09-07 14:08:10]

Nice work Dale. My plan was to wait for complete CL (with AudioPullFifo)
and try it out on my Windows machine first with Pepper Flash. Guess
functionality is more important than style initially ;-)


On Fri, Sep 7, 2012 at 3:30 PM, <scherkus@chromium.org> wrote:

> looking good so far -- haven't done a deep inspection of the actual output
> resampler bits
>
> going to take a look at henrika's AudioPullFifo CL now
>
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_**manager_base.cc<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_manager_base.cc>
> File media/audio/audio_manager_**base.cc (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_**manager_base.cc#newcode165<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_manager_base.cc#newcode165>
> media/audio/audio_manager_**base.cc:165: //dispatcher = new
> AudioOutputDispatcherImpl(**this, params, close_delay);
> remove or is this supposed to be a flag?
>
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc>
> File media/audio/audio_output_**resampler.cc (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode10<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode10>
> media/audio/audio_output_**resampler.cc:10: #include
> "base/compiler_specific.h"
> a->z ordering
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode87<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode87>
> media/audio/audio_output_**resampler.cc:87: return
> dispatcher_->StartStream(this, stream_proxy);
> which thread is calling these methods?
>
> is dispatcher_ also thread safe?
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode131<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode131>
> media/audio/audio_output_**resampler.cc:131: // TODO(dalecurtis): Copy ASB
> to local variable, release lock prior to call?
> what's ASB?
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.cc#newcode160<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.cc#newcode160>
> media/audio/audio_output_**resampler.cc:160:
> CHECK(audio_fifo_->Consume(**audio_bus, 0, frames));
> :)
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.h<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.h>
> File media/audio/audio_output_**resampler.h (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.h#newcode6<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.h#newcode6>
> media/audio/audio_output_**resampler.h:6: // always output at the
> configured hardware parameters.
> move to class def instead of top of file
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/audio/audio_output_**resampler.h#newcode21<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_output_resampler.h#newcode21>
> media/audio/audio_output_**resampler.h:21: class AudioFifo;
> nit: blank line before
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/audio_fifo_**unittest.cc<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/audio_fifo_unittest.cc>
> File media/base/audio_fifo_**unittest.cc (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/audio_fifo_**unittest.cc#newcode12<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/audio_fifo_unittest.cc#newcode12>
> media/base/audio_fifo_**unittest.cc:12: #if 0
> !
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/multi_channel_**resampler.h<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_channel_resampler.h>
> File media/base/multi_channel_**resampler.h (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/multi_channel_**resampler.h#newcode37<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_channel_resampler.h#newcode37>
> media/base/multi_channel_**resampler.h:37: // Flush all buffered data and
> reset internal indexes.
> s/indexes/indicies/
>
> $ git grep -i "indexes" . | wc -l
>      995
> $ git grep -i "indices" . | wc -l
>     2619
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/multi_channel_**resampler.h#newcode41<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_channel_resampler.h#newcode41>
> media/base/multi_channel_**resampler.h:41: friend class
> MultiChannelResamplerTest;
> hrmm.. friend only for last_frame_count_?
>
> I'd either just ignore having a test for that or if you really want to
> test it provide a last_frame_count_for_test() method
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/sinc_**resampler_unittest.cc<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_resampler_unittest.cc>
> File media/base/sinc_resampler_**unittest.cc (right):
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/sinc_**resampler_unittest.cc#**newcode74<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_resampler_unittest.cc#newcode74>
> media/base/sinc_resampler_**unittest.cc:74: resampler.r4_ + (resampler.r4_
> - resampler.r3_) - resampler.r1_);
> instead of friend test + poking around internals, perhaps we can add
> some xxx_for_test() / XxxForTest() methods on SinceResampler that does
> some of this work for us?
>
> https://chromiumcodereview.**appspot.com/10918098/diff/**
>
2001/media/base/sinc_**resampler_unittest.cc#**newcode75<https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_resampler_unittest.cc#newcode75>
> media/base/sinc_resampler_**unittest.cc:75: int buffer_size_bytes =
> buffer_size_float * sizeof(*(
> looks bizarre -- drop sizeof to next line?
>
>
https://chromiumcodereview.**appspot.com/10918098/<https://chromiumcodereview...
>

[DaleCurtis, 2012-09-07 14:17:35]

Will address nits later. Just comments for now.

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:165: //dispatcher = new
AudioOutputDispatcherImpl(this, params, close_delay);
On 2012/09/07 13:30:16, scherkus wrote:
> remove or is this supposed to be a flag?

Good question. I think it'll be removed. Should probably have a flag just for
A/B testing for issues.

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_output_re...
File media/audio/audio_output_resampler.cc (right):

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:64: audio_fifo_.reset(new AudioFifo(
On 2012/09/06 19:18:25, Chris Rogers wrote:
> Even if we're not doing sample-rate conversion, we still might need the FIFO
to
> match the client buffer size vs. the hardware buffer size.

Good point, I'll rework it so buffer size differences bypass the resampler.

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:87: return dispatcher_->StartStream(this,
stream_proxy);
On 2012/09/07 13:30:16, scherkus wrote:
> which thread is calling these methods?
> 
> is dispatcher_ also thread safe?

Called by the audio thread, AudioOutputDispatcherImpl has,  
DCHECK_EQ(MessageLoop::current(), message_loop_); everywhere so it's fine. I can
duplicate those checks here if we want.

|dispatcher_| itself is initialized during the constructor, so it's fine.

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:131: // TODO(dalecurtis): Copy ASB to
local variable, release lock prior to call?
On 2012/09/07 13:30:16, scherkus wrote:
> what's ASB?

A typo :) Should be ASCB.. Essentially instead of holding the lock for
source_callback_, copy it to a local variable and then call it, so the
OnMoreData() call isn't blocked by any callers.

http://codereview.chromium.org/10918098/diff/2001/media/audio/audio_output_re...
media/audio/audio_output_resampler.cc:133: return
source_callback_->OnMoreData(audio_bus, buffers_state);
On 2012/09/06 19:18:25, Chris Rogers wrote:
> We need to have a conditional here to check if we need to use the FIFO (when
> client buffer size does not match hardware buffer size).

Will investigate, shouldn't be too hard.

http://codereview.chromium.org/10918098/diff/2001/media/base/audio_fifo_unitt...
File media/base/audio_fifo_unittest.cc (right):

http://codereview.chromium.org/10918098/diff/2001/media/base/audio_fifo_unitt...
media/base/audio_fifo_unittest.cc:12: #if 0
On 2012/09/07 13:30:17, scherkus wrote:
> !

Just my hacked version for now. Didn't you read my published message? No nits!
:)

http://codereview.chromium.org/10918098/diff/2001/media/base/sinc_resampler_u...
File media/base/sinc_resampler_unittest.cc (right):

http://codereview.chromium.org/10918098/diff/2001/media/base/sinc_resampler_u...
media/base/sinc_resampler_unittest.cc:74: resampler.r4_ + (resampler.r4_ -
resampler.r3_) - resampler.r1_);
On 2012/09/07 13:30:17, scherkus wrote:
> instead of friend test + poking around internals, perhaps we can add some
> xxx_for_test() / XxxForTest() methods on SinceResampler that does some of this
> work for us?

I like this idea. Will change.

[DaleCurtis, 2012-09-10 12:06:24]

PTAL. Working out a quick unittest using the existing AudioOutputProxy settings
now.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_m...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_m...
media/audio/audio_manager_base.cc:165: //dispatcher = new
AudioOutputDispatcherImpl(this, params, close_delay);
On 2012/09/07 14:17:35, DaleCurtis wrote:
> On 2012/09/07 13:30:16, scherkus wrote:
> > remove or is this supposed to be a flag?
> 
> Good question. I think it'll be removed. Should probably have a flag just for
> A/B testing for issues.

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:10: #include "base/compiler_specific.h"
On 2012/09/07 13:30:16, scherkus wrote:
> a->z ordering

Removed.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:64: audio_fifo_.reset(new AudioFifo(
On 2012/09/07 14:17:35, DaleCurtis wrote:
> On 2012/09/06 19:18:25, Chris Rogers wrote:
> > Even if we're not doing sample-rate conversion, we still might need the FIFO
> to
> > match the client buffer size vs. the hardware buffer size.
> 
> Good point, I'll rework it so buffer size differences bypass the resampler.

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.cc:133: return
source_callback_->OnMoreData(audio_bus, buffers_state);
On 2012/09/07 14:17:35, DaleCurtis wrote:
> On 2012/09/06 19:18:25, Chris Rogers wrote:
> > We need to have a conditional here to check if we need to use the FIFO (when
> > client buffer size does not match hardware buffer size).
> 
> Will investigate, shouldn't be too hard.

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.h:6: // always output at the configured
hardware parameters.
On 2012/09/07 13:30:17, scherkus wrote:
> move to class def instead of top of file

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/audio/audio_o...
media/audio/audio_output_resampler.h:21: class AudioFifo;
On 2012/09/07 13:30:17, scherkus wrote:
> nit: blank line before

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
File media/base/multi_channel_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
media/base/multi_channel_resampler.h:37: // Flush all buffered data and reset
internal indexes.
On 2012/09/07 13:30:17, scherkus wrote:
> s/indexes/indicies/
> 
> $ git grep -i "indexes" . | wc -l
>      995
> $ git grep -i "indices" . | wc -l
>     2619

Done.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/multi_ch...
media/base/multi_channel_resampler.h:41: friend class MultiChannelResamplerTest;
On 2012/09/07 13:30:17, scherkus wrote:
> hrmm.. friend only for last_frame_count_?
> 
> I'd either just ignore having a test for that or if you really want to test it
> provide a last_frame_count_for_test() method

Removed.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
File media/base/sinc_resampler_unittest.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:74: resampler.r4_ + (resampler.r4_ -
resampler.r3_) - resampler.r1_);
On 2012/09/07 14:17:35, DaleCurtis wrote:
> On 2012/09/07 13:30:17, scherkus wrote:
> > instead of friend test + poking around internals, perhaps we can add some
> > xxx_for_test() / XxxForTest() methods on SinceResampler that does some of
this
> > work for us?
> 
> I like this idea. Will change.

Found a better way => Prime with junk data, Flush(), ensure correct results.

https://chromiumcodereview.appspot.com/10918098/diff/2001/media/base/sinc_res...
media/base/sinc_resampler_unittest.cc:75: int buffer_size_bytes =
buffer_size_float * sizeof(*(
On 2012/09/07 13:30:17, scherkus wrote:
> looks bizarre -- drop sizeof to next line?

Removed.

[henrika (OOO until Aug 14), 2012-09-10 12:41:16]

Some trivial high-level comments. Have not dived in to the details yet. Rather
heavy stuff.

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:130: format =
media::AudioParameters::AUDIO_PCM_LOW_LATENCY;
Some comments here perhaps.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.h:23: // which ensures audio data is always
output at given parameters.
No restrictions what so ever?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
media/base/media_switches.cc:41: // Enable AudioOutputResampler for automatic
audio resampling and rebuffering.
Sorry, nit, why add 'automatic"?

[scherkus (not reviewing), 2012-09-10 12:47:46]

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:126: //
TODO(dalecurtis): Limit to windows and 2ch linux/mac only since neither of
nit: proper capitalization on Windows/Linux/Mac

also this comment is a bit confusing -- are you saying that Windows supports
multichannel but Linux/Mac only support 2 channels? What about mono?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_m...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_m...
media/audio/audio_manager_base.cc:162: close_delay);
I think we can clean up the confusing #if code if we returned the new
AOP(dispatcher) here + deleted the else branch

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:57: // the caller asked for, we need to
use a FIFO to ensure that both side read
s/side/sides/?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:123: current_buffers_state_ =
buffers_state;
It's a bit concerning we have to save this information so that we can use it in
a completely different method (SourceCallback())

Do we have an idea on how to handle AudioBuffersState in an ideal world? For
example, should it be part of AudioBus? Should FIFOs and resamplers be
responsible for adjusting the value as they process audio callbacks?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:127: outstanding_audio_bytes_ = 0;
shouldn't this always be zero in this case?

we don't delete / create resample_ and audio_fifo_ at post-initialization do we?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:181: audio_fifo_->Consume(audio_bus,
audio_bus->frames());
this will CHECK() now -- do we need to inspect any params before calling?

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:185: // TODO(dalecurtis): Copy ASCB to
local variable, release lock prior to call?
nit: instead of referring to the type ASCB, just say |source_callback_|

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:191: void
AudioOutputResampler::WaitTillDataReady() {
ditto

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.h:23: // which ensures audio data is always
output at given parameters.
might be worth having a little ascii diagram to show which components this class
uses and in which cases (i.e., FIFO may/may not be present, resampler may/may
not be present)

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/audio_pu...
File media/base/audio_pull_fifo.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/audio_pu...
media/base/audio_pull_fifo.h:39: void Clear() { fifo_->Clear(); }
docs + de-inline (it's not a simple getter)

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
media/base/media_switches.cc:42: const char kEnableAudioOutputResampler[] =
"enable-audio-output-resampler";
if this is only used in content then it should go under content_switches.cc

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/sinc_res...
File media/base/sinc_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/sinc_res...
media/base/sinc_resampler.h:38: // Flush all buffered data and reset internal
indexes.
indices?

[henrika (OOO until Aug 14), 2012-09-10 13:23:56]

Dale,

would it be possible to add some sort of diagram of the relationship between
classes and the data flow here? There is a rather tricky scheme involved in
setting up all the different callbacks, fifos and providers.

Thanks.

[DaleCurtis, 2012-09-10 14:05:51]

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:126: //
TODO(dalecurtis): Limit to windows and 2ch linux/mac only since neither of
On 2012/09/10 12:47:47, scherkus wrote:
> nit: proper capitalization on Windows/Linux/Mac
> 
> also this comment is a bit confusing -- are you saying that Windows supports
> multichannel but Linux/Mac only support 2 channels? What about mono?

Removed comment. After thinking about it, I intend to land the resilience patch
before removing this from behind a flag. Which means we'll just fail and reopen
the device in non-low latency mode.

https://chromiumcodereview.appspot.com/10918098/diff/9009/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:130: format =
media::AudioParameters::AUDIO_PCM_LOW_LATENCY;
On 2012/09/10 12:41:17, henrika wrote:
> Some comments here perhaps.

Done.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_m...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_m...
media/audio/audio_manager_base.cc:162: close_delay);
On 2012/09/10 12:47:47, scherkus wrote:
> I think we can clean up the confusing #if code if we returned the new
> AOP(dispatcher) here + deleted the else branch

Done.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:57: // the caller asked for, we need to
use a FIFO to ensure that both side read
On 2012/09/10 12:47:47, scherkus wrote:
> s/side/sides/?

Done.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:123: current_buffers_state_ =
buffers_state;
On 2012/09/10 12:47:47, scherkus wrote:
> It's a bit concerning we have to save this information so that we can use it
in
> a completely different method (SourceCallback())
> 
> Do we have an idea on how to handle AudioBuffersState in an ideal world? For
> example, should it be part of AudioBus? Should FIFOs and resamplers be
> responsible for adjusting the value as they process audio callbacks?

We can discuss this offline later. For now it's the callers responsibility to
adjust the value if they're making changes to the data flow which impact it.
It's a non-trivial exercise to design a system for all use cases. Tying it to
AudioBus is one option, but seems poor cohesion.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:127: outstanding_audio_bytes_ = 0;
On 2012/09/10 12:47:47, scherkus wrote:
> shouldn't this always be zero in this case?
> 
> we don't delete / create resample_ and audio_fifo_ at post-initialization do
we?

SourceCallback() adds to this value. Setting it to zero here seemed more
intuitive than checking !resampler, !fifo in SourceCallback before setting the
value. I'm fine with it in either place.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:181: audio_fifo_->Consume(audio_bus,
audio_bus->frames());
On 2012/09/10 12:47:47, scherkus wrote:
> this will CHECK() now -- do we need to inspect any params before calling?

Nope, AudioFifo is the one that is picky. AudioPullFifo should handle everything
that will end up with a CHECK internally.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.cc:185: // TODO(dalecurtis): Copy ASCB to
local variable, release lock prior to call?
On 2012/09/10 12:47:47, scherkus wrote:
> nit: instead of referring to the type ASCB, just say |source_callback_|

I plan to delete this comment before submission. I left it here for feedback
from reviewers :)

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.h:23: // which ensures audio data is always
output at given parameters.
On 2012/09/10 12:47:47, scherkus wrote:
> might be worth having a little ascii diagram to show which components this
class
> uses and in which cases (i.e., FIFO may/may not be present, resampler may/may
> not be present)

Done.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/audio/audio_o...
media/audio/audio_output_resampler.h:23: // which ensures audio data is always
output at given parameters.
On 2012/09/10 12:41:17, henrika wrote:
> No restrictions what so ever?

Just channel mixing. Added a comment.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/audio_pu...
File media/base/audio_pull_fifo.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/audio_pu...
media/base/audio_pull_fifo.h:39: void Clear() { fifo_->Clear(); }
On 2012/09/10 12:47:47, scherkus wrote:
> docs + de-inline (it's not a simple getter)

Not my change, will be removed by rebase against henrik's submitted cl.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
media/base/media_switches.cc:41: // Enable AudioOutputResampler for automatic
audio resampling and rebuffering.
On 2012/09/10 12:41:17, henrika wrote:
> Sorry, nit, why add 'automatic"?

Done.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/media_sw...
media/base/media_switches.cc:42: const char kEnableAudioOutputResampler[] =
"enable-audio-output-resampler";
On 2012/09/10 12:47:47, scherkus wrote:
> if this is only used in content then it should go under content_switches.cc

It's not? It's in media/audio and content/renderer, so it needs to be in media
or we have a layering problem.

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/sinc_res...
File media/base/sinc_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/9009/media/base/sinc_res...
media/base/sinc_resampler.h:38: // Flush all buffered data and reset internal
indexes.
On 2012/09/10 12:47:47, scherkus wrote:
> indices?

Done.

[scherkus (not reviewing), 2012-09-10 14:25:32]

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:128: // Rely on
AudioOutputResampler to handle any inconsistenties between the
sp: inconsistenties -> inconsistencies

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.cc:185: // TODO(dalecurtis): Copy ASCB to
local variable, release lock prior to call?
I see that source_callback_ gets set to NULL but what is its lifetime? I notice
it's not a refcounted object.

In other words, is the following a concern:
T1: OnError()
T1: Grab lock, save source_callback_ to local var, release lock
T2: Stop/Close/Shutdown()
T2: source_callback_ = NULL
T2: Owner of source_callback_ deletes it
T1: Calls OnError() on local copy of source_callback_
*boom*

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.cc:194: if (source_callback_ &&
!outstanding_audio_bytes_)
does outstanding_audio_bytes_ need to be protected by a lock elsewhere in the
class?

which thread is WTDR() called on?

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.h:35: // Currently channel downmixing and
upmixing is not supported.
bug etc? do we have DCHECKs / etc in place?

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.h:84: AudioOutputStream::AudioSourceCallback*
source_callback_;
who owns this?

[scherkus (not reviewing), 2012-09-10 14:34:11]

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:127: if
(cmd_line->HasSwitch(switches::kEnableAudioOutputResampler)) {
Doesn't this switch need to be passed via
content/browser/renderer_host/render_process_host_impl.cc to make it to the
renderer process?

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:137:
frames_per_buffer % audio_hardware::GetOutputBufferSize() == 0) {
can we de-nest this set of branches into the following:

if (HasSwitch) {
  format = LOW_LATENCY;
} else if (low latency stuff checks out) {
  format = LOW_LATENCY;
} else {
  format = LINEAR;
}

have the else + another set of if/else is confusing

alternatively you can tack the HasSwitch() as an ||

[DaleCurtis, 2012-09-10 14:53:51]

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:127: if
(cmd_line->HasSwitch(switches::kEnableAudioOutputResampler)) {
On 2012/09/10 14:34:11, scherkus wrote:
> Doesn't this switch need to be passed via
> content/browser/renderer_host/render_process_host_impl.cc to make it to the
> renderer process?

Oh yeah. This was working previously because I didn't have the LOW_LATENCY
restriction in AudioManagerBase.

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:128: // Rely on
AudioOutputResampler to handle any inconsistenties between the
On 2012/09/10 14:25:32, scherkus wrote:
> sp: inconsistenties -> inconsistencies

Done.

https://chromiumcodereview.appspot.com/10918098/diff/4008/content/renderer/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:137:
frames_per_buffer % audio_hardware::GetOutputBufferSize() == 0) {
On 2012/09/10 14:34:11, scherkus wrote:
> can we de-nest this set of branches into the following:
> 
> if (HasSwitch) {
>   format = LOW_LATENCY;
> } else if (low latency stuff checks out) {
>   format = LOW_LATENCY;
> } else {
>   format = LINEAR;
> }
> 
> have the else + another set of if/else is confusing
> 
> alternatively you can tack the HasSwitch() as an ||

Done.

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
File media/audio/audio_output_resampler.cc (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.cc:185: // TODO(dalecurtis): Copy ASCB to
local variable, release lock prior to call?
On 2012/09/10 14:25:32, scherkus wrote:
> I see that source_callback_ gets set to NULL but what is its lifetime? I
notice
> it's not a refcounted object.
> 
> In other words, is the following a concern:
> T1: OnError()
> T1: Grab lock, save source_callback_ to local var, release lock
> T2: Stop/Close/Shutdown()
> T2: source_callback_ = NULL
> T2: Owner of source_callback_ deletes it
> T1: Calls OnError() on local copy of source_callback_
> *boom*

Yes, that's my concern as well which is why I added the lock. The lifetime is
from StartStream->StopStream/CloseStream/Shutdown as those methods are called by
AudioOutputController. I think the lock is worth keeping, I don't expect any of
these methods to call back into one another.

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.cc:194: if (source_callback_ &&
!outstanding_audio_bytes_)
On 2012/09/10 14:25:32, scherkus wrote:
> does outstanding_audio_bytes_ need to be protected by a lock elsewhere in the
> class?
> 
> which thread is WTDR() called on?

No, it's called on the same thread as OnMoreData (the audio thread).
Additionally it's only used currently by the non-low latency Mac and Windows
output streams.

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
File media/audio/audio_output_resampler.h (right):

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.h:35: // Currently channel downmixing and
upmixing is not supported.
On 2012/09/10 14:25:32, scherkus wrote:
> bug etc? do we have DCHECKs / etc in place?

Left it out since this falls under the resilience use case. I'll add one and
remove it later when that CL goes in.

https://chromiumcodereview.appspot.com/10918098/diff/4008/media/audio/audio_o...
media/audio/audio_output_resampler.h:84: AudioOutputStream::AudioSourceCallback*
source_callback_;
On 2012/09/10 14:25:32, scherkus wrote:
> who owns this?

Whoever called StartStream() (which in production is just AudioOutputController)

[scherkus (not reviewing), 2012-09-10 15:29:13]

LGTM

[scherkus (not reviewing), 2012-09-10 15:29:47]

(I tested this both w/ FIFO only and w/ FIFO+Resampler on a variety of clips and
it works!)

[DaleCurtis, 2012-09-10 17:09:10]

Rebased against landed AudioPullFifo.

+brettw for content/* and general PPAPI comments.

[brettw, 2012-09-10 23:47:57]

lgtm

[henrika (OOO until Aug 14), 2012-09-11 07:06:51]

LGTM.

I will do more tests on Windows today and get back with feedback in a separate
e-mail thread.

Really nice work Dale.

[Nico, 2012-09-11 08:27:46]

(fyi, the title doesn't end up in the commit message, so the commit message for
this change looks a bit strange)

[viettrungluu, 2012-09-11 21:25:33]

LGTM (at least the change to pepper_platform_audio_output_impl.cc). My only
concern is that if, for some reason, we fail to open/start playback using
WASAPI, we won't be able to fall back. (I'm not sure if this can reasonably
happen now. It used to fail when the hardware was set up for != 2 channels, but
I guess that's been rectified. Are there any other cases in which we'll fail?)

[henrika (OOO until Aug 14), 2012-09-11 21:48:27]

The !=2 channel case should be solved by now. I did tests with a 7.1
headset today.
However, we will most likely fail if the user selects 24-bit mode.


On Tue, Sep 11, 2012 at 11:25 PM, <viettrungluu@chromium.org> wrote:

> LGTM (at least the change to pepper_platform_audio_output_**impl.cc). My
> only
> concern is that if, for some reason, we fail to open/start playback using
> WASAPI, we won't be able to fall back. (I'm not sure if this can reasonably
> happen now. It used to fail when the hardware was set up for != 2
> channels, but
> I guess that's been rectified. Are there any other cases in which we'll
> fail?)
>
>
http://codereview.chromium.**org/10918098/<http://codereview.chromium.org/109...
>

[brettw, 2012-09-11 21:50:16]

On Tue, Sep 11, 2012 at 2:47 PM, Henrik Andreasson <henrika@chromium.org>
wrote:
> The !=2 channel case should be solved by now. I did tests with a 7.1 headset
> today.
> However, we will most likely fail if the user selects 24-bit mode.

Okay, thanks. We'll need to fix this (or be able to fall back on
wavout) before we can enable this by default.

Bre

[henrika (OOO until Aug 14), 2012-09-11 21:52:55]

I can check tomorrow and come back with more details. I know that Dale is
working on a fall-back patch as well. He might have more information.


On Tue, Sep 11, 2012 at 11:50 PM, Brett Wilson <brettw@chromium.org> wrote:

> On Tue, Sep 11, 2012 at 2:47 PM, Henrik Andreasson <henrika@chromium.org>
> wrote:
> > The !=2 channel case should be solved by now. I did tests with a 7.1
> headset
> > today.
> > However, we will most likely fail if the user selects 24-bit mode.
>
> Okay, thanks. We'll need to fix this (or be able to fall back on
> wavout) before we can enable this by default.
>
> Bre
>

[DaleCurtis, 2012-09-12 15:06:11]

On 2012/09/11 21:52:55, henrika wrote:
> I can check tomorrow and come back with more details. I know that Dale is
> working on a fall-back patch as well. He might have more information.
> 
> 
> On Tue, Sep 11, 2012 at 11:50 PM, Brett Wilson <mailto:brettw@chromium.org>
wrote:
> 
> > On Tue, Sep 11, 2012 at 2:47 PM, Henrik Andreasson
<mailto:henrika@chromium.org>
> > wrote:
> > > The !=2 channel case should be solved by now. I did tests with a 7.1
> > headset
> > > today.
> > > However, we will most likely fail if the user selects 24-bit mode.
> >
> > Okay, thanks. We'll need to fix this (or be able to fall back on
> > wavout) before we can enable this by default.
> >
> > Bre
> >

Automatic Fallback is in the CQ now: http://codereview.chromium.org/10909151/ .