media: Increase preloading and max buffer for high-bitrate videos

media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10 seconds
of data, but in some examples, only 6 seconds is preloaded because of this limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10 seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993
Committed: https://crrev.com/aa045cd99f2920d12c501b8c0b5bf2752df3e771
Cr-Commit-Position: refs/heads/master@{#430022}

[hubbe, 2016-11-03 20:44:57]

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

[hubbe, 2016-11-03 20:45:04]

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

[commit-bot: I haz the power, 2016-11-03 20:45:43]

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

[DaleCurtis, 2016-11-03 20:48:23]

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

[DaleCurtis, 2016-11-03 20:48:23]

I don't quite follow how this is limited to high bit rate videos. Can you
elaborate?

[hubbe, 2016-11-03 20:51:47]

On 2016/11/03 20:48:23, DaleCurtis wrote:
> I don't quite follow how this is limited to high bit rate videos. Can you
> elaborate?

basically we do preload = min(kMaxPreload, bitrate/8 * 10);
For most videos, bitrate/8*10 is the smaller number and this CL will make no
difference.

[watk, 2016-11-03 21:09:17]

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

[watk, 2016-11-03 21:09:18]

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

https://codereview.chromium.org/2477513003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:610: std::max(kMaxBufferSize,
kMaxBufferPreload + kPreloadHighExtra);
Drive by: It's confusing to me that kMaxBufferSize is not the max buffer size
any more.

Also, this is std::max(20MB, 51MB)? So kMaxBufferSize seems unused now.

[commit-bot: I haz the power, 2016-11-03 22:20:23]

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

[commit-bot: I haz the power, 2016-11-03 22:20:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2016-11-03 23:10:01]

Description was changed from

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

BUG=512993
==========

to

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some
opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993
==========

[hubbe, 2016-11-03 23:12:00]

Cleaned up the code a bit to make it work the way I actually wanted it to work,
and added tests. PTAL.

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

https://codereview.chromium.org/2477513003/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source.cc:610: std::max(kMaxBufferSize,
kMaxBufferPreload + kPreloadHighExtra);
On 2016/11/03 21:09:18, watk wrote:
> Drive by: It's confusing to me that kMaxBufferSize is not the max buffer size
> any more.
> 
> Also, this is std::max(20MB, 51MB)? So kMaxBufferSize seems unused now.

You're right, and that's not how I wanted it to work.
I re-wrote this part a bit, got rid of some pinning behind the reading position
(if we want to save more data we should just increase buffer_size instead).
Wrote lots of comments and added tests.

[hubbe, 2016-11-03 23:12:11]

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

[commit-bot: I haz the power, 2016-11-03 23:13:45]

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

[DaleCurtis, 2016-11-03 23:35:49]

kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that probably
shouldn't be true.

[commit-bot: I haz the power, 2016-11-04 02:26:44]

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

[commit-bot: I haz the power, 2016-11-04 02:26:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hubbe, 2016-11-04 03:57:47]

On 2016/11/03 23:35:49, DaleCurtis wrote:
> kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that probably
> shouldn't be true.

Buffer size is still gated by kMaxBufferPreload

[DaleCurtis, 2016-11-04 19:09:36]

On 2016/11/04 at 03:57:47, hubbe wrote:
> On 2016/11/03 23:35:49, DaleCurtis wrote:
> > kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that
probably
> > shouldn't be true.
> 
> Buffer size is still gated by kMaxBufferPreload

This means the maximum buffer size is kPreloadHighExtra + 2 * kMaxBufferPreload,
no? So we'll buffer up to a 100mb? Or since that size is only advisory, we'll
start GC'ing much of that?

[hubbe, 2016-11-04 19:23:18]

On 2016/11/04 19:09:36, DaleCurtis wrote:
> On 2016/11/04 at 03:57:47, hubbe wrote:
> > On 2016/11/03 23:35:49, DaleCurtis wrote:
> > > kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that
> probably
> > > shouldn't be true.
> > 
> > Buffer size is still gated by kMaxBufferPreload
> 
> This means the maximum buffer size is kPreloadHighExtra + 2 *
kMaxBufferPreload,
> no? So we'll buffer up to a 100mb? Or since that size is only advisory, we'll
> start GC'ing much of that?

Yes, the limit would be 100Mb (which would occur if the bitrate was >= 250Mbit)
I could scale the backwards pinning limit down to 10Mb though, which would make
the limit 60Mb.
The "advisory" portion doesn't really apply once preload_high > kDefaultPinSize
though, since
the pinned region and SetMaxBuffer() would be the same size in those cases.

