Wayland backend: add host-to-client clipboard sync (revived)

[ninetailedtori]

I'd welcome testing on this, it should implement the clipboard protocol correctly, without crashing randomly on tab out tab ins/copy-pasting invalid mime types into gamescope.

[cwildfoerster]

+1, works as excpected, can copy&paste fine into cs2. i guess .gitignore change should be removed.

XKB_DEFAULT_LAYOUT="de(nodeadkeys)" LD_PRELOAD="" /home/user/Downlods/gamescope/build/src/gamescope -w 1280 -h 960 -W 2560 -H 1440 -S stretch -f --force-grab-cursor --expose-wayland -- %command% -console

[ninetailedtori]

+1, works as excpected, can copy&paste fine into cs2. i guess .gitignore change should be removed.

XKB_DEFAULT_LAYOUT="de(nodeadkeys)" LD_PRELOAD="" /home/user/Downlods/gamescope/build/src/gamescope -w 1280 -h 960 -W 2560 -H 1440 -S stretch -f --force-grab-cursor --expose-wayland -- %command% -console

I'll push the revert to gitignore, good catch. :]

[ninetailedtori]

Any crashes to note? Has anyone tested if image clipboard content outside the app destroys the offer correctly and doesn't post a garbled mess? My own testing seems reasonably stable :)

[hucet]

Hi, thanks for the update! The clipboard sync worked for me.

I found a crash related to drag and drop: When I drag a file (or text) from my host and hover it over the game window, gamescope crashes immediately. The window disappears, but the process seems to remain active in the background.

The logs show a lot of these errors when the crash happens:

[gamescope-clipboard] [Error]  xdg_backend: Compositor released us but we were not acquired. Oh no.
[gamescope-clipboard] [Warn]  xwm: got the same buffer committed twice, ignoring.

My Setup:

• Distro: CachyOS (Arch based) with Gnome
• Game: World of Warcraft Classic and Battlenet via Lutris
• Arguments: -w 7680 -h 2160 -W 7680 -H 2160 -r 240 -f --force-grab-cursor

Is this happening only on my end/setup?

[ninetailedtori]

Replying to #2062 (comment)

This isn't enough log, unfortunately. Are you running with debug option on gamescope? I'd need a log on the bug reproduction to try to diagnose what was happening :]

[cwildfoerster]

[gamescope-clipboard] [Warn]  xwm: got the same buffer committed twice, ignoring.

i get this spammed 100 times per second, seems to be related to --force-grab-cursor

[ninetailedtori]

[gamescope-clipboard] [Warn]  xwm: got the same buffer committed twice, ignoring.

i get this spammed 100 times per second, seems to be related to --force-grab-cursor

Wacky, lemme find out where I've botched that up!

[ninetailedtori]

Could you send a full trace log if that's possible? Just for debugging purposes.

[PatrickFD]

gdb_gamescope.txt is the backtrace from gdb --args build/src/gamescope -- dolphin
WAYLAND_DEBUG_gamescope.txt is from WAYLAND_DEBUG=client build/src/gamescope -- dolphin

Dragging of a png-file over Dolphin inside gamescope lead to the crash.

gdb_gamescope.txt
WAYLAND_DEBUG_gamescope.txt

[ninetailedtori]

Alright, @PatrickFD can you test this build and see if that works? I think it was the illegal roundtrip re-entrant calls. Do note, to build this one, you'll need #2110 now, since the merge pulled in the libinput bump from wlroots, which is currently broken.

cd subprojects/wlroots
curl -O https://gist.githubusercontent.com/Billli11/d9e45819b87dbe82469fe2492492aff7/raw/1b3cd4aa948f0a184b063d6e6839d833b871bd81/wlroots-5261.patch
git apply wlroots-5261.patch

[fnr1r]

Hello. I just wanna chime in. Commit 7657c70 works fine for me.

I have one suggestion, though: since supportedMimeTypes is const, using std::vector looks to me like an unnecessary heap allocation. Consider changing it to something like:

static constexpr std::array supportedMimeTypesStorage = {
    "text/plain;charset=utf-8", "UTF8_STRING", "text/plain", "STRING", "TEXT",
};

