1877022 - [Wayland] wayland idle inhibit broken

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[rpigott]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:122.0) Gecko/20100101 Firefox/122.0

Steps to reproduce:

The WaylandIdleInhibit WakeLockType is regressed by [1].

The wakelock is disabled anytime it fails to acquire the lock in [2], but the InhibitWaylandIdle() method may return false for benign reasons, like firefox not having focus [3]. Firefox should use another surface in this case, ideally whichever one has the playing video.

Once the WakeLockType is disabled it won't be re-enabled, meaning if a video begins playing while firefox is not focused, idle inhibit will remain broken for the remainder of the session, even in cases where it otherwise would have succeeded.

[1] https://hg.mozilla.org/mozilla-central/rev/1c8dec8815d59d488dc44109a4361dcec573ec0a
[2] https://hg.mozilla.org/mozilla-central/rev/1c8dec8815d59d488dc44109a4361dcec573ec0a
[3] https://hg.mozilla.org/mozilla-central/rev/1c8dec8815d59d488dc44109a4361dcec573ec0a#l1.573

Actual results:

Firefox does not inhibit idle in a wayland session while playing video, if playback begins while the browser is not focused.

Expected results:

Firefox should always acquire the idle inhibitor, regardless of focus, and ideally should not permanently disable idle inhibition if it fails to acquire the inhibitor for whatever reason. It is compositor policy when to enforce the inhibition, so the client does not and should not have a need to know if the surface is focused or visible.

Comment 1

[rpigott]

Link [2] should be https://hg.mozilla.org/mozilla-central/rev/1c8dec8815d59d488dc44109a4361dcec573ec0a#l1.766

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Audio/Video: Playback' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 4

[az [:az]]

Thanks for the report! :stransky, would you have any thoughts here?

Comment 5

[Martin Stránský [:stransky] (ni? me) (PTO, Back Jun 24)]

Please run on terminal with MOZ_LOG="LinuxWakeLock:5" env variable, like:

MOZ_LOG="LinuxWakeLock:5" firefox > log.txt 2>&1

and attach the log here (or check if the lock behaves correctly). You can also run just as:

MOZ_LOG="LinuxWakeLock:5" firefox

to get real-time data about locking and check that.

Thanks.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit BugBot documentation.

Comment 7

[rpigott]

Attachment: mozlog.txt

MOZ_LOG=LinuxWakeLock:5 firefox |& tee mozlog.txt

Comment 8

[rpigott]

I just used youtube for this.

I let the video preview begin to play, which successfully acquires the idle inhibitor:

[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0] InhibitWaylandIdle() succeeded

Then I click on the video and move focus away from firefox while it is loading:

[Parent 8292: Main Thread]: D/LinuxWakeLock [7c84b158f850] WakeLockListener topic video-playing state locked-foreground request lock 1
[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0] WakeLockTopic::InhibitScreensaver() Inhibited 0
[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0] WakeLockTopic::SendInhibit() WakeLockType Unsupported
[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0] InhibitWaylandIdle()
[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0] WakeLockTopic::SwitchToNextWakeLockType() WakeLockType Unsupported
[Parent 8292: Main Thread]: D/LinuxWakeLock [7c8493d15ec0]   switched to WakeLockType (null)

The wakelock is now permanently disabled.

Comment 10

[monstrousogre0813]

Attachment: log.txt

I'm facing the same issue but the logs are different. InhibitWaylandIdle() is not even being called, even when the video starts playing. I'm using Hyprland

Comment 11

[rpigott]

This issue is just about the wayland idle inhibitor. If the wake lock doesn't work for you for some other reason, it's a different issue.

You may have encountered https://github.com/flatpak/xdg-desktop-portal/issues/1254.

Comment 12

[monstrousogre0813]

I don't know how to check what's the reason, but firefox is the only application that isn't inhibiting the idle state on wayland. Other applications like VLC don't have this problem.

Comment 13

[Vuk Mirovic]

Attachment: log.txt

Facing the same issue on Sway window manager. Screen lock kicks in while playing Youtube video. It started with Firefox 122.0.

Comment 14

[Donal Meehan [:dmeehan]]

Would anyone in the bug try with mozregression to identify what introduced this? https://mozilla.github.io/mozregression/quickstart.html
(Though might be challenging if it takes time to hit the problem)
:az :stransky anything in the info provided so far that could help the investigation?

Comment 15

[monstrousogre0813]

I bisected it with mozregression. The first bad commit is 7d45ea2f

2024-02-03T06:05:04.491000: DEBUG : Found commit message:
Bug 1860269 Implement org.freedesktop.portal.Inhibit for wakelock listener; r=stransky

Differential Revision: https://phabricator.services.mozilla.com/D192364

Let me know if you need anything else from me

Comment 16

[rpigott]

Facing the same issue on Sway window manager.

You have the xdp bug.

