[GSoC] [WIP] PipeWire support - #16590
Conversation
pri-yan-shu
changed the title
github-actions
Bot
added
the
developer experience
| target_link_libraries(mixxx-lib PRIVATE PortAudioRingBuffer) | ||
|
|
||
| # PipeWire | ||
| default_option(PIPEWIRE "Enable the PipeWire backend" "LINUX") |
There was a problem hiding this comment.
| default_option(PIPEWIRE "Enable the PipeWire backend" "LINUX") | |
| default_option(PIPEWIRE "Enable the PipeWire backend" "UNIX AND NOT APPLE") |
| libshout-idjc-dev \ | ||
| libsndfile1-dev \ | ||
| libsoundtouch-dev \ | ||
| libsqlite3-dev \ |
There was a problem hiding this comment.
| libspa-0.2-dev \ | |
| libsqlite3-dev \ |
| libmsgsl-dev \ | ||
| libopus-dev \ | ||
| libopusfile-dev \ | ||
| libpipewire-0.3-dev \ |
There was a problem hiding this comment.
Please edit the fedora and flatpak recipes as well.
There was a problem hiding this comment.
Does flatpak need any change? I only see: REQUIRED_PACKAGES=("org.kde.Platform//6.10" "org.kde.Sdk//6.10")
There was a problem hiding this comment.
I am unsure. If it works already, fine. I was judging from:
| default_option(PIPEWIRE "Enable the PipeWire backend" "LINUX") | ||
| if(PIPEWIRE) | ||
| find_package(PkgConfig REQUIRED) | ||
| if(PKG_CONFIG_FOUND) |
There was a problem hiding this comment.
please move this to the cmake/modules folder. We need one file for libpipewire the libspa part can be probably be integrated?
There was a problem hiding this comment.
If detecting through pkg-config like here, libspa is automatically included, it is required package by libpipewire.
There was a problem hiding this comment.
It failed in my case Ubuntu Jammy (dropped for main, I know) I need to double check for others.
In any case we use these find modules.
There was a problem hiding this comment.
The libspa flags are in a separate PC file:
https://github.com/PipeWire/pipewire/blob/b5555cc3f650d534f80873a094f99defeb2ec897/src/pipewire/meson.build#L124
There was a problem hiding this comment.
Hmm, perhaps its not a required dependency in pipewire's PC file on some platforms (on my distro pkg-config gives flags for libspa as well along with libpipewire), we can add libspa separately. Do I keep the detection like how it happens currently, using pkg-config, or add a manual fallback as well?
There was a problem hiding this comment.
I have never understood why all of these find fils have the manual fall back.
In this case pkgconfig is part of the upstream project so we can skip the fallback IMHO
| PW_KEY_MEDIA_NAME, | ||
| }; | ||
|
|
||
| SPA_FOR_EACH_ELEMENT_VAR(name_keys, key) { |
There was a problem hiding this comment.
Luckily C++ has a native range based loop, better known by the reader.
| SPA_FOR_EACH_ELEMENT_VAR(name_keys, key) { | |
| for (const char* key : name_keys) { |
| m_hostAPI = "PipeWire"; | ||
| m_deviceId.name = name.data(); | ||
|
|
||
| qWarning() << "PipeWire device: " << name; |
There was a problem hiding this comment.
| qWarning() << "PipeWire device: " << name; | |
| qWarning() << "PipeWire device: " << name.data(); |
|
I have some commits here which were squashed in the soundmanager-refactor, is it okay if I force push here once? All current reviews have been addressed. |
|
Yes go ahead and rebase it on main. |
|
I experience a segmentation fault after restarting pipewire-jack and "querry devices" Not sure how to fix it. |
|
Commit 8fc429a solved crash like this on my local machine, but still there are test failures on CI, whereas no test failures on my local machine. |
|
We have an issue with the Flatpack build: And more. |
| s.remove('['); | ||
| s.remove(']'); | ||
|
|
||
| QStringList parts = s.split(',', Qt::SkipEmptyParts); |
There was a problem hiding this comment.
| QStringList parts = s.split(',', Qt::SkipEmptyParts); | |
| const QStringList parts = s.split(',', Qt::SkipEmptyParts); |
|
I have still a segfault when Querry Devices after messing with QJackCrtl. |
| static void callback(void* data, spa_io_position* pos); | ||
| static void callbackDrift(void* data, spa_io_position* pos); | ||
| static void callbackClkRef(void* data, spa_io_position* pos); |
There was a problem hiding this comment.
We move private static out of the class body into the anonymous namespace of the cpp file, to keep the class definition small.
Please prefix all pointer variables with p
There was a problem hiding this comment.
One issue is that since the member function these are calling are private, these functions need to be in class definition. Is there a way out, or I make the member functions public?
| NetworkEnumerator::NetworkEnumerator(UserSettingsPointer config, | ||
| SoundManager* sm) |
There was a problem hiding this comment.
| NetworkEnumerator::NetworkEnumerator(UserSettingsPointer config, | |
| SoundManager* sm) | |
| NetworkEnumerator::NetworkEnumerator(UserSettingsPointer pConfig, | |
| SoundManager* pSm) |
| PipewireEnumerator::PipewireEnumerator( | ||
| [[maybe_unused]] UserSettingsPointer pConfig, SoundManager* pManager) |
There was a problem hiding this comment.
| PipewireEnumerator::PipewireEnumerator( | |
| [[maybe_unused]] UserSettingsPointer pConfig, SoundManager* pManager) | |
| PipewireEnumerator::PipewireEnumerator( | |
| UserSettingsPointer, SoundManager* pManager) |
|
After merging #16544 we have conflicts. Can you rebase it again? |
|
The crash happens here: |
|
This is better in case of using queryDevices after restarting PipeWire server, although it will take work to get it working after a server restart. Also, currently there is a quirk while using Pipewire API, when restarting devices, on even times ports on Mixxx's node appear in jumbled order, and then testing hotplug crashes Mixxx. |
| } | ||
|
|
||
| std::vector<SoundDevicePointer> PipewireEnumerator::queryDevices() const { | ||
| std::vector<SoundDevicePointer> devices{}; |
There was a problem hiding this comment.
| std::vector<SoundDevicePointer> devices{}; | |
| std::vector<SoundDevicePointer> devices; |
{} is redundant
| auto device = QSharedPointer<SoundDevicePipewire>::create( | ||
| m_pConfig, m_pManager, this, id, name); | ||
| emit deviceAdded(device); | ||
| m_soundDevices.insert_or_assign(id, std::move(device)); |
There was a problem hiding this comment.
Is it correct that we overwrite the device, if we have already one? Please add a comment.
There was a problem hiding this comment.
Yes, each object is assigned a unique ID, so it will be removed beforehand if same ID is reused.
| } | ||
|
|
||
| void PipewireEnumerator::registryEventGlobal(uint32_t id, | ||
| [[maybe_unused]] uint32_t permissions, |
There was a problem hiding this comment.
| [[maybe_unused]] uint32_t permissions, | |
| uint32_t, |
| device->close(); | ||
| emit pEnumerator->deviceRemoved(device); | ||
| } else if (auto* port = std::get_if<Port>(&object)) { | ||
| pEnumerator->m_soundDevices[port->nodeId]->unregisterDevicePort(port->id, port->direction); |
There was a problem hiding this comment.
I assume that m_soundDevices[port->nodeId] is a null or dangling pointer.
| } | ||
|
|
||
| void DlgPrefSound::disconnectDevice(const AudioPath* path) { | ||
| for (auto* soundItem : m_selectedInputChannelIndices.keys()) { |
There was a problem hiding this comment.
| for (auto* soundItem : m_selectedInputChannelIndices.keys()) { | |
| for (auto* pSoundItem : std::as_const(m_selectedInputChannelIndices.keys())) { |
There was a problem hiding this comment.
did you mean this:
- for (auto* pSoundItem : std::as_const(m_selectedInputChannelIndices).keys()) {
+ for (const auto* pSoundItem : std::as_const(m_selectedInputChannelIndices).keys()) {There was a problem hiding this comment.
Ah that's probably a bit tricky. We have a const pointer pointing to non const.
So we want this:
| for (auto* soundItem : m_selectedInputChannelIndices.keys()) { | |
| for (const auto& pSoundItem : std::as_const(m_selectedInputChannelIndices.keys())) { |
This is a const reference to the non const pointer. "Von hinten durch die Brust ins Auge" a German phrase.
But that matches the best practice when using range based loops and the compiler does the optimum.
| } | ||
|
|
||
| void SoundDevicePipewire::writeOutput(float* output, int channel, int framesPerBuffer, int offset) { | ||
| for (auto i = m_audioOutputs.constBegin(), e = m_audioOutputs.constEnd(); i != e; ++i) { |
There was a problem hiding this comment.
this can become a range based loop
| const CSAMPLE* pOutputBuffer = &in.getBuffer()[offset]; | ||
|
|
||
| if (iChannelCount == 1) { | ||
| for (int i = 0; i < framesPerBuffer; i++) { |
There was a problem hiding this comment.
Here the sampleutil function shall be used.
|
|
||
| if (iChannelCount == 1) { | ||
| if (input) { | ||
| for (int i = 0; i < framesPerBuffer; i++) { |
There was a problem hiding this comment.
Same here, use sampleUtil functions for this and introduce missing once if not there.
There was a problem hiding this comment.
You mean
static void interleaveBuffer(CSAMPLE* pDest, const CSAMPLE* pSrc1,
const CSAMPLE* pSrc2, SINT numSamples);
and allocate zeroed buffer if missing? What do I use for allocation, alloca?
There was a problem hiding this comment.
I think you need a new similar function because the buffer is composed in two steps.
It the other channel is never update the buffer contains the old data. This is bad on one hand, but maybe less notable than silence.
There was a problem hiding this comment.
I saw one approach where if input buffers are not available we allocate one with alloca (which allocates on the stack only but is not standardized), zero it and then we can use it with interleaveBuffer.
There was a problem hiding this comment.
The logic is already good. We cant allocate buffers in the audio thread, because this is a locking operation. I jst meant to refactor out the copy loops. Than we have a nice name and we chan check for vectorization together with the other sampleutil functions whenever we refactor anything related.
| output[i] = pOutputBuffer[i * 2]; | ||
| } | ||
| } else { | ||
| for (int i = 0; i < framesPerBuffer; i++) { |
There was a problem hiding this comment.
Here as well, you may introduce a new one if it is missing.
| void writeProcess(SINT) override { | ||
| } | ||
| QString getError() const override { | ||
| return ""; |
There was a problem hiding this comment.
| return ""; | |
| return {}; |
|
Hotplug is working fine now, I haven't seen any crash with limited testing. I'm not too sure of the approach for concurrently accessing variables from callback thread, any suggestions are welcome. |
|
We typical use ControlObjects for this purpose. But it depends. Can you point me to the case? |
|
For instance in in to update common state from pipewire thread while callback RT thread is reading same vars. |
|
Do you have an idea why we have CI errors? Flatpak: clazy Ubuntu |
|
With the latest commits, using podman docker.io/library/ubuntu:24.04, I am passing all tests. |
|
The builds don't pick up the changes from last --fixup commit, is this normal? |
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
I have added some comments regarding the atomic<shared_ptr>
Form a practically aspect on a multi core CPU the solution is probably good enough.
However let's be strict and don't introduce locks.
I don't have a receipt yet. Only two ideas.
We need a pair of
- port.pPortData / pSoundDevice
For my understanding that can change at any time because of external patch bays.
Right? We cant stop the callback for reconfiguration like we do with portaudio.
SoundDevicePipewire pSoundDevice feels foreign, like an artefact form the past.
Do we actually have the Mixxx ports, somehow the inverse of a device?
This can be a static list, somehow the input and output items.
Will this also solve the solution to see Mixxx ports in the patechbay, even though they are not selected in the Mixxx preferences?
Can we have more port.pPortData than buffers in Mixxx? That's the part I have not yet understood.
So one Idea is to make it all static and rely on the routing in pipewire.
- Mixxx preferences controls the graph like an external patchbay.
- The callback just fills and read the buffers Mixxx has. (no dynamic allocation)
The other idea is to use a input cue or a double swap. This must be implemented in a way that only the gui thread deletes old objects.
We have implemented such a cue in the effect unit, for adding and removing effect controls.
This may also give you some ideas:
https://github.com/mixxxdj/mixxx/blob/main/src/util/borrowable_ptr.h
I
| void addDevice(const SoundDevicePointer pDevice); | ||
| void removeDevice(const SoundDevicePointer pDevice); |
There was a problem hiding this comment.
using const in that case is misleading and an C++ antipatter. No const value types in function declarations. You may use const in the function definition, but that's also misleading. In the case the value the pointer is pointing to is not const.
| void addDevice(const SoundDevicePointer pDevice); | |
| void removeDevice(const SoundDevicePointer pDevice); | |
| void addDevice(SoundDevicePointer pDevice); | |
| void removeDevice(SoundDevicePointer pDevice); |
|
|
||
| using SoundDeviceMap = std::unordered_map<uint32_t, QSharedPointer<SoundDevicePipewire>>; | ||
| std::atomic<std::shared_ptr<SoundDeviceMap>> m_soundDevices; | ||
|
|
There was a problem hiding this comment.
we have her:
- atomic
* shared controls structure
* pointer
|
|- Map
- key/value pair
- Shared pointer
- key/value pair
- Map
A plain shared pointer can be accessed across different threads, because its control structure is thread safe.
The only thing is which is not thread safe is to exchange the control structure, aka setting a new pointer to the same share pointer object. It is also not thread save to access the object the pointer is pointing to, unless is object is guaranteed to be const.
std::atomic<std::shared_ptr<>> does allow to exchange the pointer at runtime from different threads. This is however not an atomic operation in the sense of real time audio,
We may still have a race condition, because concurrent read and write access to std::unordered_map<> is not allowed form different threads.
| m_ppwRegistry(nullptr), | ||
| m_ppwMetadata(nullptr), | ||
| m_ppwFilter(nullptr), | ||
| m_soundDevices(std::make_shared<SoundDeviceMap>()), |
There was a problem hiding this comment.
Here a new pointer is added during the constructor. This is always OK, because no other thread has access yet.
| auto device = QSharedPointer<SoundDevicePipewire>::create( | ||
| m_pConfig, m_pSoundManager, this, id, name); | ||
| emit deviceAdded(device); | ||
| auto pSoundDevices = std::make_shared<SoundDeviceMap>(*m_soundDevices.load()); |
There was a problem hiding this comment.
Here we have a deep copy of the SoundDeviceMap, sharing the SoundDevicePipewire pointers.
| // pipewire assigns each object with a unique ID | ||
| // any previous element is either invalid or already removed | ||
| pSoundDevices->insert_or_assign(id, std::move(device)); | ||
| m_soundDevices.store(pSoundDevices); |
There was a problem hiding this comment.
Here we write back the changed SoundDeviceMap, potentially the unsafe action, here secured by the std::atomic, which is not lock free in this case.
| Object& object = pair.mapped(); | ||
|
|
||
| if (std::get_if<Node>(&object)) { | ||
| auto pSoundDevices = std::make_shared<SoundDeviceMap>(*m_soundDevices.load()); |
|
|
||
| qWarning() << "removing device:" << pDevice->getDisplayName(); | ||
| pSoundDevices->erase(id); | ||
| m_soundDevices.store(pSoundDevices); |
| m_pSoundManager->processUnderflowHappened(framesPerBuffer); | ||
|
|
||
| auto pOpenedDevices = m_openedDevices.load(); | ||
| auto pSoundDevices = m_soundDevices.load(); |
There was a problem hiding this comment.
Using auto here, is problematic, because you can't guess the type. In addition it is complicated code.
This is the probably locking call in the audio engine thread. atomic<shared_ptr> has the atomic API, but is not lock free.
You only increase the reference counter of the SoundDeviceMap so it does not fall out of scope when it is replaces in the GUI thread. However when this is the last instance of the share pointer because the GUI thread has replaced it. the engine thread will delete it at the end if this function. This must not happen, because delete() is also a locking function.
|
|
||
| for (auto& [id, device] : *pOpenedDevices) { | ||
| auto pSoundDevice = pSoundDevices->at(id); | ||
| auto& ports = device.inputs; |
There was a problem hiding this comment.
Here is also too much auto to understand the code on a review.
|
|
||
| if (iChannelCount == 1) { | ||
| if (input) { | ||
| for (int i = 0; i < framesPerBuffer; i++) { |
There was a problem hiding this comment.
The logic is already good. We cant allocate buffers in the audio thread, because this is a locking operation. I jst meant to refactor out the copy loops. Than we have a nice name and we chan check for vectorization together with the other sampleutil functions whenever we refactor anything related.
|
The code looks good. Please rebase and squash the commits. Is it easy to remove the Mixxx presentation form pipewire if ALSA is used? If yes it would be nice to have it fixed here. Else we can fix it later. |
|
The way currently preference page works, devices appear instantaneously when PipeWire API is selected, which requires preemptive initialization. Either PipeWire initialization is delayed till
Or as a compromise, PipeWire is deinitialized on preference page close, but that just delays the above UX till next Sound API switch. Or we can initialize/deinitialize directly on Sound API comboBox change. This is the cleanest approach. This issue is other way also, portaudio initializes if PipeWire used. |
9321901 to
0323184
Compare
since PipeWire SoundDevices will be using this property portAudioIndex is inappropriate name
Hotplug is for node/port creation/destruction. Since how pipewire objects show up on the registry, when a SoundDevice is created and the preference page is signalled, it has no ports, so it is not added to DlgPrefSound or any DlgPrefSoundItem (although currently the code is signalling on node creation anyway). On subsequent port registration is the SoundDevice added to the page.
Use Mixxx samplerate and buffer size for PipeWire filter
|
The one failing test fails on main as well. |
Yes this seems the best. We also don't want to touch Portadio if Pipewire is selected. |
|
You can always amend the final commit, no fixup! required. |
I have this working, but its a 250 line diff, so we can introduce this later. |
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
Just tested this one and it still work flawlessly.
Thank you very much for this great addition already.
|
All is fine, although one issue is, when selecting PipeWire API, and connect an input (like Microphone 1) which is not running (so for instance a paused music player), the input emits garbage buffer. It becomes fine once the input (the music player) starts running. |
Head branch was pushed to by a user without write access
|
Right, so I removed zeroing out input buffers, since manually pausing the input devices (like the music player) were handled fine without explicitly zeroing, maybe PipeWire sends a null buffer at pause which is copied to input buffer, but in case of startup while the program is paused, or abrupt disconnect (by patchbay), the same buffer is processed repeatedly. So we need the interleaved zero writes, even though they are needed for one engine iteration, as onwards the buffer is zero (I think). Apologies for the hassle. |
|
The CI failure are known unrelated issues. one id fixed here: #16693 |
| target_link_libraries(mixxx-lib PRIVATE PortAudioRingBuffer) | ||
|
|
||
| # PipeWire | ||
| default_option(PIPEWIRE "Enable the PipeWire backend" "UNIX AND NOT APPLE AND NOT ANDROID") |
There was a problem hiding this comment.
Please disable Pipewire by default!
The proposal is still not approved and thus we shouldn't make this feature available till we agreed we want to keep it such a way.
CC @mixxxdj/developers
There was a problem hiding this comment.
(just stumbled upon this because I don't have libpipewire-3, yet)
I agree. Default off until it's mature / safe/polished enough for test use.
There was a problem hiding this comment.
@ronso0 we require actually libpipewire-0.3
Was it just a typo and you have libpipewire-0.3 or is there a distro out there that does not provide it?
I am asking because then we may adjust the CMake code accordingly.
There was a problem hiding this comment.
Yes, libpipewire-0.3
Just set the flag to Off until I update my distro.
If I require it before that I'll find a way.
There was a problem hiding this comment.
Ah, no problem we can make Pipwire optional, depending on the existence of that file.
There was a problem hiding this comment.
Heads up this makes pipewire dependency optional, it is compiled in by default (helpful for CI) and pipewire features can be accessed by --developer flag. Is that fine?
4 participants
This implements support for PipeWire backend.
This is tracking this proposal, any project scope related discussion can happen there.
You can test this PR by selecting the PipeWire Sound API in the Sound Hardware preference page.
From the proposal, currently what is working: