Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wayland: Suspend window after frame timeout or suspend state #87750

Merged
merged 1 commit into from Feb 16, 2024

Conversation

Riteo
Copy link
Contributor

@Riteo Riteo commented Jan 30, 2024

Depends on #88374.

Fixes #87963.

This is a pretty popular approach that took a while for me to wrap my head around and which only recently got "official" support through an update (xdg_shell version 6), so I think that this is all-in-all a better option than the overkill 2000Hz ticking we have now :P

Basically, we wait for a frame event and, if either too much time passes or we get the new suspended state, we consider the window as "hidden" and stop drawing, ticking by the low usage rate.

This should work great for KDE and Mutter, which support the new state, but not yet for sway, which is still stuck at a very old xdg_shell version and thus falls back to the timeout approach.

Be aware that if we rely on timing out the engine will have to stall for the whole timeout, which could be problematic but doensn't seem like it. Further testing is needed.

Special thanks go to the guys over at #wayland on OFTC, who very patiently explained me this approach way too many times.


FTR, the current timeout is 1 second, but I think that we could go as low as 200ms without any big consequence. Further testing is needed. Edit: Seems to not cause big issues, I think we can keep it like this for now.

Note that the new xdg-shell extension is not implemented in this PR, as I can't really test is as sway doesn't implement it yet. Edit: It is implemented now! Sway still doesn't support it though :(

Bugsquad edit:

@Riteo Riteo requested a review from a team as a code owner January 30, 2024 18:20
@Riteo Riteo changed the title Throttle frames with a locking loop and suspend on timeout Wayland: pace frames by timing out Jan 30, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 30, 2024
@akien-mga akien-mga changed the title Wayland: pace frames by timing out Wayland: Pace frames by timing out Feb 8, 2024
@akien-mga akien-mga requested a review from bruvzg February 8, 2024 11:21
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Fedora 39 KDE Wayland, works well. I can confirm the bug with the MRP from #87963 and this PR fixes it.

@pracedru also confirmed this PR fixes #88248.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

@akien-mga does the 1sec timeout cause issues when a window gets toggled in/out of view, especially in regards to audio? I couldn't test it yet.

Edit: TBC, even if it did it would probably still cause less problems than ticking at 2000hz, but I wonder if this is something we should be aware of.

@akien-mga
Copy link
Member

akien-mga commented Feb 15, 2024

@akien-mga does the 1sec timeout cause issues when a window gets toggled in/out of view, especially in regards to audio? I couldn't test it yet.

Edit: TBC, even if it did it would probably still cause less problems than ticking at 2000hz, but I wonder if this is something we should be aware of.

I didn't notice any issue, even after doing some more testing now. In verbose mode, I never saw Frame timeout, suspended., so I'm not sure if I'm doing what's needed to trigger it.

I tried:

  • Running a project from the editor (which turns the editor to low process usage mode while the game window is focused), and having a looping sound play in the editor at the same time (no sound problem, even when toggling between both windows).
  • Running a project from command line and making the window lose focus, both with and without low processor usage mode.

Edit: Actually I found a way to trigger the suspend message, by locking my Plasma session. The audio kept playing fine, and unlocking the session resumed the game fine. I can hear a significant reduction in fan speed/noise when the screen is locked, so it seems to properly throttle things, and then the fans start spinning again when unlocking.

I'm not pushing my luck to test Suspend-to-RAM, as it's broken currently on my new laptop on Fedora :P

BTW those debug prints should likely be removed eventually:

Pointing window.
Left window.

But I guess they're fine to keep for a bit while debugging things in the new experimental Wayland support.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

@akien-mga

Running a project from command line and making the window lose focus, both with and without low processor usage mode.

You don't trigger it by losing focus, instead by suspending (e.g. hiding) the window. Try to minimize the project for a few seconds or put it behind another window.

Basically, as per the comments, the whole engine will stall for one second (we could go as low as 200ms as per OP) once it gets suspended and only then enable low usage mode. That's the dirty heuristic.

BTW those debug prints should likely be removed eventually

All of the verbose logs are enabled by the WAYLAND_THREAD_DEBUG_LOGS_ENABLED and WAYLAND_DISPLAY_SERVER_DEBUG_LOGS_ENABLED macros (a la DisplayServerX11), with the exception of the window suspend thing and a few that slipped in or which I wasn't sure was a good idea to put behind such a verbose define.

But I guess they're fine to keep for a bit while debugging things in the new experimental Wayland support.

Yeah I guess it isn't worth to refine this thing right now as the backend is experimental and we'll need as much data as possible to fix any issue. Worth noting though.