I bisected it with mozregression. The first bad commit is 7d45ea2f

You also have the xdp bug.

Thanks, but this isn't helpful guys. I already gave the cause. This bug is clear from inspecting the code, and so is the fix. Firefox just needs to use the video surface (or a more suitable surrogate) for acquiring the inhibitor, rather than trying to find a focused surface.

Comment 17

[Martin Stránský [:stransky] (ni? me) (PTO, Back Jun 24)]

Attachment: Bug 1877022 [Wayland] Don't switch away from native wake lock inhibitor r?emilio

Better to try inhibit native wake lock again that to switch to Unsupported one and disable it at all.

Comment 18

[rpigott]

Better to try inhibit native wake lock again that to switch to Unsupported one and disable it at all.

Is it possible for firefox to just use the video surface? To be clear, I don't think there are any cases where the wayland idle inhibitor protocol is expected to fail. The surface does not need to have focus. While this patch will help, it looks like the wakelock will still fail if playback begins while the browser is not focused.

If it's too difficult to get the video surface, maybe the toplevel surface would be a better surrogate? This would be a better behavior than giving up if we don't have a focused surface.

Comment 19

[Martin Stránský [:stransky] (ni? me) (PTO, Back Jun 24)]

(In reply to rpigott from comment #18)

Better to try inhibit native wake lock again that to switch to Unsupported one and disable it at all.

Is it possible for firefox to just use the video surface? To be clear, I don't think there are any cases where the wayland idle inhibitor protocol is expected to fail. The surface does not need to have focus. While this patch will help, it looks like the wakelock will still fail if playback begins while the browser is not focused.

If it's too difficult to get the video surface, maybe the toplevel surface would be a better surrogate? This would be a better behavior than giving up if we don't have a focused surface.

What do you mean with 'video surface' ? There isn't any such thing AFAIK. Wayland deletes any invisible wl_surfaces and we don't track all Firefox windows from toolkit code to search any unfocused window. Also Wayland inhibitor is a fallback, we use DBus primary, so I don't it's worth to investigate much to it (but patches are welcome).

Comment 20

[rpigott]

What do you mean with 'video surface' ?

The video content could be placed in it's own surface (i.e. wl_subsurface), or it could be composited first by firefox into another surface (xdg toplevel or popup). Occluded surfaces aren't deleted, but the compositor is generally only expected to enforce the inhibitor when the associated surface is visible.

Ideally we only care to inhibit idle when the video is visible, so it makes the most sense to chose this surface if we can. If that's too much trouble (I don't see how to looking at the code) I think the toplevel surface should work reasonably well (replace GetFocusedWindow w/ GetWindow I think?). We don't actually need a focused surface, so failing when we can't find one even though we have a surface that would work to inhibit idle isn't ideal.

Comment 21

[Martin Stránský [:stransky] (ni? me) (PTO, Back Jun 24)]

(In reply to rpigott from comment #20)

What do you mean with 'video surface' ?

The video content could be placed in it's own surface (i.e. wl_subsurface), or it could be composited first by firefox into another surface (xdg toplevel or popup). Occluded surfaces aren't deleted, but the compositor is generally only expected to enforce the inhibitor when the associated surface is visible.

Ideally we only care to inhibit idle when the video is visible, so it makes the most sense to chose this surface if we can. If that's too much trouble (I don't see how to looking at the code) I think the toplevel surface should work reasonably well (replace GetFocusedWindow w/ GetWindow I think?). We don't actually need a focused surface, so failing when we can't find one even though we have a surface that would work to inhibit idle isn't ideal.

It may be possible to call something like "gtk_get_toplevel_window()" or so and get wl_surface from it. Firefox doesn't use any kind of video overlay for playback so there isn't any wl_subsurface with video content.

Comment 22

[rpigott]

Additional info for the subscribers:

Many people have xdg-deskop-portal configs that erroneously declare support for the Idle portal. Unfortunately, a current bug in xdp (same as mentioned above https://github.com/flatpak/xdg-desktop-portal/issues/1254) makes it basically impossible to correct this configuration error. This breaks the WakeLock on firefox with the new code, but is not an error in firefox. If you're here because your idle doesn't work, that is most likely the cause. Please visit the xdp issue instead.

Comment 23

[rpigott]

Also, AFAICT the focused window bit existed before the patch I identified as the regression — the part that regressed was that the WakeLock stayed dead after it was triggered, and I expect that behavior is corrected by your patch. This issue could probably be closed as completed with that patch if you want, just might be nice to fix both if it isn't too much trouble.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9309553f817
[Wayland] Don't switch away from native wake lock inhibitor r=emilio

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e9309553f817

Comment 26

[Martin Stránský [:stransky] (ni? me) (PTO, Back Jun 24)]

As native Wayland inhibitor is just a fallback I'm not going to implement case then focused window is missing. But if someone want to follow this patch I'll happily review such patch.