Skip to content

fix(sndDevices): handle device reconnection and filter capture events in OnDeviceStateChanged - #532

Open
akai07 wants to merge 1 commit into
fxsound2:develop/windowsfrom
akai07:fix/device-callback-dedup-reconnect
Open

fix(sndDevices): handle device reconnection and filter capture events in OnDeviceStateChanged#532
akai07 wants to merge 1 commit into
fxsound2:develop/windowsfrom
akai07:fix/device-callback-dedup-reconnect

Conversation

@akai07

@akai07 akai07 commented May 27, 2026

Copy link
Copy Markdown

Summary

Fixes multiple device management bugs in the sndDevices module that affect Bluetooth device reconnection and unnecessary reinitialization from capture device events.

Changes

OnDeviceStateChanged — Filter capture events

The callback now queries IMMEndpoint.GetDataFlow and returns S_OK early for non-render (capture/input) devices. Previously, any device state change — including microphone unplug/reconnect — would trigger a playback processing thread restart.

OnDeviceStateChanged — Track reconnections

When a device transitions to DEVICE_STATE_ACTIVE, its GUID is saved to reconnectedDeviceGuid and hasReconnectedDevice is set. This preserves the dedup guard (preventing callback storms on repeated same-state events) while allowing the reconnection to be detected.

sndDevicesImplementDeviceRules — Reconnection handling

Added a new check after the count-based new-device detection: if hasReconnectedDevice is set, find that device in the current list and select it. This handles Bluetooth headsets that are already enumerated (in UNPLUGGED state) and then connect — their count doesn't change, so the existing count-based logic would miss them.

sndDevicesReInit — Reconnection state cleared

Both fields are reset during reinit to prevent stale reconnection state.

Files Changed

  • audiopassthru/include/sndDevices.h — Added reconnectedDeviceGuid and hasReconnectedDevice fields
  • audiopassthru/src/sndDevices/sndDevicesDeviceCallbacks.cpp — Capture filter + reconnection tracking
  • audiopassthru/src/sndDevices/sndDevicesImplementDeviceRules.cpp — Reconnection device selection
  • audiopassthru/src/sndDevices/sndDevicesReInit.cpp — Clear reconnection state

Related Issues

  • Fixes Bluetooth device reconnect not triggering playback device switch
  • Fixes capture device events unnecessarily restarting playback thread

… in OnDeviceStateChanged

- Filter capture (eRender) device events in OnDeviceStateChanged via IMMEndpoint.GetDataFlow
  to prevent unnecessary playback reinit when input devices change state
- Track reconnected device GUID when state transitions to ACTIVE
- Add reconnection detection in sndDevicesImplementDeviceRules to auto-select
  reconnected playback devices (fixes BT headset reconnect after UNPLUGGED)
- Clear reconnection state on reinit
@bvijay74
bvijay74 changed the base branch from main to develop/windows July 21, 2026 09:33
@bvijay74

Copy link
Copy Markdown
Member

@claude can you review this PR.
Analyze if the changes complement the fixes done for device change handling.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @bvijay74's task in 6m 31s —— View job


Reviewing PR #532

  • Gather context (diff, related files, CLAUDE.md)
  • Understand device-change handling fixes this PR should complement
  • Review sndDevicesDeviceCallbacks.cpp changes
  • Review sndDevicesImplementDeviceRules.cpp changes
  • Review sndDevicesReInit.cpp / header changes
  • Post review feedback

Summary

The two fixes (filtering capture events, tracking reconnects) are well targeted and address real bugs. The capture-event filter in particular brings OnDeviceStateChanged in line with the existing render-only scoping already used by sndCheckDeviceChanges() (sndDevicesReInit.cpp:343, EnumAudioEndpoints(eRender, ...)), so it's consistent with the design intent elsewhere in the module rather than a one-off hack. The reconnect tracking is a reasonable complement to the existing count-based new-device detection in sndDevicesImplementDeviceRules. A few things worth addressing before merge:

Findings