static const std::span<const char* const> supportedMimeTypes = supportedMimeTypesStorage;

There could be a more idiomatic way to do this.

Have a nice [TIMEZONE]!

EDIT: Actually, the span isn't even needed.

static constexpr std::array supportedMimeTypes = {
    "text/plain;charset=utf-8", "UTF8_STRING", "text/plain", "STRING", "TEXT",
};

(but this exposes the length as part of the type, unlike the span)

[ninetailedtori]

Shit, didn't even catch that lol! Lemme slam an optimisation commit rq

[neonvoidx]

I've never been so hyped for a pr merge before

[ninetailedtori]

Replying to #2062 (comment)

Dw about the extra span definition, it's lightweight! After all, it's just difference between the ctime constexpr array and the span which just wraps the arr in a ref view space, no extra work 👀

[fnr1r]

Using wl_display_flush instead of wl_display_roundtrip seems to cause a deadlock for me on KWin, where Gamescope receives a data offer and waits for KWin to write to the pipe while KWin also waits for Gamescope to write to its pipe. It also freezes plasmashell.

OS: Arch Linux
Host compositor: kwin 6.6.3-1

debug_changes.diff.txt
debug.log

Maybe you should add a flag to ignore the next incoming data offer? Or go back to using wl_display_roundtrip?

[ninetailedtori]

Replying to #2062 (comment)

Shoot, is this dragging or all clipboard activity?

[fnr1r]

Replying to #2062 (comment)

