Dynamically allocate more memory to AudioBlockFifo

Dynamically allocate more memory to AudioBlockFifo.

PulseAudio input can provide larger buffer size than what we ask for. This happens if the pulse internally increase its buffer size for various reasons. This patch handles it by dynamically allocate more memory to the FIFO.

BUG=411453
TEST=1. Have Jackd own a second (USB) microphone, and have
Pulseaudio connect to Jackd with the Pulseaudio Jack
module. There are many steps:
1a. Obtain a USB mic.
1b. Find out the ALSA name of the mic and of the default
audio.
1c. Create a file ~/.jackdrc containing:
/usr/bin/jackd -dalsa -d hw:<default audio device>
1d. Run "alsa_in -d hw:<USB mic name> &" (in background)
1e. Install pulseaudio-module-jack if not installed.
1f. Run "pactl load-module module-jack-source channels=2"
1g. Go into Pulseaudio and select Input, then Jack source.
1h. Install patchage, start it up and connect the USB mic
to the Pulseaudio JACK source.
2. Start up Chrome and begin a Hangout. Make sure that
Chrome is using the Default mic.
3. Have someone (or yourself on another computer) join the
Hangout and listen to you talk.

Committed: https://crrev.com/f6734cf6f2a0a5514babc0c3ed2d4e1b2662e113
Cr-Commit-Position: refs/heads/master@{#295713}

[no longer working on chromium, 2014-09-12 13:03:02]

xians@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[no longer working on chromium, 2014-09-12 13:03:51]

Dale, could you please review this CL?

Thanks,
SX

[DaleCurtis, 2014-09-12 17:14:00]

Definitely needs a unittest too :)

