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

(WIP) wayland: add support for XDG_TOPLEVEL_STATE_SUSPENDED #15667

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Sunderland93
Copy link
Contributor

This adds support for suspended toplevel state introduced in https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/201

Currently I'm not sure how to properly plug this event to RetroArch's window, so WIP for now

@ColinKinloch

@Sunderland93 Sunderland93 marked this pull request as draft September 3, 2023 08:03
@Sunderland93 Sunderland93 changed the title (WIP) wayland: add support for XDG_TOPLEVEL_STATE_SUSPEND (WIP) wayland: add support for XDG_TOPLEVEL_STATE_SUSPENDED Sep 3, 2023
@@ -33,6 +33,10 @@
#define APP_ID "org.libretro.RetroArch"
#define WINDOW_TITLE "RetroArch"

#ifndef XDG_TOPLEVEL_STATE_SUSPENDED
Copy link
Contributor

@ColinKinloch ColinKinloch Sep 3, 2023

Choose a reason for hiding this comment

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

This is defined in the header generated by wayland-scanner from xdg-shell.xml when./configure is run.
Making sure it's defined more likely to hide issues than fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that the #define XDG_TOPLEVEL_STATE_SUSPENDED was unnecessary.
However trying it on my machine I see that XDG_TOPLEVEL_STATE_SUSPENDED isn't being defined in ./gfx/common/wayland/xdg-shell.h because ./gfx/common/wayland/generate_wayland_protos.sh is selecting my distros protocol files which are not as up to date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have increased requirement wayland-protocols version to 1.32, so if it's not found - bundled (and updated) xdg-shell will be used instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, I completely missed that.
I was trying to hack pkg-config support into ./gfx/common/wayland/generate_wayland_protos.sh.

@Sunderland93
Copy link
Contributor Author

Sunderland93 commented Sep 3, 2023

Hmm, after compiling with this PR and run on KDE Plasma Wayland I got this error:

listener function for opcode 2 of xdg_toplevel is NULL

but on Sway it works fine

@ColinKinloch
Copy link
Contributor

Ah, yes, that is because you've upped xdg_toplevel support to version 6.
Which means you also need to implement version 4 and 5. Each adds one event to the xdg_toplevel listener so it shouldn't be too complex.
Version 4: xdg_toplevel::configure_bounds
Version 5: xdg_toplevel::wm_capabilities

Empty functions may be fine.

@Sunderland93
Copy link
Contributor Author

Should I make a separate PR for this?

@ColinKinloch
Copy link
Contributor

My gut says no. If the implementation of either event handler ends up complex then split it off.

@Sunderland93
Copy link
Contributor Author

Done

@Sunderland93
Copy link
Contributor Author

@ColinKinloch should I also adapt libdecor part to this? https://gitlab.freedesktop.org/libdecor/libdecor/-/merge_requests/126

@ColinKinloch
Copy link
Contributor

Looks like the latest release of libdecor was 0.1.1 10 months ago, whereas suspend support was merged last month.
If you can hide it behind an #ifdef or something?
I guess just make sure adding suspend support doesn't break libdecor.

@Sunderland93
Copy link
Contributor Author

Sunderland93 commented Sep 4, 2023

Also, does RetroArch already have an interface to interrupt window rendering (render loop) that could be bound to the suspended state?

@ColinKinloch
Copy link
Contributor

I have a pretty poor understanding of the RetroArch code base outside of the Wayland code to be honest.
It's could be you should implement it in gfx/drivers_context/wayland_ctx.c and gfx/drivers_context/wayland_vk_ctx.c or maybe runloop.c.

@ColinKinloch
Copy link
Contributor

I've had another look at this. Here are my thoughts:

Now that libdecor 0.2.x is stable you should add support for that.

I'm still not exactly sure where implement suspend in retroarch. I guess the goal is to avoid rendering frames when the surface is suspended so that the game loop, audio etc are still processed. Adding if (wl->suspended) return; at the start of gfx_ctx_wl_swap_buffers is a rudimentary option I've used while investigating this.

Logging when the surface suspends and resumes in xdg_toplevel_handle_configure_common shows that the suspend event doesn't arrive until the surface is resumed. It seems that once the surface is off screen retroarch stalls indefinitely at vkQueuePresentKHR so part of the solution may involve adding a timeout to that.

@Sunderland93
Copy link
Contributor Author

Hello. I am unable to continue working on this PR at this time, so you can take it over if you'd like

@LibretroAdmin
Copy link
Contributor

Hi there @Sunderland93 and @ColinKinloch , sorry for the neglect with this PR.

Are you guys still willing to finalize this and get it incorporated, or has another PR superseded it? I'm still interested in getting this across the finish line. Thanks.

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

Successfully merging this pull request may close these issues.

3 participants