1625615 - Video is not getting stop when application is paused.

Status: RESOLVED FIXED

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

Description

[Randall Barker [:rbarker]]

In FxR, if a video is playing and GeckoSession.setActive(false); called on the session when it becomes not visible, the video audio continues to play. I have verified by adding logging to GeckoView that GeckoSession.setActive(false); and GeckoSessionSettings.suspendMediaWhenInactive(true); have both been called on the session. I am not able to reproduce the issue in GVE or Fenix Nightly. To reproduce in FxR visit youtube and start a video and then put FxR into the background.

Comment 1

[Randall Barker [:rbarker]]

FxR issue on github https://github.com/MozillaReality/FirefoxReality/issues/3063

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Alastor, it looks like this broke with Bug 1577890. Do you have a recommendation for restoring this functionality in GeckoView?

Comment 3

[Alastor Wu [:alwu]]

Attachment: GV-suspendMedia-draft

Hi, Randall,
As I don't have a device to use FxR, could you help me check if this patch fixes the issue for you?
If It works, then I will submit a patch for that, thank you.

Comment 4

[Randall Barker [:rbarker]]

This patch causes the content process to crash when the application is paused/resumed. You can build tne noapio version of FxR that runs on a standard android phone. You can also set it up to use a local geckoview build by adding to local.properties:

dependencySubstitutions.geckoviewTopsrcdir=/path/to/gecko/top/src/dir

Comment 5

[Randall Barker [:rbarker]]

There may have been other issues causing the crash let me test the patch again.

Comment 6

[Alastor Wu [:alwu]]

Thank you, because I'm still trying to build FXR (got some build issue and trying to fix them)

Comment 7

[Randall Barker [:rbarker]]

I can reproduce the crash in the GeckoView Example with the following patch:

diff --git a/build/application.ini.in b/build/application.ini.in
index b4516b440a4e..9433c2b8d315 100644
--- a/build/application.ini.in
+++ b/build/application.ini.in
@@ -50,7 +50,9 @@ Enabled=1
 ServerURL=https://crash-reports.mozilla.com/submit?id=@MOZ_APP_ID@&version=@MOZ_APP_VERSION@&buildid=@MOZ_BUILDID@
 #endif
 
-#if MOZ_UPDATER
 [AppUpdate]
+#if MOZ_UPDATER
 URL=https://aus5.mozilla.org/update/6/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%SYSTEM_CAPABILITIES%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
+#else
+URL=
 #endif
diff --git a/mobile/android/chrome/geckoview/GeckoViewContentChild.js b/mobile/android/chrome/geckoview/GeckoViewContentChild.js
index f6c2dd708d5c..d9fca6e6b4ab 100644
--- a/mobile/android/chrome/geckoview/GeckoViewContentChild.js
+++ b/mobile/android/chrome/geckoview/GeckoViewContentChild.js
@@ -300,6 +300,10 @@ class GeckoViewContentChild extends GeckoViewChildModule {
             this.lastViewportFit = "";
             this.notifyParentOfViewportFit();
           }
+          if (aMsg.data.suspendMedia) {
+            const event = aMsg.data.active ? "play" : "pause";
+            ChromeUtils.generateMediaControlKeysTestEvent(event);
+          }
         }
         break;
 
