Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
982553002:
Paint buffered range closest to the current play position (Closed)
|
Created:
5 years, 9 months ago by landell Modified:
5 years, 9 months ago Reviewers:
philipj_slow CC:
blink-reviews, zoltan1, blink-reviews-html_chromium.org, eae+blinkwatch, leviw+renderwatch, eric.carlson_apple.com, Dominik Röttsches, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-rendering, jchaffraix+rendering, pdr+renderingwatchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionPaint buffered range that is within delta from CT
This patch make sure to avoid cases where the current position and buffered ranges are slightly out of sync and the buffered ranges would not be painted
BUG=460904
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191830
Patch Set 1 #
Total comments: 6
Patch Set 2 : Paint ranges within a delta from CT #Patch Set 3 : Add link to specification bug #Patch Set 4 : Formatting fixup #Messages
Total messages: 17 (2 generated)
@philipj et al: Please review
Sorry, this didn't show up in my "incoming reviews" dashboard but in my "cc'd reviews" mail folder, if you add me as reviewer directly it's easier to spot. But here I am.
Thanks Mostyn, let's fix this. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... File Source/core/layout/LayoutMediaControls.cpp (left): https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:218: if (std::isnan(start) || std::isnan(end) || start > currentTime || end < currentTime) Could we fix the whole problem by tweaking this to use "start > currentTime + some delta" instead? The delta could be a new HTMLMediaElement::cachedTimeDelta() implemented as m_cachedTime - webMediaPlayer()->currentTime(), which could find use in HTMLMediaElement::currentTime() as well. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... File Source/core/layout/LayoutMediaControls.cpp (right): https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:216: unsigned nearestIdx = bufferedTimeRanges->nearestRange(currentTime); So I was thinking that we could have a getter for the nearest range, but the ranges themselves aren't objects at all. This API ends up a bit cumbersome I think. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:232: // Only paint ranges that will intersect with the slider thumb I see no explicit delta, so I guess the resolution of the slider is what serves that role? Couldn't also break in edge cases just like today, where the alignment of values is just unfortunate?
Yes, I missed adding you to the reviewers. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... File Source/core/layout/LayoutMediaControls.cpp (left): https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:218: if (std::isnan(start) || std::isnan(end) || start > currentTime || end < currentTime) On 2015/03/06 03:16:17, philipj_UTC7 wrote: > Could we fix the whole problem by tweaking this to use "start > currentTime + > some delta" instead? The delta could be a new > HTMLMediaElement::cachedTimeDelta() implemented as m_cachedTime - > webMediaPlayer()->currentTime(), which could find use in > HTMLMediaElement::currentTime() as well. I can look at such a solution. Don't know if the special handling for seeking and default playback position might get in the way though. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... File Source/core/layout/LayoutMediaControls.cpp (right): https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:216: unsigned nearestIdx = bufferedTimeRanges->nearestRange(currentTime); On 2015/03/06 03:16:17, philipj_UTC7 wrote: > So I was thinking that we could have a getter for the nearest range, but the > ranges themselves aren't objects at all. This API ends up a bit cumbersome I > think. I agree. To me the problem is that the TimeRanges interface is kind of weird and this was an attempt to play nicely with it. :) I could change it to return a TimeRanges::Range instead if you think it is better. https://codereview.chromium.org/982553002/diff/1/Source/core/layout/LayoutMed... Source/core/layout/LayoutMediaControls.cpp:232: // Only paint ranges that will intersect with the slider thumb On 2015/03/06 03:16:17, philipj_UTC7 wrote: > I see no explicit delta, so I guess the resolution of the slider is what serves > that role? Couldn't also break in edge cases just like today, where the > alignment of values is just unfortunate? My thinking was that it made sense to make the delta condition based on visibility, i.e. we avoid painting ranges that would introduce gaps between the thumb and range. It's kind of artificial though, since painting will use the currentPosition at one end anyway. As you say, there are some corner cases here but I only think it would be significant for short streams, assuming the diff is small. I could add some timeline based delta here or maybe switch to it entirely. This solution will always have corner cases though but a delta based on timestamps is probably a bit more robust.
Sorry I called you Mostyn, I don't know what I was thinking :)
It looks like m_cachedTime started out as an optimization: https://trac.webkit.org/changeset/71824 Most of the code has since been removed, the bit that is left only ensures that currentTime is wrong while paused. Very annoying. I'd be happy to just remove the code, but that would require some way of knowing when to fire the timeupdate event after the pause has completed, and there's no way to do that without changing the WebMediaPlayer interface I think. I'm having a very hard time coming up with a suggestion that doesn't feel dirty. What's the least terrible idea so far?
On 2015/03/06 16:59:29, philipj_UTC7 wrote: > It looks like m_cachedTime started out as an optimization: > https://trac.webkit.org/changeset/71824 > > Most of the code has since been removed, the bit that is left only ensures that > currentTime is wrong while paused. Very annoying. I'd be happy to just remove > the code, but that would require some way of knowing when to fire the timeupdate > event after the pause has completed, and there's no way to do that without > changing the WebMediaPlayer interface I think. > Seems like I have stumbled upon a bunch of these "should be async" platform interface problems lately. Seems like we want to avoid that until that spec bug you reported is sorted out. > I'm having a very hard time coming up with a suggestion that doesn't feel dirty. > What's the least terrible idea so far? 1. I like your idea of just tweaking the condition in the range seeking loop a bit but I have not got time to look into it yet. I am stacked up with some other stuff right now but hope to get time for it tomorrow. It's also a bit dirty but at least the changes should be small. 2. As a second choice I would prefer the current solution, since it is easy to understand the sematics, although the patch is a bit intrusive. Tweaking to some time based delta as you say would be better though. I guess we could move the nearestRange functionality to be local to the media controls instead of extending the TimeRanges interface to avoid changing that interface if that is better. 3. If your spec bug is accepted we need to change the WebMediaPlayer interface anyway, right? So, that would probably be the best solution in the long run I guess.
Yes, making WebMediaPlayer::pause() async is the only sound long term fix that I can see. I can't really see an alternative to the proposed fix for the spec bug, so if you want to try making WebMediaPlayer::pause() async that would be really great, it could presumably be done in such a way that we continue to lie to scripts about currentTime until the spec change happens but can use the correct time internally. Or... just to get things somewhat working, simply using the range after currentTime as long as it is within a smallish delta (1 second?) would be OK. I would like something that isn't vulnerable to the existing bug in edge cases which would seem to happen if we use the slider thumb position to determine what should be painted.
On 2015/03/10 08:39:20, philipj_UTC7 wrote: > Yes, making WebMediaPlayer::pause() async is the only sound long term fix that I > can see. I can't really see an alternative to the proposed fix for the spec bug, > so if you want to try making WebMediaPlayer::pause() async that would be really > great, it could presumably be done in such a way that we continue to lie to > scripts about currentTime until the spec change happens but can use the correct > time internally. > > Or... just to get things somewhat working, simply using the range after > currentTime as long as it is within a smallish delta (1 second?) would be OK. I > would like something that isn't vulnerable to the existing bug in edge cases > which would seem to happen if we use the slider thumb position to determine what > should be painted. Nice to see you online. :) I will go for the second workaround for now unless I get some unexpected time over to dig into the acync interface stuff.
On 2015/03/10 08:58:29, landell wrote: > On 2015/03/10 08:39:20, philipj_UTC7 wrote: > > Yes, making WebMediaPlayer::pause() async is the only sound long term fix that > I > > can see. I can't really see an alternative to the proposed fix for the spec > bug, > > so if you want to try making WebMediaPlayer::pause() async that would be > really > > great, it could presumably be done in such a way that we continue to lie to > > scripts about currentTime until the spec change happens but can use the > correct > > time internally. > > > > Or... just to get things somewhat working, simply using the range after > > currentTime as long as it is within a smallish delta (1 second?) would be OK. > I > > would like something that isn't vulnerable to the existing bug in edge cases > > which would seem to happen if we use the slider thumb position to determine > what > > should be painted. > > Nice to see you online. :) > I will go for the second workaround for now unless I get some unexpected time > over to dig into the acync interface stuff. Sorry about the timezone problem, I'll be back soon :) A workaround sounds good enough to me.
Uploaded new patch set. The description needs to be modified but I'll wait with that until the code is accepted.
This LGTM, nice and contained. If you could also link to the spec bug in the comment that would be nice. There's a blink-dev thread "Coding style proposal: FIXME -> TODO(name)" right now which has a lot of support, so if you're daring you can go first. If not FIXME won't raise any eyebrows.
On 2015/03/12 16:47:11, philipj_UTC7 wrote: > This LGTM, nice and contained. If you could also link to the spec bug in the > comment that would be nice. > > There's a blink-dev thread "Coding style proposal: FIXME -> TODO(name)" right > now which has a lot of support, so if you're daring you can go first. If not > FIXME won't raise any eyebrows. Ah, I actually used the TODO(name) style since I thought that was required, but apparently that is a chromium code style req. Since that discussion is ongoing I use the olde style in case the final descision is going in another direction.
The CQ bit was checked by landell@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/982553002/#ps60001 (title: "Formatting fixup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/982553002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=191830 |