@akien-mga
Copy link
Member

You don't trigger it by losing focus, instead by suspending (e.g. hiding) the window. Try to minimize the project for a few seconds or put it behind another window.

I still don't seem to trigger that logic when I minimize the window or make it fully covered by another one.

Basically, as per the comments, the whole engine will stall for one second (we could go as low as 200ms as per OP) once it gets suspended and only then enable low usage mode. That's the dirty heuristic.

Well we could give 200ms a try and see if anyone reports issues, WDYT? Otherwise I'm fine merging as is and see if anyone runs into issues with that delay.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

@akien-mga

I still don't seem to trigger that logic when I minimize the window or make it fully covered by another one.

A window is considered suspended here when it doesn't get a frame event for one second. A quick search in a WAYLAND_DEBUG=1 log for frame should clear any doubts.

I'll test ASAP to confirm whether this works, but I have a feeling that KDE is still ticking minimized windows.

Edit: oops missed the second part

Well we could give 200ms a try and see if anyone reports issues, WDYT? Otherwise I'm fine merging as is and see if anyone runs into issues with that delay.

Yeah, I think it might be safer. That would bring the threshold up to 5 FPS but it should be very unlikely I think. Let's see, either way we have to test I suppose. I'll also add the print to the macro at this point.

@akien-mga
Copy link
Member

I still don't seem to trigger that logic when I minimize the window or make it fully covered by another one.

A window is considered suspended here when it doesn't get a frame event for one second. A quick search in a WAYLAND_DEBUG=1 log for frame should clear any doubts.

I'll test ASAP to confirm whether this works, but I have a feeling that KDE is still ticking minimized windows.

Indeed I still see frame ticks when minimized with WAYLAND_DEBUG=1.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

@akien-mga gotcha. I suppose I'll have to test this a bit more myself on sway, where this behavior happens.

Edit: I did some research and it might be related to the fact that KDE supports the new "suspended" state (xdg-shell version 6) while sway does not. Might be a new behavior. I'll investigate further.

@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

All right, some updates:

I've tested the 1 sec timeout on sway and audio plays properly. I think that, with audio taken out of the equation, we can assume that an application can stall for 1 second without exploding.

I've plumbed the new xdg_shell state in for the "native" window handling, although we need to update libdecor if we want to add it for users running godot through it. Because of this, I'm marking this as WIP until everything is in place.

Note that recent versions of KDE and Mutter (and probably also hyprland) already support this new state, so hopefully the only big compositor onto which we'll have to rely a lot on timing out is sway (see also the updated PR description).

I think we're almost done :D

Edit: libdecor is now plumbed too, but this particular state will require some testing. I have no idea how but I'll look if I can test this up too, although I don't know if I have a new enough KDE setup at hand.

@Riteo Riteo marked this pull request as draft February 15, 2024 19:13
@Riteo Riteo force-pushed the wayland-timeout-loop branch 2 times, most recently from 3c5b456 to 2e07dcf Compare February 15, 2024 22:48
This is a pretty popular approach that took a while for me to wrap my
head around and which only recently got "official" support through an
update (xdg_shell version 6), so I think that this is all-in-all a
better option than the overkill 2000Hz ticking we have now :P

Basically, we wait for a frame event and, if either too much time passes
or we get the new `suspended` state, we consider the window as "hidden"
and stop drawing, ticking by the low usage rate.

This should work great for KDE and Mutter, which support the new state,
but not yet for sway, which is still stuck at a very old xdg_shell
version and thus falls back to the timeout approach.

Be aware that if we rely on timing out the engine will have to stall for
the whole timeout, which _could_ be problematic but doensn't seem like
it. Further testing is needed.

Special thanks go to the guys over at #wayland on OFTC, who very
patiently explained me this approach way too many times.
@Riteo
Copy link
Contributor Author

Riteo commented Feb 15, 2024

All right, this was faster and simpler than I expected, making this up ready for review again :D

Note that I still haven't tested whether the suspend state works as intended as I still haven't had the chance to run this on a proper KDE setup (I'm pretty sure I have one on an external disk though)

@Riteo Riteo marked this pull request as ready for review February 15, 2024 22:50
@Riteo Riteo changed the title Wayland: Pace frames by timing out Wayland: suspend window after frame timeout or suspend state Feb 16, 2024
@akien-mga akien-mga changed the title Wayland: suspend window after frame timeout or suspend state Wayland: Suspend window after frame timeout or suspend state Feb 16, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again briefly on KDE, seems to work fine!

@akien-mga akien-mga merged commit 5c48275 into godotengine:master Feb 16, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants