fix(linux/xdgportal): Properly support multiple screens by exposing pipewire streams as separate displays

[Kishi85]

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:

• Improve logging done by portalgrab.cpp for easier debugging (could be moved to a separate PR if desired)
• Change screencast source selection mode to multiple
• Expose separate displays for each screen stream provided by the screencast portal (ordering streams consistently by screen position)
• Fix (more or less defunct) session caching and properly invalidate the cache only when screens are added
• Fix the (currently broken) keyboard shortcut for the switch_display_event
• Use a better way to handle stopping the stream via XDG notification icon (as doing it based on matching the display resolution will break display switching)
• Adds a few more fail safes based on newly added stream_connected indicator which is determined by stream events.

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 #4943

Screenshot

Issues Fixed or Closed

• Fixes XDG portalgrab with multiple displays squishes all into one stream #4914

Roadmap Issues

Type of Change

• feat: New feature (non-breaking change which adds functionality)
• fix: Bug fix (non-breaking change which fixes an issue)
• docs: Documentation only changes
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc.)
• refactor: Code change that neither fixes a bug nor adds a feature
• perf: Code change that improves performance
• test: Adding missing tests or correcting existing tests
• build: Changes that affect the build system or external dependencies
• ci: Changes to CI configuration files and scripts
• chore: Other changes that don't modify src or test files
• revert: Reverts a previous commit
• BREAKING CHANGE: Introduces a breaking change (can be combined with any type above)

Checklist

• Code follows the style guidelines of this project
• Code has been self-reviewed
• Code has been commented, particularly in hard-to-understand areas
• Code docstring/documentation-blocks for new or existing methods/components have been added or updated
• Unit tests have been added or updated for any new or modified functionality

AI Usage

• None: No AI tools were used in creating this PR
• Light: AI provided minor assistance (formatting, simple suggestions)
• Moderate: AI helped with code generation or debugging specific parts
• Heavy: AI generated most or all of the code changes

[Kishi85]

@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.

[psyke83]

@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 switch_display_event->peek() case when the combination is first pressed, but the peek event then repeatedly triggers. The artificial reinit is not completing successfully for some reason.

[psyke83]

Look:

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 1221 to 1230 in b172a98

	case platf::capture_e::timeout:
	if (!push_captured_image_cb(std::move(img_out), false)) {
	return platf::capture_e::ok;
	}
	break;
	case platf::capture_e::ok:
	if (!push_captured_image_cb(std::move(img_out), true)) {
	return platf::capture_e::ok;
	}
	break;

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.

[Kishi85]

Look:

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 1221 to 1230 in b172a98

	case platf::capture_e::timeout:
	if (!push_captured_image_cb(std::move(img_out), false)) {
	return platf::capture_e::ok;
	}
	break;
	case platf::capture_e::ok:
	if (!push_captured_image_cb(std::move(img_out), true)) {
	return platf::capture_e::ok;
	}
	break;

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]

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 Warning: PipeWire stream stopped by user. in the logs.

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:

• Have the check detect if it is currently handling a switch_display_event. I've already had to extend it by adding the current stream position values to the mix so we can discern switching different screens without disconnecting.

• Figure out another way to detect the user disconnecting the portal manually and change that part of the stream logic.

• Update video.cpp to not reinit if switching to the same display currently active and just keep streaming (might have side-effects I can't assess).

[Kishi85]

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

Changing the check to also include currently active switch_display_events directly works (not sure if this is the best way to do it):

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: Fatal: 10 seconds passed, yet Sunshine's still running: Forcing shutdown or Fatal: Hang detected! Session failed to terminate in 10 seconds. when the session is closed on the client-side.

[Kishi85]

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

Changing the check to also include currently active switch_display_events directly works (not sure if this is the best way to do it):

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()) {

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.

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: Fatal: 10 seconds passed, yet Sunshine's still running: Forcing shutdown or Fatal: Hang detected! Session failed to terminate in 10 seconds. when the session is closed on the client-side.

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).
After running with a debugger I'm seeing the SEGV in https://github.com/FFmpeg/FFmpeg/blob/15504610b0dc12c56e5e9f94ff06c873382368f5/libavcodec/hw_base_encode.c#L508 related to ctx being invalid/missing. Could we have an issue with switching from one pipewire_stream to another upon calling portal_t->init() multiple times? Simply adding a lock mutex for the whole init function does not seem to do the trick. It's likely somewhere in the capture or encoder logic.

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.

