FIX(windows client): Fix audio dropout on RDP session start/end#7134
FIX(windows client): Fix audio dropout on RDP session start/end#7134MatthewFagan wants to merge 4 commits intomumble-voip:masterfrom
Conversation
…or default devices
WalkthroughThis change introduces a 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mumble/WASAPINotificationClient.cpp (1)
46-55:⚠️ Potential issue | 🔴 CriticalData race: Missing mutex lock when accessing
wantedDevices.
OnDeviceAddedaccesseswantedDevices.contains(device)without acquiringlistsMutex. This is a data race sincewantedDevicescan be modified by the audio thread viaenlistDeviceAsWanted/unlistDeviceAsWantedwhile the COM notification thread callsOnDeviceAdded. Compare withOnPropertyValueChanged(line 36-37) which correctly acquires the lock.Proposed fix
HRESULT STDMETHODCALLTYPE WASAPINotificationClient::OnDeviceAdded(LPCWSTR pwstrDeviceId) { const QString device = QString::fromWCharArray(pwstrDeviceId); qDebug() << "WASAPINotificationClient: Device added=" << device; + QMutexLocker lock(&listsMutex); if (wantedDevices.contains(device)) { restartAudio(); } return S_OK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mumble/WASAPINotificationClient.cpp` around lines 46 - 55, OnDeviceAdded is reading wantedDevices without holding listsMutex, causing a data race; wrap the access to wantedDevices (the contains check) inside the same lock used elsewhere (listsMutex) like in OnPropertyValueChanged so that the COM notification thread acquires listsMutex before checking wantedDevices and then releases it before calling restartAudio if needed, ensuring thread-safe access in WASAPINotificationClient::OnDeviceAdded.src/mumble/WASAPI.cpp (1)
1010-1012:⚠️ Potential issue | 🟡 MinorReset
ns,lastspoke, andmixedon retry.The variables
ns,lastspoke, andmixedare declared before the retry loop but used within it. On retry afterAUDCLNT_E_DEVICE_INVALIDATED, these variables retain their previous values, which could cause incorrect behavior:
nsis used to count channel masks and would accumulate across retrieslastspokeaffects volume attenuation statemixedaffects audio mixing stateProposed fix
bool doOuterRetry = true; while (doOuterRetry) { hr = 0; + ns = 0; + lastspoke = false; + mixed = false; hEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr);Also applies to: 1025-1027
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mumble/WASAPI.cpp` around lines 1010 - 1012, The variables ns, lastspoke, and mixed are declared outside the retry loop but used inside it, so on an AUDCLNT_E_DEVICE_INVALIDATED retry they retain stale state; move the declarations or reset them at the top of the retry loop (before the logic that counts channel masks and handles attenuation/mixing) so ns is zeroed, lastspoke is set to false, and mixed is set to its default value each attempt; update references to ns, lastspoke, and mixed in the functions handling channel mask counting and volume/mixing to rely on these freshly initialized values.
🧹 Nitpick comments (2)
src/mumble/WASAPI.h (1)
41-66: Consider explicitly deleting copy constructor and copy assignment.The
WASAPIDevicestruct implements move semantics but doesn't explicitly delete the copy constructor and copy assignment operator. While the compiler may implicitly delete them because a move constructor is declared, it's better practice to be explicit for clarity and to prevent accidental copies.Proposed fix
struct WASAPIDevice { public: WASAPIDevice(); WASAPIDevice(nullptr_t); WASAPIDevice(WASAPIDevice&& other); + WASAPIDevice(const WASAPIDevice&) = delete; ~WASAPIDevice(); void ClearDevice(); void ClearUsage(); operator bool() const; operator IMMDevice*() const; IMMDevice* operator->() const; WASAPIDevice& operator=(WASAPIDevice&& other); + WASAPIDevice& operator=(const WASAPIDevice&) = delete;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mumble/WASAPI.h` around lines 41 - 66, The struct WASAPIDevice currently declares move ctor/assign but not the copy operations; explicitly delete the copy constructor and copy copy-assignment by adding declarations WASAPIDevice(const WASAPIDevice&) = delete; and WASAPIDevice& operator=(const WASAPIDevice&) = delete; inside the WASAPIDevice struct (near the other special-member declarations like WASAPIDevice(WASAPIDevice&&) and WASAPIDevice& operator=(WASAPIDevice&&)) so accidental copying is prevented and intent is clear.src/mumble/WASAPI.cpp (1)
500-528: Consider re-evaluatingdoechoon retry.The
doechoflag is set once at line 500 before the retry loop. If the echo device fails to open (line 526-527 setsdoecho = false), subsequent retry iterations won't attempt to open the echo device again, even if it becomes available. If the goal is full recovery on device invalidation, consider re-evaluatingdoechoat the start of each retry iteration.Proposed fix
// Outer retry loop - For handling AUDCLNT_E_DEVICE_INVALIDATED which requires complete cleanup and then re-creation of audio endpoint. bool doOuterRetry = true; while (doOuterRetry) { hr = 0; + doecho = Global::get().s.doEcho(); hEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mumble/WASAPI.cpp` around lines 500 - 528, doecho is initialized once from Global::get().s.doEcho() before the outer retry loop, so if pEchoDevice.OpenNamedOrDefaultDevice fails and sets doecho = false, subsequent retries will never attempt to reopen the echo device even if it becomes available; move or re-evaluate the initial assignment so that doecho is (re)read from Global::get().s.doEcho() at the start of each outer retry iteration (or reset doecho before calling pEchoDevice.OpenNamedOrDefaultDevice inside the loop), ensuring pEchoDevice.OpenNamedOrDefaultDevice is retried on each doOuterRetry cycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mumble/WASAPI.cpp`:
- Around line 383-387: The move assignment operator
WASAPIDevice::operator=(WASAPIDevice&& other) is missing a return statement;
update the function to return *this at the end after calling ClearDevice(),
ClearUsage() and Move(std::move(other)) so it correctly returns a reference to
the assigned object and avoids undefined behavior in chained assignments.
In `@src/mumble/WASAPINotificationClient.cpp`:
- Around line 57-64: OnDeviceRemoved reads usedDefaultDevices and usedDevices
without holding listsMutex; lock listsMutex (e.g., QMutexLocker or
std::lock_guard on listsMutex) before checking contains(), set a local flag
(e.g., bool doRestart) if either contains() is true, then release the lock and
call restartAudio() only if doRestart is true to avoid holding the mutex while
restarting; update WASAPINotificationClient::OnDeviceRemoved accordingly using
the listsMutex, usedDefaultDevices, usedDevices, and restartAudio symbols.
---
Outside diff comments:
In `@src/mumble/WASAPI.cpp`:
- Around line 1010-1012: The variables ns, lastspoke, and mixed are declared
outside the retry loop but used inside it, so on an AUDCLNT_E_DEVICE_INVALIDATED
retry they retain stale state; move the declarations or reset them at the top of
the retry loop (before the logic that counts channel masks and handles
attenuation/mixing) so ns is zeroed, lastspoke is set to false, and mixed is set
to its default value each attempt; update references to ns, lastspoke, and mixed
in the functions handling channel mask counting and volume/mixing to rely on
these freshly initialized values.
In `@src/mumble/WASAPINotificationClient.cpp`:
- Around line 46-55: OnDeviceAdded is reading wantedDevices without holding
listsMutex, causing a data race; wrap the access to wantedDevices (the contains
check) inside the same lock used elsewhere (listsMutex) like in
OnPropertyValueChanged so that the COM notification thread acquires listsMutex
before checking wantedDevices and then releases it before calling restartAudio
if needed, ensuring thread-safe access in
WASAPINotificationClient::OnDeviceAdded.
---
Nitpick comments:
In `@src/mumble/WASAPI.cpp`:
- Around line 500-528: doecho is initialized once from Global::get().s.doEcho()
before the outer retry loop, so if pEchoDevice.OpenNamedOrDefaultDevice fails
and sets doecho = false, subsequent retries will never attempt to reopen the
echo device even if it becomes available; move or re-evaluate the initial
assignment so that doecho is (re)read from Global::get().s.doEcho() at the start
of each outer retry iteration (or reset doecho before calling
pEchoDevice.OpenNamedOrDefaultDevice inside the loop), ensuring
pEchoDevice.OpenNamedOrDefaultDevice is retried on each doOuterRetry cycle.
In `@src/mumble/WASAPI.h`:
- Around line 41-66: The struct WASAPIDevice currently declares move ctor/assign
but not the copy operations; explicitly delete the copy constructor and copy
copy-assignment by adding declarations WASAPIDevice(const WASAPIDevice&) =
delete; and WASAPIDevice& operator=(const WASAPIDevice&) = delete; inside the
WASAPIDevice struct (near the other special-member declarations like
WASAPIDevice(WASAPIDevice&&) and WASAPIDevice& operator=(WASAPIDevice&&)) so
accidental copying is prevented and intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed96d898-57b2-406e-9730-3dc9ae5f8b0c
📒 Files selected for processing (4)
src/mumble/WASAPI.cppsrc/mumble/WASAPI.hsrc/mumble/WASAPINotificationClient.cppsrc/mumble/WASAPINotificationClient.h
MatthewFagan
changed the title
MatthewFagan
changed the title
2 participants
FIX(windows client): Fix audio dropout on RDP session start/end
WASAPI notifications were not being handled correctly, resulting in complete audio dropout when the system audio config was changed, such as during RDP session start/stop or when starting/changing audio software such as a DAW.
WASAPINotification: The original code only triggered an audio restart if a notification was for a device currently in-use. However, during RDP session start, all audio devices are removed from the system, and then re-added. Once they are all removed, they are no longer in-use, and so the notification was not triggering when the devices got re-added. I changed it so that WASAPIInput and WASAPIOutput registered "wanted" devices if they cannot open the desired device (and also wantDefault if opening the default device fails) with WASAPINotification. When device added event occurs, it can see that we want to use that device and restart the audio system which picks that up.
WASAPIInput/WASAPIOutput: A tightly related issue was the AUDCLNT_E_DEVICE_INVALIDATED error. This occurs not when a device is removed then added, but when the configuration of the device changes (such as bit rate). It previously just caused the audio thread to exit, which stopped the audio until it was manually restarted. So I changed it to add a retry loop for only when this error occurs. (See https://learn.microsoft.com/en-us/windows/win32/coreaudio/recovering-from-an-invalid-device-error for an explanation)
Talking indicator: The talking indicator also continued to show talking/shouting if a mic was disconnected while the user was talking/shouting. This has now been fixed to clear the indicator if no mic can be opened. The indicator gets updated again on the first frame captured once a mic is re-connected.
Fixes #4992
Potentially fixes #5542
Checks