Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
562493003:
Allow seeks to zero on streaming sources. (Closed)
|
Created:
6 years, 3 months ago by DaleCurtis Modified:
6 years, 2 months ago Reviewers:
philipj_slow CC:
blink-reviews, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium, scherkus (not reviewing) Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionAllow seeks to zero on streaming sources.
Adds a [0, 0] range to seekable() when a streaming source
with a known duration is present. Doing so allows the seek
triggered by loop attribute presence upon ended to correctly
seek back to zero.
To facilitate testing, the serve-video.php script now supports
a new argument "norange" which serves files with a 200 response
instead of 206.
BUG=412562
TEST=new layout test.
Patch Set 1 #
Total comments: 13
Patch Set 2 : Expose 0,0 range. #Patch Set 3 : Add tests. #
Messages
Total messages: 42 (4 generated)
dalecurtis@chromium.org changed reviewers: + acolwell@chromium.org
If this looks reasonable I'll add a layout test for it.
Can you split the TimeRanges fix into a separate CL?
philipj@opera.com changed reviewers: + philipj@opera.com
Thanks for working on this, let's see if we can agree on a fix :) https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3231: } Doesn't compile ;) https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3233: // For non-streaming media, emulate a seekable range around zero so looping works. I don't think this is fixing the problem at the appropriate point. I assume we're talking about finite resources that don't support range requests. When playback has ended, we will know its duration and that could be reported in the seekable attribute, right? In Opera's old GStreamer backend, when we reached end-of-stream and didn't have a duration yet, we reported the current time as the duration, and the seekable ranges were simply [0,duration]. https://codereview.chromium.org/562493003/diff/1/Source/core/html/TimeRangesT... File Source/core/html/TimeRangesTest.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/TimeRangesT... Source/core/html/TimeRangesTest.cpp:311: ASSERT_EQ(2, ranges->nearest(3)); Can you also test the case where the point is exactly in between the ranges? It may be best to change the ranges to avoid a non-integer 3.5. The spec says "If two positions both satisfy that constraint (i.e. the new playback position is exactly in the middle between two ranges in the seekable attribute) then use the position that is closest to the current playback position." so you'll have to pass that in as well.
https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3233: // For non-streaming media, emulate a seekable range around zero so looping works. On 2014/09/10 12:51:18, philipj wrote: > I don't think this is fixing the problem at the appropriate point. I assume > we're talking about finite resources that don't support range requests. When > playback has ended, we will know its duration and that could be reported in the > seekable attribute, right? But we don't want to advertise that we can actually seek anywhere inside [0, duration). The idea is that we are signalling that we only can seek to the beginning. > > In Opera's old GStreamer backend, when we reached end-of-stream and didn't have > a duration yet, we reported the current time as the duration, and the seekable > ranges were simply [0,duration]. The FFmpegDemuxer should be doing similar behavior here ( https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...). https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3239: ranges->add(now, now + std::numeric_limits<double>::epsilon()); Have you tested this with https://codereview.chromium.org/456343002/ applied? I would be very surprised if it actually worked the way you want. I think we should leave this out, just fix the looping problem, and the followup with this in a different CL once https://codereview.chromium.org/456343002/ lands.
Thanks for taking a look guys! I'll split it up shortly. Just comments for now until we can all agree on an approach. https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3231: } On 2014/09/10 12:51:18, philipj wrote: > Doesn't compile ;) Heh, whoops, must have hit CtrlZ once too many before uploading. https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3239: ranges->add(now, now + std::numeric_limits<double>::epsilon()); On 2014/09/10 17:38:43, acolwell wrote: > Have you tested this with https://codereview.chromium.org/456343002/ applied? I > would be very surprised if it actually worked the way you want. I think we > should leave this out, just fix the looping problem, and the followup with this > in a different CL once https://codereview.chromium.org/456343002/ lands. I haven't, but that CL just removes the noSeekRequired flag which this code transitively bypasses already. I'll patch it in and give it a shot to be sure though. How are you proposing to fix the issue if we leave this out? It's the actual fix :) https://codereview.chromium.org/562493003/diff/1/Source/core/html/TimeRangesT... File Source/core/html/TimeRangesTest.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/TimeRangesT... Source/core/html/TimeRangesTest.cpp:311: ASSERT_EQ(2, ranges->nearest(3)); On 2014/09/10 12:51:18, philipj wrote: > Can you also test the case where the point is exactly in between the ranges? It > may be best to change the ranges to avoid a non-integer 3.5. > > The spec says "If two positions both satisfy that constraint (i.e. the new > playback position is exactly in the middle between two ranges in the seekable > attribute) then use the position that is closest to the current playback > position." so you'll have to pass that in as well. Damn, you caught me! :) I was trying to avoid any more changes here, but in retrospect it's a tiny change, I'll add it in the separate CL.
https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3239: ranges->add(now, now + std::numeric_limits<double>::epsilon()); On 2014/09/10 17:45:18, DaleCurtis wrote: > On 2014/09/10 17:38:43, acolwell wrote: > > Have you tested this with https://codereview.chromium.org/456343002/ applied? > I > > would be very surprised if it actually worked the way you want. I think we > > should leave this out, just fix the looping problem, and the followup with > this > > in a different CL once https://codereview.chromium.org/456343002/ lands. > > I haven't, but that CL just removes the noSeekRequired flag which this code > transitively bypasses already. I'll patch it in and give it a shot to be sure > though. > > How are you proposing to fix the issue if we leave this out? It's the actual fix > :) You only need the [0,epsilon) range for loop to work right? I thought the [now, now + epsilon) is just a convenience if the webapp tries to seek elsewhere.
https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3239: ranges->add(now, now + std::numeric_limits<double>::epsilon()); On 2014/09/10 17:45:18, DaleCurtis wrote: > On 2014/09/10 17:38:43, acolwell wrote: > > Have you tested this with https://codereview.chromium.org/456343002/ applied? > I > > would be very surprised if it actually worked the way you want. I think we > > should leave this out, just fix the looping problem, and the followup with > this > > in a different CL once https://codereview.chromium.org/456343002/ lands. > > I haven't, but that CL just removes the noSeekRequired flag which this code > transitively bypasses already. I'll patch it in and give it a shot to be sure > though. Oh right. I didn't happen to notice that the nearest() call was after this code and assumed the "seek to now" caused by nearest would end up on that path. Sorry. It doesn't look like Pipeline special cases "seek to now" so won't this end up causing the FFmpegDemuxer to do a completely new fetch for the resource and potentially download the whole thing again?
https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3233: // For non-streaming media, emulate a seekable range around zero so looping works. On 2014/09/10 17:38:43, acolwell wrote: > On 2014/09/10 12:51:18, philipj wrote: > > I don't think this is fixing the problem at the appropriate point. I assume > > we're talking about finite resources that don't support range requests. When > > playback has ended, we will know its duration and that could be reported in > the > > seekable attribute, right? > > But we don't want to advertise that we can actually seek anywhere inside [0, > duration). The idea is that we are signalling that we only can seek to the > beginning. Oh, is a streaming cache used for non-seekable HTTP resources? I was assuming that they were written to cache like, say, images and that reaching the end implied that you have it all. If the response has a Content-Length header, then one should be able to decide up front which kind of cache is appropriate. Since there can be different strategies, assuming one here seems suboptimal. The WebMediaPlayer is closer to the relevant facts, could this logic be put there? I'm also curious what the real-world case that led to the discovery of this bug was. If it was something like a short looping wav file, then a new network request for each loop would be bad.
Our caching story is a little complicated. The media/ code has no insight into whether it's actually caching (by design). So with short clips (even with 200 response), the full clip is most likely cached which leads to efficient seeks. Even though some portions may be cached, we can't know until we try to request them. Further even if one part is, if we reach a part that isn't, we'll still have to request the full resource up until the point needed. https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3233: // For non-streaming media, emulate a seekable range around zero so looping works. On 2014/09/10 18:42:10, philipj wrote: > On 2014/09/10 17:38:43, acolwell wrote: > > On 2014/09/10 12:51:18, philipj wrote: > > > I don't think this is fixing the problem at the appropriate point. I assume > > > we're talking about finite resources that don't support range requests. When > > > playback has ended, we will know its duration and that could be reported in > > the > > > seekable attribute, right? > > > > But we don't want to advertise that we can actually seek anywhere inside [0, > > duration). The idea is that we are signalling that we only can seek to the > > beginning. > > Oh, is a streaming cache used for non-seekable HTTP resources? I was assuming > that they were written to cache like, say, images and that reaching the end > implied that you have it all. If the response has a Content-Length header, then > one should be able to decide up front which kind of cache is appropriate. > > Since there can be different strategies, assuming one here seems suboptimal. The > WebMediaPlayer is closer to the relevant facts, could this logic be put there? > > I'm also curious what the real-world case that led to the discovery of this bug > was. If it was something like a short looping wav file, then a new network > request for each loop would be bad. Some are cached, some may not be depending on flags: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/cache_... Note, this doesn't actually impact what the Cache is actually doing, it just tries to assume why a resource won't be cached. It's not completely accurate and errs on the side of thinking something will not be cached. I have another active task to clean this up, but it should give you an idea of what's going on. With FFmpegDemuxer, even after playback the duration won't be updated. ChunkDemuxer won't take this path since resources are appended through JavaScript, thus a seekable range is always defined. I admit this is a transitive assumption. See the bug for more details, but just using a python SimpleHTTPServer with a short clip will show that looping isn't working with non-range requests. It will currently hang at ended and never resume played or transition the player into the paused / error state. https://codereview.chromium.org/562493003/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3239: ranges->add(now, now + std::numeric_limits<double>::epsilon()); On 2014/09/10 18:41:05, acolwell wrote: > On 2014/09/10 17:45:18, DaleCurtis wrote: > > On 2014/09/10 17:38:43, acolwell wrote: > > > Have you tested this with https://codereview.chromium.org/456343002/ > applied? > > I > > > would be very surprised if it actually worked the way you want. I think we > > > should leave this out, just fix the looping problem, and the followup with > > this > > > in a different CL once https://codereview.chromium.org/456343002/ lands. > > > > I haven't, but that CL just removes the noSeekRequired flag which this code > > transitively bypasses already. I'll patch it in and give it a shot to be sure > > though. > Oh right. I didn't happen to notice that the nearest() call was after this code > and assumed the "seek to now" caused by nearest would end up on that path. > Sorry. > > It doesn't look like Pipeline special cases "seek to now" so won't this end up > causing the FFmpegDemuxer to do a completely new fetch for the resource and > potentially download the whole thing again? > Ah I thought you were asking to remove all ranges, yes, moving the now,now range to another patch or finding a different solution is fine me. Technically yes, but since this is such a short resource it's all actually cached. We're just not allowing seeks into it because we can't assume that (though we could make some guess based on duration). Additionally, I'd like to see us special case seek to now before landing the patch you linked to avoid unnecessary seeks. I think it should land before hand to avoid issues.
TimeRanges split out here: https://codereview.chromium.org/559993004/ -- no changes to this CL yet pending discussion resolution.
Won't having a range [0,epsilon] mean that a script-initiated seek to e.g. 1 will clamp down to epsilon and try to seek the WebMediaPlayer to that position?
On 2014/09/10 18:56:49, DaleCurtis wrote: > The media/ code has no insight into whether it's actually caching (by design). OK, that explains some things. BTW, cache_util.h's GetReasonsForUncacheability() is only used for
Oops. cache_util.h's GetReasonsForUncacheability() is only used for UMA, right? I'm not sure how to best deal with this situation. At some level the problem is that we must report which ranges we can seek to, but can't know if we really can seek to them without trying. The same ought to be true even of a seek to 0, if that requires a new network request the resource could be gone, but I guess an error event will be fired. How are the buffered ranges reported for resources like this? Indeed for any resource, accurate reporting buffered without knowledge of the cache seems like an impossibility.
On 2014/09/11 11:31:21, philipj wrote: > Oops. > > cache_util.h's GetReasonsForUncacheability() is only used for UMA, right? Yes, it's not an accurate reflection at this time either. > > I'm not sure how to best deal with this situation. At some level the problem is > that we must report which ranges we can seek to, but can't know if we really can > seek to them without trying. The same ought to be true even of a seek to 0, if > that requires a new network request the resource could be gone, but I guess an > error event will be fired. Thinking about this some more, if we allow any seeking at all (even to zero) we may hit unexpected playback stalls later while waiting for data. The cache may evict data after position zero, such that the initial seek is cached. > > How are the buffered ranges reported for resources like this? Indeed for any > resource, accurate reporting buffered without knowledge of the cache seems like > an impossibility. Here's the flow today. First, they're reported via BufferedDataSourceHost: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... Which hands them off to the BufferedDataSourceHostImpl: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer... Second, FFmpegDemuxer also tracks "buffered" ranges as they're demuxed and reports them to the BufferedDataSourceHostImpl: https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp... WebMediaPlayerImpl consults both the Pipeline and BufferedDataSourceHostImpl to return information: https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed... As such... we don't actually consider the cache when reporting buffered.
Actually, I need to amend that last comment. While we don't have access to the HttpCache, we can know what BufferedResourceLoader has in its SeekableBuffer. We could enhance BRL to update its buffered byte ranges when purging backwards capacity. We might then be able to use the buffered() ranges as indicators for whether a seek should be allowed... even for streaming / live sources. The complication would be in mapping byte ranges to time ranges in a sane way. cc: scherkus who is more familiar with the BufferedResourceLoader in case he has any ideas.
Alright I think I have a good proposal which extends past fixing this issue: - Change BufferedResourceLoader to notify clients via the ProgressCB when regions of the buffer are evicted. - Change BufferedDataSourceHostImpl() to support removing ranges. - Change HTMLMediaElement::seekable() to return [0,epsilon] + buffered() when streaming sources with a known duration are present. - Optionally change BufferedResourceLoader to use the MediaSource limits for streaming sources (e.g., 150mb) w/ known content-length instead of the 20mb ones used today. WDYT?
Sorry for letting this sit in limbo, I have many reviews to go through and haven't gotten into the rhythm of it. The situation with reporting buffered ranges sounds rather complicated. In Presto (Opera 12) we simply had access to what was in the cache and mapped byte ranges to time ranges using the keyframe index, or by simple interpolation if there was no keyframe index. I was pretty happy with how were able to accurately report any number of buffered ranges, including buffered ranges before currentTime. What is the role of BufferedResourceLoader here, is it a caching system on top of the cache which you are allowed to peek into? Is BufferedResourceLoader allowed to peek at the underlying cache, and if so can that information be propagated. At the end of the day, I'm not so happy with exposing [0,epsilon] in seekable, because it's so easily observable to Web content and seems like the kind of quirk that may get stuck on the Web. If there isn't any imminent solution for accurately reporting buffered and seekable in these situations, how about simply skipping the seekable check when seeking to 0 for these kinds of resources? That's also Web observable and not per spec, but at least only if one actually tries to seek.
On 2014/09/16 19:37:32, philipj wrote: > Sorry for letting this sit in limbo, I have many reviews to go through and > haven't gotten into the rhythm of it. No rush, I appreciate your time! > > The situation with reporting buffered ranges sounds rather complicated. In > Presto (Opera 12) we simply had access to what was in the cache and mapped byte > ranges to time ranges using the keyframe index, or by simple interpolation if > there was no keyframe index. I was pretty happy with how were able to accurately > report any number of buffered ranges, including buffered ranges before > currentTime. > > What is the role of BufferedResourceLoader here, is it a caching system on top > of the cache which you are allowed to peek into? Is BufferedResourceLoader > allowed to peek at the underlying cache, and if so can that information be > propagated. Yes, that's its role today, it initiates the WebURLRequest and creates an in memory cache of the contents returned, subject to allocation limits. If BRL can't fulfill a request it reports a kCacheMiss and the BufferedDataSource will create a new ResourceLoader in an attempt to read the requested data. BRL has no access to the underlying http level cache, only it's internal one. > > At the end of the day, I'm not so happy with exposing [0,epsilon] in seekable, > because it's so easily observable to Web content and seems like the kind of > quirk that may get stuck on the Web. If there isn't any imminent solution for > accurately reporting buffered and seekable in these situations, how about simply > skipping the seekable check when seeking to 0 for these kinds of resources? > That's also Web observable and not per spec, but at least only if one actually > tries to seek. I'm okay with special casing seek to zero, I think it's the least bad of possible solutions. Even if we exposed buffered() with 100% accuracy we may not have the 0 position (due to eviction), so we'd still need a workaround for looping 200-type resources in that case. acolwell: You voted against special casing zero seeks. In light of Philip's commentary and the discussions here, does your opinion remain steadfast?
On 2014/09/16 20:12:12, DaleCurtis wrote: > On 2014/09/16 19:37:32, philipj wrote: > > > > At the end of the day, I'm not so happy with exposing [0,epsilon] in seekable, > > because it's so easily observable to Web content and seems like the kind of > > quirk that may get stuck on the Web. If there isn't any imminent solution for > > accurately reporting buffered and seekable in these situations, how about > simply > > skipping the seekable check when seeking to 0 for these kinds of resources? > > That's also Web observable and not per spec, but at least only if one actually > > tries to seek. > > I'm okay with special casing seek to zero, I think it's the least bad of > possible > solutions. Even if we exposed buffered() with 100% accuracy we may not have the > 0 > position (due to eviction), so we'd still need a workaround for looping 200-type > resources in that case. > > acolwell: You voted against special casing zero seeks. In light of Philip's > commentary and the discussions here, does your opinion remain steadfast? I'm still not a fan of violating the spec since that is also a Web visible quirk that we could get stuck with. In my opinion the [0, epsilon) actually reflects the type of seek the current code supports while also staying spec compliant. Even if this isn't what the user expects, the behavior can always be justified in terms of the HTML5 spec and what our code is able to do in the 200 response scenario. Ultimately you folks are the ones that have to maintain this code and respond to user complaints so I'll leave the final decision up to you.
So let's figure out what the best long-term solution would be and if that isn't allowed by the spec, ask to have it changed before looking at other options. 1. Report seekable ranges [0, epsilon] Allowed by spec, although the epsilon obviously doesn't correspond to anything in the media resource. What happens when currentTime is set to epsilon or >epsilon while paused? Per current spec, one should end up at epsilon, but that seems like it would need special casing to support. If the earliest possible position is not 0, should it instead be [n,n+epsilon]? 2. Report no seekable ranges but allow seeking to zero by restarting the request Needs a spec change to allow the seek to 0. 3. Report seekable range [0,0]. Needs a spec change to allow an empty range. What do you all think about this one?
On 2014/09/16 21:56:13, philipj wrote: > So let's figure out what the best long-term solution would be and if that isn't > allowed by the spec, ask to have it changed before looking at other options. > > 1. Report seekable ranges [0, epsilon] > > Allowed by spec, although the epsilon obviously doesn't correspond to anything > in the media resource. What happens when currentTime is set to epsilon or > >epsilon while paused? Per current spec, one should end up at epsilon, but that > seems like it would need special casing to support. If the earliest possible > position is not 0, should it instead be [n,n+epsilon]? I agree that this should actually be [n, n+epsilon) where n is the "earliest possible position". I had meant to call that out in my last comment, but got distracted. :) > > 2. Report no seekable ranges but allow seeking to zero by restarting the request > > Needs a spec change to allow the seek to 0. This seems a little weird to me since the seekable ranges are supposed to reflect where one can seek to. I like no ranges meaning you can't seek. > > 3. Report seekable range [0,0]. > > Needs a spec change to allow an empty range. What do you all think about this > one? I'd be fine with this one, but I don't have a full grasp of the impact of this change. It looks like the ranges are intended to be inclusive so perhaps the "The start of a range must be less than the end of that same range" is not allowing start == end by accident.
On 2014/09/16 22:18:27, acolwell_leaving_chromium_9-23 wrote: > On 2014/09/16 21:56:13, philipj wrote: > > So let's figure out what the best long-term solution would be and if that > isn't > > allowed by the spec, ask to have it changed before looking at other options. > > > > 1. Report seekable ranges [0, epsilon] > > > > Allowed by spec, although the epsilon obviously doesn't correspond to anything > > in the media resource. What happens when currentTime is set to epsilon or > > >epsilon while paused? Per current spec, one should end up at epsilon, but > that > > seems like it would need special casing to support. If the earliest possible > > position is not 0, should it instead be [n,n+epsilon]? > > I agree that this should actually be [n, n+epsilon) where n is the "earliest > possible position". I had meant to call that out in my last comment, but got > distracted. :) > > > > > 2. Report no seekable ranges but allow seeking to zero by restarting the > request > > > > Needs a spec change to allow the seek to 0. > > This seems a little weird to me since the seekable ranges are supposed to > reflect where one can seek to. I like no ranges meaning you can't seek. > > > > > 3. Report seekable range [0,0]. > > > > Needs a spec change to allow an empty range. What do you all think about this > > one? > > I'd be fine with this one, but I don't have a full grasp of the impact of this > change. It looks like the ranges are intended to be inclusive so perhaps the > "The start of a range must be less than the end of that same range" is not > allowing start == end by accident. I'm also fine with the last one. I think that's technically cleanest.
In the interest of progress, I've uploaded the 0,0 version. TimeRanges accepts such ranges without issue and things all appear to work properly. If we agree on this I'll add a layout test shortly. The original annoyance of random clicks on the timeline restarting playback will be present with this approach. Towards that annoyance, I've put up the next portion of my proposal above here: https://codereview.chromium.org/574253002
On 2014/09/17 01:43:24, DaleCurtis wrote: > In the interest of progress, I've uploaded the 0,0 version. TimeRanges accepts > such ranges without issue and things all appear to work properly. If we agree > on this I'll add a layout test shortly. The original annoyance of random clicks > on the timeline restarting playback will be present with this approach. We could fix that pretty easily for the native controls by requiring a click close to the beginning of the timeline for this case. > Towards that annoyance, I've put up the next portion of my proposal above here: > > https://codereview.chromium.org/574253002 Do you mean that seekable should be a superset of buffered? That makes sense, but one would have to be a lot more careful about how to map byte ranges to time ranges, simple interpolation could result in lies. To report that time n is buffered, one must be very sure that all the bytes needed to reach time n are in fact available, which at least in Presto was a bit tricky but I think making use of the keyframe index is sound, assuming that one has control over the demuxer and it doesn't try to do anything clever.
The spec doesn't allow a [0,0] range, do you want me to open a spec bug for it? In the meantime, I can think of two tests for this: 1. Verify that [0,0] is exposed to scripts. 2. Verify that seeking to 1 ends up at 0, as opposed to failing or even successfully reaching 1. If possible, try to write the tests using testharness.js, so that they can be upstreamed to web-platform-tests.
On 2014/09/17 08:09:01, philipj wrote: > On 2014/09/17 01:43:24, DaleCurtis wrote: > > In the interest of progress, I've uploaded the 0,0 version. TimeRanges > accepts > > such ranges without issue and things all appear to work properly. If we agree > > on this I'll add a layout test shortly. The original annoyance of random > clicks > > on the timeline restarting playback will be present with this approach. > > We could fix that pretty easily for the native controls by requiring a click > close to the beginning of the timeline for this case. > > > Towards that annoyance, I've put up the next portion of my proposal above > here: > > > > https://codereview.chromium.org/574253002 > > Do you mean that seekable should be a superset of buffered? That makes sense, > but one would have to be a lot more careful about how to map byte ranges to time > ranges, simple interpolation could result in lies. To report that time n is > buffered, one must be very sure that all the bytes needed to reach time n are in > fact available, which at least in Presto was a bit tricky but I think making use > of the keyframe index is sound, assuming that one has control over the demuxer > and it doesn't try to do anything clever. That's an interesting and reasonable concern. Are seekable() ranges required to be that exact? It seems this would fall under a quality of implementation issue: http://dev.w3.org/html5/spec-preview/media-elements.html#best-practices-for-i... Today our demuxer just adds ranges as it encounters timestamps, with no regard to keyframes. Under the hood we always seek backwards to the nearest keyframe and drop frames / audio until we reach the desired time point. Lining up the byte estimates with 100% accuracy may be tricky, especially with content that may not be muxed in a normalized fashion. What we can do is take the intersection of the
On 2014/09/17 08:11:01, philipj wrote: > The spec doesn't allow a [0,0] range, do you want me to open a spec bug for it? That'd be great, thanks a lot! If for no other reason than to see what the spec authors will say. > > In the meantime, I can think of two tests for this: > 1. Verify that [0,0] is exposed to scripts. > 2. Verify that seeking to 1 ends up at 0, as opposed to failing or even > successfully reaching 1. > > If possible, try to write the tests using testharness.js, so that they can be > upstreamed to web-platform-tests. Thanks for the tips. I'll write these up and update this Cl when it's ready.
On 2014/09/17 19:50:48, DaleCurtis wrote: > On 2014/09/17 08:09:01, philipj wrote: > > On 2014/09/17 01:43:24, DaleCurtis wrote: > > > In the interest of progress, I've uploaded the 0,0 version. TimeRanges > > accepts > > > such ranges without issue and things all appear to work properly. If we > agree > > > on this I'll add a layout test shortly. The original annoyance of random > > clicks > > > on the timeline restarting playback will be present with this approach. > > > > We could fix that pretty easily for the native controls by requiring a click > > close to the beginning of the timeline for this case. > > > > > Towards that annoyance, I've put up the next portion of my proposal above > > here: > > > > > > https://codereview.chromium.org/574253002 > > > > Do you mean that seekable should be a superset of buffered? That makes sense, > > but one would have to be a lot more careful about how to map byte ranges to > time > > ranges, simple interpolation could result in lies. To report that time n is > > buffered, one must be very sure that all the bytes needed to reach time n are > in > > fact available, which at least in Presto was a bit tricky but I think making > use > > of the keyframe index is sound, assuming that one has control over the demuxer > > and it doesn't try to do anything clever. > > That's an interesting and reasonable concern. Are seekable() ranges required to > be that exact? It seems this would fall under a quality of implementation issue: > > http://dev.w3.org/html5/spec-preview/media-elements.html#best-practices-for-i... For buffered the spec does cut allow some inaccuracy, but also note "As a general rule, user agents are urged to be conservative rather than optimistic. For example, it would be bad to report that everything had been buffered when it had not." For seekable, the spec doesn't say anything special, and the seeking algorithm doesn't have a step for adjusting the resulting currentTime other than clamping it to the seekable ranges. Off-topic: https://html.spec.whatwg.org/ is the most upstream copy and always most up to date. > Today our demuxer just adds ranges as it encounters timestamps, with no regard > to keyframes. Under the hood we always seek backwards to the nearest keyframe > and drop frames / audio until we reach the desired time point. Lining up the > byte estimates with 100% accuracy may be tricky, especially with content that > may not be muxed in a normalized fashion. > > What we can do is take the intersection of the Cut off the end? :)
Here's the spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=26847 I can't add you to CC, I think you need an account in that Bugzilla. Let me know if you want that and I'll try again.
acolwell@chromium.org changed reviewers: - acolwell@chromium.org
I'm following along on that bug. While the TimeRanges question is separate to our seeking practices, I'm beginning to wonder if we should really worry about preventing seeks on 200-type media. I suspect in a lot of those cases, we're unnecessarily worrying about preventing seeks. Unfortunately we don't have any metrics into how many responses are streaming versus not, hence http://crbug.com/413451 :( For now I think the 0,0 TimeRange fix is the best idea, but we should be able to answer the question how many of our requests are range-requests.
Sorry for the delay, perf season is upon us at Google. :| I see that Ian seems onboard with allowing 0,0 so I've added the tests discussed. I'm happy to make the controls change in this patch, but I'm not sure which code is responsible for limiting clickable seeks. Any pointers?
On 2014/09/24 00:59:58, DaleCurtis wrote: > Sorry for the delay, perf season is upon us at Google. :| I see that Ian seems > onboard with allowing 0,0 so I've added the tests discussed. > > I'm happy to make the controls change in this patch, but I'm not sure which code > is responsible for limiting clickable seeks. Any pointers? That would be in MediaControlTimelineElement::defaultEventHandler. Possibly the easiest way to make it behave is to have MediaControlTimelineElement::willRespondToMouseClickEvents() return false where there are no non-empty seekable ranges.
OK, the spec change is in, together with some strong hints directed at us: https://html5.org/r/8819 Unrelated to this CL, I've had an Opera-internal request to replace "double WebMediaPlayer::maxTimeSeekable()" with a new "WebTimeRanges WebMediaPlayer::seekable()" to allow the backend to report a seekable range that doesn't start at 0, in this case a live stream where there's a bit of buffering but not infinite. How would you feel about changing the WebMediaPlayer API and implementing the [0,0] bit on the Chromium side? Blink could limit itself to asserting that the reported ranges are valid and maybe that buffered is a subset of seekable.
I think it's a great idea. Sorry for the delay in this CL, hopefully I can get to it tomorrow.
It would be great if you can work out how to solve the bug (412562) with my proposed changes. I'm assuming that it'll now make more sense on the Chromium side, but who knows :)
https://code.google.com/p/chromium/issues/detail?id=417669 is finished now.
Patchset #4 (id:60001) has been deleted
Thanks! I'm closing this issue in favor of these three: https://codereview.chromium.org/621573002/ https://codereview.chromium.org/615193004/ https://codereview.chromium.org/614263002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