[psyke83]

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

	if (stream_stopped.load()) {
	BOOST_LOG(warning) << "PipeWire stream stopped by user."sv;
	capture_running.store(false);
	stream_stopped.store(false);
	previous_height.store(0);
	previous_width.store(0);
	pipewire.frame_cv().notify_all();
	return platf::capture_e::error;

And here:

Sunshine/src/platform/linux/portalgrab.cpp

Lines 1393 to 1399 in b172a98

	case platf::capture_e::interrupted:
	capture_running.store(false);
	stream_stopped.store(false);
	previous_height.store(0);
	previous_width.store(0);
	pipewire.frame_cv().notify_all();
	return status;

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).

[Kishi85]

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:

• 9146245 Adds a prefix to all log messages from portalgrab.cpp so messages can be easily identified in the log (could do that as a separate PR as well if desired)
• Added a lot more logging on what goes on with pipewire_t and the session cache. Things learned from that:
  • When video.cpp switches displays it calls reset_display() which in term resets the shared_ptr to the current display implicitly destroying pipewire_t and then reinitialzing a new display from scratch.
  • pipewire_t::cleanup_stream() always invalidates the session cache. Since it is called by the destructor of pipewire_t the session cache is never actually used? (c432cb6 fixes this without any noticable changes in behaviour)
• 20af084 Temporarily disables the session ending logic via XDG notification icon
  • If you try to access an ended session pipewire returns a specific error Pipewire Error, id:2 seq:8 message: no target node available (causing a green screen on the connected client) that could be used to better track a user ending the session via the xdg notification.

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.

I'm wondering if this is related to the whole session caching (which is currently more or less defunct on master anyway) and will try to remove the whole session cache to see what happens when this is using a fresh portal every time a new display is constructed. video.cpp already ensures that only one capture is active (due to other capture methods limitations) so multiple XDG notifications should never be an issue in the first place Without session cache the client is just getting a greenscreen.

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]

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:

Sunshine/src/video.cpp

Lines 1478 to 1484 in b172a98

	// Process any pending display switch with the new list of displays
	if (switch_display_event->peek()) {
	display_p = std::clamp(*switch_display_event->pop(), 0, (int) display_names.size() - 1);
	}
	// reset_display() will sleep between retries
	reset_display(disp, encoder.platform_formats->dev_type, display_names[display_p], capture_ctxs.front().config);

All other issues I've had with this are basically ironed out now:

• Hanging issue for switching to the same display is (almost fully) solved.
• XDG notification stream end is implemented via the associated pipewire error and not by matching screen properties
• The session cache is now properly invalidated only when a display_name that is not in the list is requested. This can happen as display_names enumerate directly from a separate portal instance (so that is always the current state). As display_names are just position and resolution values as "x,y,w,h" portal->init matches pipewire_streams based on those. Also we do not have to invalidate the session cache for removed portals as the related streams will just stay in there unused.

Things still todo before I'm likely to remove the draft status:

• Tone down logging to debug level for most of the newly added messages.

[Kishi85]

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.

[codecov]

Bundle Report

Bundle size has no change ✅

[Kishi85]

Linter issues fixed as reported

[codecov]

Codecov Report

❌ Patch coverage is 0.52356% with 190 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@ba4db46). Learn more about missing BASE report.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/platform/linux/portalgrab.cpp	0.52%	80 Missing and 110 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #4931   +/-   ##
=========================================
  Coverage          ?   18.16%           
=========================================
  Files             ?      108           
  Lines             ?    23405           
  Branches          ?    10330           
=========================================
  Hits              ?     4252           
  Misses            ?    14146           
  Partials          ?     5007           

Flag	Coverage Δ
Archlinux	11.61% <0.00%> (?)
FreeBSD-14.3-amd64	13.73% <0.53%> (?)
Homebrew-ubuntu-22.04	13.93% <0.00%> (?)
Linux-AppImage	12.46% <0.53%> (?)
Windows-AMD64	14.92% <ø> (?)
Windows-ARM64	13.23% <ø> (?)
macOS-arm64	19.02% <ø> (?)
macOS-x86_64	18.39% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/platform/linux/portalgrab.cpp	8.58% <0.52%> (ø)

