Adds media::AudioPullFifo class to Chrome.

Adds media::AudioPullFifo class to Chrome.

A FIFO (First In First Out) buffer to handle mismatches in buffer sizes
between a provider and consumer. The consumer will pull data from this FIFO.
If data is already available in the FIFO, it is provided to the consumer.
If insufficient data is available to satisfy the request, the FIFO will ask
the provider for more data to fulfill a request.

BUG=none
TEST=--gtest_filter=AudioPullFifoTest.*

[henrika (OOO until Aug 14), 2012-09-06 15:14:03]

Initial raw version. Compiles but I have not tested it at all.

Dale: feel free to give it a spin in your classes.

I will add a proper unit test tomorrow. Early feedback is OK so I can avoid
testing invalid parts.

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

Added unit test and cleaned things up. Ready for review now.

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

Will add AudioFifo changes based on feedback from Andrew to this CL as well.

[DaleCurtis, 2012-09-07 13:48:38]

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
File media/base/audio_pull_fifo.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo.cc:11: AudioPullFifo::AudioPullFifo(int channels, int
frames,
Do we really need |read_frames|? It's kind of confusing to have |frames|,
|read_frames|, and |frames_to_consume|. I'd just use frames as read_frames or
vice versa.

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo.cc:14: DCHECK_LE(read_frames, frames);
I don't think you should make this assumption since PPAPI can request whatever
buffer size it wants. So far it's always greater than the hardware output size.
On Mac/Win it's unlikely they'd ever request a smaller buffer size (and expect
it to work), but on Linux it's possible they will choose something less than the
2k fixed buffer size we use.

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo.cc:29: FillBuffer(frames_to_consume -
audio_fifo_->frames_in_fifo());
Per my comment above, if you don't pre-consume the data out of the FIFO this can
end up in a FIFO overflow is frames_to_consume is larger than |frames|. I think
it's safer to repeatedly Consume what you can from the buffer into
|destination|. That makes this FIFO more general purpose and allows it to not
make any assumption on the request sizes of each side.

You'll need the start_frame modification like we discussed previously (and I
used in my CL) for this to work.

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
File media/base/audio_pull_fifo_unittest.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo_unittest.cc:16: //          2048      ---->      
(4096)      ---->     ~441     [#frames]
As noted in my other comments, it's possible the Provider could ask for a 1024
buffer size and the hardware wants a 2048 buffer.

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo_unittest.cc:37: {440, 441, 442, 440, 440, 441, 442,
440, 441, 442, 441, 441, 442};
SincResampler will actually look something like 544, 512, 512, 512, 512. So I'd
increase the spread on these test values some more.

https://chromiumcodereview.appspot.com/10915123/diff/2001/media/base/audio_pu...
media/base/audio_pull_fifo_unittest.cc:90: for (int i = 0; i <
sizeof(kConsumeFrames) / sizeof(kConsumeFrames[0]); ++i) {
use the arraysize() macro from basictypes.h

[scherkus (not reviewing), 2012-09-07 14:19:03]

few nits -- leaving it up to dale/crogers to review the in-depth bits

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_fi...
File media/base/audio_fifo.h (right):

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_fi...
media/base/audio_fifo.h:24: // This call crash fail if the allocated space is
not sufficient.
s/This call crash fail/Push() will crash/?

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_fi...
media/base/audio_fifo.h:30: // frames or if there is not sufficient space in
|destination| to store the
ditto

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

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_pu...
media/base/audio_pull_fifo.h:15: // between a provider and consumer. The
consumer will pull data from this FIFO.
s/provider/producer/

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_pu...
File media/base/audio_pull_fifo_unittest.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/6002/media/base/audio_pu...
media/base/audio_pull_fifo_unittest.cc:44: ~AudioPullFifoTest() {}
virtual

[scherkus (not reviewing), 2012-09-07 14:23:57]

also can you update the CL description to include a few more details on use,
etc?

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

I had a long discussion with Dale and we decided to change the design so that we
no longer need the read_frames parameter. It is a bit messy to work with three
frame sizes and we can do with two if we add start_frame to
AudioFifo::Consume().

I did those changes and we (Dale and I) modified the inner pars as well. The
unit test passes but I must clean up the code. Will do that plus take input from
Andrew into account next.

http://codereview.chromium.org/10915123/diff/2001/media/base/audio_pull_fifo.cc
File media/base/audio_pull_fifo.cc (right):

http://codereview.chromium.org/10915123/diff/2001/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.cc:11: AudioPullFifo::AudioPullFifo(int channels, int
frames,
On 2012/09/07 13:48:38, DaleCurtis wrote:
> Do we really need |read_frames|? It's kind of confusing to have |frames|,
> |read_frames|, and |frames_to_consume|. I'd just use frames as read_frames or
> vice versa.

Done.

http://codereview.chromium.org/10915123/diff/2001/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.cc:14: DCHECK_LE(read_frames, frames);
On 2012/09/07 13:48:38, DaleCurtis wrote:
> I don't think you should make this assumption since PPAPI can request whatever
> buffer size it wants. So far it's always greater than the hardware output
size.
> On Mac/Win it's unlikely they'd ever request a smaller buffer size (and expect
> it to work), but on Linux it's possible they will choose something less than
the
> 2k fixed buffer size we use.

Done.

http://codereview.chromium.org/10915123/diff/2001/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.cc:29: FillBuffer(frames_to_consume -
audio_fifo_->frames_in_fifo());
On 2012/09/07 13:48:38, DaleCurtis wrote:
> Per my comment above, if you don't pre-consume the data out of the FIFO this
can
> end up in a FIFO overflow is frames_to_consume is larger than |frames|. I
think
> it's safer to repeatedly Consume what you can from the buffer into
> |destination|. That makes this FIFO more general purpose and allows it to not
> make any assumption on the request sizes of each side.
> 
> You'll need the start_frame modification like we discussed previously (and I
> used in my CL) for this to work.

