Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
435303004:
Fix buffered ranges when duration is infinite. (Closed)
|
Created:
6 years, 4 months ago by DaleCurtis Modified:
6 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix buffered ranges when duration is infinite.
Debug builds would hit a DCHECK(), but release builds would overflow
the int64 when manipulating kInfiniteDuration according to position.
Trying to estimate based on bytes when the duration is infinite is
just plain wrong, so skip estimation in that case. The buffered ranges
will now only update according to what FFmpegDemuxer/ChunkDemuxer
report in the infinite duration case.
BUG=400442
TEST=New layout test https://codereview.chromium.org/499513002/
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291504
Patch Set 1 #Patch Set 2 : No inf. #Patch Set 3 : Derp. #
Messages
Total messages: 27 (0 generated)
Not sure if this is what we want. Do either of you know what the intended behavior of the code should be?
On 2014/08/05 00:00:59, DaleCurtis wrote: > Not sure if this is what we want. Do either of you know what the intended > behavior of the code should be? It's been too long for me to remember this well, but my quick analysis: * AddBufferedByteRange() isn't called unless the length is known (probably this should be changed). So we always know the relative progress. * Real buffered time ranges are saved even if the total length isn't known (but in some cases are approximate). * The slider is not rendered if the duration is unknown. * Without help from the demuxer, there isn't a meaningful way to do this conversion. * So, at least for rendering the slider, it doesn't matter what we do as long as it doesn't crash. I suspect that a good overall solution is to not call AddBufferedTimeRanges() at all (from WebMediaPlayerImpl::buffered()) if the duration is unknown. I'm fine with your current implementation under the same reasoning.
lgtm
On 2014/08/05 00:37:22, sandersd wrote: > On 2014/08/05 00:00:59, DaleCurtis wrote: > > Not sure if this is what we want. Do either of you know what the intended > > behavior of the code should be? > > It's been too long for me to remember this well, but my quick analysis: > > * AddBufferedByteRange() isn't called unless the length is known (probably > this should be changed). So we always know the relative progress. > * Real buffered time ranges are saved even if the total length isn't known > (but in some cases are approximate). > * The slider is not rendered if the duration is unknown. > * Without help from the demuxer, there isn't a meaningful way to do this > conversion. > * So, at least for rendering the slider, it doesn't matter what we do as long > as it doesn't crash. > > I suspect that a good overall solution is to not call AddBufferedTimeRanges() at > all (from WebMediaPlayerImpl::buffered()) if the duration is unknown. Yeah that's what I was thinking as well... Do we know why we report infinite duration when we set the file via src as opposed to MSE? Reporting a buffered duration of infinity doesn't seem like the right thing.
Not sure, ChunkDemuxer does some tricky things with the duration though: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/chun... I'll give it a closer look tomorrow.
So MSE has it's own buffered() algorithm which is based on the parsed data handed in so far, so it is always able to return a buffered range irrespective of infinite duration. FFmpeg isn't able to parse a duration for the file, so we end up in a bad state. An alternate fix might be to always return 0,inf when duration == kInfiniteDuration(). I'm not sure that's any better than this though.
Ping?
On 2014/08/07 20:56:39, DaleCurtis wrote: > Ping? I feel we should only call AddBufferedTimeRanges() when a valid duration is known since if we did the real fix (http://crbug.com/349847) we wouldn't return [0,inf] or [inf,inf]
What would you return to buffered() then? An empty range? https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
On 2014/08/07 23:04:35, DaleCurtis wrote: > What would you return to buffered() then? An empty range? > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... I actually noticed that prior to sandersd@'s r262892 we actually would report valid-ish numbers for buffered.end(0): http://sleep.sea/demos/crbug400442/ It'd be good to understand what changed. We can then either decide to revert us back to reporting pre-r262892 numbers or move forward with an empty range.
On 2014/08/07 23:59:03, scherkus wrote: > On 2014/08/07 23:04:35, DaleCurtis wrote: > > What would you return to buffered() then? An empty range? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > I actually noticed that prior to sandersd@'s r262892 we actually would report > valid-ish numbers for buffered.end(0): > http://sleep.sea/demos/crbug400442/ > > It'd be good to understand what changed. We can then either decide to revert us > back to reporting pre-r262892 numbers or move forward with an empty range. This is where that change happened: https://codereview.chromium.org/224093011/diff/210001/media/base/pipeline.cc
Hmm, perhaps clock_->Duration() != MediaDuration. I'll take a closer look.
Hmm, I just downloaded r262871 and get junk values for the buffered end range there too. Which revision did you use? If I skip back to r250015 I get a value which is junky in a different way, ~0.15 vs ~600 quadrillion. Neither is a good value. I think we should just return [0, inf] when the duration is infinite. I've changed the patch set in this way.
scherkus: Ping?
On 2014/08/20 00:54:59, DaleCurtis wrote: > scherkus: Ping? No matter how you slice it returning [0, inf] doesn't make sense. The whole point of this bytes-to-time-range conversion hack is to try and return a credible lie/approximation to Blink because we haven't fixed http://crbug.com/349847. Returning [0, inf] isn't a credible lie/approximation. Something like [0, currentTime] would be a more credible lie/approximation. Heck even [0, 0] would at least be consistent with Firefox! I noticed that FFmpegDemuxer tries to call AddBufferedTimeRange() as it parses the file. Can we rely on that information instead of attempting the bytes conversion when duration is unknown?
Derp, that's what I get for spending like 5 seconds on just fixing the DCHECK. Here's the correct fix which does not perform extrapolation when duration is reported as infinite. Instead the buffered range will be extended as playback goes on based on the ranges reported via FFmpegDemuxer. ChunkDemuxer performs similarly, so this will work in both cases. PTAL. Sorry for the lame last patch.
On 2014/08/21 19:54:21, DaleCurtis wrote: > Derp, that's what I get for spending like 5 seconds on just fixing the DCHECK. > Here's the correct fix which does not perform extrapolation when duration is > reported as infinite. > > Instead the buffered range will be extended as playback goes on based on the > ranges reported via FFmpegDemuxer. ChunkDemuxer performs similarly, so this > will work in both cases. > > PTAL. Sorry for the lame last patch. no worries! this is definitely the way to go -- check out my test page where it shows we're correctly incrementing buffered ahead of current time http://sleep.sea/demos/crbug400442/index.html one day we can rip out all this code but it involves a good amount of FFmpegDemuxer spelunking now we just need an updated CL description -- also some form of test would be nice (layout test with a minimal webm that doesn't advertise a duration?!)
Test here: https://codereview.chromium.org/499513002/ will have to wait until after this lands though.
lgtm thanks for writing a layout test!
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/435303004/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/435303004/40001
Message was sent while issue was closed.
Committed patchset #3 (40001) as 291504 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
