pipewire: Destroy registry object with remote #1319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rmader
commented
Mar 31, 2024
src/pipewire.c
Outdated
| @@ -245,7 +219,10 @@ pipewire_remote_destroy (PipeWireRemote *remote) | |||
| pw_loop_destroy_source (loop, g_steal_pointer (&remote->roundtrip_timeout)); | |||
| } | |||
|
|
|||
| spa_hook_remove (&remote->registry_listener); | |||
There was a problem hiding this comment.
It's actually possible that the crashes in fbe2653 were caused by not adding this.
GeorgesStavracas
approved these changes
Apr 1, 2024
It looks good to me. |
GeorgesStavracas
added
bug
pipewire
rmader
force-pushed
the
fix-camera-portal-weirdness
branch
from
ed48897 to
5177150
Compare
|
Minor changes to silence compiler complaints, hopefully making the CI happy. Also some small wording changes in the commit message. |
|
This is showing up in CI: |
|
I think you can only call |
Oh wild. Maybe an issue with older Pipewire? Tests are passing fine locally: |
|
I think that's because CI doesn't have a running PipeWire server, so the code fails to create a PipeWire context and bails out early. I bet (hope) you have a working PipeWire setup on your computer 🙂 |
I tested that, too! And it seems to work fine on newer versions, but let me try. |
rmader
force-pushed
the
fix-camera-portal-weirdness
branch
from
5177150 to
6441dc9
Compare
This practically reintroduces commit fbe2653 which got reverted in ff8a8de, which I only discovered after coming up with this solution on its own. The main difference is that `discover_node_factory_sync()` gets removed as it doesn't really make much sense to keep around, as well small clean up improvements. The commit message of fbe2653 (by msizanoen1 <[email protected]>)> ``` Currently, the PipeWire registry object is destroyed when discover_node_factory_sync finishes its functionality. This causes registry events to not be delivered to the global_removed_cb and global_added_cb callbacks after discover_node_factory_sync, breaking the functionality of the org.freedesktop.portal.Camera.IsCameraPresent property in case of camera device hot(un)plug. This changes the discover_node_factory_sync function to store the registry in the PipeWireRemote structure and destroy it in pipewire_remote_destroy. ``` This holds true to this day and causes issues for the camera portal, most importantly causing a race condition where cameras that were not yet available during initialization would never get recognized. Whatever crashes it caused at some point I can't reproduce - and Gnome-Shell, which has a very similar implementation for its camera monitor doesn't seem to suffer from crashes as well.
rmader
force-pushed
the
fix-camera-portal-weirdness
branch
from
6441dc9 to
e3b5b7b
Compare
|
Looks like our investigation was correct 🙂 Merged |
Yours ;) Thanks! |
|
@GeorgesStavracas: given that this is a bug fix, what do you think about a 1.18 backport / 1.18.3 release? |
|
From Matrix:
Since 1.20 is not on the near horizon, a backport would probably be best. |
|
This seems to have re-introduced the crash for me that caused the original revert in ff8a8de I am on 7209054 |
grulja
added a commit
to grulja/xdg-desktop-portal
that referenced
this pull request
Aug 9, 2024
This is causing a crash with older PipeWire clients. Partly undoes flatpak#1319 which introduced this issue. Fixes flatpak#1407
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This practically reintroduces commit fbe2653 which got reverted in ff8a8de, which I only discovered after coming up with this solution on its own. The only difference is that
discover_node_factory_sync()gets removed as it doesn't really make much sense to keep around.The commit message of fbe2653 (by msizanoen1
[email protected])>
This holds true to this day and causes issues for the camera portal. Whatever crashes it caused at some point I can't reproduce - and Gnome-Shell, which has a very similar implementation for its camera monitor doesn't seem to suffer from crashes as well.
P.S.: the originally described issue:
does not describe the main problem that many people are facing: if Pipewire initializes cameras a little too late, the portal won't see them.
cc: @GeorgesStavracas, @msizanoen1