1173844 - Video would not playback after seek seekbar if media.autoplay.enabled = false

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]:

Steps To Reproduce:
1. Make sure media.autoplay.enabled = false in about:config
2. Open any video
   (e.g. http://www.youtube.com/watch?v=BH0BNbwqNK8 )
3. Start Play (you may need click play button twice , I will file a bug later)
4. Seek seekbar within/without buffered range

Actual Results:
Video playback stops. 


Expected Results:
Video should playback properly


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3ba889bb0741&tochange=e637319144d3

Suspect : Bug 659285

Comment 1

[Randall Barker [:rbarker]]

When media.autoplay.enabled = false, anytime a media element's play() function is invoked without human interaction, it will be blocked. This will break many video players that implement their own controls where the play invocation is not invoked directly by human interaction. This will particularly break youtube. I think the only way to handle this case is to allow users to white list sites they want to allow to invoke the play() call without human interaction.

Comment 2

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061212-4542b9e.patch

Comment 3

[Randall Barker [:rbarker]]

Comment on attachment 8621770 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061212-4542b9e.patch

This patch is intended to stop blocking autoplay once the user has interacted with media element. My concern is that since it is GetPlayedOrSeeked that a script could seek a short distance into the media and then auto play and bypass the check. Should I add a different bool just to detect if it has only been played or is there something else I can use to detect that the user has previously interacted with the element?

Comment 4

[Randall Barker [:rbarker]]

Comment on attachment 8621770 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061212-4542b9e.patch

After experimenting with test pages and reading through the code, I don't believe it is currently possible to bypass the block by advancing the video by setting currentTime.

Comment 5

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

Comment on attachment 8621770 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061212-4542b9e.patch

Review of attachment 8621770 [details] [diff] [review]:
-----------------------------------------------------------------

Do we also need to bail out in Seek() when the pref is set if the seek is not user initiated? Otherwise script could video.currentTime=0;video.play() to defeat this?

::: dom/html/HTMLMediaElement.cpp
@@ +2181,5 @@
>  HTMLMediaElement::Play(ErrorResult& aRv)
>  {
>    // Prevent media element from being auto-started by a script when
>    // media.autoplay.enabled=false
> +  if (!GetPlayedOrSeeked() && !IsAutoplayEnabled()

Please format these as one condition per line.

Comment 6

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

(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 8621770 [details] [diff] [review]
> 0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.
> autoplay.enabled-false-15061212-4542b9e.patch
> 
> Review of attachment 8621770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we also need to bail out in Seek() when the pref is set if the seek is
> not user initiated? Otherwise script could video.currentTime=0;video.play()
> to defeat this?

ni? for this.

Comment 7

[Randall Barker [:rbarker]]

(In reply to Chris Pearce (:cpearce) from comment #6)
> (In reply to Chris Pearce (:cpearce) from comment #5)
> > Comment on attachment 8621770 [details] [diff] [review]
> > 0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.
> > autoplay.enabled-false-15061212-4542b9e.patch
> > 
> > Review of attachment 8621770 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Do we also need to bail out in Seek() when the pref is set if the seek is
> > not user initiated? Otherwise script could video.currentTime=0;video.play()
> > to defeat this?
> 
> ni? for this.

See comment #4 in this bug. I just blew away my build but I will post the script I tried once I have rebuilt an double checked it.

Comment 8

[Randall Barker [:rbarker]]

<html>
<head>
<title>Video Auto Play Bypass</title>
</head>
<body>
Script Auto Play:<br>
<video src="chromecast.mp4" id="vid"></video><br>
<div>
<button onclick="document.getElementById('vid').play()">Play</button>
<button onclick="document.getElementById('vid').pause()">Stop</button>
</div>
<script>
var v = document.getElementById("vid");
var listen = function () {
  console.log("got canplay event");
  v.removeEventListener("canplay", listen);
  v.currentTime=0;
  v.play();
};
v.addEventListener("canplay", listen);
</script>
</body>
</html>

This is the page I used to try and bypass the auto play block. It is located:

http://people.mozilla.org/~rbarker/video-bypass.html

Comment 9

[Randall Barker [:rbarker]]

Additionally:

<html>
<head>
<title>Video Auto Play Bypass</title>
</head>
<body>
Script Auto Play:<br>
<video src="chromecast.mp4" id="vid"></video><br>
<div>
<button onclick="document.getElementById('vid').play()">Play</button>
<button onclick="document.getElementById('vid').pause()">Stop</button>
</div>
<script>
var v = document.getElementById("vid");
v.currentTime=0;
v.play();
</script>
</body>
</html>

Will not work in firefox regardless of auto play blocking becuase:

E/GeckoConsole(15142): [JavaScript Error: "InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable" {file: "http://people.mozilla.org/~rbarker/video-bypass.html" line: 14}]

Which is referring to v.currentTime=0;

Comment 10

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061616-4ca7125.patch

Comment 11

[Randall Barker [:rbarker]]

Comment on attachment 8623349 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061616-4ca7125.patch

Addressed code formatting issue.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Adding a tracking flag for FF41 as seeking should play the video back at the seek'd location.

Comment 13

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

Comment on attachment 8623349 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061616-4ca7125.patch

Review of attachment 8623349 [details] [diff] [review]:
-----------------------------------------------------------------

Your logic fails if script calls play() in the "seeked" event handler, as by that stage GetPlayedOrSeeked() returns true, causing your logic to not block the play().

For example:

<html><head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<title>Video Auto Play Bypass</title>
</head>
<body>
Script Auto Play:<br>
<video src="Video%20Auto%20Play%20Bypass_files/chromecast.mp4" id="vid"></video><br>
<div>
<button onclick="document.getElementById('vid').play()">Play</button>
<button onclick="document.getElementById('vid').pause()">Stop</button>
</div>
<script>
var v = document.getElementById("vid");
var name = "canplay";
var listen = function () {
  console.log("got " + name + " event");
  v.removeEventListener(name, listen);
  v.currentTime = 0;
  v.addEventListener("seeked", function() {v.play();});
};
v.addEventListener(name, listen);
</script>


</body></html>



I think you should just need to bypass the test if we've played not before, and you can quickly test that by checking whether GetPlayed() is returning a TimeRanges object with length 0.

Comment 14

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

Comment on attachment 8623349 [details] [diff] [review]
0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061616-4ca7125.patch

Review of attachment 8623349 [details] [diff] [review]:
-----------------------------------------------------------------

Tell you what, let's not go another round; r+ with the change I list below, assuming it fixes the bug for you (seems to for me).

::: dom/html/HTMLMediaElement.cpp
@@ +2181,5 @@
>  HTMLMediaElement::Play(ErrorResult& aRv)
>  {
>    // Prevent media element from being auto-started by a script when
>    // media.autoplay.enabled=false
> +  if (!GetPlayedOrSeeked()

Make is:

nsRefPtr<TimeRanges> played(Played());
if (played->Length() == 0
  && !IsAutoplayEnabled()
  && !EventStateManager::IsHandlingUserInput()
  && !nsContentUtils::IsCallerChrome()) {

Comment 15

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1173844-Video-would-not-playback-after-seek-seekbar-if-media.autoplay.enabled-false-15061917-4a31074.patch

carry forward r+ from :cpearce

Comment 16

[Randall Barker [:rbarker]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de1158e3599c

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd399cc8c6ca

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd399cc8c6ca