https://codereview.chromium.org/557693003/diff/1/media/base/audio_block_fifo.cc
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/557693003/diff/1/media/base/audio_block_fifo....
media/base/audio_block_fifo.cc:86: void AudioBlockFifo::Append(int blocks) {
Append feels like it may be giving too much of the internal structure of
AudioBlockFIFO away. I wonder if this should instead be something like
IncreaseCapacity(<frames>).

The AudioBlockFIFO can then determine the correct number of blocks and such as
well as any internal reshuffling that needs to be done to accommodate the new
blocks.

I.e., consider what happens right now if block are added to the end when the
read pointer points to the back of the block list and the write pointer has
already wrapped to the front somewhere. You'll need to be careful that
read/write pointers are in the right place after this operation.

[no longer working on chromium, 2014-09-15 15:41:33]

On 2014/09/12 17:14:00, DaleCurtis wrote:
> Definitely needs a unittest too :)
> 
>
https://codereview.chromium.org/557693003/diff/1/media/base/audio_block_fifo.cc
> File media/base/audio_block_fifo.cc (right):
> 
>
https://codereview.chromium.org/557693003/diff/1/media/base/audio_block_fifo....
> media/base/audio_block_fifo.cc:86: void AudioBlockFifo::Append(int blocks) {
> Append feels like it may be giving too much of the internal structure of
> AudioBlockFIFO away. I wonder if this should instead be something like
> IncreaseCapacity(<frames>).
> 
> The AudioBlockFIFO can then determine the correct number of blocks and such as
> well as any internal reshuffling that needs to be done to accommodate the new
> blocks.
> 
> I.e., consider what happens right now if block are added to the end when the
> read pointer points to the back of the block list and the write pointer has
> already wrapped to the front somewhere. You'll need to be careful that
> read/write pointers are in the right place after this operation.

Good points, I did not have time to upload a new version today, but will do it
tomorrow.

[no longer working on chromium, 2014-09-16 18:00:28]

Hi Dale, PTAL.

Thanks,
SX

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:90: // Insert the new blocks of buffers right
before the read block.
we insert the new buffers before the read pointer so that those new buffers are
always last buffer to read in the FIFO.

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:98: if (write_block_ >= read_block_)
if write_block_ < read_block_, it means write pointer is below the new blocks of
buffer, so there is no need to update the write pointer.

[DaleCurtis, 2014-09-16 19:36:53]

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:86: void AudioBlockFifo::IncreaseCapacity(int
blocks) {
DCHECK_GT(blocks, 0); ?

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:89: 
Instead of repeatedly shifting the elements due to multiple inserts, here's a
more efficient add-everything-to-the-back-and-shift-once algorithm with
std::rotate():

const int original_size = audio_blocks_.size();
for (int i = 0; i < blocks; ++i) {
  audio_blocks_.push_back(
      AudioBus::Create(channels_, block_frames_).release());
}

if (!original_size)
  return;

std::rotate(audio_blocks_.begin() + read_block_,
            audio_blocks_.begin() + original_size,
            audio_blocks_.end());

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:102: read_block_ += blocks %
audio_blocks_.size();;
This should never wrap. You can remove the modulus if you gate it behind the
!original_size check I recommend above.  It might be nice to add a DCHECK_LT()
here too then.

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo_unittest.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo_unittest.cc:40: AudioPowerMonitor
audio_level(dummy_sample_rate,
This is kind of confusing, I'd just check for non-zero values, it'll be simpler
and easier for future readers to follow.

[no longer working on chromium, 2014-09-18 12:42:49]

Dale, PTAL.

Thanks,
SX

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:86: void AudioBlockFifo::IncreaseCapacity(int
blocks) {
On 2014/09/16 19:36:53, DaleCurtis wrote:
> DCHECK_GT(blocks, 0); ?

Done.

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:89: 
On 2014/09/16 19:36:53, DaleCurtis wrote:
> Instead of repeatedly shifting the elements due to multiple inserts, here's a
> more efficient add-everything-to-the-back-and-shift-once algorithm with
> std::rotate():
> 
> const int original_size = audio_blocks_.size();
> for (int i = 0; i < blocks; ++i) {
>   audio_blocks_.push_back(
>       AudioBus::Create(channels_, block_frames_).release());
> }
> 
> if (!original_size)
>   return;
> 
> std::rotate(audio_blocks_.begin() + read_block_,
>             audio_blocks_.begin() + original_size,
>             audio_blocks_.end());

Done with using rotate.

Though I don't really think insert is that costly, and the new approach is using
|blocks| times of push_back and one rotate, it seems more operations to me :)

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:98: if (write_block_ >= read_block_)
On 2014/09/16 18:00:28, xians1 wrote:
> if write_block_ < read_block_, it means write pointer is below the new blocks
of
> buffer, so there is no need to update the write pointer.

Done.

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:102: read_block_ += blocks %
audio_blocks_.size();;
On 2014/09/16 19:36:53, DaleCurtis wrote:
> This should never wrap. You can remove the modulus if you gate it behind the
> !original_size check I recommend above.  It might be nice to add a DCHECK_LT()
> here too then.

Done.

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo_unittest.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo_unittest.cc:40: AudioPowerMonitor
audio_level(dummy_sample_rate,
On 2014/09/16 19:36:53, DaleCurtis wrote:
> This is kind of confusing, I'd just check for non-zero values, it'll be
simpler
> and easier for future readers to follow. 

Done, now it only checks the value.

[DaleCurtis, 2014-09-18 17:12:03]

lgtm

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
File media/base/audio_block_fifo.cc (right):

https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
media/base/audio_block_fifo.cc:89: 
On 2014/09/18 12:42:49, xians1 wrote:
> On 2014/09/16 19:36:53, DaleCurtis wrote:
> > Instead of repeatedly shifting the elements due to multiple inserts, here's
a
> > more efficient add-everything-to-the-back-and-shift-once algorithm with
> > std::rotate():
> > 
> > const int original_size = audio_blocks_.size();
> > for (int i = 0; i < blocks; ++i) {
> >   audio_blocks_.push_back(
> >       AudioBus::Create(channels_, block_frames_).release());
> > }
> > 
> > if (!original_size)
> >   return;
> > 
> > std::rotate(audio_blocks_.begin() + read_block_,
> >             audio_blocks_.begin() + original_size,
> >             audio_blocks_.end());
> 
> Done with using rotate.
> 
> Though I don't really think insert is that costly, and the new approach is
using
> |blocks| times of push_back and one rotate, it seems more operations to me :)

Vectors have contiguous memory layout and insert() inserts elements before the
specified iterator position, so under the hood for each inserted block it has to
move all elements after the insert position over 1 position.  std::rotate() will
ideally do this with a single move, though implementations may vary. With the
reserve() call above push_back is really just a constant time operation in also.

Since |blocks| is small this isn't a big deal, but I wanted to call it out and
recommend an alternative if you weren't aware of what was happening here and
found yourself in a similar situation in the future.

[no longer working on chromium, 2014-09-19 12:24:12]

On 2014/09/18 17:12:03, DaleCurtis wrote:
> lgtm
> 
>
https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
> File media/base/audio_block_fifo.cc (right):
> 
>
https://codereview.chromium.org/557693003/diff/20001/media/base/audio_block_f...
> media/base/audio_block_fifo.cc:89: 
> On 2014/09/18 12:42:49, xians1 wrote:
> > On 2014/09/16 19:36:53, DaleCurtis wrote:
> > > Instead of repeatedly shifting the elements due to multiple inserts,
here's
> a
> > > more efficient add-everything-to-the-back-and-shift-once algorithm with
> > > std::rotate():
> > > 
> > > const int original_size = audio_blocks_.size();
> > > for (int i = 0; i < blocks; ++i) {
> > >   audio_blocks_.push_back(
> > >       AudioBus::Create(channels_, block_frames_).release());
> > > }
> > > 
> > > if (!original_size)
> > >   return;
> > > 
> > > std::rotate(audio_blocks_.begin() + read_block_,
> > >             audio_blocks_.begin() + original_size,
> > >             audio_blocks_.end());
> > 
> > Done with using rotate.
> > 
> > Though I don't really think insert is that costly, and the new approach is
> using
> > |blocks| times of push_back and one rotate, it seems more operations to me
:)
> 
> Vectors have contiguous memory layout and insert() inserts elements before the
> specified iterator position, so under the hood for each inserted block it has
to
> move all elements after the insert position over 1 position.  std::rotate()
will
> ideally do this with a single move, though implementations may vary. With the
> reserve() call above push_back is really just a constant time operation in
also.
> 
> Since |blocks| is small this isn't a big deal, but I wanted to call it out and
> recommend an alternative if you weren't aware of what was happening here and
> found yourself in a similar situation in the future.

Got it, thanks for the technical details.

[no longer working on chromium, 2014-09-19 12:24:17]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-09-19 12:24:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/557693003/40001

[commit-bot: I haz the power, 2014-09-19 15:27:06]

Committed patchset #3 (id:40001) as e893ef58704a97b254c34021b0b7f3d128148cc6

[commit-bot: I haz the power, 2014-09-19 15:27:36]

Patchset 3 (id:??) landed as
https://crrev.com/f6734cf6f2a0a5514babc0c3ed2d4e1b2662e113
Cr-Commit-Position: refs/heads/master@{#295713}