Done.

[DaleCurtis, 2012-09-07 16:24:00]

https://chromiumcodereview.appspot.com/10915123/diff/11002/media/base/audio_p...
File media/base/audio_pull_fifo.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/11002/media/base/audio_p...
media/base/audio_pull_fifo.cc:24: DCHECK(destination);
Add a DCHECK_LE(frames_to_consume, destination->frames());

https://chromiumcodereview.appspot.com/10915123/diff/11002/media/base/audio_p...
media/base/audio_pull_fifo.cc:33: read_cb_.Run(bus_.get());
This is the part we can optimize later, as right now we have:

|read_cb_| writes into |bus_|
Push() (memcpy) into |fifo_| from |bus_|
Consume (memcpy) from |fifo_| into |destination|

One way around this would be to expose the AudioBus used by AudioFifo when the
Fifo is empty:

AudioBus* AudioFifo::PushBus() {
  CHECK_EQ(frames_, 0);
  return audio_bus_.get();
}

Then provide that to the caller:

AudioPullFifo::Consume(...) {
  ...
  read_cb.Run(AudioFifo::PushBus());
  ...
}

This allows AudioPullFifo to have its callback write directly into the fifo
instead of having to have the caller write into a temporary bus and then Push()
that into the fifo before Consume().

As I said, we can do this in a later CL, but since we discussed it on chat, I
wanted to make note of it here.

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

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_fifo.h
File media/base/audio_fifo.h (right):

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:24: // This call crash fail if the allocated space is
not sufficient.
On 2012/09/07 14:19:03, scherkus wrote:
> s/This call crash fail/Push() will crash/?

Done.

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:30: // frames or if there is not sufficient space in
|destination| to store the
On 2012/09/07 14:19:03, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.h:15: // between a provider and consumer. The
consumer will pull data from this FIFO.
OK. Can be discussed I guess but it sounds better. Will change.

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_pull_fifo_...
File media/base/audio_pull_fifo_unittest.cc (right):

http://codereview.chromium.org/10915123/diff/6002/media/base/audio_pull_fifo_...
media/base/audio_pull_fifo_unittest.cc:44: ~AudioPullFifoTest() {}
On 2012/09/07 14:19:03, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/10915123/diff/11002/media/base/audio_pull_fifo.cc
File media/base/audio_pull_fifo.cc (right):

http://codereview.chromium.org/10915123/diff/11002/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.cc:24: DCHECK(destination);
On 2012/09/07 16:24:01, DaleCurtis wrote:
> Add a DCHECK_LE(frames_to_consume, destination->frames());

Done.

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

PTAL Dale and Andrew. 

Dale, I clean up the version we came up with last Friday. Hope you like it. Also
added a test sequence with a higher variance.

I did not add TODO for "reduce usage of memcpy" in this CL. I will fix it but
wanted to land this one first so you can merge.