1. Reconnect-priority path isn't wired into the other device-change detection mechanism (audiopassthru/src/sndDevices/sndDevicesReInit.cpp:321-409, sndDevicesImplementDeviceRules.cpp:234-268)
This codebase has two independent device-change detectors: the push-based OnDeviceStateChanged callback (this PR) and the poll-based sndCheckDeviceChanges(), invoked on-demand from FxController::checkDeviceChanges() when the user opens the system tray or endpoint-list menu. sndCheckDeviceChanges() already tracks per-device state (cast_handle->deviceState[j]) and detects the same kind of transition, but it never sets hasReconnectedDevice/reconnectedDeviceGuid. So if a reconnect is picked up via that path instead of (or before) the OnDeviceStateChanged callback, the new reconnection-priority selection in sndDevicesImplementDeviceRules won't trigger — only the generic stopAudioCaptureAndPlaybackLoop restart will happen, and the device gets selected by the older default/history-based rules. Worth mirroring the same tracking in sndCheckDeviceChanges() for consistency.

2. Reconnect handling always wins over the current default device (sndDevicesImplementDeviceRules.cpp:234-268)
This new block runs before the "current default device" branch and jumps straight to PlaybackDeviceIsSelected once a match is found — regardless of whether the reconnected device is actually the system's current default. If a paired Bluetooth device silently reconnects in the background (state → ACTIVE) while the user is actively listening through a different device, this will force a switch to the Bluetooth device purely because its radio came back online. This may be intentional given the bug report ("BT reconnect not triggering playback switch"), but it's worth confirming this is the desired UX vs. gating the reconnect-priority on the device also being (or becoming) the Windows default.

3. hasReconnectedDevice/reconnectedDeviceGuid is a single scalar, not a queue (sndDevices.h:430-431)
If two devices transition to ACTIVE before sndDevicesImplementDeviceRules next runs (e.g. resume from sleep with multiple endpoints reactivating), the second write silently clobbers the first device's GUID and it's never considered. Low likelihood, but worth a comment at minimum since the struct field is intentionally not an array.

4. Capture filter fails open on lookup errors (sndDevicesDeviceCallbacks.cpp:214-239)
If CoCreateInstance/GetDevice/QueryInterface(IMMEndpoint)/GetDataFlow fails for any reason — including a device that's already been removed from the enumerator by the time dwNewState == DEVICE_STATE_NOTPRESENT fires — the code falls through without filtering, i.e. it's treated as a render event and can still trigger a playback restart. This may be an acceptable safe default (avoids swallowing a real render event on transient COM errors), but it's worth explicitly testing the disconnect path (not just connect) to confirm GetDevice reliably resolves capture-device IDs there, and adding a trace log on failure would help diagnose it in the field.

5. Minor: per-callback COM overhead (sndDevicesDeviceCallbacks.cpp:217)
OnDeviceStateChanged can fire ~10-12 times for a single physical connect/disconnect (per the existing code comments at lines 244/250). Each invocation now does a fresh CoCreateInstance + GetDevice + QueryInterface round-trip. Not incorrect, just unnecessary overhead on a hot notification path — CsndDevicesMMNotificationClient already has an unused _pEnumerator member (sndDevices.h:223) that could be initialized once and reused instead.

6. Nit: the comment "when state changes to ACTIVE from a non-active state" (sndDevicesDeviceCallbacks.cpp:259) implies a previous-state check that isn't actually performed — the code just trusts that Windows only fires this callback on genuine transitions. Harmless, but slightly overstates what the code verifies.

Note on branch state

This branch (fix/device-callback-dedup-reconnect) was forked from an older point on develop/windows (466c85a) and appears to be a number of commits behind the current tip. The PR's own file diff (+90/-10 across 4 files) looks clean and scoped, so this shouldn't cause conflicts on merge, but it's worth rebasing onto the latest develop/windows before landing just to verify the reconnect logic interacts correctly with anything else that's landed there since (e.g. mute-state handling and NULL-safety changes in the same files).

No changes were implemented — this was a review-only request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants