-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PipeWire-based Camera #5728
PipeWire-based Camera #5728
Conversation
The only localization file you need to edit is the English one -- all the others will be handled by CrowdIn, including the string deletions. The parlance we currently use is "Video capture device" rather than "Camera" or "Webcam", since the device could be a capture card rather than an actual camera. (Yes, there's an argument to be made about using the term "camera"/"webcam" for the same of understandability, but that discussion is out of scope for this PR IMO -- instead, for the time being I think we should maintain consistency in terminology.) |
Thanks for your note!
The localization stuff is ... sigh .. an artefact/rebase error of #5394 and will disappear before this will be marked as ready. Since #5394 will probably wait for the big cmake change (Tell me if someone wants to merge it earlier) I would like to change that at that time.
"Video capture device" is fine for me. This uses the official "Camera portal", but I guess that includes other capture devices? This might be changed to use the "device portal" if that becomes the way to go. @GeorgesStavracas: If there are restriction on what devices this portal will announce, please suggest a more fitting name ;) |
5a0164a
to
437dcca
Compare
About this: I'd be interesting how such a capture device is handled by the linux kernel. Is it just treated as another v4l2 device? The reason I'm asking is that we can use the more optimized code path if we don't have to deal with YUY2 et. al.. Sadly I don't have a capture card to test it. |
77d8dc5
to
dbce8fd
Compare
dbce8fd
to
a23c103
Compare
@GeorgesStavracas We separate camera plugin and registry.
An alternative would be to not lazyload the registry, but start it when obs is launched. This way we can only add the camera plugin, if obs is allowed to access the portal. If we lazy load I would suggest to work asynchronous with callbacks from the registry to the plugin after it's initialized, but that's just some idea. Anyways: should I just continue to rebase the current state, or should I continue with my new proposal? |
In principle, I think separating different moving parts into their own unit is good. My idea with #6645 is that all the code under I don't know if it would be feasible or even possible, but I think you can already make this
That way the camera portal code will be able to react to PipeWire adding and removing cameras, without having to deal with PipeWire itself. What do you think?
I think the best approach here is to do what I've done with the screencasting code, and completely separate camera portal code from PipeWire itself. What the plugin needs to do is to expose the pw-camera sources all the time, and only query the Device Access portal when the user actually adds a pw-camera source. If camera access is denied, we show an error message in the properties dialog.
Considering what I've suggested above, feel free to continue with your new proposal |
Moved this discussion to #6669 |
@GeorgesStavracas I create the registry when the plugin is loaded, I didn't want to deal with lazy loading once for now. The registry stuff works pretty nice, but creating a new camera source will hang obs if you create a new plugin and obs creates the property and starts connecting to the camera. Does this look like the right direction for you? |
a23c103
to
e6b6e9e
Compare
e6b6e9e
to
c8374e6
Compare
Me and @columbarius discussed this yesterday and today about this PR, and we agreed on me pushing commits over. This is my iteration of the work I've been meaning to do for a while. There's quite a large preparation in the first 19 commits, and the last commit actually implements the Camera portal integration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedef struct _obs_pipewire obs_pipewire; | ||
typedef struct _obs_pipewire_stream obs_pipewire_stream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that names that begin with underscore are reserved, though I might be misunderstanding the "reserved names" section of the C standard. @kkartaltepe may understand it more than I.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any kind of name reservation mechanism in C. The C language has reserved keyword (if, else, while, ...), and the standard types and functions of libc
must not be overriden by application code, but I don't think there's anything preventing applications from defining structs like here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the following regarding identifiers that begin with an underscore:
- https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html
- https://en.cppreference.com/w/c/language/identifier
I generally would opt to just avoid starting any identifier with an underscore just to avoid having to wonder if it's acceptable according to the standard.
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.
This commit makes the stuff in pipewire.c more independent from the actual plugin by providing a wrapper to create a stream with a set or properties defined by the plugin.
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 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 existance - the screencast portal code - this distinction is irrelevant, but from an API prespective 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.
This will make code easier to follow after next commit.
Sorry, this is a painful commit to review :( Until now, the only consumer of the PipeWire code is the screen cast portal code. This portal code only ever has one PipeWire connection, and one PipeWire stream, every time a monitor or a window is selected. This is reflected on obs_pipewire, which only handles a single stream for any given connection. For cameras and audio, however, a single PipeWire connection can and most likely always will provide multiple streams. For example, computers with multiple webcams will have one PipeWire connection reporting multiple PipeWire nodes, one node for each camera. This commit breaks this one-stream-per-connection assumption, and makes each call to obs_pipewire_connect_stream() return a completely separate stream object. A new structure, obs_pipewire_stream, is introduced to represent a PipeWire stream. The screencast portal code continues to only ever have one connection and one stream, and that's made explicit in the code.
Make obs_pipewire hold a list of streams created by itself, and destroy these streams when closing the connection.
We'll need to peek information about the source to determine how to process frames, so store it for later usage.
So far we've been treating format info on a per-connection basis, but now that a single connection is capable of hosting multiple streams, and each stream might negotiate a different format, it is necessary to move format info to each stream individually.
And use it instead of exhaustively listing each field of the anonymous struct into the arguments of the lookup function.
The media API requires it's own format (enum video_format) and bytes per pixel (bpp).
Cameras and audio streams will use the async output routines, but not screencasting. This needs to be handled differently in the processing callback.
This will be reused by the camera portal code.
This will allow consumers of obs_pipewire to handle the device registry themselves. This is not useful for the screencast code since there's always only one node in the registry, but it will be used by the camera and audio code to list hardware devices.
Generalize the async server roundtrip wait so that future consumers can, for example, wait for all devices to be listed before acting on the registry.
This plugin adds a camera source for capture devices available via PipeWire using the Camera portal.
c8374e6
to
282ee75
Compare
(gpointer *)&device)) { | ||
const char *device_name; | ||
|
||
device_name = pw_properties_get(device->properties, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a PipeWire issue, but for now, we should use more than PW_KEY_NODE_DESCRIPTION
. This value is for my webcam "0807" for some reason. I'd suggest maybe adding the object.path Property to that string (Maybe descr (path)
) which is sth. like v4l2:/dev/video0
. This would also allow people to notice why a webcam used by PipeWire can't be accessed via v4l2 from another application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also allow people to notice why a webcam used by PipeWire can't be accessed via v4l2 from another application.
Is that really a worry? My understanding right now is that people expect the camera to only be used by one application at a time, the concept of having several apps use the same camera at the same time is completely foreign to them and might not even cross their mind.
device_id = spa_dict_lookup(props, SPA_KEY_NODE_NAME); | ||
|
||
device = camera_device_new(id, props); | ||
device->proxy = pw_registry_bind(registry, id, type, version, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason, why we create a proxy for each PipeWire? I think we don't need that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what pw-dump does, and apparently tracks the life of each device individually
Having no success running obs-studio wrapped with |
This is obsoleted by #9771 |
Description
This MR allows users to add a webcam as video source via PipeWire.
Motivation and Context
PipeWire is the more modern multimedia framework for linux and other unix systems. One improvement over v4l2 is that PipeWire allows multiple clients to access the same device.
How Has This Been Tested?
Webcam (PipeWire)
Types of changes
Checklist:
Requires and contains: #5394
Disclaimer:
This is a somewhat early WIP from @GeorgesStavracas and me. While there are still some very rough spots (including bad performance and probably crashes) I think the changes in the shared code base are worth discussing.
Additionally this should avoid parallel development.