[DaleCurtis, 2012-09-10 08:28:25]

LGTM % nits. Thanks Henrik!

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
File media/base/audio_pull_fifo.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo.cc:7: #include "media/base/audio_pull_fifo.h"
Needs to be first include, move above <algorithm>

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo.cc:47: int frames = std::min(fifo_->frames(),
*frames_to_provide);
DCHECK() frames_to_provide and write_pos.

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
File media/base/audio_pull_fifo.h (right):

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo.h:35: // If the FIFO does not have enough data, we
ask the producer to give us more
Odd wrapping; move to end of above line.

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
File media/base/audio_pull_fifo_unittest.cc (right):

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo_unittest.cc:26: static const int kConsumeFrames[] =
How long does this take to run? We can probably cut this list in half or more
and still have excellent coverage. The problem with tests which move a lot of
data around like this is that valgrind/asan/tsan take an order of magnitude more
to run. So we want to make sure we're adding value with that additional time
overhead.

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo_unittest.cc:71: TEST_F(AudioPullFifoTest, Consume) {
Instead of using an array of test values like this you can templatize using
TEST_P and extending  AudioPullFifoTest from testing::TestWithParam<int> and
then use INSTANTIATE_TEST_CASE_P().

See sinc_resampler_unittest for an example.

https://chromiumcodereview.appspot.com/10915123/diff/11004/media/base/audio_p...
media/base/audio_pull_fifo_unittest.cc:80: int frames_to_consume =
kConsumeFrames[i];
SCOPED_TRACE() with the kConsumeFrames information?

[henrika (OOO until Aug 14), 2012-09-10 09:35:30]

Done.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo.cc
File media/base/audio_pull_fifo.cc (right):

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.cc:7: #include "media/base/audio_pull_fifo.h"
On 2012/09/10 08:28:26, DaleCurtis wrote:
> Needs to be first include, move above <algorithm>

Done.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.cc:47: int frames = std::min(fifo_->frames(),
*frames_to_provide);
On 2012/09/10 08:28:26, DaleCurtis wrote:
> DCHECK() frames_to_provide and write_pos. 

Done.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo.h:35: // If the FIFO does not have enough data, we
ask the producer to give us more
On 2012/09/10 08:28:26, DaleCurtis wrote:
> Odd wrapping; move to end of above line.

Done.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
File media/base/audio_pull_fifo_unittest.cc (right):

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo_unittest.cc:26: static const int kConsumeFrames[] =
It takes ~10ms on my machine. Will keep as is.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo_unittest.cc:71: TEST_F(AudioPullFifoTest, Consume) {
Nice tip; did not know about it. Will modify.

http://codereview.chromium.org/10915123/diff/11004/media/base/audio_pull_fifo...
media/base/audio_pull_fifo_unittest.cc:80: int frames_to_consume =
kConsumeFrames[i];
Thanks.

[DaleCurtis, 2012-09-10 09:40:53]

lgtm!

[scherkus (not reviewing), 2012-09-10 10:56:35]

lgtm w/ nits

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h
File media/base/audio_fifo.h (right):

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:24: // Push() will crash if the allocated space is not
sufficient.
nit: s/not sufficient/insufficient/

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:30: // frames or if there is not sufficient space in
|destination| to store the
nit: s/not sufficient/insufficient/

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.h:40: void Clear() { fifo_->Clear(); };
de-inline (it's not a simple getter)

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

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h
File media/base/audio_fifo.h (right):

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:24: // Push() will crash if the allocated space is not
sufficient.
On 2012/09/10 10:56:35, scherkus wrote:
> nit: s/not sufficient/insufficient/

Done.

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_fifo.h#new...
media/base/audio_fifo.h:30: // frames or if there is not sufficient space in
|destination| to store the
On 2012/09/10 10:56:35, scherkus wrote:
> nit: s/not sufficient/insufficient/

Done.

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_pull_fifo.h
File media/base/audio_pull_fifo.h (right):

http://codereview.chromium.org/10915123/diff/3015/media/base/audio_pull_fifo....
media/base/audio_pull_fifo.h:40: void Clear() { fifo_->Clear(); };
On 2012/09/10 10:56:35, scherkus wrote:
> de-inline (it's not a simple getter)

Done.

[scherkus (not reviewing), 2012-09-10 16:27:43]

Due to henrika's checkout self-immolating I uploaded a rebased copy as
http://codereview.chromium.org/10914178/ and landed as r155744