[psyke83]

Apologies, I won't have time to look at the code right now, but I did a quick test on Plasma 6.6.3 and found some issues.

• In single display mode, resolutions switching is broken by this PR; seems to always trigger a green (vulkan) or black (vaapi) screen that requires a service restart to recover from.
• In multi display mode with display 2 set to the left and (primary) display 1 to the right, changing the resolution of display 1 causes it to switch to display 2 on reinit.

When testing resolution changes, I used a simple script like the following for a single monitor:

#!/bin/bash

OUTPUT="1"
RES_A="1920x1080@60"
RES_B="3840x2160@60"
TIME=$1

if [[ -z "$TIME" ]]; then
  TIME=5
fi

for i in {1..10}
do
   echo "Iteration $i: Switching to $RES_B"
   kscreen-doctor output.$OUTPUT.mode.$RES_B
   sleep $TIME
   echo "Iteration $i: Switching to $RES_A"
   kscreen-doctor output.$OUTPUT.mode.$RES_A
   sleep $TIME
done

echo "Stress test complete."

This doesn't account for multi-monitors, but it can still trigger the issue.

[Kishi85]

• In single display mode, resolutions switching is broken by this PR; seems to always trigger a green (vulkan) or black (vaapi) screen that requires a service restart to recover from.

I can reproduce this and know where it is breaking but without further looking into it this does not make much sense to me but maybe you have an idea.

Due to the resolution change the display will be seen as a new screen which in term invalidates the session_cache and thereby restarts the portal (also fetching a new pipewire_fd and pipewire_streams vector): https://github.com/Kishi85/Sunshine/blob/d3eb98a2cc4ae095547d6b86447f3468e295397a/src/platform/linux/portalgrab.cpp#L1308-L1314

I've seen similar greenscreen behaviour when disabling the session_cache and trying to use dbus->connect_to_portal directly but couldn't make any sense of it at the time. Since I needed the session_cache anyway for detecting user ending via XDG notification this seems to have slipped during my testing.

• In multi display mode with display 2 set to the left and (primary) display 1 to the right, changing the resolution of display 1 causes it to switch to display 2 on reinit.

This one could be related to the display_naming scheme being based on both position and resolution. Does the log note falling back to the first stream?
It might work better if we're matching display streams by only position as the resolution can change and then things won't line up and it would fall back to the first stream in the sorted list.

[Kishi85]

• In single display mode, resolutions switching is broken by this PR; seems to always trigger a green (vulkan) or black (vaapi) screen that requires a service restart to recover from.

I can reproduce this and know where it is breaking but without further looking into it this does not make much sense to me but maybe you have an idea.

Due to the resolution change the display will be seen as a new screen which in term invalidates the session_cache and thereby restarts the portal (also fetching a new pipewire_fd and pipewire_streams vector): https://github.com/Kishi85/Sunshine/blob/d3eb98a2cc4ae095547d6b86447f3468e295397a/src/platform/linux/portalgrab.cpp#L1308-L1314

I've seen similar greenscreen behaviour when disabling the session_cache and trying to use dbus->connect_to_portal directly but couldn't make any sense of it at the time. Since I needed the session_cache anyway for detecting user ending via XDG notification this seems to have slipped during my testing.

UPDATE: Quick test shows that the pipewire stream does connect like it should do after session_cache invalidation/new cached portal session (so basically the same state as my previous problem when using dbus->connect_to_portal directly) but only gets to stream state paused and does not start streaming hence the green screen. Any quick ideas to why this would happen?

• In multi display mode with display 2 set to the left and (primary) display 1 to the right, changing the resolution of display 1 causes it to switch to display 2 on reinit.

This one could be related to the display_naming scheme being based on both position and resolution. Does the log note falling back to the first stream? It might work better if we're matching display streams by only position as the resolution can change and then things won't line up and it would fall back to the first stream in the sorted list.

UPDATE: This one is a bit tricky as changing resolution is currently seen as removing the screen with its old resolution and adding a new one with the new resolution. This is detected by video.cpp and handled by picking the first display from the displaynames list for re-init:

Sunshine/src/video.cpp

