1464922 - Do not allow autoplay in media without audio tracks

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1464922 - Don't allow media without audio tracks to autoplay.

I don't think we should allow media without audio tracks to autoplay because:
* It means play() before loaded metadata behaves differently than play()
called after loaded metadata.
* With the current impl we dispatch the "play" event and then the "pause"
event when we decide we should block, which may confuse some sites' controls.
* Delaying running the play() algorithm until we've loaded metadata would add
significant complexity, and may break sites.
* Chrome doesn't have this provision, meaning the complexity required to
support it will not result in much benefit to us.

Review commit: https://reviewboard.mozilla.org/r/247368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247368/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1464922 - Remove HTMLMediaElement::mAttemptPlayUponLoadedMetadata.

We don't need to track this state anymore, as we don't need to delay calling
MediaDecoder::Play() or delay firing the "playing" event anymore.

Review commit: https://reviewboard.mozilla.org/r/247370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247370/

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8981272 [details]
Bug 1464922 - Remove HTMLMediaElement::mAttemptPlayUponLoadedMetadata.

https://reviewboard.mozilla.org/r/247370/#review253732

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/818e455e568b
https://hg.mozilla.org/mozilla-central/rev/aa7ca0083dcc