diff --git a/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java b/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
index 997d827faa57..b1150828f584 100644
--- a/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
+++ b/mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
@@ -736,6 +736,7 @@ public class GeckoViewActivity
     private TabSession createSession() {
         TabSession session = mTabSessionManager.newSession(new GeckoSessionSettings.Builder()
                 .usePrivateMode(mUsePrivateBrowsing)
+                .suspendMediaWhenInactive(true)
                 .fullAccessibilityTree(mFullAccessibilityTree)
                 .viewportMode(mDesktopMode
                         ? GeckoSessionSettings.VIEWPORT_MODE_DESKTOP

Comment 8

[Alastor Wu [:alwu]]

Thank you for your patch, I found that the old method I used is wrong, I will update a new patch later. Now I can build FxR successfully, but it would crash immediately when I open it.

Comment 9

[Randall Barker [:rbarker]]

(In reply to Alastor Wu [:alwu] from comment #8)

Thank you for your patch, I found that the old method I used is wrong, I will update a new patch later. Now I can build FxR successfully, but it would crash immediately when I open it.

You probably need to do a clean before you build FxR in AndroidStudio. It's a know issue when doing a build using a local build of GeckoView.

Comment 10

[Alastor Wu [:alwu]]

Attachment: suspendMediaWhenInactive.diff

(In reply to Randall Barker [:rbarker] from comment #9)

You probably need to do a clean before you build FxR in AndroidStudio. It's a know issue when doing a build using a local build of GeckoView.

What do you mean do a clean before build FxR? I've tried clean project in AndroidStudio, but it didn't work.

Could you help me test if this patch works for you? I've tested it on desktop version Firefox by calling my new method directly and it works for me.

Thank you.

Comment 11

[Randall Barker [:rbarker]]

I tried this patch and the audio and video are paused, however when FxR resumes with a youtube video, only the audio resumes. The video is frozen. The page is still live as the scrubber is still moving with the audio, only the video image is not getting updated.

Comment 12

[Randall Barker [:rbarker]]

The youtube breakage might be unrelated. Vimeo works as expected.

Comment 13

[Randall Barker [:rbarker]]

Actually I can reproduce the youtube issue in GeckoView Example. STR:

1. switch to desktop mode
2. go to youtube and play a video.
3. while video is playing create a new tab
4. switch back to youtube tab

Actual
Video image is frozen while audio plays.

Expected
Video resumes playing with the audio.

Comment 14

[Alastor Wu [:alwu]]

Hmm, now I can reproduce this issue on desktop version Firefox as well, but all other websites I tested, including Twitch, Vimeo, Netflix, bilibili and a test page using a pure HTML5 video player, all work well. I'm investigating what's happened.

Comment 15

[Alastor Wu [:alwu]]

okay I found the root cause, will update my patch later.

Comment 16

[Alastor Wu [:alwu]]

Attachment: suspendMedia-v2.diff

Hi, Randall,
This patch works on Youtube, Vimeo, Netflix, bilibili and a test page using a pure HTML5 video player for me, could you help verify it again?
Thank you so much.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

I feel like we could just do this.browser.docShellIsActive = <value> in here, no?

Comment 18

[Alastor Wu [:alwu]]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #17)

I feel like we could just do this.browser.docShellIsActive = <value> in here, no?

The mechanism to suspend/resume the media is actually relying on this.browser.docShellIsActive=<value>. When calling that, it would trigger nsDocShell::SetIsActive() [1] that would change the document's visibility and eventually trigger the media element's NotifyOwnerDocumentActivityChanged() [2] where we would determine if we need to suspend or resume the media element. That is the way we currently use to suspend/resume media when it enters/leaves BFCache.

Therfore, we have to ensure that we set the flag of suspendMediaWhenInactive before running all those methods.

[1] https://searchfox.org/mozilla-central/rev/4ccefc3181f9d237ef4ca8bd17b4e7c101ddf7b5/docshell/base/nsDocShell.cpp#4844
[2] https://searchfox.org/mozilla-central/rev/4ccefc3181f9d237ef4ca8bd17b4e7c101ddf7b5/dom/html/HTMLMediaElement.cpp#6439

Comment 19

[Randall Barker [:rbarker]]

This patch is working for me. Thanks!

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Alastor Wu [:alwu] from comment #18)

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #17)

I feel like we could just do this.browser.docShellIsActive = <value> in here, no?

The mechanism to suspend/resume the media is actually relying on this.browser.docShellIsActive=<value>. When calling that, it would trigger nsDocShell::SetIsActive() [1] that would change the document's visibility and eventually trigger the media element's NotifyOwnerDocumentActivityChanged() [2] where we would determine if we need to suspend or resume the media element. That is the way we currently use to suspend/resume media when it enters/leaves BFCache.

Right, we're not setting docShellIsActive, but I think we should. We used to. Tom do you remember why that got removed when we added the new stuff there?

Comment 21

[Alastor Wu [:alwu]]

Because we've already set docShellIsActive here?
https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewContent.jsm#126-130

Comment 22

[Thomas Wisniewski [:twisniewski]]

Tom do you remember why that got removed when we added the new stuff there?

I actually don't actually recall ever seeing such code for me to remove. It's not there in the patch context in D55343 and D51852, where I just added some code.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ah, I see we call it in the parent here: https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewContent.jsm#126

Comment 24

[Alastor Wu [:alwu]]

I'm still testing my patches, but get an intermittent failure on my new added test, will update them after I fix that issue.

Comment 25

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part0 : create and set the flag `suspendMediaWhenInactive` on docShell.

Implemecurnt a flag suspendMediaWhenInactive on the docShell that indicates media in that shell should be suspended when the shell is inactive. Currently, only GeckoView is using this flag.

Comment 26

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part2 : suspend or resume media element according to docShell's `SuspendMediaWhenInactive`

If docShell's SuspendMediaWhenInactive is true, then we should suspend or resume the media element according to the docshell active state when the docshell changes it active state.

Comment 27

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part3 : prevent media starting playing when inactive docshell wants to suspend any media.

When the docShell's SuspendMediaWhenInactive flag is true, no media should be allowed to start playing. Therefore, we add a check in Play(), CanActivateAutoplay() to prevent media from playing. In addition, we should also prevent the audio channel agant from starting.

Comment 28

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part4 : start listener if we haven't started listener yet.

There is actually possible to start the listener already while running SuspendOrResumeElement(), so we should remove the assertion and use a check instead.

Eg. JS can call play(), which would start the listener, before we run this method. This situation can be found when browsing Youtube on the GeckoView.

Comment 29

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part5 : add test-only attribute and event for media element.

Comment 30

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part6 : add test.

Comment 31

[Emilio Cobos Álvarez [:emilio]]

I'm confused, why is all this code (the new docshell flag and so on) needed? Can you explain what the root cause is / how do these patches fix the bug in the commit messages? It's not clear to me.

Comment 32

[Alastor Wu [:alwu]]

When I removed the legacy Fennec code in bug1577890, I thought that setting docshell to inactive is enough to suspend the media, because we already have a mechanism which would suspend/resume media when document becomes inactive/active [1]. Therefore, I removed the old way, which is actually not designed for this purpose, to suspend/resume the media.

[1] https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/html/HTMLMediaElement.cpp#6453

However, the active state of document is actually different from what I thought it was. Setting docshell to inactive won't change the document's active state, because that indicates if the document is the current active document for the docshell [2] (docshell can have multiple documents), instead of indicating if the docshell is active or not.

[2] https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/dom/base/Document.h#2627-2633

Therefore, we have to add another flag to indicate if the docshell wants to suspend its media when it's inactive, in order to use current mechanism to suspend/resume media.

---

I've updated this information to the commit message of D69669 [3].

[3] https://phabricator.services.mozilla.com/D69669

Comment 33

[Alastor Wu [:alwu]]

Attachment: Bug 1625615 - part1 : update 'suspendMediaWhenInactive' when GeckoView setting changes

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9299fc48796
part0 : create and set the flag `suspendMediaWhenInactive` on docShell. r=baku,farre
https://hg.mozilla.org/integration/autoland/rev/3f03a8703a8a
part1 : update 'suspendMediaWhenInactive' when GeckoView setting changes r=snorp
https://hg.mozilla.org/integration/autoland/rev/45942c8dc380
part2 : suspend or resume media element according to docShell's `SuspendMediaWhenInactive`  r=bryce
https://hg.mozilla.org/integration/autoland/rev/4cd231b1f3fb
part3 : prevent media starting playing when inactive docshell wants to suspend any media. r=bryce
https://hg.mozilla.org/integration/autoland/rev/d709f5a72c35
part4 : start listener if we haven't started listener yet. r=bryce
https://hg.mozilla.org/integration/autoland/rev/acea7c78db20
part5 : add test-only attribute and event for media element. r=bryce,emilio
https://hg.mozilla.org/integration/autoland/rev/f239d24658c9
part6 : add test. r=bryce

Comment 35

[Natalia Csoregi [:nataliaCs]]

Backed out for multiple failures e.g. /test_windowedhistoryframes.html

backout: https://hg.mozilla.org/integration/autoland/rev/13b5ce2ab11490930ed068213d3a50be7633cf2f

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f239d24658c93972244221bc3d8fb9fe65bd64f0&group_state=expanded&selectedJob=297809721

failure logs:

• /page-visibility/unload-bubbles.html | visibilitychange event bubbles when fired on unload - Test timed out - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297809717&repo=autoland&lineNumber=5192
• /test_windowedhistoryframes.html | Test timed out. - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297809721&repo=autoland&lineNumber=5559
• and others

Comment 36

[BugBot [:suhaib / :marco/ :calixte]]

:alwu, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Comment 37

[Alastor Wu [:alwu]]

After applying patch0 and patch1, then I got many test failures on Android and most of them are timeout.

While I investigated those faliures, it seems like calling suspendMediaWhenInactive on browser element would somehow change the timing of some DOM events or behaviors? For example, the timeout reason for test_windowedhistoryframes.html is because the load event was received before setting the listener.

I've changed that order of listening event for that, and then it can pass correctly. However, there are not only one timeout test, but many of them [2]. And I'm petty sure that those tests should not be affected by the new attribute suspendMediaWhenInactive I added in the docshell, because what I do is simply updating its value.

So I wonder if it's possible that the timing of dispatching DOM event is affected by my patches? Any suggestion about this issue?

Thank you.

[1] https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/dom/tests/mochitest/general/historyframes.html#161
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4c0483a4aa760a04655884919f7f71143aa69f7&selectedJob=298171020

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Does some of the patches possibly force-create about:blank?
But I didn't see anything obvious in patches 0/1.

Comment 39

[Alastor Wu [:alwu]]

I don't think my patches would create any page, which are about creating a new attribute on docShell and update them on GeckoView. So it seems that I hit an Android specific issue...?

Comment 40

[Alastor Wu [:alwu]]

I observed that if we use BrowserHost too early to propagate the value before setting the docShell's active flag, then I found the docShell would somehow become inactive during testing (but I didn't know why) So now I use a workaround to avoid that, and have pushed my patches to the try server to see how it works.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/208d2317ac56
part0 : create and set the flag `suspendMediaWhenInactive` on docShell. r=baku,farre
https://hg.mozilla.org/integration/autoland/rev/0c600374b15a
part1 : update 'suspendMediaWhenInactive' when GeckoView setting changes r=snorp
https://hg.mozilla.org/integration/autoland/rev/f252d9b19a38
part2 : suspend or resume media element according to docShell's `SuspendMediaWhenInactive`  r=bryce
https://hg.mozilla.org/integration/autoland/rev/78f93ea99543
part3 : prevent media starting playing when inactive docshell wants to suspend any media. r=bryce
https://hg.mozilla.org/integration/autoland/rev/31ffed2b2185
part4 : start listener if we haven't started listener yet. r=bryce
https://hg.mozilla.org/integration/autoland/rev/d678075212ea
part5 : add test-only attribute and event for media element. r=bryce,emilio
https://hg.mozilla.org/integration/autoland/rev/ccce357eaa05
part6 : add test. r=bryce

Comment 42

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/208d2317ac56
https://hg.mozilla.org/mozilla-central/rev/0c600374b15a
https://hg.mozilla.org/mozilla-central/rev/f252d9b19a38
https://hg.mozilla.org/mozilla-central/rev/78f93ea99543
https://hg.mozilla.org/mozilla-central/rev/31ffed2b2185
https://hg.mozilla.org/mozilla-central/rev/d678075212ea
https://hg.mozilla.org/mozilla-central/rev/ccce357eaa05