PipeWire link hotplug - #16772
Conversation
Instead of internally copying the buffers, now connect both FL and FR channels of Mixxx input to the single channel of the SoundDevice. The behaviour is unchanged for outputs, now instead of checking the ChannelGroup for picking the port to output audio to, we query both ports for buffers, in case of single connection we only get buffer on one port.
f4f2d89 to
cd84efa
Compare
| CSAMPLE* getInputBuffer(const AudioInput& input) { | ||
| if (!m_inputBuffers.contains(input)) { | ||
| qWarning() << "getInputBuffer does not have" << input.getString(); | ||
| for (const auto& [input, buffer] : m_inputBuffers.asKeyValueRange()) { |
There was a problem hiding this comment.
Unfortunately this does not compile with QT 6.2
| for (const auto& [input, buffer] : m_inputBuffers.asKeyValueRange()) { | |
| for (auto it = m_inputBuffers.keyValueBegin(); it != m_inputBuffers.keyValueEnd(); ++it) { | |
| const auto& [input, buffer] = *it; |
|
Just did some test.
Just some wild thoughts (maybe bad) For the pachbay mode:
We may also just copy the Patchbay code into Mixxxx, it is also QT, because we will end up with pretty much the same if we do our own patch bay. But "There Can Be Only One", the "Higlander" issue. Do you know the movie: Default behavior of Mixxx, like Firefox, If Mixxx has no connection, connect default sink to Main output. Detect "exclusive mode" of the patch bay and gray out the Mixxx options in that case. Remove Pop up in case Mixxx is managed by a Patchbay. |
|
I kept the preference page, now we have a checkbox at the bottom (can be moved to a separate pipewire specific region). If it is checked, Mixxx UI tracks external links and updates accordingly, and all connections to any input/output path are handled. If unchecked, Mixxx UI is static, and external connections are untouched. There are still some edge cases, when checkbox is checked/unchecked while some connections are still made, and it enters invalid state, and preference page is not updated correctly (if it cannot be solved we can make this startup only value). Also currently routing multiple devices to single input/output is removed when using the preference page UI (Mixxx does not support multiple device routing anyway, but it works fine with patchbay only). |
|
Just tested a bit and got an assertion violation after removing a connection in the patch bay. I have connected the deck output with the patch bay, added the Main output via Mixxx and removed the main output again via the patch bay. I also noted a "flanger" effect. The deck has a phase offset compared to the main output. Not sure what is doing this. |
|
Do you have "Sync with external patchbay" checked on or off? I tried following your steps but couldn't recreate the assert violation. Do you have rest of the log?
Is this reproducible? I have also noticed this couple of times, even before this PR, when the sound is much wider than normal, maybe due to a delay. After disconnecting and reconnecting links it goes away, maybe its a PipeWire quirk. But I have only noticed it with Mixxx. |
|
I need to investigate the flanger thing. A first test would be to compare the output buffers of deck and main. |
|
Ups, I actually did not notice the switch. So I guess it was off. What is our strategy for such users like me? I am afraid we have the use cases not yet well thought. |
|
When the "Sync with external patchbay" checkbox is off (the default), Mixxx operates like it used to do, where the preference page is unaware of patchbay changes. One improvement is that on selecting devices, patchbay connections are untouched. When checkbox is on, Mixxx UI is synced with patchbay changes, and selecting devices affect patchbay links as well. Both modes should support editing connections from patchbay and Mixxx UI alternatingly, although changing the checkbox at runtime is not tested thouroughly, I'm debating if we make it startup only (like PipeWire API checkbox) and avoid some effort. I was unable to recreate the assert violation myself. It could be a specific case I missed. If you have logs it will help to pinpoint the cause. |
|
I think Mixxx can't prevent that a user uses the patch bay anyway. How about this start up flow.
In Mixxx we can have a new Cobobox:
If Mixxx detects any changes that does not match the current setting switch to "externally" Use cases:
|
|
About setting default output, it is the session manager's (wireplumber) responsibility to handle default input/output (which you manually set with Of the use cases you mention, the Ubuntu Studio analogue is currently "Sync with external patchbay" mode, where we sync the UI to external patchbay. If you turn that off, Mixxx handles like the 3rd case, where we use Mixxx UI and depend on Mixxx configuration file. Even when we get the "default output" working, that will work fine with "Sync with external patchbay", and user can opt in if they want to connect to default output. Is there any usecase this current setup missing? One point I can see is completely disabling Mixxx own config, but that can be a systemwide checkbox, not just PipeWire only. |
|
What's your plan with this PR? |
|
About the workflow you mentioned, we have these cases:
So we have a total of 3 checkboxes,
Is there any usecase that is not addressed? Or any reason you feel this is not optimal? |
|
I feel it should be more automated, cooperative. Thats why the timer idea comes up, with no check box. |
|
So the current state is:
Is this fine? |
|
The current Mixxx preference page behaviour to patchbay changes (only happens with sync patchbay checkbox on) is a bit confusing and undocumented, so I'll explain here, and provide feedback if it's good or not.
|
Unsure. This is the version where the "no sound configured" popup box will be retired. We loose the option to run Mixxx in broadcasting only mode. We have still a popup if users enable microphones with no mic configured. Maybe we want that for headphones as well. The alternative is to keep that Popup and do nothing on the first run like before. For now I prefer the proposed version. So we can implement it and decide during the beta if it was a good decision or not. |
|
All your considerations are well thought. Thank you. Our biggest challenge is however the timeline. If we try to fix all the nitty gritty details with the drafted cooperative mode, we might miss our schedule. Can we please focus only on these two simple modes, that likely cover 90 % of all use case?
Let's allow to connect additional ports with a patchbay after "Apply" but just ignore them in the Mixxx preferences. If users "Apply" again in Internal mode shall remove external connections again. This gives a solid base line for everything else. The mixed mode attempt is still nice for a future PR, but needs some love to have all corner cased right. |
That seems to be reasonable (in a future PR). Maybe something more generic because it can also be a crazy routing with single channel. Just to mention swapping FL and FR. |
|
now pipewire_patchbay_sync can be changed at runtime, and config is loaded anyway, just not applied if using external patchbay. |
|
Just tested this:
|
|
The rest work quite nice. |
|
One observation, when patchbay is active, even after removing the connections, patchbay reconnects, but we cannot do anything about it. Now all connections are removed, so even if internally the device configuration does not change, but externally connections were made, those connections are removed. |
That's OK it still feels natural. The whole PR work good now. I think the "Sync" checkbox should be moved up near the "Query Device". |
|
Pipewire works great now. Unfortunately storing the setting in ALSA mode is broken now. |
|
This is fixed, thanks. |
daschuer
left a comment
There was a problem hiding this comment.
First chunk of review comments:
| std::unordered_map<AudioOutput, PortPair> m_outputs; | ||
|
|
||
| PollingControlProxy m_audioLatencyUsage; | ||
| ControlObject m_COmanageExternalLinks; |
There was a problem hiding this comment.
The var name doe no longer match the CO name:
| ControlObject m_COmanageExternalLinks; | |
| ControlObject m_coPipeWirePatchbaySync; |
There was a problem hiding this comment.
Name needs to be same as ConfigKey? otherwise PipeWire is redundant here.
If need to be same, do we only capitalise at '_' in snake case form, so should it be m_coPipewirePatchbaySync.
|
|
||
| connect(this, &PipewireEnumerator::deviceAdded, m_pSoundManager, &SoundManager::addDevice); | ||
| connect(this, &PipewireEnumerator::deviceRemoved, m_pSoundManager, &SoundManager::removeDevice); | ||
| connect(&m_COmanageExternalLinks, &ControlObject::valueChanged, this, [this](double value) { |
There was a problem hiding this comment.
get() on a control object is already threadsafe and atomic. So we can dispose the m_manageExternalLinks bool and use directly the CO
| qWarning() << "PipewireEnumerator::initialize pw_context_new " | ||
| "failed with error:" | ||
| << spa_strerror(errno); | ||
| qDebug() << "PipewireEnumerator::initialize pw_context_new " |
There was a problem hiding this comment.
This, needs a source code comment. When can it happen? Is a qDebug message the right choice or do we want to inform the user better, along with a call for action?
There was a problem hiding this comment.
It fails if no valid pipewire config exists, or PW_KEY_CONFIG_OVERRIDE_NAME has invalid name (doesn't end in .conf) or it doesn't exist, or any pipewire function/libc function fails (like calloc/mmap), or config cannot be parsed. Create a popup with spa_strerror(errno) so user can see if the error is due to something they can control?
| qWarning() << "PipewireEnumerator::initialize pw_context_connect " | ||
| "failed with error:" | ||
| << spa_strerror(errno); | ||
| qDebug() << "PipewireEnumerator::initialize pw_context_connect " |
There was a problem hiding this comment.
The same here. If we don't expect that this ever fails, we can even user a VERIFY_OR_DEBUG_ASSERR()
There was a problem hiding this comment.
It can fail for many reasons, but one is if pipewire server is not running, so assert might not be optimal. Create a popup?
There was a problem hiding this comment.
If server is not running, spa_strerror gives: Unknown error -112. Not helpful, but probably because there might be multiple causes for this error.
|
|
||
| for (auto it = m_inputs.begin(); it != m_inputs.end(); ++it) { | ||
| it.value() = createInputPorts(it.key()); | ||
| createInputPorts(it->first, it->second); |
There was a problem hiding this comment.
This code is hard to read, because of using a Pair.
Is it possible to replace it with a struct or at least a Structured bindings?
| connect(&m_COmanageExternalLinks, &ControlObject::valueChanged, this, [this](double value) { | ||
| // TODO(pri-yan-shu) cleanup any invalid state here, or make the toggle startup only | ||
| m_manageExternalLinks = static_cast<bool>(value); | ||
| }); |
There was a problem hiding this comment.
I think the comment is now resolved, because the setting is grayed out when sync is disabled. Not sure if we want an extra safety net?
| << spa_strerror(res); | ||
| } | ||
|
|
||
| m_coreSyncSeq = pw_core_sync(m_pPwCore, PW_ID_CORE, 0); |
There was a problem hiding this comment.
Can this fail?
I have not fully understand how exactly it works. Maybe a source code comment will help.
There was a problem hiding this comment.
It is to synchronize configuration loading after pipewire device enumeration is complete, this makes pipewire server emit a callback after all the graph object registration (node/port/link) is complete. Documentation does not mention any failure.
|
Just did another test: Issues:
I think a reasonable choice would be "nothing" to have the "no Sounddervice configured" popup. |
|
Yes, currently it is using the mechanism of loading the first device, which doesn't make sense in PipeWire context. It is removed now. We will have a default device, and we will select only that (maybe lookup by name).
I couldn't recreate it, but now in one of the changes, the checkbox is only queried at startup, and then cached, so any changes to it should not matter. Can you test again? Is only one link connected in the patchbay? |
daschuer
left a comment
There was a problem hiding this comment.
The timer aka patchbay detection is not yet implemented, right?
This is OK for this PR. but than we should remove future artefacts of that feature.
The rest works and looks good now.
| void registerOutput(const AudioOutput& output, AudioSource* src); | ||
|
|
||
| private: | ||
| void patchbayWaitTimer(); |
There was a problem hiding this comment.
Is this function defined? I cannot find it.
Is this an artifact form a future PR?
There was a problem hiding this comment.
Its left from when I removed the timer. Timer can be added later, maybe in future there is no need for it (only patchbay sync mode).
daschuer
left a comment
There was a problem hiding this comment.
LGTM, thank you. Can you rebase the fixups? Than we can merge.
Detect any link creation to and from Mixxx node, and configure/unconfigure input/output accordingly.
Because PipeWire device enumeration is async, the configuration loading was happening before any devices were reported, which resulted in no config device being opened at all. Now PipeWire manually triggers config load after device enumeration is complete after initialization. For simplicity, SoundManager::loadConfig is called everytime PipeWire is initialized, effectively whenever "Query Devices" button is pressed.
If external patchbay connects Mixxx ports then skip loading config
…nfiguration For cases when no Mixxx device is configured, like when using PipeWire patchbay mode
|
Thank you. Can we remove the draft state now? |
|
The unit test fault is unrelated and tacked here: #16448 |
|
Thank you. |
Preemptively allocate all input buffers, and detect link connections and configure/unconfigure AudioSource/AudioDestination appropriately.