Closed Bug 1382574 Opened 7 years ago Closed 6 years ago

Implement new autoplay policy for the pref "media.autoplay.enabled.user-gestures-needed"

Categories

(Core :: Audio/Video: Playback, defect, P3)

Other Branch
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

()

Details

Attachments

(3 files)

STR.
1. set the pref "media.autoplay.enabled=false"
2. go to https://player.vimeo.com/embed
3. play video

Actual.
4. video doesn't start playing

Expected.
4. video starts playing
Summary: Can't play vimeo video when set → Can't play vimeo video when setting "media.autoplay.enabled=false"
It looks like a MSE issue, the video can start playing when I turn off the pref "media.mediasource.enabled".
Summary: Can't play vimeo video when setting "media.autoplay.enabled=false" → Can't play vimeo MSE video when setting "media.autoplay.enabled=false"
Ah, it's not related with MSE, video got stuck here. [1]

[1] http://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#6766
Summary: Can't play vimeo MSE video when setting "media.autoplay.enabled=false" → Can't play vimeo video when setting "media.autoplay.enabled=false"
The pref "media.autoplay.enabled" provides the way to (1) stop the media with keyword "autoplay" and (2) stop the untrusted play() invocations calling by script.

For the second point, we block the untrusted play() invocations once it's not triggered by user's input via the checking |EventStateManager::IsHandlingUserInput()| in bug659285. However, this function is problematic and it can't provide the correct result.

Actually, this pref is like a double-edged sword, and I think its disadvantage is over than its advantage. 
The two main disadvantages are (1) broke lots of custom controls (2) the UI is unsynchronized with the playing state. I'd describe more details about these disadvantages in later comment.

---

The best thing is that we can fix the problem of |EventStateManager::IsHandlingUserInput()| easily, then we should do it. But yesterday I've discussed it with DOM team's guy, it seem not easy to be fixed.

If so, I'd like to recommend to remove all stuffs related with the pref "media.autoplay.enabled", because it's too buggy.

But we still need a way to prevent the autoplay, here are my proposal. (for desktop)

(1) using add-on
Both Chrome and Opera are not supported blocking autoplay natively. The add-on is a common solution.
However, using add-on would still get the UI unsynchronized issue.

(2) mute autoplay
Maybe we can mute the autoplay instead of blocking them. And then resume their sound when user's cursor is hovering over it.
Flags: needinfo?(cpearce)
Flags: needinfo?(bwu)
Flags: needinfo?(ajones)
Here I’ll describe more details about the disadvantages.

(1) break custom controls
We’ve known it before (in bug659285 comment4).

Based on the result of the |EventStateManager::IsHandlingUserInput()|, we would decide whether need to block the play() invocations. 

However, it only works for the first sync JS function, it won't lose efficacy in the follow-up async function. The website like Vimeo, it did lots of async operation before calling play(), see bug1231886 comment20. 

Here is one easy example [1], clicking button "click me (sync)" can start playing the audio, but it can't work for the button "click me (async)".

[1] https://media-testing.updog.co/clickHandler.html

(2) UI unsynchronized issue

I guess that might be caused that custom controls doesn’t handle the error or rejected promise from play(), so it causes the player doesn’t show the correct UI. [2]

It’s quite minor issue compared with the above one, but it’s still annoying because you need to click twice to start the video.

