Dimidium Totius (in preparation for PipeWire camera and audio, part 1) #8153
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
GeorgesStavracas
added
Enhancement
6 tasks
kkartaltepe
approved these changes
Jan 29, 2023
RytoEX
requested changes
Feb 10, 2023
RytoEX
requested changes
Feb 10, 2023
There was a problem hiding this comment.
Commit message nits:
Remove the pipewire_node field, and just create the stream directly
from the passed now.
Perhaps missing a word? I feel like something is amiss.
- existance -> existence
- prespective -> perspective
- screencast -?> ScreenCast
- ying -> yin
Other than nits, and the previous review, I really don't have any items I'm worried about.
Instead of returning an opaque void* pointer, return the leaf C type obs_pipewire_data*. This commit introduces no functional changes.
When the linux-pipewire plugin is unloaded, make sure to cleanup the D-Bus proxy for the ScreenCast portal too. This effectively doesn't change anything, but it's always good to keep up with the code hygiene.
And let each portal pass the stream properties relevant to them. This makes the pipewire.c more independent of the actual portal by providing a wrapper to create a stream. This will be relevant later, when linux-pipewire introduces more portals like the Camera one.
This separation of obs_pipewire_create() and play_pipewire_stream() was an artifact of how the original code was written, and there is no reason to keep this separation anymore. Inlining it will help future commits too.
We don't actually need to restore the node id anymore, since this is handled by the screencast-portal.c code now. Remove the pipewire_node field, and just create the stream directly from the passed file descriptor now.
This commit introduces an important distinction in the initialization process that is essential to future camera and audio work: it splits creating and connecting to the PipeWire socket, from connecting to specific PipeWire nodes. Right now, for the only consumer in existence - the ScreenCast portal code - this distinction is irrelevant, but from an API perspective it just makes sense to model it this way.
Next commits will introduce new obs_pipewire_* types, so this renaming will make it slightly easier to read the code with different types.
A trivial cosmetic change. This groups the constructor and destructor together. Two sides of the same coin, yin and yang, bound by their nature, and linked by their position in code.
GeorgesStavracas
force-pushed
the
gbsneto/linux-pipewire-cleanups
branch
from
3a76148
to
0a6ef85
Compare
|
Thanks for the review. The commit messages were adjusted. |
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.
Description
Various cleanups in preparation for making
linux-pipewirehandle cameras and audio. This pull request contains the "easy" half of #6669 and should not be controversial.What is done here is:
Motivation and Context
Prepare the
linux-pipewireplugin to receive support for cameras and audio. This is also an effort to get #6669 unstuck.How Has This Been Tested?
Types of changes
Checklist: