Summary: | [GStreamer] Ensure SleepDisabler is not held by pages in page cache | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
Component: | Media | Assignee: | Philippe Normand <pnormand> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, eric.carlson, mcatanzaro, pnormand, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=178485 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 181432, 181471 | ||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-11-30 07:20:34 PST
> 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> 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. |