fix(linux/xdgportal): Properly support multiple screens by exposing pipewire streams as separate displays#4931
Conversation
1f4fa32 to
58efcab
Compare
|
@psyke83 Sorry to ping you here but can you think of a reason why the keyboard shortcut to switch displays would not work with portalgrab? I've been trying to do so after enumerating them properly for display_names (current state of this PR) but the switch_display_event from video.cpp does not seem to be firing or handled when using portalgrab and I can't figure what I'm missing. |
If you add some debug to video.cpp you'll see that your current code is hitting the |
|
Look: Sunshine/src/platform/linux/kmsgrab.cpp Lines 1221 to 1230 in b172a98 It seems that portalgrab is not checking the push_captured_image_cb callback the same way in the capture loop. The same logic may need to be added. I will look at this further later when I have time, if you don't figure it out. |
That is exactly what was missing to get the keyboard shortcut to work. Thanks! |
5fa6e6f to
6d7942e
Compare
|
I've got the basic functionality working now and can switch multiple displays using keyboard shortcuts. The main issue right now is when the same display currently actively streaming is requested to be switched to again (e.g. currently streaming display 0 and trying to switch to it via shortcut) then the stream will disconnect due to https://github.com/Kishi85/Sunshine/blob/6d7942e0c82aeedfca16b77735f0fc6cf716a4ec/src/platform/linux/portalgrab.cpp#L1294-L1309 which is causing the stream to disconnect with Problem here is that this is necessary to detect if the user stopped the stream via the XDG notification (according to @psyke83 notes in #4914 (comment)). Possible solutions that need more research:
|
6d7942e to
3a22dff
Compare
auto switch_display_event = mail::man->event<int>(mail::switch_display);
if (previous_width.load() == width && previous_height.load() == height && previous_pos_x.load() == pos_x && previous_pos_y.load() == pos_y && switch_display_event->peek()) {Next issue is when changing displays via shortcut too quickly there seems to be a possible race condition which can happen that freezes Sunshine hard until it is restarted and then it is exiting with: |
e097695 to
79773f8
Compare
Scratch that solution as I've had a typo (missing inversion) there and didn't realize that the event is already popped when the display gets reset by video.cpp and the portal re-initialzies. Back to square one on that.
Sunshine hanging when switching too quickly is still an issue. UPDATE: This could be related to chosen encoder, having tried vulkan and vaapi yields different results (hang vs. SEGV). UPDATE 2: More or less consistently reproducible when switching screens back and forth without waiting for the stream to update to the new screen. Does not seem to trigger if waiting for the stream to update to the new screen after switching plus a second or two more. |
36c8067 to
073e73d
Compare
|
The crashing (whether hang or segfault) may be due to the pipewire loop still being active during fake reinit. See this block here: Sunshine/src/platform/linux/portalgrab.cpp Lines 1363 to 1370 in b172a98 And here: Sunshine/src/platform/linux/portalgrab.cpp Lines 1393 to 1399 in b172a98 You may also need to set these same variables when you intend to signal an artificial reinit (i.e., when push_captured_image_cb returns false) to ensure the encoder fully stops. Regarding the race condition that causes a hang, one of the changes in #4875 resolves a hang after disconnect that happens when there are no screen updates happening. Since the on_process callback doesn't fire, the capture loop gets stuck. I worked around that by checking the pull_free_image_cb result during timeout, so you may want to see if you need to do a similar check elsewhere (or perhaps find a better fix). |
|
I'll stop force-pushing most things im doing right now and will be squashing the whole thing in the end after we've got all the kinks ironed out. A few things I've done and learned now:
With all these changes I seemingly have managed to get a stable reproducer for the crash (SEGV) by switching to the same display that is currently streaming. This is working on a single monitor setup as well, allowed by the implementation in video.cpp and should just fully reset the display (+pipewire stream) without segfaulting. The crash only occurs when switching to the same display. Switching to another display and then back to the first one does not seem to crash (as easily) unless you're switching very quickly but that might just be resulting in a switch to the same display internally in the end.
UPDATE: Testing this again and again leads to the same result: Segfault is only happening when trying to switch to the same screen that is already streaming even though the display reset/initialization looks the same in the log as when switching from one screen to another. It is properly reusing the session cache now and connecting to the same pipewire_node for each display. Could this be some quirk when repeatedly streaming the same node from pipewire? Switching to a different node and then back to the first one does not seem to trigger this issue. |
ed38479 to
12c20b7
Compare
|
After a lot of trial and error I've managed to cut the hangs when switching display down to only when very quickly, repeatedly switching (to the same? haven't tried others) display. You have to basically mash the shortcut to get a segfault. My theory for that is that when switching really quickly something in the capture logic combined with pipewire just cannot keep up (some thread sync issue?). This is still an issue but I'm wondering if it can be mitigated in video.cpp as it seems unreasonable to allow queuing another switch_display_event while the previous one's reset_display has not finished: Lines 1478 to 1484 in b172a98 All other issues I've had with this are basically ironed out now:
Things still todo before I'm likely to remove the draft status:
|
aa2ff1f to
b346dd1
Compare
|
Squashed, rebased and description+title updated. This is ready for review. There's still one known issue (that was there before this PR just masked, see description for details) but maybe someone reviewing this has an idea on how to fix or work around that. |
|
So I've just forced myself to bend my head around that XDG stop signal handling on the portal (which took me 2 hours in the end) and managed to implement it and reduce complexity for that case by a lot (see 910b2ce). I'm still testing if everything works as it should as I had to rip out the old stream_stopped logic in the process because now that was breaking display switching. |
|
With the working session_cache I've noted that the XDG notification is always present while Sunshine is running which is not really desirable. So 57534af cuts that feature out (not really necessary as Sunshine ensures there's just one active display anyway) and by doing that the XDG notification only appears when Sunshine has an active Display that is using portalgrab (like on current master due to cleanup_stream including session_cache::invalidate). This was not possible before as the XDG stop event needed to be implemented properly via the DBus signal first. Removing the session caching reduces complexity further and ensures clean states on the pipewire end. This might also fix the issues with resolution changing on GNOME but I've currently no way to test it. I'm really curious on your opinion on this one @psyke83 as I'm sure you had implemented the cache for reasons. |
3616a13 to
57534af
Compare
|
I'm not the original author of portalgrab or responsible for the session cache code; I only contributed small changes such as variable rate capture support before it was merged to master, so I'm not really the person to answer your question. I wasn't aware that the XDG icon was persisting after disconnect; stuff like that is harder for me to see when running exclusively headless, so that seems like a positive change. The latest PR has an improvement: resolution change appears to be working properly on GNOME, but stopping via the XDG icon on both GNOME and KDE has regressed again. Relevant debug logs taken from moment XDG is clicked until stream unpauses/reinits on KDE: On GNOME: |
|
I'll put this back into a draft state while I'm reworking the commit structure as it is just all over the place right now. From your logs it looks like the end signal isn't working properly. It should look something like this with the latest changes when you do XDG end: On your logs it's still doing a session reset (which it shouldn't really do now, as the stop is based on the proper DBus signal) |
a2b9472 to
6ae0ee8
Compare
|
I've reworked the commit structure into 5 distinct commits and will try to do all further changes by updating a related commit. I'm also marking this ready for review again. @psyke83 I've added a 100ms delay to the dead stream handling in the capture code as your logs show that the portal closed signaling from DBUS was too late for the portal to be regarded in time before forcing the re-init. I'm unsure why this happend but there's likely nothing we can do to speed things up. It would be better to have a sync-way of querying the portal status but so far I've not found anything related to that. UPDATE: It's sometimes simpler than one thinks. I've updated the |
4393d2e to
46070be
Compare
So the log can easily be filtered for messages related to xdg portalgrab
7dd27c2 to
e483a17
Compare
|
Unfortunately the XDG stop still doesn't stop for me on KDE or GNOME. My CPU should not be excessively slow (5700X), but I tried bumping the wait time to eliminate the possibility: Still unsuccessful: Same issue on GNOME: I also checked to see if it's an issue related to an idle desktop not firing callbacks, but keeping 60fps content running on the desktop while clicking the XDG icon has the same result. |
I've changed the check to do a synchronized call (that fetches the version property of the portal.Session) and if that one fails then the portal must have closed. Could you try with the latest version of this PR? It should work there, if it's still broken then something very weird is going on and the pipewire stream pauses/stops before the portal closed. |
|
It's working now! The previous iteration of your PR was failing on my setup due to my Apologies for not specifying my config earlier; I didn't expect that the systray setting would have such an interaction with portalgrab that we were seeing here. |
|
All good. Honestly I'd never have guessed that the system tray would have interacted with the signaling. It's still better to do it with a calling a DBus Method on the session anyway as that gives an immediate and correct result without having to worry about synchronizing states between DBus and Pipewire. Also less complexity to understand if someone else has to look at it again. |
e483a17 to
3633d14
Compare
…using DBus. This improves stopping the stream drastically as it does no longer re-init before erroring out. This commit also removes all of the old stream_stopped logic in favor of just erroring out on stream_dead if the portal session is closed.
…nt XDG notification When the session_cache is working properly the XDG notification for a running session is always active while Sunshine is running. To avoid this and also reduce complexity of the portalgrab logic remove the session_cache handling for the portal session itself but keep it intact for other uses (like tracking maxFramerateFailed). Removing caching fully is possible after implementing the Portal.Session::Closed signal first. Note: Sunshine already ensures that there is just one running Display (therefore Portalsession) at a time.
* Add additional debug logging * Ensure pipewire_fd's are not needlessly duplicated and closed at the correct point. * Add minimal error handling for pipewire context/core to avoid obvious segfaults * (From Sonarqube:) Add exception handler for cleanup_stream in destructor
…ipewire streams as separate displays Enables display switching on the client-side with Sunshine's existing shortcuts (CTRL+ALT+Fxx) when selecting multiple screens on the screencast source selection dialog (or automatically all availabe screens when using a combined remotedesktop+screencast session, tested with KDE). Each pipewire stream given by the portal will be available as a separate display (consistently ordered by position).
3633d14 to
5149ada
Compare
|



Description
This PR adds proper multi-monitor support to Sunshine's XDG portal grab by exposing all streams of a capture portal as separate screens (utilizing the multiple capture mode of the screencast portal).
Things this PR does:
Known issues:
Can segfault when trying to switch displays really quickly (by mashing the screen switch shortcut multiple times within a second) and even then it's not fully reproducible. This would have been happening even before this PR when just having a single stream but was masked due to the shortcut handling not being properly implemented in the capture logic (missing return on !push_captured_image_cb).This is a separate issue concerning other capturemethods as well (tested with KMS), see Coredump with SIGSEGV when switching displays too quickly #4943Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage