-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Audio] Add Pipewire playback/capture sink #4094
Conversation
This looks great! Regarding the points of contention...
Aside from that I did have a couple questions:
|
Putting it under Jack means that it will never start unless explicitly specified, as iterating through the audio driver array will get stuck on OSS, which seems to always be included in the Linux build and initializes successfully, but doesn't actually do anything on modern systems. Maybe it's time to move OSS further down the priority list as it's been deprecated since 2001? Unless an objection is raised by the steering committee in the next day or so, Fedora 34 is set to replace Pulse and Jack with Pipewire when it releases in a couple of months. I think Pipewire is going to be a Gnome dependency starting in 40, so odds are that other distros will start moving in that direction soon as well. I don't know if there will ever be a case where you can 100% count on Pipewire being the audio mixer, for as long as Pulseaudio exists, someone out there will be using it. I'd love to figure out a way to query if Pipewire is the system audio mixer without resorting to hacky methods such as looking for sinks/sources that aren't guaranteed to be present at application startup (for example, the only output device may be Bluetooth and not yet connected).
No, it's just used for node property strings and might show up in node editors or control panels. It doesn't affect the processing of the audio in any way.
Answered in the linked issue. Pretty simple, just query a string in the node properties. |
That all makes sense to me - we'll have to defer to @icculus for the driver ordering since there's probably a good reason it's ordered the way it is. I think detecting Pulse v Pipewire would be the only major worry for defaulting to Pipewire, but that's about all I can think of. Excited for F34 either way! |
Yes, but also we should just refuse to initialize OSS if there isn't a /dev/sound* device node at startup. Even though one could be added dynamically, I think it's a reasonable heuristic. (Alternately: does anything use Open Sound System anymore? Surely everything has moved to ALSA by now, if not a higher layer like PulseAudio...?!) I'm going to look at this patch shortly, but I haven't yet. |
FreeBSD might (not 100% sure though.) |
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 looks reasonable at first glance. The immediate and obvious nitpick is that we use One True Brace format:
if (x) { // brace on same line _even if it's a single statement inside the if block_
y();
}
I'll clean this up if you don't, but I'd appreciate it if you'd push another commit to change that. We'll sort out the proper order of audio backends once this is shipping in Fedora. After we're comfortable that this backend is stable, we'd likely move it to the top, to favor it over Pulse, if that's the direction distros are heading.
Sorry about that, force of habit. I'll take care of it. |
Style issues are fixed. I'll have another commit soon that should get it ready for #4109. |
…or sinks/sources Extend device enumeration to retrieve the channel count and default sample rate for sink and source nodes. This required a fairly significant rework of the enumeration procedure as multiple callbacks are involved now. Sink/source nodes are tracked in a separate list during the enumeration process so they can be cleaned up if a device is removed before completion. These changes also simplify any future efforts that may be needed to retrieve additional configuration information from the nodes.
Reworked the enumeration process to retrieve additional information (channel count and default sample rate). It just needs the SDL_AddAudioDevice() lines updated with the SDL_AudioSpec paramter to work with #4109 now. |
src/audio/pipewire/SDL_pipewire.c
Outdated
/* No intermediate stream, call the application callback directly */ | ||
if (!SDL_AtomicGet(&this->paused)) { | ||
SDL_LockMutex(this->mixer_lock); | ||
this->callbackspec.callback(this->callbackspec.userdata, src, this->callbackspec.size); |
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.
Recording like this won't work. The latency specified in PIPEWIRE_NODE_LATENCY only guarantees a minimal latency to the app. The actual latency might be lower depending on the graph. For example, just opening pavucontrol or recording from the same source from a different app that request a lower latency will lower the latency of the graph enough, that chunk->size < this->spec.size is always true and we get broken recording. We probably need to put an additional ringbuffer in between.
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.
Ah, I see. Rewriting it to always buffer the audio is simple enough, and it's just a bit of overhead from copying some bytes around. I wanted to do some work on that function anyways since I realized I'm ignoring the n_datas member of spa_buf. Not an issue in the output callback as we only use one buffer, but there is no guarantee of that when dealing with incoming audio.
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 might be wrong, but i''m pretty sure the n_datas member will always be 1 when you request an interleaved audio format, so it should be fine. It would be different with planar audio formats, but since SDL doesn't support those, we don't have to care.
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.
That's what it sounds like in the documentation, although it's a little vague.
Each data block is, for example, a video plane or an audio channel. There are n_datas of those blocks.
This is where it would be nice to have a detailed spec stating something like
If the audio sample format is interleaved, n_datas will always be 1
Probably not worth worrying about though.
The audio callbacks should pass the callbackspec.userdata parameter to the callback, not spec.userdata Co-authored-by: Oschowa <Oschowa@web.de>
One idea to figure this out might be to just do an initial round of device detection in PIPEWIRE_Init, and fail initialization if we don't get at least one node that's 'Audio/Sink' or 'Audio/Source'. That should be a pretty clear sign that pipewire handles audio on the system.. Edit: actually I'm not sure that true, disregard this until I can maybe confirm it. |
The latency of source nodes can change depending on the overall latency of the processing graph. Incoming audio must therefore always be buffered to ensure uninterrupted delivery. The SDL_AudioStream path was removed in the input callback as the only thing it was used for was buffering audio outside of Pipewire's min/max period sizes, and that case is now handled by the omnipresent buffer.
This is the obvious solution, however there are many cases where it could fail, such as where the only sink devices are an HDMI device that's powered off at application startup or a disconnected Bluetooth speaker. I was thinking that looking for ALSA or Bluez factory objects in general might be more reliable, but even that seems sketchy since it's still making assumptions about the system underneath. |
Probably best to ask on the pipewire gitlab or IRC directly and move it below Pulse/ALSA/JACK for the time being if there is no clean solution. Otherwise this looks good to me now and also works fine in testing. Nice. |
Further refactor the device enumeration code to retrieve the default sink/source node IDs from the metadata node. Use the retrieved IDs to sort the device list so that the default devices are at the beginning and thus are the first reported to SDL.
…escriptive Rename the add/remove/clear list functions and rename connected_device to io_node, as a sink/source node isn't necessarily a device.
Move the Pipewire audio driver below others in the list so it won't be mistakenly initialized when it's not the system mixer.
Use the 'R' (rear) prefixed designations for the rear audio channels instead of 'S' (surround). Surround designated channels are only used in the 8 channel configuration.
Solely FreeBSD nowadays use Open Sound System. |
Replace "magic numbers" with #defines, explain the requirements when using the userdata pointer in the node_object struct and a few other minor code and comment cleanups.
The Arch wiki has instructions on how to set it up on Linux. I doubt that any significant number of people are using it, but one of the universal certainties of Linux is that if you can use it, someone is using it. |
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.
Only thing that's missing at this point is the vim lines at the bottom of the new files (and the style described inside):
/* vi: set ts=4 sw=4 expandtab: */
That's all style though - functionally it looks good!
@@ -113,6 +116,7 @@ static const AudioBootStrap *const bootstrap[] = { | |||
#if SDL_AUDIO_DRIVER_DUMMY | |||
&DUMMYAUDIO_bootstrap, | |||
#endif | |||
|
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.
Extra space
With the latest changes and cleanups I'm pretty happy with the code at this point, so marking as ready for final review.
|
I think it is wiser to land this PR first into SDL2. #4109 isn't finished yet. It seems to be far from finished too. |
Added the lines and fixed indentation. I had my format script set to 2 instead of 4 indentation spaces, so it's a big update, but it's all style fixes. |
Increase indentation spacing from 2 to 4 to comply with style standards.
Pipewire audio playback and capture
This adds a Pipewire playback/capture sink for the audio subsystem. Pipewire is designed to be suitable for both typical desktop and low-latency audio applications and is slated to replace both Jack and Pulseaudio, while providing compatibility layers, in Fedora 34. Pipewire has measurably lower overhead than Pulseaudio and streams can be routed with standard Jack graph processing tools such as Catia and QJackCtl.
Barring an application requesting extremely short or long mixing periods (0.67 to 170ms at 48KHz), in which case an intermediate stream is required, when a Pipewire stream callback fires the buffers are simply passed directly through to the application mixing callback, so overhead is minimized as SDL is little more than a shim in this case. Filling and submitting the buffer is a zero-copy operation, and, even if the input/output device is disconnected, the application callback will continue to fire at regular intervals as the Pipewire streams are created with the PW_KEY_NODE_ALWAYS_PROCESS flag. Note that this requires a fairly recent version of Pipewire as the audio mixing has only been considered ready for public testing in the last couple of months and the headers for versions earlier than 0.3.18 caused some warnings when building SDL. Pipewire 0.3 is considered API stable at this point.
Points of contention