Bug 180197 – [GStreamer] Ensure SleepDisabler is not held by pages in page cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Michael Catanzaro]

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.

[youenn fablet]

> 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...

[Philippe Normand]

Michael can you upload your patch? Did you implement MediaPlayerPrivate::setShouldDisableSleep() too?

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 330811 [details]
Patch

[Carlos Garcia Campos]

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.

[Philippe Normand]

Created attachment 330812 [details]
Patch

[Philippe Normand]

Hi Eric, can you please review this patch?

[Philippe Normand]

Committed r226630: <https://trac.webkit.org/changeset/226630>

[Radar WebKit Bug Importer]

<rdar://problem/36374983>

[Michael Catanzaro]

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!

[Michael Catanzaro]

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.