[DaleCurtis, 2016-11-04 19:30:16]

On 2016/11/04 at 19:23:18, hubbe wrote:
> On 2016/11/04 19:09:36, DaleCurtis wrote:
> > On 2016/11/04 at 03:57:47, hubbe wrote:
> > > On 2016/11/03 23:35:49, DaleCurtis wrote:
> > > > kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that
> > probably
> > > > shouldn't be true.
> > > 
> > > Buffer size is still gated by kMaxBufferPreload
> > 
> > This means the maximum buffer size is kPreloadHighExtra + 2 *
kMaxBufferPreload,
> > no? So we'll buffer up to a 100mb? Or since that size is only advisory,
we'll
> > start GC'ing much of that?
> 
> Yes, the limit would be 100Mb (which would occur if the bitrate was >=
250Mbit)
> I could scale the backwards pinning limit down to 10Mb though, which would
make the limit 60Mb.
> The "advisory" portion doesn't really apply once preload_high >
kDefaultPinSize though, since
> the pinned region and SetMaxBuffer() would be the same size in those cases.

Seems worthwhile to apply some sanity to the bitrate_ value since it's
untrusted. What's the maximum you'd expect?

[hubbe, 2016-11-04 19:37:17]

On 2016/11/04 19:30:16, DaleCurtis wrote:
> On 2016/11/04 at 19:23:18, hubbe wrote:
> > On 2016/11/04 19:09:36, DaleCurtis wrote:
> > > On 2016/11/04 at 03:57:47, hubbe wrote:
> > > > On 2016/11/03 23:35:49, DaleCurtis wrote:
> > > > > kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that
> > > probably
> > > > > shouldn't be true.
> > > > 
> > > > Buffer size is still gated by kMaxBufferPreload
> > > 
> > > This means the maximum buffer size is kPreloadHighExtra + 2 *
> kMaxBufferPreload,
> > > no? So we'll buffer up to a 100mb? Or since that size is only advisory,
> we'll
> > > start GC'ing much of that?
> > 
> > Yes, the limit would be 100Mb (which would occur if the bitrate was >=
> 250Mbit)
> > I could scale the backwards pinning limit down to 10Mb though, which would
> make the limit 60Mb.
> > The "advisory" portion doesn't really apply once preload_high >
> kDefaultPinSize though, since
> > the pinned region and SetMaxBuffer() would be the same size in those cases.
> 
> Seems worthwhile to apply some sanity to the bitrate_ value since it's
> untrusted. What's the maximum you'd expect?

You think I should get rid of the buffer limits and apply the sanity to the
bitrate instead?
Not sure what an unreasonable bitrate might be though since I could imagine 8k
video going all the way up to 150Mbit (and spiking higher), and that's not
counting 3d, HDR, HFR and other crazy things that might increase the bitrate.

[DaleCurtis, 2016-11-04 19:44:48]

This seems fine as a solution, so lgtm, but I wonder if clamping bitrate_ is
easier.

[hubbe, 2016-11-04 19:51:42]

On 2016/11/04 19:44:48, DaleCurtis wrote:
> This seems fine as a solution, so lgtm, but I wonder if clamping bitrate_ is
> easier.

I think you might be right that it might be easier.
Since I think this Cl is already an improvement I will check it in and try to
clean up more in a separate CL.

[hubbe, 2016-11-04 19:51:53]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-11-04 19:52:47]

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-11-04 21:39:21]

Description was changed from

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some
opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993
==========

to

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some
opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993
==========

[commit-bot: I haz the power, 2016-11-04 21:39:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-04 21:42:54]

Description was changed from

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some
opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993
==========

to

==========
media: Increase preloading and max buffer for high-bitrate videos

Currently we limit preloading to 20Mb. This can be a problem with high bitrate
videos which mux a lot of data per chunk. Usually our goal is to preload 10
seconds
of data, but in some examples, only 6 seconds is preloaded because of this
limit.

This CL increases the max buffer limit to 50Mb. This should only impact videos
with
average bitrates over 16Mbit/s. Videos with bitrates up to 41Mbit/s will have 10
seconds
of preloading.

Also simplify the buffering calculations a bit by getting rid of some
opportunistic pinning behind the curring reading position.

Finally, add some tests.

BUG=512993

Committed: https://crrev.com/aa045cd99f2920d12c501b8c0b5bf2752df3e771
Cr-Commit-Position: refs/heads/master@{#430022}
==========

[commit-bot: I haz the power, 2016-11-04 21:42:55]

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