Closed
Bug 1464922
Opened 6 years ago
Closed 6 years ago
Do not allow autoplay in media without audio tracks
Categories
(Core :: Audio/Video: Playback, enhancement, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(2 files)
Our design for block-autoplay calls for allowing autoplay of media which don't have an audio track. I think we should remove this behaviour.
This behaviour causes the behaviour of play() to change based on how far through the load you are; if you call play() before metadata is loaded, you may be blocked, but once metadata is loaded and we know we don't have audio tracks, the play() call may succeed.
This is bad because it means play() is racy.
I tried to address the racy behaviour above by allowing the play() call before loadedmetadata to advance, dispatching "play" but not "playing", and dispatching "pause" if we later decide to block the play. That's undesirable as it means we effectively start playing and then stop playing, and some video controls may not handle that well. We also need to have code to ensure we don't dispatch "playing", as that confuses YouTube's controls. It's possible that other sites' controls will assume that once "play" is dispatched, that we're playing.
So it's probably best to just not dispatch "play" for plays that we're going to block (props to Bryce for suggesting this when we reviewed my earlier patch).
So I tried to return early from HTMLMediaElement::Play() and asynchronously wait until we've loaded metadata before calling HTMLMediaElement::PlayInternal(), so we'd avoid dispatching play events until we know we're going to load or not. That caused dozens of web platform tests to fail, and a bunch of our WebAudio tests to fail due to timing changes.
Additionally, Chrome does not take into account whether a media has an audio track. In Chrome, in order to autoplay, you need to set the "muted" attribute or set volume=0 on the media element. Being different to Chrome here isn't likely to benefit us much.
Supporting allowing autoplay for elements without an audio track adds significant complexity and compat risk, and is likely to not add much value since Chrome doesn't support it, so I think we should just remove this provision from our block-autoplay feature.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8981271 [details]
Bug 1464922 - Don't allow media without audio tracks to autoplay.
https://reviewboard.mozilla.org/r/247368/#review253686
::: dom/media/test/file_autoplay_policy_play_before_loadedmetadata.html:49
(Diff revision 1)
> - is(playingEventFired, testCase.shouldPlay, playingEventMsg);
> let playMsg = testCase.resource + " should " + (!testCase.shouldPlay ? "not " : "") + "play";
> is(played, testCase.shouldPlay, playMsg);
> - is(playEventFired, true, testCase.resource + " - we should always get a play event");
> - is(pauseEventFired, !testCase.shouldPlay, testCase.resource + " - if we shouldn't play, we should get a pause event");
> + is(playEventFired, testCase.shouldPlay, testCase.resource + " - should get play event if we played");
> + is(playingEventFired, testCase.shouldPlay, testCase.resource + "- should get playing event if we played");
> + is(pauseEventFired, false, testCase.resource + " - should not get pause event if we played");
To confirm, we should never get a pause event, if we play or if we did not play? I.e. if we play we don't pause, if we do not play we do not transition from another state to paused and thus do not fire the event -- we start with element.paused and do not transition away from it.
Attachment #8981271 -
Flags: review?(bvandyk) → review+
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8981272 [details]
Bug 1464922 - Remove HTMLMediaElement::mAttemptPlayUponLoadedMetadata.
https://reviewboard.mozilla.org/r/247370/#review253732
Attachment #8981272 -
Flags: review?(bvandyk) → review+
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to Bryce Van Dyk (:bryce) from comment #3)
> Comment on attachment 8981271 [details]
> Bug 1464922 - Don't allow media without audio tracks to autoplay.
>
> https://reviewboard.mozilla.org/r/247368/#review253686
>
> ::: dom/media/test/file_autoplay_policy_play_before_loadedmetadata.html:49
> (Diff revision 1)
> > - is(playingEventFired, testCase.shouldPlay, playingEventMsg);
> > let playMsg = testCase.resource + " should " + (!testCase.shouldPlay ? "not " : "") + "play";
> > is(played, testCase.shouldPlay, playMsg);
> > - is(playEventFired, true, testCase.resource + " - we should always get a play event");
> > - is(pauseEventFired, !testCase.shouldPlay, testCase.resource + " - if we shouldn't play, we should get a pause event");
> > + is(playEventFired, testCase.shouldPlay, testCase.resource + " - should get play event if we played");
> > + is(playingEventFired, testCase.shouldPlay, testCase.resource + "- should get playing event if we played");
> > + is(pauseEventFired, false, testCase.resource + " - should not get pause event if we played");
>
> To confirm, we should never get a pause event, if we play or if we did not
> play? I.e. if we play we don't pause, if we do not play we do not transition
> from another state to paused and thus do not fire the event -- we start with
> element.paused and do not transition away from it.
That's correct. If we block the playback, we do so before we advance to a non-paused state, so when we block, so we should not pause the media element when play is blocked.
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/818e455e568b
Don't allow media without audio tracks to autoplay. r=bryce
https://hg.mozilla.org/integration/autoland/rev/aa7ca0083dcc
Remove HTMLMediaElement::mAttemptPlayUponLoadedMetadata. r=bryce
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/818e455e568b
https://hg.mozilla.org/mozilla-central/rev/aa7ca0083dcc
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in
before you can comment on or make changes to this bug.
Description
•