Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preparations for Portals + PipeWire camera, part 2 #9103

Conversation

GeorgesStavracas
Copy link
Member

@GeorgesStavracas GeorgesStavracas commented Jun 18, 2023

Description

This is the second batch of commits from #6669, rebased on top of master and adjusted to the most recent changes.

The biggest change that this round of preparations introduce is making the PipeWire code able to handle async sources. There are no actual async sources that use PipeWire, but this is such a big change that I reasoned it's better reviewed independently from the next - and even bigger - reworks.

There should be no functional changes to the screencapture code.

Motivation and Context

I don't want code reviewers crying in a corner when reviewing the PipeWire camera merge request, so let's go incremental and chop choppy and handle this massive amount of code in digestible chunks 馃檪

How Has This Been Tested?

  • Run OBS Studio on Linux
  • Add a PipeWire capture of any kind
  • It should continue to work as it used to before

Types of changes

  • Tweak (non-breaking change to improve existing functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

GeorgesStavracas and others added 6 commits June 18, 2023 14:27
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 obs_pipewire_connect_stream() return an independent object
(obs_pipewire_stream) that represents a single stream.

The screencast portal code continues to only ever have one connection
and one stream.
Make obs_pipewire hold a list of streams created by itself, and
destroy these streams when closing the connection.
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 open coding 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).
We'll need to peek information about the source to determine how
to process frames, so store it for later usage.
@GeorgesStavracas GeorgesStavracas added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable Linux Affects Linux labels Jun 18, 2023
@GeorgesStavracas GeorgesStavracas added the Seeking Testers Build artifacts on CI label Jun 18, 2023
columbarius and others added 3 commits June 19, 2023 09:22
Cameras and audio streams will use the async output routines, but
not screencasting. This needs to be handled differently in the
processing callback.

As it is, the code is starting to get a bit messy, as we're now
dealing with two different frame processing approaches. Later on,
this code will be cleaned up and split in more logical pieces.

Co-authored-by: Georges Basile Stavracas Neto
<georges.stavracas@gmail.com>
The D-Bus signal subscription is a very boring aspect of the
portals infrastructure that shouldn't really matter when reading
the code of pipewire.c.

Move it out to its natural place, portal.c.
@GeorgesStavracas GeorgesStavracas force-pushed the gbsneto/pipewire-unified-wrappers-part2 branch from 8013f88 to 88f686a Compare June 19, 2023 12:22
@Penwy
Copy link
Contributor

Penwy commented Jun 19, 2023

Ubuntu 22.04
libpipewire 0.3.48
kernel 5.19.0-43-generic

No regression noticed as of latest commit.
Fixes #9049

This is a workaround to the compiler complaining about directive
arguments being null.
@GeorgesStavracas GeorgesStavracas force-pushed the gbsneto/pipewire-unified-wrappers-part2 branch from 88f686a to f8d2594 Compare June 20, 2023 01:06
@dimtpap
Copy link

dimtpap commented Jun 20, 2023

Would you like me to start working on a version of #6207 that uses the wrappers?

@GeorgesStavracas
Copy link
Member Author

Would you like me to start working on a version of #6207 that uses the wrappers?

Eventually, but I don't think it's worth investing that time on this iteration of the wrappers. I think it'll be worth for you to rebase on top of a (future) part 3 of the PipeWire cleanups, but I'm still working on it.

@Lain-B Lain-B self-requested a review June 24, 2023 23:10
@GeorgesStavracas
Copy link
Member Author

Let's go \o/

@GeorgesStavracas GeorgesStavracas merged commit bdfead8 into obsproject:master Jul 15, 2023
12 checks passed
@GeorgesStavracas GeorgesStavracas deleted the gbsneto/pipewire-unified-wrappers-part2 branch July 15, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Enhancement Improvement to existing functionality Linux Affects Linux Seeking Testers Build artifacts on CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when selecting a window in pipewire capture if one is already selected
4 participants