search

psmokotnin / osm

Open sound meter. FFT based application for tuning sound systems.
GNU General Public License v3.0
233 stars 37 forks source link

ALSA plugin: Reduce mutex lock duration #57

Open NexAdn opened 2 years ago

NexAdn commented 2 years ago

To mitigate deadlock issues on switching the audio input device, this commit reduces the time m_deviceListMutex is locked by switching std::lock_guard and manual locking for std::unique_lock and deferring locking only to places where access to the device list is needed.

Important note: During testing I sometimes had issues where the RTA and sidebar level meter didn't work correctly with some input configurations (especially the time window and input source settings). But since I am unfamiliar with the codebase, I can't tell if this issue is related.

Closes: https://github.com/psmokotnin/osm/issues/56

NexAdn commented 2 years ago

Regarding the display issues: I was just able to fix that by using a different audio interface. But I guess that's is unrelated to the original issue that this PR fixes.

NexAdn commented 1 year ago

Any updates on this?

psmokotnin commented 1 year ago

@NexAdn Thanks for the PR. Sorry was very loaded the last time. This issue requires a more detailed investigation and the solution also.

NexAdn commented 1 year ago

Well, from what the name suggests, m_deviceListMutex is responsible for handling access to m_devices, so passing the mutex on to the AlsaPCMDevice is not a good idea in the first place. Otherwise, I'd consider this variable poorly named as it seems to cover things beyond its name, while the name is pretty specific in that the mutex is there to serialize access to the device list, not the devices. Another thing to consider is that keeping a mutex for longer than it's actually needed (which normally boils down to accessing a shared resource) is almost always a bad idea as it leads to problems like this.

I've now been using this fix for quite a while without any problems. The semantics of your code seem work out perfectly fine to ensure your devices aren't deleted before they are finished doing their things. Thus, I don't think there is any reason to keep any locks for a long time.

If you think that my understanding of your locking mechanism here is wrong, please let me know. Then I can have a fresh look at it, if you lack the time or resources to do so.

psmokotnin commented 1 year ago

This PR doesn't solve the issue, I'll need more time to investigate the real reason for this deadlock and apply changes to this PR or I'll make a new fix for this issue. AudioPlugin tells the application what devices are now available, that's why it requires this mutex.