Tweak multibuffer buffering strategy.

Tweak multibuffer buffering strategy.

Separate buffer size from pin range.
Buffering is now calculated as follows:

target buffer size is max(25Mb, 12 seconds * bitrate)

prefetch_low is max(20Mb, 10 seconds * bitrate)
prefetch_high is prefetch_low + 1Mb

pin_backwards is min(2 seconds * bitrate, 25Mb - prefetch_high)
pin_forwards is 21Mb

This means that the target buffer size is equivialent to BufferedDataSource.
Note that we pin 21Mb in front of the current location no matter what, this
means that if we happen to have that data in the cache we temporarily go over
the target buffer size to avoid freeing something and then re-fetch it right
afterwards.

BUG=608974
Committed: https://crrev.com/08d81e2c010c6ad0fddd9d389c3208c263d3e424
Cr-Commit-Position: refs/heads/master@{#391691}

[hubbe, 2016-05-04 19:39:29]

hubbe@chromium.org changed reviewers:
+ dalecurtis@google.com

[DaleCurtis, 2016-05-04 21:39:31]

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

[DaleCurtis, 2016-05-04 21:39:32]

The previous tests did not catch this issue, is there a new one you can add that
would have caught this problem?

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:572: kMinBufferPreload, kMaxBufferSize -
preload_high);
Seems like the subtraction could go negative?

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (right):

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:64: ;
extra ;

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:65:
multibuffer_->IncrementMaxSize(new_buffer_size - current_buffer_size_);
dcheck cur <= new_buffer_size ?

[DaleCurtis, 2016-05-04 21:39:47]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@google.com

[DaleCurtis, 2016-05-04 21:39:47]

Also, what was the practical effect of this bug?

[hubbe, 2016-05-04 21:55:06]

> Also, what was the practical effect of this bug?

The bug is that the DCHECK is bogus.
This CL fixes that and also optimizes the buffer strategy a little.

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:572: kMinBufferPreload, kMaxBufferSize -
preload_high);
On 2016/05/04 21:39:32, DaleCurtis wrote:
> Seems like the subtraction could go negative?

Adding a static assert to make sure it cannot.

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
File media/blink/multibuffer_reader.cc (right):

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:64: ;
On 2016/05/04 21:39:32, DaleCurtis wrote:
> extra ;

Done.

https://codereview.chromium.org/1954493002/diff/1/media/blink/multibuffer_rea...
media/blink/multibuffer_reader.cc:65:
multibuffer_->IncrementMaxSize(new_buffer_size - current_buffer_size_);
On 2016/05/04 21:39:32, DaleCurtis wrote:
> dcheck cur <= new_buffer_size ?

Decreasing the buffer size is allowed.
(As is calling IncrementMaxSize with a negative value.)

[DaleCurtis, 2016-05-04 22:06:27]

lgtm

[hubbe, 2016-05-04 22:43:19]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-05-04 22:43:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954493002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954493002/20001

[commit-bot: I haz the power, 2016-05-04 23:35:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-04 23:37:54]

Description was changed from

==========
Tweak multibuffer buffering strategy.

Separate buffer size from pin range.
Buffering is now calculated as follows:

target buffer size is max(25Mb, 12 seconds * bitrate)

prefetch_low is max(20Mb, 10 seconds * bitrate)
prefetch_high is prefetch_low + 1Mb

pin_backwards is min(2 seconds * bitrate, 25Mb - prefetch_high)
pin_forwards is 21Mb

This means that the target buffer size is equivialent to BufferedDataSource.
Note that we pin 21Mb in front of the current location no matter what, this
means that if we happen to have that data in the cache we temporarily go over
the target buffer size to avoid freeing something and then re-fetch it right
afterwards.

BUG=608974
==========

to

==========
Tweak multibuffer buffering strategy.

Separate buffer size from pin range.
Buffering is now calculated as follows:

target buffer size is max(25Mb, 12 seconds * bitrate)

prefetch_low is max(20Mb, 10 seconds * bitrate)
prefetch_high is prefetch_low + 1Mb

pin_backwards is min(2 seconds * bitrate, 25Mb - prefetch_high)
pin_forwards is 21Mb

This means that the target buffer size is equivialent to BufferedDataSource.
Note that we pin 21Mb in front of the current location no matter what, this
means that if we happen to have that data in the cache we temporarily go over
the target buffer size to avoid freeing something and then re-fetch it right
afterwards.

BUG=608974

Committed: https://crrev.com/08d81e2c010c6ad0fddd9d389c3208c263d3e424
Cr-Commit-Position: refs/heads/master@{#391691}
==========

[commit-bot: I haz the power, 2016-05-04 23:37:54]

Patchset 2 (id:??) landed as
https://crrev.com/08d81e2c010c6ad0fddd9d389c3208c263d3e424
Cr-Commit-Position: refs/heads/master@{#391691}