186971 2018-06-23 07:13:09 -0700 [GStreamer] Media player does not properly inhibit, uninhibit sleep 2020-11-30 06:52:54 -0800 1 1 1 Unclassified WebKit Media Other Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=219010 https://bugs.webkit.org/show_bug.cgi?id=219353 https://bugs.webkit.org/show_bug.cgi?id=219354 https://bugs.webkit.org/show_bug.cgi?id=219355 InRadar P2 Normal --- 1 bugzilla calvaris bugs-noreply calvaris cgarcia eric.carlson ews-watchlist glenn gustavo jer.noble mcatanzaro menard pgriffis philipj pnormand sergio vjaquez webkit-bug-importer oldest_to_newest 1435841 0 bugzilla 2018-06-23 07:13:09 -0700 webkit2gtk3-2.20.2-1.fc28.x86_64 1. Open a page like https://www.twitch.tv/videos/276566160 2. Press play, idle is inhibited 3. Press pause, idle is still inhibited 4. Open a new tab, idle stop being inhibited You can check the inhibited actions with: gdbus introspect --session --dest org.gnome.SessionManager --object-path /org/gnome/SessionManager | grep InhibitedActions After 3, idle should be uninhibited. 1435845 1 mcatanzaro 2018-06-23 07:28:17 -0700 I see at the very top of HTMLMediaElement::shouldDisableSleep: if (!m_player || m_player->paused() || loop()) return SleepType::None; which should turn off the SleepDisabler. So some debugging will be required to investigate why this code is not working as intended. 1436086 2 pnormand 2018-06-25 09:38:06 -0700 The problem is two-fold: - the visibility of the media element changed but HTMLMediaElement::shouldDisableSleep() returns too early the None result, before checking the m_elementIsHidden variable. - Even if the m_elementIsHidden test is moved up, a System sleep disabler will be created, but our SleepDisablerGlib doesn't support that type. 1436139 3 mcatanzaro 2018-06-25 11:00:12 -0700 (In reply to Philippe Normand from comment #2) > - Even if the m_elementIsHidden test is moved up, a System sleep disabler > will be created, but our SleepDisablerGlib doesn't support that type. Umm... I see I wrote this comment when writing SleepDisablerGlib: // We don't support suspend ("System") inhibitors, only idle inhibitors. // To get suspend inhibitors, we'd need to use the fancy GNOME // SessionManager API, which requires registering as a client application, // which is not practical from the web process. Secondly, because the only // current use of a suspend inhibitor in WebKit, // HTMLMediaElement::shouldDisableSleep, is suspicious. There's really no // valid reason for WebKit to ever block suspend, only idle. There are three things wrong with this comment: * Nowadays, GNOME suspends the computer automatically on inactivity, and we actually do want to prevent that. * Inhibiting idle does affect the autosuspend countdown. So the current code should prevent automatic suspend, just not manual suspend. (I think I stand by my comment that we do not want to inhibit normal suspend, since that would just be annoying.) * I don't know why I thought HTMLMediaElement::shouldDisableSleep was suspicious. Anyway, I guess the behavior we want is to ignore the Type argument and just always inhibit idle. That is, treat type=System the same way we current treat type=Display. I couldn't decide which to do when I wrote this code, but in retrospect that would have been a better option. 1436184 4 pnormand 2018-06-25 12:36:11 -0700 (In reply to Philippe Normand from comment #2) > The problem is two-fold: > > - the visibility of the media element changed but > HTMLMediaElement::shouldDisableSleep() returns too early the None result, > before checking the m_elementIsHidden variable. > Eric, would it be acceptable to move the the m_elementIsHidden test up in HTMLMediaElement::shouldDisableSleep() ? 1436203 5 eric.carlson 2018-06-25 13:15:41 -0700 (In reply to Philippe Normand from comment #4) > (In reply to Philippe Normand from comment #2) > > The problem is two-fold: > > > > - the visibility of the media element changed but > > HTMLMediaElement::shouldDisableSleep() returns too early the None result, > > before checking the m_elementIsHidden variable. > > > > Eric, would it be acceptable to move the the m_elementIsHidden test up in > HTMLMediaElement::shouldDisableSleep() ? We don't want to enable sleep for a "playing" that doesn't have audio and video, so where would we move the m_elementIsHidden test? 1466041 6 pnormand 2018-10-03 06:51:27 -0700 I started debugging this again. In shouldDisableSleep(), both hasVideo() and hasAudio() calls return false because a null MediaPlayerPrivate has been created meanwhile. I don't know why yet... So this shouldn't happen, the video is paused and should have both audio and video... So shouldBeAbleToSleep becomes true there, instead of false. 1466042 7 pnormand 2018-10-03 06:53:23 -0700 I meant, So shouldBeAbleToSleep becomes false there, instead of true. :) 1504866 8 mcatanzaro 2019-02-11 08:08:15 -0800 (In reply to Michael Catanzaro from comment #3) > There are three things wrong with this comment: Split all of this out to bug #194500. Leaving this bug for the HTMLMediaElement problems. 1547437 9 mcatanzaro 2019-06-24 11:38:13 -0700 Well this bug as reported no longer occurs, because nowadays we never disable sleep. In bug #192530, I found HTMLMediaElement::updateSleepDisabling is not being called when paused/unpaused. 1596708 10 mcatanzaro 2019-12-08 10:20:03 -0800 This continues to be pretty annoying. Conclusion from bug #194500 is that SleepDisabler will inhibit idle now, as expected. So that's solved. Conclusion from bug #192530: "Now, HTMLMediaElement::updateSleepDisabling is not being called when paused/unpaused, which is bad, but I believe we have other bug reports for that." This should be solved here. 1611055 11 mcatanzaro 2020-01-25 09:34:49 -0800 (In reply to Philippe Normand from comment #6) > I started debugging this again. > > In shouldDisableSleep(), both hasVideo() and hasAudio() calls return false > because a null MediaPlayerPrivate has been created meanwhile. I don't know > why yet... So this shouldn't happen, the video is paused and should have > both audio and video... So shouldBeAbleToSleep becomes true there, instead > of false. I wasted a bunch of time debugging this today because I didn't read your comment. Conclusion: this is exactly what's still happening. When starting a YouTube video, m_mediaPlayer is either valid and paused() (during a brief window before playback begins), or else it's NULL (it seems to always be NULL during media playback). So next question to debug is: why is media playing when MediaPlayer is NULL? 1650066 12 mcatanzaro 2020-05-07 06:35:11 -0700 (In reply to Michael Catanzaro from comment #11) > So next question to debug is: why is media playing when MediaPlayer is NULL? 1705254 13 calvaris 2020-11-06 08:32:49 -0800 (In reply to Michael Catanzaro from comment #12) > (In reply to Michael Catanzaro from comment #11) > > So next question to debug is: why is media playing when MediaPlayer is NULL? The media player is null in some other HTMLMediaElement, not in the one taking care of the main playback. 1706040 14 413683 calvaris 2020-11-10 03:29:30 -0800 Created attachment 413683 Patch We were not updating the playback state in the MSE player and therefore, when getting to PLAYING the HTMLMediaElement didn't know. 1706099 15 mcatanzaro 2020-11-10 09:05:24 -0800 I tested this today. I see an inhibitor is now acquired when I begin playing a YouTube video, which is an improvement. However, the inhibitor is not released when I stop the video. Debug patch: diff --git a/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp b/Source/WebCore/PAL/pal/system/glib/SleepDisabl erGLib.cpp index 6f922e118afd..2fc2d15487a4 100644 --- a/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp +++ b/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp @@ -104,6 +104,7 @@ SleepDisablerGLib::~SleepDisablerGLib() void SleepDisablerGLib::acquireInhibitor() { +WTFLogAlways("%s this=%p", __FUNCTION__, this); if (m_screenSaverProxy) { ASSERT(!m_inhibitPortalProxy); acquireInhibitorViaScreenSaverProxy(); @@ -158,6 +159,7 @@ void SleepDisablerGLib::acquireInhibitorViaInhibitPortalProxy() void SleepDisablerGLib::releaseInhibitor() { +WTFLogAlways("%s this=%p", __FUNCTION__, this); if (m_screenSaverProxy) { ASSERT(!m_inhibitPortalProxy); releaseInhibitorViaScreenSaverProxy(); 1707976 16 mcatanzaro 2020-11-16 13:27:31 -0800 (In reply to Michael Catanzaro from comment #15) > I tested this today. I see an inhibitor is now acquired when I begin playing > a YouTube video, which is an improvement. However, the inhibitor is not > released when I stop the video. The inhibitor is also not released when I close the browser while a video is playing. I guess the SleepDisabler is leaked, which I presume will cause the inhibit to last forever? Not sure. I've also noticed the inhibitor is sometimes not acquired when starting a video, but I haven't figured out how to reproduce that. Finally, Calvaris noticed that SleepDisabler doesn't actually work. There are two problems: first, the ScreenSaver D-Bus API is not available inside the sandbox. (OK.) Second, the portal API fails because we try using g_get_prgname() instead of a valid app ID. These are problems in SleepDisabler itself, which I will address in a forthcoming bug report. Let's use this bug only for the original problem in HTMLMediaElement that Calvaris has investigated. 1708000 17 mcatanzaro 2020-11-16 14:29:57 -0800 (In reply to Michael Catanzaro from comment #16) > I've also noticed the inhibitor is sometimes not acquired when starting a > video, but I haven't figured out how to reproduce that. This often -- but not always -- happens when starting a video by clicking the "Skip Ads" button. 1708015 18 mcatanzaro 2020-11-16 15:09:21 -0800 (In reply to Michael Catanzaro from comment #17) > This often -- but not always -- happens when starting a video by clicking > the "Skip Ads" button. Hm, actually it seems like it always happens. So summary of remaining media-level problems: * SleepDisabler not created when "Skip Ads" is used to start the video * SleepDisabler not destroyed when stopping the video * SleepDisabler leaked when closing tab (though it is destroyed when refreshing the tab!) (In reply to Michael Catanzaro from comment #16) > Finally, Calvaris noticed that SleepDisabler doesn't actually work. There > are two problems: first, the ScreenSaver D-Bus API is not available inside > the sandbox. (OK.) Second, the portal API fails because we try using > g_get_prgname() instead of a valid app ID. I'm continuing to investigate these. 1708036 19 mcatanzaro 2020-11-16 16:17:51 -0800 (In reply to Michael Catanzaro from comment #16) > Second, the portal API fails because we try using > g_get_prgname() instead of a valid app ID. That's not what's happening. We only pass our own prgname to the ScreenSaver D-Bus API, which is blocked in the sandbox because it's only for use by trusted applications. We don't pass our own app ID to the portal API, because we are sandboxed and not trusted to not lie about it. We can continue this thread in bug #219010. 1708152 20 bugzilla 2020-11-17 01:31:03 -0800 (In reply to Michael Catanzaro from comment #19) > (In reply to Michael Catanzaro from comment #16) > > Second, the portal API fails because we try using > > g_get_prgname() instead of a valid app ID. > > That's not what's happening. We only pass our own prgname to the ScreenSaver > D-Bus API, which is blocked in the sandbox because it's only for use by > trusted applications. We don't pass our own app ID to the portal API, > because we are sandboxed and not trusted to not lie about it. We can > continue this thread in bug #219010. The org.freedesktop.ScreenSaver API is implemented in gnome-settings-daemon, and doesn't filter clients: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/screensaver-proxy/gsd-screensaver-proxy-manager.c https://github.com/WebKit/webkit/blob/master/Source/WebCore/PAL/pal/system/glib/SleepDisablerGLib.cpp#L54 1709999 21 calvaris 2020-11-24 07:17:00 -0800 The playback part is done. I think this can/should land regardless the problem with the flatpak. If there's something else I can help with, I guess another bug can be filed. 1710006 22 mcatanzaro 2020-11-24 07:46:44 -0800 (In reply to Michael Catanzaro from comment #18) > (In reply to Michael Catanzaro from comment #17) > > This often -- but not always -- happens when starting a video by clicking > > the "Skip Ads" button. > > Hm, actually it seems like it always happens. So summary of remaining > media-level problems: > > * SleepDisabler not created when "Skip Ads" is used to start the video > * SleepDisabler not destroyed when stopping the video > * SleepDisabler leaked when closing tab (though it is destroyed when > refreshing the tab!) These problems are not bugs in SleepDisabler itself. At least the first two seem very likely to be media playback bugs, yes? I suppose the third one could be caused by something else leaking the entire MediaPlayer? 1710420 23 413683 mcatanzaro 2020-11-28 06:20:48 -0800 Comment on attachment 413683 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413683&action=review > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:588 > + // Emit play state change notification only when going to PLAYING so that > + // the media element gets a chance to enable its page sleep disabler. > + // Emitting this notification in more cases triggers unwanted code paths > + // and test timeouts. But is this really correct? What is the plan to ensure the SleepDisabler gets released when playback stops? 1710540 24 413683 calvaris 2020-11-30 01:27:36 -0800 Comment on attachment 413683 Patch (In reply to Michael Catanzaro from comment #23) > Comment on attachment 413683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=413683&action=review > > > Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:588 > > + // Emit play state change notification only when going to PLAYING so that > > + // the media element gets a chance to enable its page sleep disabler. > > + // Emitting this notification in more cases triggers unwanted code paths > > + // and test timeouts. > > But is this really correct? What is the plan to ensure the SleepDisabler > gets released when playback stops? Once the sandboxing issues are fixed, you can test it and file a new bug if there is such a problem. 1710542 25 ews-feeder 2020-11-30 01:30:08 -0800 Committed r270235: <https://trac.webkit.org/changeset/270235> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413683. 1710544 26 webkit-bug-importer 2020-11-30 01:31:30 -0800 <rdar://problem/71798034> 1710584 27 mcatanzaro 2020-11-30 06:43:02 -0800 > * SleepDisabler not created when "Skip Ads" is used to start the video > * SleepDisabler not destroyed when stopping the video > * SleepDisabler leaked when closing tab (though it is destroyed when > refreshing the tab!) Issuse with SleepDisabler creation/destruction are not related to the sandboxing issues, so I'll report new bugs now. 1710593 28 mcatanzaro 2020-11-30 06:52:19 -0800 Reported: bug #219353, bug #219354, bug #219355. 413683 2020-11-10 03:29:30 -0800 2020-11-30 01:30:09 -0800 Patch bug-186971-20201110122929.patch text/plain 2842 calvaris U3VidmVyc2lvbiBSZXZpc2lvbjogMjY5NTY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjQyNzM3NDVlZDFjYzEz NjhiMDg2YWUwNDUwNzFlNGY0YzQwOGM2MC4uZDEzNjdmYWRiNTNhYmMyODY3OWE5OGJlMTFmMGM2 MTc4MTQxZDVkZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDIwLTExLTEwICBYYWJp ZXIgUm9kcmlndWV6IENhbHZhciAgPGNhbHZhcmlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dT dHJlYW1lcl0gTWVkaWEgcGxheWVyIGRvZXMgbm90IHByb3Blcmx5IGluaGliaXQsIHVuaW5oaWJp dCBzbGVlcAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTg2OTcxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvbXNlL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJl YW1lck1TRS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVBsYXllclByaXZhdGVHU3RyZWFt ZXJNU0U6OnVwZGF0ZVN0YXRlcyk6IENhbGwgdGhlCisgICAgICAgIHBsYXllciB0byB1cGRhdGUg dGhlIHBsYXliYWNrIHN0YXRlIGFzIGl0IGlzIGRvbmUgaW4gdGhlIHJlZ3VsYXIKKyAgICAgICAg cGxheWVyLgorCiAyMDIwLTExLTA3ICBSb2IgQnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CiAKICAg ICAgICAgRml4IHdhcm5pbmcgcmVsYXRlZCB0byB1bnNpZ25lZCA+PTAgQVNTRVJUCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvbXNlL01lZGlh UGxheWVyUHJpdmF0ZUdTdHJlYW1lck1TRS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9n cmFwaGljcy9nc3RyZWFtZXIvbXNlL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck1TRS5jcHAK aW5kZXggYzQ0Y2EzOTY3YWIwZmYwNjkwZjBjZDJlZTYzMzRkY2M1ZDBkN2ZiOC4uMmFjMjIzZDgw NmZmZjk4MjY2ZTIxNWJlNTlhZThhYjYyY2IyMWQ0ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL21zZS9NZWRpYVBsYXllclByaXZhdGVHU3Ry ZWFtZXJNU0UuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJl YW1lci9tc2UvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyTVNFLmNwcApAQCAtNDk4LDggKzQ5 OCwxNCBAQCB2b2lkIE1lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck1TRTo6dXBkYXRlU3RhdGVz KCkKICAgICBNZWRpYVBsYXllcjo6TmV0d29ya1N0YXRlIG9sZE5ldHdvcmtTdGF0ZSA9IG1fbmV0 d29ya1N0YXRlOwogICAgIE1lZGlhUGxheWVyOjpSZWFkeVN0YXRlIG9sZFJlYWR5U3RhdGUgPSBt X3JlYWR5U3RhdGU7CiAgICAgR3N0U3RhdGUgc3RhdGUsIHBlbmRpbmc7CisgICAgYm9vbCBzdGF0 ZVJlYWxseUNoYW5nZWQgPSBmYWxzZTsKIAogICAgIEdzdFN0YXRlQ2hhbmdlUmV0dXJuIGdldFN0 YXRlUmVzdWx0ID0gZ3N0X2VsZW1lbnRfZ2V0X3N0YXRlKG1fcGlwZWxpbmUuZ2V0KCksICZzdGF0 ZSwgJnBlbmRpbmcsIDI1MCAqIEdTVF9OU0VDT05EKTsKKyAgICBpZiAoc3RhdGUgIT0gbV9jdXJy ZW50U3RhdGUpIHsKKyAgICAgICAgbV9vbGRTdGF0ZSA9IG1fY3VycmVudFN0YXRlOworICAgICAg ICBtX2N1cnJlbnRTdGF0ZSA9IHN0YXRlOworICAgICAgICBzdGF0ZVJlYWxseUNoYW5nZWQgPSB0 cnVlOworICAgIH0KIAogICAgIGJvb2wgc2hvdWxkVXBkYXRlUGxheWJhY2tTdGF0ZSA9IGZhbHNl OwogICAgIHN3aXRjaCAoZ2V0U3RhdGVSZXN1bHQpIHsKQEAgLTU3Niw2ICs1ODIsMTUgQEAgdm9p ZCBNZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXJNU0U6OnVwZGF0ZVN0YXRlcygpCiAgICAgICAg ICAgICBHU1RfREVCVUcoIlJlcXVlc3RlZCBzdGF0ZSBjaGFuZ2UgdG8gJXMgd2FzIGNvbXBsZXRl ZCIsIGdzdF9lbGVtZW50X3N0YXRlX2dldF9uYW1lKHN0YXRlKSk7CiAgICAgICAgIH0KIAorICAg ICAgICAvLyBFbWl0IHBsYXkgc3RhdGUgY2hhbmdlIG5vdGlmaWNhdGlvbiBvbmx5IHdoZW4gZ29p bmcgdG8gUExBWUlORyBzbyB0aGF0CisgICAgICAgIC8vIHRoZSBtZWRpYSBlbGVtZW50IGdldHMg YSBjaGFuY2UgdG8gZW5hYmxlIGl0cyBwYWdlIHNsZWVwIGRpc2FibGVyLgorICAgICAgICAvLyBF bWl0dGluZyB0aGlzIG5vdGlmaWNhdGlvbiBpbiBtb3JlIGNhc2VzIHRyaWdnZXJzIHVud2FudGVk IGNvZGUgcGF0aHMKKyAgICAgICAgLy8gYW5kIHRlc3QgdGltZW91dHMuCisgICAgICAgIGlmIChz dGF0ZVJlYWxseUNoYW5nZWQgJiYgKG1fb2xkU3RhdGUgIT0gbV9jdXJyZW50U3RhdGUpICYmICht X29sZFN0YXRlID09IEdTVF9TVEFURV9QQVVTRUQgJiYgbV9jdXJyZW50U3RhdGUgPT0gR1NUX1NU QVRFX1BMQVlJTkcpKSB7CisgICAgICAgICAgICBHU1RfSU5GT19PQkpFQ1QocGlwZWxpbmUoKSwg IlBsYXliYWNrIHN0YXRlIGNoYW5nZWQgZnJvbSAlcyB0byAlcy4gTm90aWZ5aW5nIHRoZSBtZWRp YSBwbGF5ZXIgY2xpZW50IiwgZ3N0X2VsZW1lbnRfc3RhdGVfZ2V0X25hbWUobV9vbGRTdGF0ZSks IGdzdF9lbGVtZW50X3N0YXRlX2dldF9uYW1lKG1fY3VycmVudFN0YXRlKSk7CisgICAgICAgICAg ICBzaG91bGRVcGRhdGVQbGF5YmFja1N0YXRlID0gdHJ1ZTsKKyAgICAgICAgfQorCiAgICAgICAg IGJyZWFrOwogICAgIH0KICAgICBjYXNlIEdTVF9TVEFURV9DSEFOR0VfQVNZTkM6Cg==