improve detection of sound hardware from configuration #2253

Be-ing · 2019-09-01T00:51:15Z

PortAudio provides no guarantees that the PaDeviceIndex for any device will be stable across restarts of Mixxx. Abusing the PaDeviceIndex as a persistent identifier caused Mixxx to falsely claim that audio interfaces were not available and annoyingly require the user to reconfigure all their sound I/O even when nothing about the available sound hardware actually changed. It is the responsibility of the sound API to provide persistent names for devices to PortAudio.

I have tested this with JACK and ALSA and it works as I expect.

PortAudio provides no guarantees that the PaDeviceIndex for any device will be stable across restarts of Mixxx. Abusing the PaDeviceIndex as a persistent identifier caused Mixxx to falsely claim that audio interfaces were not available and annoyingly require the user to reconfigure all their sound I/O even when nothing about the available sound hardware actually changed. It is the responsibility of the sound API to provide persistent names for devices to PortAudio.

daschuer

Thank you for fixing this annoying bug.

Unfortunately this is not a one liner, because it breaks backwards compatibility for every user.
We must also make sure, that in case of two devices with the same name, Mixxx does not open the same device twice.

Portaudio is missing a Unique ID for the devices but I guess that in case of two devices with the same name, the index will be shifted but the order will not be swapped. Here an old discussion:
https://app.assembla.com/spaces/portaudio/tickets/11-add-callback-api-for-detecting-connection-removal-of-devices/details#

Currently the user can only distinguish it by the place in the combobox. So I think we finally still need to use index during one PA device enumeration. However I am fine to just ommit the index when loading the config and ignore this rare case for now as long maintain backward compatibility.

Relevant Code is here:

mixxx/src/preferences/dialog/dlgprefsounditem.cpp

Line 261 in 9da873a

int index = deviceComboBox->findData(deviceName);

mixxx/src/preferences/dialog/dlgprefsounditem.cpp

