Closed Bug 1593843 Opened 5 years ago Closed 4 years ago

Implement play permission request for GeckoView

Categories

(Core :: Audio/Video: Playback, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We are going to send a play permission on GeckoView when media starts playing, which is used to let the embedding app (ex. Fenix) to decide if the audible/inaudible media can be allowed to play in that page.

One page could have at most one audible request and one inaudible request per living, once the request has been responded by the embedding app (ex. Fenix), then the result of the request would continuously be effective until the page is closed or being navigated to another page. Once we leave the previous page, we would drop those reqeust, which is equal to a denial.

For media, if we're waiting for the result of the request, that is an async process, so the media would be postponed to start playing until the we get the result of request. However, if we have already got the result before, we would use the previous result and would not send a request again. In addition, if the blocking autoplay model has allowed the page to play (ex. the page has been activated or the play is trigged by user input), then we would also allow media to start without sending a request.

Summary: Ask the embedding app if the site is allowed to autoplay, when we create media element first time on the site → Implement play permission request for GeckoView

I have been implementing that still, and will update my patches until I finish all of them.

Implement GVAutoplayPermissionRequest, that is used to provide an ability for GeckoView (GV) to allow its embedder (application) to decide whether the autoplay media should be allowed or denied on the page.

We have two types of request, one for audible media, another one for inaudible media. Each page would at most have one request per type at a time, and the result of request would be effective on that page until the page gets reloaded or closed.

As we added new files, which affects the build bundle created by unified build. Therefore, add missing scope definition and included header to pass the build.

If the page has already had same type pending request, then we don't want to send another request. If the page has already got the response from the request, we also don't want to send another request again.

In order to achieve that, we decide to store the request's staus on the top-browsing context to make sure that all media elements in the same browsing context tree can share the same status. Thereforw, we could avoid sending redudant request when there is a pending request, and reuse the previous request's result.

Move helper functions out from AutoplayPolicy, makes us clearly know which one could really be used to determine the blocking autoplay result. It also give other classes an ability to use those helper functions if they have a need.

Implement GVAutoplayPermissionRequestor, which provides a method to request a play permission asynchronously for media element on GeckoView by returning a non-exclusive promise. We would check if media element is audible to decide which type of request we would dispatch.

When we insert the new document to the window in the top-level browsing context, we should reset the status of the request which is used for the previous document.

On Android, we would send a play request via GeckoView to the embedding app (ex. Fenix) to ask for a play permission before media starts. On all the other platforms, we don't need to do that, we can simply check AutoplayPolicy::IsAllowedToPlay().

Therefore, create a function AutoplayPolicy::RequestForPlay() as a general function to wrap all these details.

As GV specific policy is different from the policy we use in other platform, so we should have one special version for a use on GV.

I found out that the async implementation would somehow break the spec, because the spec said that the internal play steps should be run before returning a play promise [1]. However, if we follow the patten in patch7 [2], to return a play promise first and then asynchronously call internal play steps when we get the response for play, it violates the order of running internal play steps , which would cause the media.paused and media.networkState incorrect....

[1] https://html.spec.whatwg.org/multipage/media.html#dom-media-play
[2] https://phabricator.services.mozilla.com/D52438


Jya,
May I have your suggestion here?

Flags: needinfo?(jyavenard)

OK, I think I am going to decouple the request asking from play(), so that we won't break the spec.

Flags: needinfo?(jyavenard)
Attachment #9107643 - Attachment description: Bug 1593843 - part5 : implement 'GVAutoplayPermissionRequestor' to manage the request. → Bug 1593843 - part4 : implement 'GVAutoplayPermissionRequestor' to manage the request.
Attachment #9107645 - Attachment description: Bug 1593843 - part6 : reset request status when insert a new document to the window in the top-level browsing context. → Bug 1593843 - part5 : reset request status when insert a new document to the window in the top-level browsing context.
Attachment #9107974 - Attachment description: Bug 1593843 - part9 : add a static pref to control this feature. → Bug 1593843 - part6 : add a static pref to control this feature.
Attachment #9107971 - Attachment description: Bug 1593843 - part8 : add debug logs. → Bug 1593843 - part7 : add debug logs.
Attachment #9107640 - Attachment description: Bug 1593843 - part4 : move helper functions out from 'AutoplayPolicy'. → Bug 1593843 - part8 : move helper functions out from 'AutoplayPolicy'.
Attachment #9108313 - Attachment description: Bug 1593843 - part10 : use GV specific autoplay check. → Bug 1593843 - part9 : use specific autoplay check when we enable the request asking on GeckoView
Attachment #9107649 - Attachment description: Bug 1593843 - part7 : ensure a play request for media element before it starts playing. → Bug 1593843 - part10 : ask for the play permission when creating media element and audio context.

If we don't enable blocking web audio, then we have no need to check other conditions, so we should check that earlier.

According to the nit in patch2, rearrange included files by their alphabet order.

Attachment #9110086 - Attachment description: Bug 1593843 - part12 : arrange included files by alphabet order. → Bug 1593843 - part12 : rearrange included files by alphabet order.

Andreas, the async permission request approach is not sufficient to give Fenix control of autoplay in general.

Do you know whether providing an autoplay setting as part of ShowInfo would be the best way to make that setting available on a per-site basis before any script is run?

Flags: needinfo?(afarre)

I'm going to answer karl's question from D52434 here, because posting on bug would be easily to be seen for future readers.

The original idea is indeed to ask the permission when play() is being called, however, as I mentioned in phabricator befored, that actually doesn't fit the requirement of the spec [1]. You can see if we don't allow media to play, we would direct reject the promise and not call internal play steps, which would modify paused to true, which can also indicate that "if paused doesn't be changed, then we know the media is not allowed to play", because if the media is allowed to play, paused would be modified to true immediately after calling play().

So if we ask the request when play() is being called, then after the play() is finished, the paused is still being false because we would only modify paused after getting the response from Fenix, which somehow indiacates that media is not allowed to start playing. That is actually not our intention.

[1] https://html.spec.whatwg.org/multipage/media.html#dom-media-play


Considering Fenix would show UI when media is not allowed to play, so we don't want to arbitrary asking for a request for any page, which would cause page to show UI even if there is not any media inside. Instead, in current implementation, we only ask for permission when the page has media element or audio context (we can consider to implement it later).

I know both methods, asking when media created or asking when play is being called, has a small defect, so comparing with breaking the spec, I would choose the former way, which probably can solve most cases but would fail on some edge cases.


Now my plan is that, I want to land these patches first, because we have established a way to notify Fenix and they can start to implement their work based on permission autoplay-media-audible and autoplay-media-inaudible. Then we can see how it goes and if this method is enough, or we have to enhence our method, in order to ask request in more proper timing.

I don't want to block them because of our internal implementation. Even if we change our mind and decide to ask for a request when play() is being called, that is not goint to affect Fenix's implementation, because the interface betweeen Gecko and GeckoView (permission name) is still keeping unchanged.

Therefore, I'm going to split the related implementation of web audio out from this bug, in order not to block this one. In addition, GV team actually doesn't mention their need of handling web audio as well, and blocking web audio is still in experiment, which only be enabled on Nightly.

Blocks: 1598721
Attachment #9107649 - Attachment description: Bug 1593843 - part10 : ask for the play permission when creating media element and audio context. → Bug 1593843 - part10 : ask for the play permission when creating media element.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1c50ee60a78
part1 : implement GVAutoplayPermissionRequest. r=Ehsan,bryce
https://hg.mozilla.org/integration/autoland/rev/5a24fc933cd6
part2 : fix build problem under unified build. r=bryce
https://hg.mozilla.org/integration/autoland/rev/4ab855869725
part3 : store the request status in top-level browsing context. r=farre
https://hg.mozilla.org/integration/autoland/rev/80895462dddf
part4 : implement 'GVAutoplayPermissionRequestor' to manage the request. r=bryce
https://hg.mozilla.org/integration/autoland/rev/02ac8c39f46d
part5 : reset request status when insert a new document to the window in the top-level browsing context. r=farre
https://hg.mozilla.org/integration/autoland/rev/0c7e831d11bf
part6 : add a static pref to control this feature. r=bryce
https://hg.mozilla.org/integration/autoland/rev/0d4313c436c5
part7 : add debug logs. r=bryce
https://hg.mozilla.org/integration/autoland/rev/486c8562b816
part8 : move helper functions out from 'AutoplayPolicy'. r=bryce
https://hg.mozilla.org/integration/autoland/rev/1a79176d3f7b
part8.5 - check if we enable blocking web audio or not before checking other conditions. r=bryce
https://hg.mozilla.org/integration/autoland/rev/ad02bb934459
part9 : use specific autoplay check when we enable the request asking on GeckoView r=bryce
https://hg.mozilla.org/integration/autoland/rev/f67081ea84ee
part10 : ask for the play permission when creating media element. r=bryce
https://hg.mozilla.org/integration/autoland/rev/3db512256b0d
part11 : add test. r=bryce
https://hg.mozilla.org/integration/autoland/rev/b6e3fa6363bd
part12 : rearrange included files by alphabet order. r=bryce

(In reply to Karl Tomlinson (:karlt) from comment #17)

Andreas, the async permission request approach is not sufficient to give Fenix control of autoplay in general.

Do you know whether providing an autoplay setting as part of ShowInfo would be the best way to make that setting available on a per-site basis before any script is run?

I'm not sure, re-dircting ni to Olli who should know better than me.

Flags: needinfo?(afarre) → needinfo?(bugs)

Why would we need to know autoplay eagerly? That sounds a bit unexpected approach.
(but I don't have the current autoplay-for-desktop setup in my mind atm)

Flags: needinfo?(bugs)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c586ac118fac
part1 : implement GVAutoplayPermissionRequest. r=Ehsan,bryce
https://hg.mozilla.org/integration/autoland/rev/24b0c30f3fcd
part2 : fix build problem under unified build. r=bryce
https://hg.mozilla.org/integration/autoland/rev/5d486661c2cc
part3 : store the request status in top-level browsing context. r=farre
https://hg.mozilla.org/integration/autoland/rev/3307590c54e2
part4 : implement 'GVAutoplayPermissionRequestor' to manage the request. r=bryce
https://hg.mozilla.org/integration/autoland/rev/e0aafd82ced3
part5 : reset request status when insert a new document to the window in the top-level browsing context. r=farre
https://hg.mozilla.org/integration/autoland/rev/bc38cbd18b0b
part6 : add a static pref to control this feature. r=bryce
https://hg.mozilla.org/integration/autoland/rev/83e447087c58
part7 : add debug logs. r=bryce
https://hg.mozilla.org/integration/autoland/rev/d633a3ed841a
part8 : move helper functions out from 'AutoplayPolicy'. r=bryce
https://hg.mozilla.org/integration/autoland/rev/9c5878aaa8ac
part8.5 - check if we enable blocking web audio or not before checking other conditions. r=bryce
https://hg.mozilla.org/integration/autoland/rev/3d2c0acacef9
part9 : use specific autoplay check when we enable the request asking on GeckoView r=bryce
https://hg.mozilla.org/integration/autoland/rev/5c30a18dab19
part10 : ask for the play permission when creating media element. r=bryce
https://hg.mozilla.org/integration/autoland/rev/1ed27d3b8500
part11 : add test. r=bryce
https://hg.mozilla.org/integration/autoland/rev/9d9e5d7c3515
part12 : rearrange included files by alphabet order. r=bryce
Flags: needinfo?(alwu)

I'm not seeing the ability to toggle these in GeckoRuntimeSettings (in Android Components). Is this something GV still needs to expose?

My understanding is this should have given us the ability to select between four different options:

  1. Allow audio and video
  2. Allow audio and video on WiFi only
  3. Block audio
  4. Block audio and video
Flags: needinfo?(shindli)
Flags: needinfo?(alwu)

(In reply to Sawyer Blatz [:sblatz] from comment #25)

I'm not seeing the ability to toggle these in GeckoRuntimeSettings (in Android Components). Is this something GV still needs to expose?

My understanding is this should have given us the ability to select between four different options:

  1. Allow audio and video
  2. Allow audio and video on WiFi only
  3. Block audio
  4. Block audio and video

You have to use GeckoView's permission delegate [1], then you would see two different autoplay requests [2]. We don't provide any default options, the embedder of GeckoView can have their own handling when those permission request happening.

[1] https://mozilla.github.io/geckoview/consumer/docs/permissions
[2] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/api.txt#793-794

Flags: needinfo?(alwu)
Flags: needinfo?(shindli)

Maybe there's something I'm missing here, but the autoplay is currently implemented as a GeckoRuntimeSetting. Since the autoplay setting is implemented there, shouldn't the autoplay of audio on WiFi only, etc. be implemented similarly? This doesn't feel like a permission that we want to asking the user about (those are more like "camera," "location," etc. no?) Why is this implemented in the PermissionDelegate when it's a setting the user should have globally similar to how autoplay currently works.

See https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngine.kt#L395-L403 for what I'm talking about here.

Flags: needinfo?(alwu)

Sorry for my late reply, the way to implement this via the permission delegate is requested by GV team, which allows them to combine different conditions (such like what current preference we're using, or like what you said, we could also check if we are in wifi mode or not)

As I'm not familiar with GeckoRuntimeSettings stuff, maybe :snorp would give you the answer you're looking for.

Flags: needinfo?(alwu) → needinfo?(snorp)

The GeckoRuntimeSettings bit is to globally allow or block autoplay. The permission delegate stuff[1] was added afterwards to allow overrides when blocking is enabled.

[1] https://mozilla.github.io/geckoview/javadoc/mozilla-central/org/mozilla/geckoview/GeckoSession.PermissionDelegate.html#PERMISSION_AUTOPLAY_AUDIBLE

Flags: needinfo?(snorp)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: