Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827573004:
Allow play/pause on default controls to clear media errors. (Closed)
|
Created:
4 years, 9 months ago by DaleCurtis Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow play/pause on default controls to clear media errors.
Simple change to the default controls that lets the user continue
playback with subsequent play/pause button presses. Useful in cases
where the error is transient (e.g., network related).
In the event of an error the default controls will now pause playback.
It'd be nice if we could restore current time in this case too, but
it seems to be cleared in unpredictable ways upon decode error.
BUG=none
TEST=play after decode / network error restarts from beginning.
Committed: https://crrev.com/e317ac947e2c62d9df653bf48d7881c85f9a23b6
Cr-Commit-Position: refs/heads/master@{#383884}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Force pause on error. #Patch Set 3 : Disable MediaSource and MediaStream URLs. #
Total comments: 3
Messages
Total messages: 23 (5 generated)
dalecurtis@chromium.org changed reviewers: + liberato@chromium.org, philipj@opera.com
WDYT? The current behavior is the player simply dies forever without possibility of recovery by the user unless the site takes special action.
On 2016/03/24 01:12:40, DaleCurtis wrote: > WDYT? The current behavior is the player simply dies forever without possibility > of recovery by the user unless the site takes special action. lg*m. i think it makes a lot of sense. in fact, if we choose not to do this, then we should change the play button to a sad tab picture when an error happens. :)
This is a bit of a mess on the spec side: https://github.com/whatwg/html/issues/346 Any feedback on how to better deal with transient errors much appreciated :)
https://codereview.chromium.org/1827573004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1827573004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:288: mediaElement().load(); I think an error doesn't actually set paused to true, at least I can't see where that happens. So it might happen that this still looks like a pause button, but pressing it reloads and plays. Better than nothing of course, but is that what you were hoping for? Something to consider could be a "play again" icon, which is used at the end of playback and also after an error. That way it'd be a bit more obvious what the button does before clicking it. We have that as an overlay button in Opera for Android, something I'd like to upstream. I don't think that needs to block this CL of course, but do you think it would make sense?
Description was changed from ========== Allow play/pause on default controls to clear media errors. Simple change to the default controls that lets the user continue playback with subsequent play/pause button presses. Useful in cases where the error is transient (e.g., network related). It'd be nice if we could restore current time in this case too, but it seems to be cleared in unpredictable ways upon decode error. BUG=none TEST=play after decode / network error restarts from beginning. ========== to ========== Allow play/pause on default controls to clear media errors. Simple change to the default controls that lets the user continue playback with subsequent play/pause button presses. Useful in cases where the error is transient (e.g., network related). In the event of an error the default controls will now pause playback. It'd be nice if we could restore current time in this case too, but it seems to be cleared in unpredictable ways upon decode error. BUG=none TEST=play after decode / network error restarts from beginning. ==========
https://codereview.chromium.org/1827573004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1827573004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:288: mediaElement().load(); On 2016/03/24 at 07:00:05, philipj_UTC7 wrote: > I think an error doesn't actually set paused to true, at least I can't see where that happens. So it might happen that this still looks like a pause button, but pressing it reloads and plays. Better than nothing of course, but is that what you were hoping for? > > Something to consider could be a "play again" icon, which is used at the end of playback and also after an error. That way it'd be a bit more obvious what the button does before clicking it. We have that as an overlay button in Opera for Android, something I'd like to upstream. I don't think that needs to block this CL of course, but do you think it would make sense? Ah, I thought that was a bug, I was going to hunt it down today. You're correct that it is stuck in the paused state when it dies. Errors do cause a reset of the controls though, so we could have them toggle the paused / error state when MediaControls::reset() is called. I agree that we should have some iconography here. Either Opera's button or reach out to the visual design team for a new material design button. Play again sounds like a good choice. Though, I actually like the idea of a tiny sad tab-like play button. Might be useful in differentiating reports from users. I don't think we need to make any update in this CL though -- especially since desktop and mobile controls are different at present. I'd prefer to make any updates after we've removed the old controls or limit them to the new UI.
dalecurtis@chromium.org changed reviewers: + wolenetz@chromium.org
+wolenetz from MSE side to see if there are any concerns there.
On 2016/03/24 18:50:33, DaleCurtis wrote: > +wolenetz from MSE side to see if there are any concerns there. I tried adding locally to this CL a one-time parse failure on first MediaSourceState::OnNewBuffers() invocation, and was a bit surprised that this CL let a another videoTag.play() subsequent to that failure reach successful playback on a simple MSE player (on a Release build of Chrome). I think that's a lucky fluke, though, since any further appendData() attempt would/should per spec yield InvalidStateException (since the parent element's error attribute is non-null and not reset). MSE specs and impls also extend some of the mess in HTML5 spec related to standardizing a more flexible way to recover from errors. See also https://github.com/w3c/media-source/issues/36 I chatted w/dalecurtis, too. I recommend keeping (for now) the recoverability of playback on error in Chrome scoped to non-MSE players. Specs and implementation of at least MSE will need to be updated to provide the JS players transparency into what kinds of errors are recoverable, so they can, for instance, for recoverable errors interoperably resume media fetching and appending to SourceBuffers. With src=, Chrome has much more control over the fetching/buffering/etc, so we can move more rapidly to trying out error recovery support. I also encountered a Debug-build ASSERT in my "one-time parse failure" attempt, above, that I'll fix separately from this.
On 2016/03/24 20:11:49, wolenetz wrote: ... > I also encountered a Debug-build ASSERT in my "one-time parse failure" attempt, > above, that I'll fix separately from this. FWIW, I think the ASSERT is to be fixed soon as part of fixing https://crbug.com/254744 and https://github.com/w3c/media-source/issues/10.
Latest patch disables this for MediaStream and MediaSource URLs.
From MSE perspective: LGTM https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:291: if (mediaElement().error() && !HTMLMediaElement::isMediaStreamURL(url) && !HTMLMediaSource::lookup(url)) A web app could have explicitly revoked the MediaSource objectURL by this time. So this logic could mistake an MSE-src'ed mediaElement() as a plain src mediaElement(). We're luckily protected by Chrome's current HTMLMediaElement::load() path, which checks the registry each time: 1) If the element still had an attached mediasource, it's closed and forgotten by the element. (as part of either the error noneSupported() path, and certainly as part of invokeLoadAlgorithm()'s call to resetMediaPlayerAndMediaSource(). 2) Then the MSE object is [re-]attached. Importantly, Blink uses the same registry lookup to see if it's indeed an MSE-src'ed element as this line in this CL. The only reason I thought I was luckily getting MSE to work was that my simple MSE player had not canceled the 'onsourceopen' event handler, which was re-fired upon the play later. I'm not so clear that the MSE and HTML specs match Blink's implementation that detaches and re-attaches MSE as part of load(), but it does help protect against some weirdness.
On 2016/03/24 23:05:04, wolenetz wrote: > I'm not so clear that the MSE and HTML specs match Blink's implementation that > detaches and re-attaches MSE as part of load(), but it does help protect against > some weirdness. For the curious, these spec mismatches are tracked work in MSE spec [1] [2] [3] (in addition to w3c HTML5.1 spec). [1] https://github.com/w3c/media-source/issues/17 [2] https://github.com/w3c/media-source/issues/18 [3] https://github.com/w3c/media-source/pull/56
lgtm % protocolIsInHTTPFamily suggestion. https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:291: if (mediaElement().error() && !HTMLMediaElement::isMediaStreamURL(url) && !HTMLMediaSource::lookup(url)) On 2016/03/24 23:05:04, wolenetz wrote: > A web app could have explicitly revoked the MediaSource objectURL by this time. > So this logic could mistake an MSE-src'ed mediaElement() as a plain src > mediaElement(). We're luckily protected by Chrome's current > HTMLMediaElement::load() path, which checks the registry each time: > > 1) If the element still had an attached mediasource, it's closed and forgotten > by the element. (as part of either the error noneSupported() path, and certainly > as part of invokeLoadAlgorithm()'s call to resetMediaPlayerAndMediaSource(). > 2) Then the MSE object is [re-]attached. Importantly, Blink uses the same > registry lookup to see if it's indeed an MSE-src'ed element as this line in this > CL. > > The only reason I thought I was luckily getting MSE to work was that my simple > MSE player had not canceled the 'onsourceopen' event handler, which was re-fired > upon the play later. > > I'm not so clear that the MSE and HTML specs match Blink's implementation that > detaches and re-attaches MSE as part of load(), but it does help protect against > some weirdness. > Maybe just gate this on KURL::protocolIsInHTTPFamily()? Are there any other protocols where we expect that trying again would make a difference?
https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:291: if (mediaElement().error() && !HTMLMediaElement::isMediaStreamURL(url) && !HTMLMediaSource::lookup(url)) On 2016/03/29 at 13:24:15, philipj_UTC7 wrote: > On 2016/03/24 23:05:04, wolenetz wrote: > > A web app could have explicitly revoked the MediaSource objectURL by this time. > > So this logic could mistake an MSE-src'ed mediaElement() as a plain src > > mediaElement(). We're luckily protected by Chrome's current > > HTMLMediaElement::load() path, which checks the registry each time: > > > > 1) If the element still had an attached mediasource, it's closed and forgotten > > by the element. (as part of either the error noneSupported() path, and certainly > > as part of invokeLoadAlgorithm()'s call to resetMediaPlayerAndMediaSource(). > > 2) Then the MSE object is [re-]attached. Importantly, Blink uses the same > > registry lookup to see if it's indeed an MSE-src'ed element as this line in this > > CL. > > > > The only reason I thought I was luckily getting MSE to work was that my simple > > MSE player had not canceled the 'onsourceopen' event handler, which was re-fired > > upon the play later. > > > > I'm not so clear that the MSE and HTML specs match Blink's implementation that > > detaches and re-attaches MSE as part of load(), but it does help protect against > > some weirdness. > > > > Maybe just gate this on KURL::protocolIsInHTTPFamily()? Are there any other protocols where we expect that trying again would make a difference? Depending on the failure type data/blob URLs may have transient errors as well, so I'd prefer to limit this only to situations we know won't work for consistency. Matt, I don't really follow your comment. The load() called here is synchronous, the result of HTMLMediaSource::Lookup() in this code and within load() will return the same value. Can you elaborate more on what your concern is here?
On 2016/03/29 21:41:14, DaleCurtis wrote: > https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp > (right): > > https://codereview.chromium.org/1827573004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:291: if > (mediaElement().error() && !HTMLMediaElement::isMediaStreamURL(url) && > !HTMLMediaSource::lookup(url)) > On 2016/03/29 at 13:24:15, philipj_UTC7 wrote: > > On 2016/03/24 23:05:04, wolenetz wrote: > > > A web app could have explicitly revoked the MediaSource objectURL by this > time. > > > So this logic could mistake an MSE-src'ed mediaElement() as a plain src > > > mediaElement(). We're luckily protected by Chrome's current > > > HTMLMediaElement::load() path, which checks the registry each time: > > > > > > 1) If the element still had an attached mediasource, it's closed and > forgotten > > > by the element. (as part of either the error noneSupported() path, and > certainly > > > as part of invokeLoadAlgorithm()'s call to > resetMediaPlayerAndMediaSource(). > > > 2) Then the MSE object is [re-]attached. Importantly, Blink uses the same > > > registry lookup to see if it's indeed an MSE-src'ed element as this line in > this > > > CL. > > > > > > The only reason I thought I was luckily getting MSE to work was that my > simple > > > MSE player had not canceled the 'onsourceopen' event handler, which was > re-fired > > > upon the play later. > > > > > > I'm not so clear that the MSE and HTML specs match Blink's implementation > that > > > detaches and re-attaches MSE as part of load(), but it does help protect > against > > > some weirdness. > > > > > > > Maybe just gate this on KURL::protocolIsInHTTPFamily()? Are there any other > protocols where we expect that trying again would make a difference? > > Depending on the failure type data/blob URLs may have transient errors as well, > so I'd prefer to limit this only to situations we know won't work for > consistency. > > Matt, I don't really follow your comment. The load() called here is synchronous, > the result of HTMLMediaSource::Lookup() in this code and within load() will > return the same value. Can you elaborate more on what your concern is here? tl;dr: We're luckily protected by Chrome's current HTMLMediaElement::load() path, which checks the registry each time ((synchronously with this lookup)). My comments were notes to self, especially considering the current MSE spec issues around clarifying what happens during MSE attachment/detachment. Sorry if they distracted.
Thanks for review, will land this.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827573004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827573004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Allow play/pause on default controls to clear media errors. Simple change to the default controls that lets the user continue playback with subsequent play/pause button presses. Useful in cases where the error is transient (e.g., network related). In the event of an error the default controls will now pause playback. It'd be nice if we could restore current time in this case too, but it seems to be cleared in unpredictable ways upon decode error. BUG=none TEST=play after decode / network error restarts from beginning. ========== to ========== Allow play/pause on default controls to clear media errors. Simple change to the default controls that lets the user continue playback with subsequent play/pause button presses. Useful in cases where the error is transient (e.g., network related). In the event of an error the default controls will now pause playback. It'd be nice if we could restore current time in this case too, but it seems to be cleared in unpredictable ways upon decode error. BUG=none TEST=play after decode / network error restarts from beginning. Committed: https://crrev.com/e317ac947e2c62d9df653bf48d7881c85f9a23b6 Cr-Commit-Position: refs/heads/master@{#383884} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e317ac947e2c62d9df653bf48d7881c85f9a23b6 Cr-Commit-Position: refs/heads/master@{#383884} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