Lines 1241 to 1251 in d3eb98a

	// The old display was removed, so we'll start back at the first display again
	BOOST_LOG(warning) << "Previous active display ["sv << current_display_name << "] is no longer present"sv;
	} else {
	for (int x = 0; x < display_names.size(); ++x) {
	if (display_names[x] == output_name) {
	current_display_index = x;
	return;
	}
	}
	}
	}

Unless we find a way to generate persistent display_names here the display switch on resolution change might have to be categorised as expected behaviour. Persistently naming displays is hard when using portals as the streams are sorted by pipewire_node which is literally random so you have to do some sorting to get a consistent list. I'm using screen position in this PR but even position might change here on resolution change if you're on a stretched layout, so leaving the resolution out of the display_name won't help as much (unless its the display at 0,0 but that is picked as the first one anyway). Also since we're using the screencast portal we might not even have all available screens (as they user can select them in a screencast-only session) to map them to real displays and do the display naming/matching based on whatever else which always would be a lot more complicated.

[psyke83]

UPDATE: Quick test shows that the pipewire stream does connect like it should do after session_cache invalidation/new cached portal session (so basically the same state as my previous problem when using dbus->connect_to_portal directly) but only gets to stream state paused and does not start streaming hence the green screen. Any quick ideas to why this would happen?

Is the PR reinitializing to an existing stream? If the pipewire state is set to paused and reconnected with an active stream, ensure_stream will be a no-op. Perhaps you need to add an else clause to the end that resumes the stream via setting pw_stream_set_active to true.

Unless we find a way to generate persistent display_names here the display switch on resolution change might have to be categorised as expected behaviour. Persistently naming displays is hard when using portals as the streams are sorted by pipewire_node which is literally random so you have to do some sorting to get a consistent list. I'm using screen position in this PR but even position might change here on resolution change if you're on a stretched layout, so leaving the resolution out of the display_name won't help as much (unless its the display at 0,0 but that is picked as the first one anyway). Also since we're using the screencast portal we might not even have all available screens (as they user can select them in a screencast-only session) to map them to real displays and do the display naming/matching based on whatever else which always would be a lot more complicated.

