[Draft] wayland: Improve seat / input handling

[markbolhuis]

This PR improves the handling of seats and keyboard input on Wayland.

• Handle multiple seats. Wayland compositors may advertise more than one seat. MangoHud right now only binds to the first one. I've added a SeatInfo class that maintains it's own wl_seat, wl_keyboard, xkb_state, and pressed keysyms.
  I've moved most functionality into this new class and the Wayland callbacks are simple redirects.

• Handle wl_seat removal. The compositor may remove a wl_seat global at any time. So if that happens the SeatInfo instance is destroyed.

• Handle keyboard removal. If a wl_seat loses keyboard functionality (because the user unplugs their keyboard for instance) a wl_seat.capabilities event is sent. In this case the wl_seat, wl_keyboard, and xkb_state are destroyed, and pressed keysyms are cleared.

• More robust implementation. There was a lot of error checking omitted previously, which I've added.

• wl_seat version selection, rather than just binding blindly to version 5. Which would result in a crash if the compositor only supports up to version 4. Instead we bind to any version up to 5 and implement proper version checks in the code.

• When vkDestroyInstance or eglTerminate is called the Wayland objects are cleaned up.

This is a draft, because there are a few things left to resolve that would require a more invasive change of MangoHud's code than just a rewrite of wayland_keybinds.cpp. Namely how to properly handle key modifiers, and multiple wl_display instances.

[flightlessmango]

Ping @Etaash-mathamsetty

[Etaash-mathamsetty]

looks good other than that, ill try modifying this soon to support multiple wl_displays. Should be possible with just a rewrite of wayland_keybinds.cpp and slight modifications to the other stuff

Probably should null check the user data in all the callbacks

[Etaash-mathamsetty]

wl_display_roundtrip_queue was here twice so that the Seat object is constructed before the function returns, should still work without it in this case but it would be simpler if everything was already setup when this function returned

[markbolhuis]

A roundtrip does not guarantee that, the only thing it guarantees is that any events the wl_display has received thus far are read and dispatched. The exception is the registry which the spec explicitly notes. Even then, not every seat will be in the registry when mangohud starts.

I think it's fine to omit the round-trips.

[markbolhuis]

looks good other than that, ill try modifying this soon to support multiple wl_displays. Should be possible with just a rewrite of wayland_keybinds.cpp and slight modifications to the other stuff

Can Mangohud make use of VK_EXT_private_data for the vulkan side? So you store the wl_surface in the VkSurface data slot, and use wl_proxy_get_display

Probably should null check the user data in all the callbacks

Why?

[markbolhuis]

Just busy refactoring the vulkan and egl hooks a bit, and noticed that libwayland-client.so is loaded using dlopen, but it is not used anywhere?
As a test I've removed it and void *wl_handle, and it works just fine. Building with with_wayland=disabled works as well since all wayland code is behind HAVE_WAYLAND

Is it the intention to dynamically load libwayland-client.so or can it be just dynamically linked?

[Etaash-mathamsetty]

Just busy refactoring the vulkan and egl hooks a bit, and noticed that libwayland-client.so is loaded using dlopen, but it is not used anywhere? As a test I've removed it and void *wl_handle, and it works just fine. Building with with_wayland=disabled works as well since all wayland code is behind HAVE_WAYLAND

Is it the intention to dynamically load libwayland-client.so or can it be just dynamically linked?

there used to be an issue where wayland functions were called without the library loaded, so dlopen with RTLD_NOLOAD was used to check if the library was actually loaded or not. Now that we don't hook wayland functions anymore it's not needed i guess

[Etaash-mathamsetty]

looks good other than that, ill try modifying this soon to support multiple wl_displays. Should be possible with just a rewrite of wayland_keybinds.cpp and slight modifications to the other stuff

Can Mangohud make use of VK_EXT_private_data for the vulkan side? So you store the wl_surface in the VkSurface data slot, and use wl_proxy_get_display

Probably should null check the user data in all the callbacks

Why?

not sure why that these fancy things are needed, since mangohud runs one instance per process my idea is to just maintain a list of wl_display and delete elements as they become invalid (through return value of wl_display_dispatch_queue_pending) and check inputs for each wl_display and each display's seats. But 99% of applications will not connect to 2 wl_displays at the same time so something this advanced is probably unnecessary, instead an easier method would be make a queue of all wl_displays that were grabbed and just use the first element until it becomes invalid (then pop that element, reconstruct everything and continue).

[markbolhuis]

not sure why that these fancy things are needed, since mangohud runs one instance per process my idea is to just maintain a list of wl_display and delete elements as they become invalid (through return value of wl_display_dispatch_queue_pending) and check inputs for each wl_display and each display's seats.

This would likely crash the program.

Side note: Mangohud running as one instance per process leads to some strange behaviour, for example, each window reports the combined fps from all windows.. And hotkeys affect all windows not just the focused one.

But 99% of applications will not connect to 2 wl_displays at the same time so something this advanced is probably unnecessary, instead an easier method would be make a queue of all wl_displays that were grabbed and just use the first element until it becomes invalid (then pop that element, reconstruct everything and continue).

I'll update the PR using a std::map that maintains a display per VkSurface and EGLDisplay.

Actually on second thoughts I'll just clean up what I've done so far. Looking further it seems that handling multiple wl_displays properly is not worth it.

[Etaash-mathamsetty]

not sure why that these fancy things are needed, since mangohud runs one instance per process my idea is to just maintain a list of wl_display and delete elements as they become invalid (through return value of wl_display_dispatch_queue_pending) and check inputs for each wl_display and each display's seats.

This would likely crash the program.

Side note: Mangohud running as one instance per process leads to some strange behaviour, for example, each window reports the combined fps from all windows.. And hotkeys affect all windows not just the focused one.

But 99% of applications will not connect to 2 wl_displays at the same time so something this advanced is probably unnecessary, instead an easier method would be make a queue of all wl_displays that were grabbed and just use the first element until it becomes invalid (then pop that element, reconstruct everything and continue).

I'll update the PR using a std::map that maintains a display per VkSurface and EGLDisplay.

Actually on second thoughts I'll just clean up what I've done so far. Looking further it seems that handling multiple wl_displays properly is not worth it.

I mean these issues with multiple surfaces per process are also issues on x11, so I agree with the sentiment that it's not worth it at least right now. Maybe later we can implement it for both x11 and Wayland in one PR.

Why would the first way crash the program btw?

[markbolhuis]

Why would the first way crash the program btw?

Because 'invalid' in this case is after wl_display_disconnect is called, and thus the pointer is dangling.
Also before wl_display_disconnect is called, any event queues and proxies have to be be cleaned up.

[nokia8801]

I got this error while building with this commit. Maybe it'll help.

build-error.txt

I was building with:

        -Dmangoapp=false \
        -Dmangohudctl=false \
        -Dwith_nvml=disabled \
        -Dwith_xnvctrl=disabled \
        -Dwith_x11=disabled \
        -Dwith_wayland=enabled \
        -Dwith_dbus=enabled \