Just copy-pasting. Apparently, dragging and dropping (which I haven't tested until now) causes an abort. That seems to be a different issue, though. So now there are two issues.

The 1st one still exists. I've redone the testing with xnedit and it happens with that too, so it's not exclusive to Kate.

The 2nd one is caused by CWaylandBackend::s_DataOfferListener having NULL callbacks.

debug2.log
bug2_backtrace.txt

[ninetailedtori]

Replying to #2062 (comment)

So, the second issue, I might be able to fix with this commit, having analysed the file? The first bug that occurs is the annoying one, I'm gonna see what I can do with that. Lemme read the log in more detail just to see the best method here.

[ninetailedtori]

See if this commit fixed things please! I've attempted to separate the processing from the blocking and attach it to its own queue.

[fnr1r]

7c92683 does actually fix drag-and-drop aborts. I fixed it in the same way. That works.

But currently, Gamescope segfaults because m_pDataDevice is accessed before it's initialized from wl_data_device_manager_get_data_device. That can be fixed by just moving the code lower.

0001-fix-backend-wayland-fix-data-dev-segfault.patch

Another problem is that we're not dispatching events from m_pDataDeviceQueue, and I have no idea where to add that. The input thread has its own class, so maybe clipboard handling should too?

Also, maybe the input and clipboard queues should be given names to avoid confusion. (with wl_display_create_queue_with_name) ("Input Queue" and "Clipboard Queue")

With the segfault fixed and queue names added for clarity on fnr1r@125bbc3, WAYLAND_DEBUG=client ./build/src/gamescope -- ${SOME_TEXT_EDITOR} 2>&1 | grep "Clipboard" only shows outgoing events. Sending the current selection to the host works because wl_data_device_manager and its child wl_data_sources still use the default queue, which does dispatch events.

wl_log_snippet.log

Side note - from looking at the source, I dislike that the Wayland backend is a single file. And that Gamescope doesn't have a consistent style. (.editorconfig says tabs, src/Backends/WaylandBackend.cpp uses spaces) But that's besides the point.

Happy Holidays to everyone tracking this issue!

[ninetailedtori]

7c92683 does actually fix drag-and-drop aborts. I fixed it in the same way. That works.

But currently, Gamescope segfaults because m_pDataDevice is accessed before it's initialized from wl_data_device_manager_get_data_device. That can be fixed by just moving the code lower.

0001-fix-backend-wayland-fix-data-dev-segfault.patch

Another problem is that we're not dispatching events from m_pDataDeviceQueue, and I have no idea where to add that. The input thread has its own class, so maybe clipboard handling should too?

Also, maybe the input and clipboard queues should be given names to avoid confusion. (with wl_display_create_queue_with_name) ("Input Queue" and "Clipboard Queue")

With the segfault fixed and queue names added for clarity on fnr1r@125bbc3, WAYLAND_DEBUG=client ./build/src/gamescope -- ${SOME_TEXT_EDITOR} 2>&1 | grep "Clipboard" only shows outgoing events. Sending the current selection to the host works because wl_data_device_manager and its child wl_data_sources still use the default queue, which does dispatch events.

wl_log_snippet.log

I've actually forgone the need for queue entirely, see if that fixes things!

Regarding the clipboard events, this should fix things? Theoretically I could've tied the queue setting within dataoffer_offer, which is very much possible through:

	void CWaylandBackend::Wayland_DataDevice_DataOffer(struct wl_data_device *pDevice, struct wl_data_offer *pOffer) {
		m_CurrentOfferMimeTypes.clear();
		wl_proxy_set_queue(pOffer, m_pDataDeviceQueue);
		wl_data_offer_add_listener(pOffer, &s_DataOfferListener, this);
	}

But honestly, the mutex SHOULD be cheaper, and equally safe?

[fnr1r]

Replying to #2062 (comment)

I should have said that the indentation thing wasn't a criticism of you specifically, just the current state of Gamescope. I'm sorry.

I misdiagnosed this issue in #2062 (comment). It's also only now that I understand this, so maybe I should relay it (but now better):

• Gamescope sends create_data_source, offers and set_selection - in order
• The host compositor sends that information to every client with a wl_data_device_listener set up, including Gamescope
• Gamescope dispatches the received selection before send
• The file descriptor Gamescope sent is never written to (since Gamescope is reading), but also never closed
• Every other process with a wl_data_device_listener that receives Gamescope's offer also freezes, unless it uses async IO
• The issue is resolved by killing Gamescope, which closes the last write end of the pipe. Clients reading from the pipe stop blocking, and read returns 0

wl_display_roundtrip worked because it dispatched the send event before it read from the FD.

Your current solution is functionally equivalent to 7657c70. The mutex is unnecessary since there's only one thread interacting with the clipboard. I don't understand why it was added there since it's not going to do anything.

I tried using a flag (instead of a mutex) to ignore the next wl_data_offer after sending the wl_data_source to the host locally (setting it to true in CWaylandConnector::SetSelection and false in CWaylandBackend::Wayland_DataSource_Send and returning in CWaylandBackend::Wayland_DataDevice_Selection if it's true), but this can be broken by ALT-TABbing. KWin sends a new wl_data_offer every time you re-focus the window.

Sorry. It's my fault. I should have read up on this more before commenting.

[ninetailedtori]

Replying to #2062 (comment)

I can fix this, one sec. I'll keep the mutex but expand it for the read thread as well, just to synchronise the offers anyway.

[ninetailedtori]

Alright, I'm gonna push an experimental fix to offload to a readerthread and see if that works? it SHOULD handle the blocking reads, the deadlock with the host, maintaining mutex and should handle mult offers.

[fnr1r]

CWaylandBackend::CWaylandReaderThread is never actually ran and m_ClipboardReadThread never gets set. I think you forgot to spawn that thread.

Also, don't forget to add a destructor to CWaylandClipboardState that joins, detaches or whatever else C++ requires you to do with threads, so it doesn't crash while quitting, as the SDL backend does. I doubt m_ClipboardReadThread will be joinable while quitting, but it's better to add it anyway.

[ninetailedtori]

CWaylandBackend::CWaylandReaderThread is never actually ran and m_ClipboardReadThread never gets set. I think you forgot to spawn that thread.

Also, don't forget to add a destructor to CWaylandClipboardState that joins, detaches or whatever else C++ requires you to do with threads, so it doesn't crash while quitting, as the SDL backend does. I doubt m_ClipboardReadThread will be joinable while quitting, but it's better to add it anyway.

whoopsie!

[ninetailedtori]

Check this one!

[fnr1r]

It works. I'll try to look at it later and offer some feedback.