Fix initial buffer sizes and improve partial underflow support.

Fix initial buffer sizes and improve partial underflow support.

On devices with massive buffer sizes, src= playback will often
use a size too small to satisfy the OS request. So we end up
only filling most of it, but end up with enough data such that
the next call does not cause an underflow.

The fixes are two fold:
1. Use a better initial size when possible.
2. Increase queue capacity anytime we have a partial fill.

BUG=650601
TEST=new unittests. --audio-buffer-size=11160 doesn't glitch.

Committed: https://crrev.com/95d0b149bbb2ec028a24d833ddb5351912f40a93
Cr-Commit-Position: refs/heads/master@{#421691}

[DaleCurtis, 2016-09-28 00:47:34]

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

[DaleCurtis, 2016-09-28 00:47:39]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 00:48:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 03:04:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 03:04:26]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chcunningham, 2016-09-28 19:59:30]

https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
media/renderers/audio_renderer_impl.cc:858: // If we only partially filled the
request and should have more data, go
When do we expect this to be triggered? Given your change above to the initial
buffer size, I'm not sure if this is actually used?

Could this end up being triggered when we're just underflowing? I.e. we wrote >
0 frames but less than requested, not because our queue size was too small, but
just because decoding is too slow or not enough audio has downloaded. 

I think you can solve that second concern by checking the current queue capacity
against the size of frames_requested -- if its already pretty big then don't
increase.

[DaleCurtis, 2016-09-28 21:43:28]

https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
File media/renderers/audio_renderer_impl.cc (right):

https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
media/renderers/audio_renderer_impl.cc:858: // If we only partially filled the
request and should have more data, go
On 2016/09/28 at 19:59:30, chcunningham wrote:
> When do we expect this to be triggered? Given your change above to the initial
buffer size, I'm not sure if this is actually used?
> 
> Could this end up being triggered when we're just underflowing? I.e. we wrote
> 0 frames but less than requested, not because our queue size was too small,
but just because decoding is too slow or not enough audio has downloaded. 
> 
> I think you can solve that second concern by checking the current queue
capacity against the size of frames_requested -- if its already pretty big then
don't increase.

The other change fixes the known cases, but buffer sizes are flexible in the
renderer. If a device change happens, we may end up on a device which needs a
larger buffer size. Without this, we can't adapt in the event of partial fills.

I expect it to be triggered slightly before we underflow now; potentially
avoiding an actual pause-for-underflow event. What's your concern with this
piece of code?

[chcunningham1, 2016-09-28 22:08:34]

On 2016/09/28 21:43:28, DaleCurtis wrote:
>
https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
> File media/renderers/audio_renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/2375713003/diff/1/media/renderers/audio_rende...
> media/renderers/audio_renderer_impl.cc:858: // If we only partially filled the
> request and should have more data, go
> On 2016/09/28 at 19:59:30, chcunningham wrote:
> > When do we expect this to be triggered? Given your change above to the
initial
> buffer size, I'm not sure if this is actually used?
> > 
> > Could this end up being triggered when we're just underflowing? I.e. we
wrote
> > 0 frames but less than requested, not because our queue size was too small,
> but just because decoding is too slow or not enough audio has downloaded. 
> > 
> > I think you can solve that second concern by checking the current queue
> capacity against the size of frames_requested -- if its already pretty big
then
> don't increase.
> 
> The other change fixes the known cases, but buffer sizes are flexible in the
> renderer. If a device change happens, we may end up on a device which needs a
> larger buffer size. Without this, we can't adapt in the event of partial
fills.
> 

Ack.

> I expect it to be triggered slightly before we underflow now; potentially
> avoiding an actual pause-for-underflow event. What's your concern with this
> piece of code?

After looking closer I realize my concern doesn't matter. My concern was that we
should not treat underflow the same as capacity-too-small-for-hardware and the
if(...) doesn't distinguish. I realize now that we actually do respond to
underflow in the same way -- increasing the queue capacity just a few lines up
in the case where frames_written == 0 -- so there's no need to distinguish.

[chcunningham1, 2016-09-28 22:08:40]

chcunningham@google.com changed reviewers:
+ chcunningham@google.com

[chcunningham1, 2016-09-28 22:08:40]

lgtm

[chcunningham, 2016-09-28 22:09:38]

On 2016/09/28 22:08:40, chcunningham1 wrote:
> lgtm

@chromium lgtm

[DaleCurtis, 2016-09-28 22:49:50]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-09-28 22:50:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 00:14:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-29 00:20:16]

Description was changed from

==========
Fix initial buffer sizes and improve partial underflow support.

On devices with massive buffer sizes, src= playback will often
use a size too small to satisfy the OS request. So we end up
only filling most of it, but end up with enough data such that
the next call does not cause an underflow.

The fixes are two fold:
1. Use a better initial size when possible.
2. Increase queue capacity anytime we have a partial fill.

BUG=650601
TEST=new unittests. --audio-buffer-size=11160 doesn't glitch.
==========

to

==========
Fix initial buffer sizes and improve partial underflow support.

On devices with massive buffer sizes, src= playback will often
use a size too small to satisfy the OS request. So we end up
only filling most of it, but end up with enough data such that
the next call does not cause an underflow.

The fixes are two fold:
1. Use a better initial size when possible.
2. Increase queue capacity anytime we have a partial fill.

BUG=650601
TEST=new unittests. --audio-buffer-size=11160 doesn't glitch.

Committed: https://crrev.com/95d0b149bbb2ec028a24d833ddb5351912f40a93
Cr-Commit-Position: refs/heads/master@{#421691}
==========

[commit-bot: I haz the power, 2016-09-29 00:20:17]

Patchset 1 (id:??) landed as
https://crrev.com/95d0b149bbb2ec028a24d833ddb5351912f40a93
Cr-Commit-Position: refs/heads/master@{#421691}