Line 59 in 9da873a

    
           void DlgPrefSoundItem::refreshDevices(const QList<SoundDevicePointer>& devices) {

mixxx/src/preferences/dialog/dlgprefsounditem.cpp

Line 225 in 9da873a

int newDevice = deviceComboBox->findData(m_savedDevice);

mixxx/src/soundio/soundmanager.cpp

Line 365 in dd5de6b

m_config.getInputs().values(pDevice->getInternalName())) {

mixxx/src/soundio/soundmanager.cpp

Line 388 in dd5de6b

m_config.getOutputs().values(pDevice->getInternalName());

daschuer · 2019-09-01T06:20:12Z

src/soundio/sounddeviceportaudio.cpp

@@ -107,8 +107,7 @@ SoundDevicePortAudio::SoundDevicePortAudio(UserSettingsPointer config,
    // Setting parent class members:
    m_hostAPI = Pa_GetHostApiInfo(deviceInfo->hostApi)->name;
    m_dSampleRate = deviceInfo->defaultSampleRate;
-    m_strInternalName = QString("%1, %2").arg(QString::number(m_devId),
-            deviceInfo->name);
+    m_strInternalName = deviceInfo->name;


Unfortunately this will one time break the sound hardware hardware config of every user.
I am afraid we need to deal with the index, in case we have two sound cards with the same name.

Please also add a note to the code, that we cannot use QString::fromLocal8Bit(deviceInfo->name); here to not break the backwards compatibility.

Can you also rename m_devId to m_deviceIndex, to clarify that it is not an Identification Number.

Even now I often have to reconfigure my settings depending on which devices are found. I don't think that this fix causes any severe usability issues other than that we already have.

This is a Linux only issue, where devices in use are not enumerated.
After merging this PR, all users have to reconfigure their Hardware preferences.
This is a bad idea since the minority of our users are on Windows.

No. You are wrong.
This has been happening on Windows too.
That index changes, sometimes every reboot.
I assume that with "backwards compatibility" you mean when loading on an older Mixxx the configuration created with this PR.
That might be worth considering for development and people trying before upgrading. In fact, keeping compatibility could beneficiate the newer version more than the older one, in this case. So yes, trying to ignore the index on load rather than not saving it could be a better solution, and documenting why.
Finally, there is still that problem that you mention wih the name and "hw:xx". What do you propose?

I mean: on Linux the issue is worse, because it also happens without reboot, if a device is in use by an other application (pulse).

Finally, there is still that problem that you mention wih the name and "hw:xx". What do you propose?

We need a special case that excludes the hw:x,x and the index if not found. How about looking for the full string fist and if not found use the inner part only.

Mixxx users are accustomed to having to reconfigure their sound hardware frequently, so I am not concerned about making them do it once more.

I am very certain nobody minds to reconfigure their audio config. That's probably the task which all users do regularly (switching equipment) and it's easy.

daschuer · 2019-09-01T07:28:35Z

It is the responsibility of the sound API to provide persistent names for devices to PortAudio.

That is correct, but unfortunately it is not guaranteed across re-enumerations and after the the hardware config has changed (Hot Plug and Play)

daschuer · 2019-09-01T07:36:59Z

daschuer · 2019-09-01T07:39:48Z

You see, that by unplugging the USBMIC, the name of SAA7134 has changed.
A fix for Mixxx will involve omitting PaDeviceIndex and the alsa index (hw:x,x)

daschuer · 2019-09-01T11:56:23Z

It is correct, that the Device name is constant per enumeration as well as the index.

The index does not add any value with ALSA where the the more reliable hw:x,y index is already part of the name. It not unique across reboots though.

On Windows there is no such unique index. Without the own artificial index Mixxx cannot identify two similar devices even during a single enumeration run because Mixxx uses a QMap that looses the original order.

So I propose a solution that keeps the index.

JosepMaJAZ · 2019-09-01T12:31:22Z

Do you mean that the problem is worse on Windows because of a QMap?

daschuer · 2019-09-01T14:29:54Z

Yes, but only in the rare case of using two devices of the same model. In this case and without PaDeviceIndex in the name, the QMap will only return one of the two devices.

Be-ing · 2019-09-01T15:20:44Z

A fix for Mixxx will involve omitting PaDeviceIndex and the alsa index (hw:x,x)

Then we would have nothing to distinguish two identical audio interfaces. With this branch in its current state, Mixxx can reliably distinguish between my two Xone K2 audio interfaces as long as their ALSA indices remain consistent. Unfortunately I don't know of any way to tell ALSA to keep consistent indices for two identical devices. It might be possible with a udev rule distinguishing the devices by serial number.

So I propose a solution that keeps the index.

So do you propose storing both the PortAudio index and ALSA hw index and falling back to just the name if those indices do not match?

daschuer · 2019-09-01T16:15:51Z

Yes.

In the ALSA case we may completely ignore the PaDeviceIndex, since it has its own index and we need to ignore the ALSA index if no matching device is found.

In the windows case we need to use the PaDeviceIndex. For the other APIs we have to decide in the same way. Not a thing we expect from an API wrapper like Portaudio, but it is a known bug without progress unfortunately. (linked above)

This allows for more reliable identification of hardware devices from the configuration file so users do not need to reconfigure their sound hardware as often as before.

Be-ing · 2019-09-30T02:19:28Z

In the ALSA case we may completely ignore the PaDeviceIndex, since it has its own index and we need to ignore the ALSA index if no matching device is found.
In the windows case we need to use the PaDeviceIndex.

Done. That turned out to be tricky, but it was a good learning experience for using custom types with QVariant.

Be-ing · 2019-09-30T02:20:11Z

Testing on Windows and macOS would be appreciated.

Be-ing · 2019-09-30T03:38:34Z

I have tested the following and they all work:

Configuring one ALSA device, quitting Mixxx, replugging USB audio interfaces in a different order so the ALSA device numbers change (and restarting Linux), restart Mixxx. Mixxx is able to identify the configured device regardless of the ALSA device number.
Configuring non-hw devices with ALSA ("default" and "pulse") and restarting Mixxx.
Configuring two ALSA devices with identical names and restarting Mixxx. Mixxx is able to identify the configured devices so long as the ALSA device numbers remain the same.
Configuring Mixxx to use JACK and restarting Linux so the PortAudio device indices for JACK devices change (verified by looking at ~/.mixxx/soundconfig.xml).
Configuring the same input device and same input channels for multiple decks' vinyl control inputs.

It isn't really a name, just an ID that ALSA defines.

daschuer

Thank you for fixing this finally.
Unfortunately, you have introduced a new storage file format. This is a unessary regression. Because the original format already included all info. You just need to reuse the name parsing function.

Does the alsaHwDevice contain the whole Alsa name or only the suffix? I think we may use the whole name in addition to name. This way it we don't need to special case "default" and it is immune against future issues with the used regex.

daschuer · 2019-09-30T16:11:08Z

src/soundio/soundmanagerutil.h

+  public:
+    QString name;
+    QString alsaHwDevice;
+    int portAudioIndex;


Please give an example as comment what is stored where.

As the name implies, alsaHwDevice only stores the hw:X,Y from the name PortAudio gives Mixxx. For non-hardware ALSA devices, alsaHwDevice remains empty.

src/soundio/soundmanagerutil.h

Be-ing · 2019-09-30T18:43:03Z

Unfortunately, you have introduced a new storage file format. This is a unessary regression. Because the original format already included all info. You just need to reuse the name parsing function.

Ehh, using a regular expression to parse a string from an XML file feels hacky to me. IMO it defeats the point of using XML. I'd rather not do this to avoid a minor one time disruption when upgrading to 2.2.3. As discussed before, users are already accustomed to having to reconfigure their sound hardware often. Making them do it only one more time is no big deal.

uklotzde · 2019-09-30T19:13:55Z

src/mixxxapplication.cpp

@@ -65,6 +66,8 @@ void MixxxApplication::registerMetaTypes() {
    qRegisterMetaType<mixxx::ReplayGain>("mixxx::ReplayGain");
    qRegisterMetaType<mixxx::Bpm>("mixxx::Bpm");
    qRegisterMetaType<mixxx::Duration>("mixxx::Duration");
+    qRegisterMetaType<SoundDeviceId>("SoundDeviceId");


The string argument is not needed if symbols are part of the global namespace

Yes, I thought it was good to include it for consistency with the surrounding code.

src/preferences/dialog/dlgprefsounditem.cpp

src/soundio/sounddeviceportaudio.cpp

src/soundio/soundmanagerconfig.cpp

src/soundio/soundmanagerconfig.h

uklotzde · 2019-09-30T19:25:01Z

src/soundio/soundmanagerutil.h

+    QString alsaHwDevice;
+    int portAudioIndex;
+
+    QString debugName() const {


Or overload QDebug operator<<(QDebug, const DeviceId&)?

Ok, the overload already exists. Those multiple representations are slightly confusing.

I tried defining only the operator<<, but there are use cases for this debugName in the name for Tracers in SoundDevicePortAudio. Do you have suggestions for another way to handle this?

src/soundio/soundmanagerutil.h

uklotzde · 2019-09-30T19:30:33Z

No concerns from my side. Reselecting the sound device after an upgrade is acceptable if it avoids complicating the code.

daschuer · 2019-09-30T21:36:52Z

It does not really complicate the code, we have the name parser already.

Please remember that only a minority of user suffers this bug. With an incompatible change, all users have to reconfigure the hardware. Reconfigure the hardware preferences is not a no brainer. This is a no-go in a point release for me.

A migration would work for me, but that is IMHO not worth the work just to avoid a reg expression we use anyway in the code.

Be-ing · 2019-09-30T22:41:05Z

Please remember that only a minority of user suffers this bug.

AFAIK this affects every user. @JosepMaJAZ confirmed above that it affects Windows.

Reconfigure the hardware preferences is not a no brainer.

Users have been dealing with it frequently for many years.

It is not so much about complicating the code as adding code for a one time minor issue that will likely stay there for many years for no good reason, then one day someone will look at the code again and not be sure why it is still there.

daschuer · 2019-10-01T06:10:12Z

It is effects only a few lines of code.
Changing the file format is not necessary here and we are in a point release.
We can change the file format at any time, there is no need to do it here in a rush.

If you like to change the file format immigrate from the old.

Be-ing · 2019-10-01T14:07:50Z

We cannot simply reuse the regex in SoundDevicePortAudio for parsing the string from the old file format. The old file format hacked the PortAudio index together with the device name and ALSA device into a single string. If I implement code to parse that, I may as well revert all the work I did to implement the new SoundDeviceId class and go back to hacking multiple pieces of information into a single QString. There is no need to be pedantic about migrating from an old format when the real world impact is trivial.

This ensures the strings are the same in the readFromDisk and writeToDisk functions.

daschuer · 2019-10-01T15:43:12Z

I don't mind to revert to the original string based solution. Anything that not required to reconfigure the sound hardware will work for me.

Be-ing · 2019-10-01T15:47:33Z

I did not mean to propose that as an actual solution. Defining and parsing our own structured string format would be an ugly hack. That is what XML is for.

daschuer · 2019-10-01T15:58:36Z

So we need to migrate from the old format.
Also ok for me.

ywwg · 2019-10-01T16:05:14Z

This is probably out of scope for this PR but I wanted to make sure if we are updating sound card setup that we think about supporting users who may connect different sound cards at different times -- if mixxx can try to choose the user's preferred sound card depending on what's available, that would be really nice! https://bugs.launchpad.net/mixxx/+bug/1516035

Be-ing · 2019-10-01T16:05:47Z

@ywwg yes that would be nice, but beyond the scope of this PR.

... which users have been doing for years anyway. Revert this commit after releasing Mixxx 2.2.3.

daschuer · 2019-10-01T18:43:46Z

Thank you for the migration code. LGTM.

daschuer · 2019-10-01T18:45:02Z

and for picking this annoying and trick bug up ..

Be-ing added this to the 2.2.3 milestone Sep 1, 2019

daschuer requested changes Sep 1, 2019

View reviewed changes

uklotzde added the devices label Sep 1, 2019

track device name, ALSA device, and PortAudio device index separately

725a15a

This allows for more reliable identification of hardware devices from the configuration file so users do not need to reconfigure their sound hardware as often as before.

make SoundDeviceId qDebuggable

28603cf

Be-ing added 2 commits September 29, 2019 22:49

alsaDeviceName -> alsaHwDevice

a35a392

It isn't really a name, just an ID that ALSA defines.

DlgPrefSound: don't reinitialize sound hardware when nothing changes

8ffb16e

Be-ing changed the title ~~don't identify PortAudio devices by PaDeviceIndex in configuration~~ improve detection of sound hardware from configuration Sep 30, 2019

Be-ing mentioned this pull request Sep 30, 2019

Fix race condition(s) while unloading/loading tracks #2305

Merged

daschuer requested changes Sep 30, 2019

View reviewed changes

Be-ing force-pushed the audio_interface_bug branch from 1ed54e0 to c7e4d1f Compare September 30, 2019 17:57

SoundDeviceId: add comments; code formatting

9d0bc65

Be-ing force-pushed the audio_interface_bug branch from c7e4d1f to 9d0bc65 Compare September 30, 2019 18:04

uklotzde reviewed Sep 30, 2019

View reviewed changes

Be-ing added 3 commits October 1, 2019 09:58

DlgPrefSoundItem: use default-constructed SoundDeviceId for "None"

b115e44

SoundDevicePortAudio: make ALSA hw device regex a constant

1ce84d7

SoundManagerConfig: use named constants for XML attributes & elements

1e5d6a4

This ensures the strings are the same in the readFromDisk and writeToDisk functions.

Be-ing force-pushed the audio_interface_bug branch 2 times, most recently from 9498938 to 047a088 Compare October 1, 2019 15:11

Be-ing added 2 commits October 1, 2019 10:12

SoundDeviceId: simplify constructor

09a3449

SoundManagerConfig: make constructor explicit

c61cef4

Be-ing force-pushed the audio_interface_bug branch from 047a088 to c61cef4 Compare October 1, 2019 15:12

overcomplicate code to avoid reconfiguring sound HW one more time

5e58431

... which users have been doing for years anyway. Revert this commit after releasing Mixxx 2.2.3.

daschuer merged commit 7dd4915 into mixxxdj:2.2 Oct 1, 2019

Be-ing mentioned this pull request Oct 5, 2022

Attempt automatic reconfigure after sound server restart #8498

Closed

daschuer mentioned this pull request Apr 7, 2023

describe Sound Hardware settings in log #9073

Open

improve detection of sound hardware from configuration #2253

improve detection of sound hardware from configuration #2253

Conversation

Be-ing commented Sep 1, 2019

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Sep 1, 2019

daschuer commented Sep 1, 2019

daschuer commented Sep 1, 2019

daschuer commented Sep 1, 2019

JosepMaJAZ commented Sep 1, 2019

daschuer commented Sep 1, 2019

Be-ing commented Sep 1, 2019

daschuer commented Sep 1, 2019

Be-ing commented Sep 30, 2019

Be-ing commented Sep 30, 2019

Be-ing commented Sep 30, 2019 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Sep 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Sep 30, 2019

daschuer commented Sep 30, 2019 • edited Loading

Be-ing commented Sep 30, 2019

daschuer commented Oct 1, 2019

Be-ing commented Oct 1, 2019

daschuer commented Oct 1, 2019

Be-ing commented Oct 1, 2019

daschuer commented Oct 1, 2019

ywwg commented Oct 1, 2019

Be-ing commented Oct 1, 2019

daschuer commented Oct 1, 2019

daschuer commented Oct 1, 2019

Be-ing commented Sep 30, 2019 •

edited

Loading

daschuer commented Sep 30, 2019 •

edited

Loading