[2] https://goo.gl/3CoFxP
(In reply to Alastor Wu [:alwu][please needinfo? me][7/26-8/3 PTO] from comment #3)
> For the second point, we block the untrusted play() invocations once it's
> not triggered by user's input via the checking
> |EventStateManager::IsHandlingUserInput()| in bug659285. However, this
> function is problematic and it can't provide the correct result.

How about:
1. 'bless' a media element when the user click on it.
2. allow autoplay for the 'blessed' media element.

This will fix the issue where playback is blocked when play() is invoked asynchronously in the 'onlick' handler.
(In reply to Alastor Wu [:alwu][please needinfo? me][7/26-8/3 PTO] from comment #4)
> (1) break custom controls

It would happen when we play the *NON-AUTOPLAY* media, see comment0.

> (2) UI unsynchronized issue

It would happen when we successfully stop the *AUTOPLAY* media, eg. [1]

[1] https://www.getup.org.au/campaigns/climate-action-now/video/fund-solutions-not-pollution
Why an addon would be better than implementing in browser?

Can user get a list of elements in some context menu and enable any desired elements? Perhaps clicking around would not be easy to implement. But if user has a list of media elements to enable, that should solve the issue, no?

wrt muting - that's a bad solution IMO. It wastes your bandwidth. As well showing bad video on screen can equally well be undesirable like playing sound.
There is already the addon "FlashStopper" (which works also with HTML5 video players). It does have a similar problem with the Play button on Vimeo. However, with FlashStopper one can still start the video by clicking on the video image.

The author has said he will consider fixing the problem in a future version. Alastor Wu, maybe you would like to talk about it with him?

There's some discussion here: http://forums.mozillazine.org/viewtopic.php?p=14756730#p14756730

By the way, it seems that Apple has released this function in the next version of Safari (now in public beta). I would assume they figured out how not to break Vimeo with it, although I haven't tried.

In my opinion, this should be part of the browser, and should be integrated with the Permissions tab in the Page Info window for per-site configuration. That's basically how Apple has done it.
Do we reject the promise when we block the play() method?
Flags: needinfo?(ajones)
IMO, a pref would be better than addon. We should do our best to see if we could find a way to disable auto playback for all websites since there are more and more auto playbacks and now it gets more annoying to users. Providing a perf or setting could be a good start. 

Comment 5 looks promising and our current mechanism for blocking background auto play should be able to apply to the foreground case.
Flags: needinfo?(bwu)
We ship UI which toggles this feature on Fennec, so we can't just remove it everywhere. Potentially we could remove it on Desktop only.

I would prefer to find a way to make this work on Desktop, rather than remove it, as autoplay videos are very annoying.
Flags: needinfo?(cpearce)
OK, I'll try to way mentioned in comment5 and see whether it can work.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #9)
> Do we reject the promise when we block the play() method?

Yes, we would reject the promise if we don't allow to play [1].

[1] https://goo.gl/fw73Yc
Unfortunately, the way in comment5 won't work. The reason is most websites won't use video element alone, they often use lots <div> to compose their own controls interface, so actually the video we see is covered by many different <div> layers. 

You might think you're clicking the <video>, but actually you're clicking on the <div>. The <div> and <video> could be non-related, so the click event won't be triggered on the <video>. Therefore, you can't know whether the video is clicked.

Second reason is, the control can be separated with <video>, eg. [1], we can trigger video via buttons, and we don't click video itself. Therefore, it's wrong to use "is video clicked by user" as the condition to allow the play invocation or not.

If we want to keep this feature, we MUST fix the |EventStateManager::IsHandlingUserInput()| as I explained in the comment4.

[1] https://media-testing.updog.co/clickHandler.html
Hi, BZ,
Per comment4, we need someone help us to fix the |EventStateManager::IsHandlingUserInput()|.
Do you know who is familiar with that field? Or do you have any better idea about that?
Thanks!
Flags: needinfo?(bzbarsky)
Olli (smaug) should know about IsHandlingUserInput.

That said, the whole point of IsHandlingUserInput is to be true only while the input event is actually being processed.  It sounds like your problem is that you want some other condition ("input event happened on this element" or "input event happened recently" or something?), but it's not really clear what condition you actually want, right?

Note bug 1185052 which might be relevant here.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #16)
> That said, the whole point of IsHandlingUserInput is to be true only while
> the input event is actually being processed.  It sounds like your problem is
> that you want some other condition ("input event happened on this element"
> or "input event happened recently" or something?), but it's not really clear
> what condition you actually want, right?

I want the |IsHandlingUserInput()| can return true in the async function of the event callback.

Eg.

```
element.onclick = function() {
  let promise = SOME_FUNC();
  promise.then(() => {
    // Do something, and I hope the |IsHandlingUserInput()| can be true here
  }, () => {});
};
```

---

Hi, smaug,
Could you give us some thought about this proposal? 
If it's good to do that, do you know who is available to implement it?
Thanks.
Flags: needinfo?(bugs)
Is this about promise handling? Once we have proper scheduling for promises, then() callback would get resolved while IsHandlingUserInput is true, assuming the promise is resolved inside event listener.

But in general passing some flag whether promise was created when IsHandlingUserInput is true sounds bad.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #18)
> Is this about promise handling? Once we have proper scheduling for promises,
> then() callback would get resolved while IsHandlingUserInput is true,
> assuming the promise is resolved inside event listener.

All async function won't be marked as user input, not just about promise handling.

Eg. 

```
element.onclick = function() {
  setTimeout(() => {
  // Do something, and I hope the |IsHandlingUserInput()| can be true here
  }, 1000);
};

My problem seems similar as bug1129227.
Depends on: 1185052
>  // Do something, and I hope the |IsHandlingUserInput()| can be true here

I would hope not, for this specific timeout value!  The idea is that the action should be clearly connected with user input in the user's mind.  That's why even if you're synchronously under input event dispatch after 1s we no longer consider it to be user input.
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #20)
> I would hope not, for this specific timeout value!  The idea is that the
> action should be clearly connected with user input in the user's mind. 
> That's why even if you're synchronously under input event dispatch after 1s
> we no longer consider it to be user input.

Sorry, I should take some shorter time as example.
What I want to express is that even the time is zero, |IsHandlingUserInput()| still returns the wrong result, like bug1129227.
According to other bug reports, here are some websites which would also fail to play audio/video when "media.autoplay.enabled=false".

Spotify web player : https://open.spotify.com/browse
RDS radio : http://www.rds.it/diretta/
Yahoo video : https://tw.video.yahoo.com/

We guess that they're using the similar approach like Vimeo does [1], so the video.play() would also be regard as non-user input and then be blocked.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1231886#c20
(In reply to Alastor Wu [:alwu][please needinfo me][GMT+8] from comment #26)
> According to other bug reports, here are some websites which would also fail
> to play audio/video when "media.autoplay.enabled=false".

Check more details on [1], see "Websites testing".
https://docs.google.com/document/d/1J0kgSHEOhb8aiRQTEPBxeKZGWni4gP7GiH0MGCUkNcU/edit#
After discussed with ajones, we would like to try another way to block autoplay, Chrome also uses that way.
That is, blocking autoplay if there aren't any user trigger input in the page, once we get the any user input, we would *bless* whole document and then allow any autoplay media.
Depends on: 1415444
No longer depends on: 1185052
Blocks: 1415478
The "fix" in comment 28 will basically make the feature useless. I can't understand why the idea to present all possible playable media in the page as a list to the user wouldn't cover all use cases. If user wants to bless some page entirely, (s)he can do it anyway. It just needs a good UI.

I understand that some people are against people taking decisions that they cannot understand. But this situation is different. Choice is not security related. User will very well understand implications of enabling media playback individually or for a specific page or entirely for every page.

Browser doesn't need to be too smart here because it cannot be. It is a personal choice for every user how media playback should be handled. It also depends on user environment ATM whether the user would want one or another behavior. Browser simply can't guess that and shouldn't try to.

Thank you.
tl;dr, we would align our autoplay policy to Chrome's [1] (but not all of them), the website would be allowed to autoplay after it has been activated by specific user gestures.

[1] https://sites.google.com/a/chromium.org/dev/audio-video/autoplay

---

The present way which checks whether the play() is called by user couldn't provide us right information. After discussed with DOM team folk, the spec didn't define well about how to handle the async operation calling.

That is, the spec only defines that the sync operation would be regarded as an user input if it happened within specific time (~1s) after user input was called.

Async operation is an undefined behavior, it is still a issue need to be discussed [2]. DOM team would only implement it after the spec is ready.

Due to this point, we decide to change our autoplay policy.

[2] https://github.com/whatwg/html/issues/1903 

---

However, we won't follow Chrome for all their design. Eg. we won't take "Media Engagement Idx", we want to let user decide which website they want to allow autoplay

The first step of the autoplay project is, to change the basic behavior, allow website to autoplay until it has been activated by user gestures.

Then, we might implement the related UI, or change autoplay as a new permission which could be controlled on setting. But it's still discussing, we don't have final decision yet.
Comment on attachment 8928118 [details]
Bug 1382574 - part2 : remove HTMLMediaElement::GetHasUserInteraction().

https://reviewboard.mozilla.org/r/199366/#review206748
Attachment #8928118 - Flags: review?(bechen) → review+
Comment on attachment 8928117 [details]
Bug 1382574 - part1 : create new class AutoplayPolicy to handle autoplay logic.

https://reviewboard.mozilla.org/r/199364/#review206756

::: dom/media/AutoplayPolicy.h:31
(Diff revision 2)
> + * 2) TODO...
> + */
> +class AutoplayPolicy
> +{
> +public:
> +  AutoplayPolicy();

Remove unsued constructor/destructor.

::: dom/media/AutoplayPolicy.cpp:30
(Diff revision 2)
> +}
> +
> +/* static */ bool
> +AutoplayPolicy::IsMediaElementAllowedToPlay(HTMLMediaElement* aElement)
> +{
> +  MOZ_ASSERT(aElement);

Use NotNull<>
Attachment #8928117 - Flags: review?(jwwang) → review+
Comment on attachment 8928119 [details]
Bug 1382574 - part3 : remove HTMLMediaElement::mHasUserInteraction.

https://reviewboard.mozilla.org/r/199368/#review206758

::: dom/html/HTMLMediaElement.cpp:1971
(Diff revision 2)
>  
> -  // Detect if user has interacted with element so that play will not be
> -  // blocked when initiated by a script. This enables sites to capture user
> -  // intent to play by calling load() in the click handler of a "catalog
> -  // view" of a gallery of videos.
>    if (EventStateManager::IsHandlingUserInput()) {

Do we also need to remove related code about EventStateManager::IsHandlingUserInput()?
Attachment #8928119 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #39)
> Do we also need to remove related code about
> EventStateManager::IsHandlingUserInput()?

We should still preserve IsHandlingUserInput() here since it would be used to decide whether set the urgent-start loading flag.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f1d7b316594
part1 : create new class AutoplayPolicy to handle autoplay logic. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/8938ef573e4f
part2 : remove HTMLMediaElement::GetHasUserInteraction(). r=bechen
https://hg.mozilla.org/integration/autoland/rev/356b50c5becf
part3 : remove HTMLMediaElement::mHasUserInteraction. r=jwwang
Summary: Can't play vimeo video when setting "media.autoplay.enabled=false" → Implement new autoplay policy for the pref "media.autoplay.enabled.user-gestures-needed"
Depends on: 1420488
Blocks: 1421518
You need to log in before you can comment on or make changes to this bug.