|
|
Descriptionmedia: 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}
Patch Set 1 #
Total comments: 2
Patch Set 2 : logic simplified, test added #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
hubbe@chromium.org changed reviewers: + dalecurtis@google.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
I don't quite follow how this is limited to high bit rate videos. Can you elaborate?
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@chromium.org changed reviewers: + watk@chromium.org
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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 ==========
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.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kMaxBufferSize seems unused now? Since |bitrate_| is untrusted, that probably shouldn't be true.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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
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?
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.
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?
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.
This seems fine as a solution, so lgtm, but I wonder if clamping bitrate_ is easier.
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.
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/aa045cd99f2920d12c501b8c0b5bf2752df3e771 Cr-Commit-Position: refs/heads/master@{#430022} |