After implementing PAL::SleepDisabler for GTK in bug #178485, I decided to try removing the early return at the top of HTMLMediaElement::shouldDisableSleep for non-Cocoa ports, hoping that HTMLMediaElement would create a SleepDisabler during video playback and destroy it at the end of video playback, to keep the display from turning off when a video is playing. I tested it on youtube.com and noticed the following unexpected behavior: (1) When playing a video on YouTube, no SleepDisabler is initially created during normal video playback, so the display will turn off undesirably. (2) But a SleepDisabler *is* created when pressing the browser's Back button during video playback. (3) The SleepDisabler is then only destroyed when (a) closing the browser tab that had been displaying YouTube, or (b) when pressing Forward to bring back the original YouTube page. So I think monitor sleep is disabled when the YouTube page is in the page cache, and not reenabled until the page exits the page cache. That's not good! I wonder if this is a GTK-specific problem, or if it's also broken on macOS and iOS.
> So I think monitor sleep is disabled when the YouTube page is in the page > cache, and not reenabled until the page exits the page cache. That's not > good! I wonder if this is a GTK-specific problem, or if it's also broken on > macOS and iOS. This works as expected in macOS and iOS for YouTube so there might be something specific to GTK. Maybe some debugging would help here to know why there is no SleepDisabler in the first place when clicking play/pause...
Michael can you upload your patch? Did you implement MediaPlayerPrivate::setShouldDisableSleep() too?
The problem seems to be with that line for the GStreamer ports: bool shouldBeAbleToSleep = !hasVideo() || !hasAudio(); the value is true, so that's why no sleep disabler is created. This is likely an issue with the GStreamer backend. I suspect the pipeline is not fully built when this code is called.
Created attachment 330811 [details] Patch
Comment on attachment 330811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330811&action=review > Source/WebCore/ChangeLog:13 > + The sleep disabler is now checked again whenever the media player > + playback state or other charasteristic has changed in the media > + engine. On GStreamer backend side we now properly emit playback > + state changes and characteristic updates to the client when the > + pipeline state changes and when audio/video tracks changes are > + detected. The GST part is related but independent, right? I would split the patch.
Created attachment 330812 [details] Patch
Hi Eric, can you please review this patch?
Committed r226630: <https://trac.webkit.org/changeset/226630>
<rdar://problem/36374983>
Note this change does more than indicated by the commit message: media playback now (hopefully) causes sleep to be disabled for the first time. That is great! Thanks!
This has uncovered a bug in GNOME: https://bugzilla.gnome.org/show_bug.cgi?id=705942#c12 WebKit is now doing the right thing, so I don't suggest we change anything.