fix(linux/xdgportal): Properly support multiple screens by exposing pipewire streams as separate displays#4931
Conversation
Kishi85
changed the title
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
2 times, most recently
from
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! |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
4 times, most recently
from
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:
|
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
from
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: |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
2 times, most recently
from
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. |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
2 times, most recently
from
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. |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
2 times, most recently
from
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:
|
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
3 times, most recently
from
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. |
|
I've taken the time to cleanup the commits a bit and update a few commit messages with more detail to make them easier to understand. I can do another squash if necessary before this gets merged and/or put 69c05b1 into a separate PR. |
|
Here's a full log taken from the latest PR changes: My comments regarding the pointer location was aimed at figuring out which monitor is to be selected on first connect and persisted on resolution change. Perhaps that won't be needed when the display names are detected correctly? As-is, my setup is still not detecting the name correctly, defaults to the wrong monitor (DP-2), and resolution change on DP-1 still causes it to switch to DP-2. |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
from
5aec74c to
be75524
Compare
|
The wl::monitors() correlation wasn't using logical_width/logical_height to match against so scaled displays wouldn't work correctly but after seeing @psyke83's first comment I've started looking and in the meantime we both came up with the same solution. So this is fixed now as well. The only remaining feature that would be nice to have is to do stream sorting additionally based on screen priorities (mainly to have the stream start on the primary screen) but that'll need changes to wayland.cpp/h to expose the necessary information (mainly the preferred/primary indicator). I'll try to implement this but might move it to a follow-up PR if it's a larger change as this PR is already doing quite a lot of things (although all are related to and necessary to make this feature work properly). UPDATE: From looking at other capture methods it looks like those have similar sorting issues (e.g. wlgrab takes the displays in the order that wl::monitors() provides them, which seems to be sorted alphabetically by wl_name). There also does not seem to be a unified Wayland interface for screen priorities/primary monitor (at least I've not found one so far). Adding sorting based on additional parameters is trivial but we have to get those parameters somehow and that's IMHO something for a follow-up PR as it will be a bit more involved. |
|
I agree with your assessment that primary/active detection is out of scope. I don't think Wayland has a direct method to probe the primary monitor, hence the suggestion to query the pipewire cursor position. But if it is feasible to do generically via Wayland, it belongs in a separate PR as it that could also be leveraged by kmsgrab. Until then, users can just arrange their displays so that the primary is leftmost. I'm seeing an issue with the xdg stop event. When stopping the stream via the xdg notification icon, it can't resume the session. Log is here; happens on KDE and GNOME. |
This is a tricky one and I might have an idea on how to handle this. I'll try to explain the issue here, maybe you have an idea:
|
|
@psyke83 I've changed the current implementation of XDG stop and made it so that it invalidates the session_cache after the capture errors out, making it possible to connect again without restarting sunshine. It's not the ideal solution I've described but should fix the immediate problem you noted. |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
3 times, most recently
from
a404fd5 to
efe6597
Compare
|
I've refined the fix for detecting closed portals a bit more as I can't seem to figure out that whole portal closed detection for dbus right now. The basic flow when closing the portal session is now:
As I can't figure out the Session::Closed signal handling right now I'd rather move that optimization to a follow-up PR as well. This flow is stable (after some testing) and good enough to handle the case. |
So the log can easily be filtered for messages related to xdg portalgrab
…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).
…iffer from dbus during display names retrieval Fix capture issues (black- or greenscreen) when changing resolution due problems when doing session cache invalidation during portal display init. The pipewire stream is working properly when session cache check and invalidation are done during portal_display_names while discovering available displays, so do it there. Also make sure pipewire_fd opened by dbus_t are closed by it.
… usage pipewire_screenstream_t -> pipewire_streaminfo_t as it is only containing the metadata necessary to identify the stream streams_to_display_names -> portal_streams_to_display_names so it can be easily identified as portal related
…hat as display_name To ensure display_name is independent of postion/resolution so swichting to the same display on resolution change based re-init works properly. On failing correlation fall back to position/resolution matching for a stream to have at least basic display switching working. To have uniquely distinguishable display_names prefix them with 'n' for monitor_name matching and 'p' for position/resolution matching.
…pipewire_streaminfo_t Reduce complexity in other code parts and have the display_name generation/matching done next to each other for better maintainability.
…validate session_cache This should fix the permanent error after closing the stream by ending it using the XDG portal notification until we can implement the XDG stop event properly.
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
from
efe6597 to
71a9566
Compare
|
Everything is working fine (including the XDG stop case) with the latest changes on KDE. GNOME is another story, unfortunately. If multiple screens are selected, only one stream is detected by Sunshine. The more critical issue, however, is that changing the resolution is broken, triggering the stream to disconnect no matter whether one or two screens are selected for use by the remote portal or if the second screen is disabled via GNOME display settings. Log taken when just DP-1 is selected by the portal and DP-2 is disabled via GNOME's display settings, after trying to change the resolution of DP-1: https://gist.github.com/psyke83/dd95c00b814996868cba6dcdacdb02e4 Apart from Sunshine's log, this warning is printed on mode change: (As a sanity check since this is not my preferred DE, I verified that resolution change is working OK with GNOME on the current master branch.) |
The logical reason here is that the 'multiple' option of the Screencast portal does not work (properly or at all) with xdg-desktop-portal-gnome, which is unfortunate.
From the logs it seems that the Screencast Portal closes on Gnome when the resolution changes which would trigger the new logic for the XDG stop event. On master everything's working as the portal will get invalidated on pipewire::cleanup_stream making it reload on every session_cache::get_or_create_session anyway likely masking the issue (due to the pipewire_fd from the newer portal_session). |
…ed handling This improves stopping the stream drastically as it does no longer retry before erroring out. This commit also removes all of the old stream_stopped logic and just error out on stream_dead if the portal session is closed instead of doing a re-init.
|
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. 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. |
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
from
d73337a to
3616a13
Compare
…t 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 now due to implementing the Portal.Session::Closed signal before. Note: Sunshine already ensures that there is just one running Display (therefore Portalsession) at a time.
Kishi85
force-pushed
the
xdgportalgrab-better-multi-monitor-support
branch
from
3616a13 to
57534af
Compare
|
3 participants
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