The description for id seems to suggest that the order may be stable since we are using a restore token. Have you checked to see if this remains stable when reinitializing? That may not solve Sunshine starting to the wrong screen (as we don't persist anything on cold start), but it may help for mid-stream resolution changes, at least.

[Kishi85]

UPDATE: Quick test shows that the pipewire stream does connect like it should do after session_cache invalidation/new cached portal session (so basically the same state as my previous problem when using dbus->connect_to_portal directly) but only gets to stream state paused and does not start streaming hence the green screen. Any quick ideas to why this would happen?

Is the PR reinitializing to an existing stream? If the pipewire state is set to paused and reconnected with an active stream, ensure_stream will be a no-op. Perhaps you need to add an else clause to the end that resumes the stream via setting pw_stream_set_active to true.

Unfortunately that won't help as the display gets full reset so we're working with a newly initializing portal_display when the stream state gets stuck in paused. There's also no logging for pipewire parameters negotiated (like Video Format and so on) anymore for any capture re-init after the session_cache gets invalidated for the new display.

Unless we find a way to generate persistent display_names here the display switch on resolution change might have to be categorised as expected behaviour. Persistently naming displays is hard when using portals as the streams are sorted by pipewire_node which is literally random so you have to do some sorting to get a consistent list. I'm using screen position in this PR but even position might change here on resolution change if you're on a stretched layout, so leaving the resolution out of the display_name won't help as much (unless its the display at 0,0 but that is picked as the first one anyway). Also since we're using the screencast portal we might not even have all available screens (as they user can select them in a screencast-only session) to map them to real displays and do the display naming/matching based on whatever else which always would be a lot more complicated.

The description for id seems to suggest that the order may be stable since we are using a restore token. Have you checked to see if this remains stable when reinitializing? That may not solve Sunshine starting to the wrong screen (as we don't persist anything on cold start), but it may help for mid-stream resolution changes, at least.

I'll have a look at this but the descriptors might be initialised in a random order that would shuffle displays around again. I'm also curious what will happen when a screen gets added or removed. It might be consistent and persistent per portal token but might just be that and also not guaranteed to stay that way on resetting the token. Using position to sort the stream list was the most logical choice so far.

[Kishi85]

The description for id seems to suggest that the order may be stable since we are using a restore token. Have you checked to see if this remains stable when reinitializing? That may not solve Sunshine starting to the wrong screen (as we don't persist anything on cold start), but it may help for mid-stream resolution changes, at least.

I'll have a look at this but the descriptors might be initialised in a random order that would shuffle displays around again. I'm also curious what will happen when a screen gets added or removed. It might be consistent and persistent per portal token but might just be that and also not guaranteed to stay that way on resetting the token. Using position to sort the stream list was the most logical choice so far.

Could the resolution change be handled without a capture re-init? Because that's what is causing the display switch in the first place.

[Kishi85]

The description for id seems to suggest that the order may be stable since we are using a restore token. Have you checked to see if this remains stable when reinitializing? That may not solve Sunshine starting to the wrong screen (as we don't persist anything on cold start), but it may help for mid-stream resolution changes, at least.

KWIN isn't providing this id value at all in it's Session.Start response (as seen from simple capture with bustle):

"streams": <[(uint32 121, {"position": <(1920, 0)>, "size": <(1920, 1080)>, "source_type": <uint32 1>}), (119, {"position": <(0, 0)>, "size": <(1920, 1080)>, "source_type": <uint32 1>})]>

It's just providing position, size and source_type for each stream on a pipewire_node. We could probably do a differentiated name generation/matching based on what is available but at least for KDE the current behaviour is the only way that can be done consistently with what the Portal Session provides on metadata for streams.
Only thing (I can think of atm that would be) working for KDE is to not do a capture::re-init in case the resolution changes as that is causing a reset_display (destroying the current portal_display and initializing a new one but since it cannot match the old name it'll fall back to the first one) and just change the parameters for the running stream on the fly but I have no idea if this is possible and/or supported.

I'll continue trying to figure out the pipewire stream paused issue over the weekend when I have time for it and post further updates on my findings.

[Kishi85]

Looks like I've found the root of the greenscreen problem with the session invalidation by sheer luck and it's because the pipewire_fd is closed at the wrong point (in session_cache_t instead of dbus_t where it's essentially created by calling open_pipewire_remote). This was not a problem before but since this PR is using a separate dbus_t Portal for the display names enumeration the pipewire_fd's from that enumeration call never get closed and basically block the pipewire_fd that gets created after session cache invalidation.

TODO: Change the Portal call for display enumeration so that it does not use open_pipewire_remote (which is totally unnecessary for it as we just need the streamlist from Session.Start).

[Kishi85]

Looks like I've found the root of the greenscreen problem with the session invalidation by sheer luck and it's because the pipewire_fd is closed at the wrong point (in session_cache_t instead of dbus_t where it's essentially created by calling open_pipewire_remote). This was not a problem before but since this PR is using a separate dbus_t Portal for the display names enumeration the pipewire_fd's from that enumeration call never get closed and basically block the pipewire_fd that gets created after session cache invalidation.

This change by itself does not seem to be catching the full issue. Additionally invalidating the cache on every portal_display init so new streams are directly available does fix it however (f01ed84). Doing it this way does not change behaviour compared to current master as there the session cache gets invalidated on every pipewire::cleanup_stream resulting in recreating the session on every portal_display init as well. I'm still curious as to why the issue is occurring if we just invalidate on demand when a display cannot be matched to a stream.

EDIT: Since video.cpp is already ensuring that we only have one display active at the same time (hence never having multiple tray icons and it even destroys the current display for reenumeration of display_names) I'm wondering if we need the session_cache for dbus_t at all. It's currently also tracking if maxFramerate failed and the portal session has been closed by the user and for that it will still be necessary.

TODO: Change the Portal call for display enumeration so that it does not use open_pipewire_remote (which is totally unnecessary for it as we just need the streamlist from Session.Start).

I've tried this by adding a parameter to connect_portal but doing it that way breaks mouse input? I wonder why this is happening. No issue leaving it the way it is now though as the pipewire_fd will just get closed unused.

[Kishi85]

Checking if the session cache and display names from a new portal match and doing the invalidation should they not during the display_names enumeration leads to a working portal_display init with a working cache that is only invalidated when the screen configuration changes.

[sonarqubecloud]

Quality Gate failed

Failed conditions
1 New issue
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE