Bug 219353 - [GStreamer] SleepDisabler not destroyed when video playback stops
Summary: [GStreamer] SleepDisabler not destroyed when video playback stops
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks:
 
Reported: 2020-11-30 06:46 PST by Michael Catanzaro
Modified: 2021-06-28 07:14 PDT (History)
19 users (show)

See Also:


Attachments
Patch (4.54 KB, patch)
2021-06-25 08:34 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2021-06-26 01:09 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (4.96 KB, patch)
2021-06-26 04:36 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-11-30 06:46:05 PST
Bug #186971 fixed the media player such that it now (usually) creates a SleepDisabler when starting video playback. However, the SleepDisabler is not destroyed when the media is stopped. For example, say I stop a YouTube video and leave it open in a browser tab that I'm not currently viewing. The SleepDisabler will continue to inhibit sleep indefinitely (or would if SleepDisabler were working correctly; it is currently broken due to bug #219010.)

The SleepDisabler should be destroyed when video playback stops, then recreated when playback resumes.
Comment 1 Philippe Normand 2021-06-25 08:34:03 PDT
Created attachment 432274 [details]
Patch
Comment 2 Philippe Normand 2021-06-25 08:41:46 PDT
Eric, is this an issue on Apple ports too? The new test fails on the mac EWS.
Comment 3 Eric Carlson 2021-06-25 09:05:10 PDT
(In reply to Philippe Normand from comment #2)
> Eric, is this an issue on Apple ports too? The new test fails on the mac EWS.

I haven't seen reports of it, so I don't believe it is a problem on the Apple ports. 

I'm not surprised the test fails as written though because we don't explicitly call `updateSleepDisabling` when playback ends.

I assume it is called as a side effect of something that happens later on Apple ports.  It would be better to make it explicit, something like this maybe:

diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
index 3f21dfe8ed17..69f4bb870622 100644
--- a/Source/WebCore/html/HTMLMediaElement.cpp
+++ b/Source/WebCore/html/HTMLMediaElement.cpp
@@ -5809,8 +5809,10 @@ void HTMLMediaElement::dispatchEvent(Event& event)
 {
     DEBUG_LOG(LOGIDENTIFIER, event.type());
 
-    if (m_removedBehaviorRestrictionsAfterFirstUserGesture && event.type() == eventNames().endedEvent)
+    if (m_removedBehaviorRestrictionsAfterFirstUserGesture && event.type() == eventNames().endedEvent) {
         document().userActivatedMediaFinishedPlaying();
+        updateSleepDisabling();
+    }
 
     HTMLElement::dispatchEvent(event);
Comment 4 Philippe Normand 2021-06-25 09:17:14 PDT
Yes, indeed. Explicit is better!
Comment 5 Philippe Normand 2021-06-26 01:09:02 PDT
Created attachment 432323 [details]
Patch
Comment 6 Philippe Normand 2021-06-26 04:36:43 PDT
Created attachment 432326 [details]
Patch
Comment 7 EWS 2021-06-27 02:32:46 PDT
Committed r279313 (239189@main): <https://commits.webkit.org/239189@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432326 [details].