Repository navigation
fix(audio): read the device id instead of its description on macOS - #931
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Select and compare output-device keys src/infra/audio/player.rs, CHANGELOG.md |
The sink stores a device ID on macOS and a device name on other platforms. The default-output check compares the stored key with the current default output key. Comments, tests, and the changelog describe the key-based lookup. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- Audio dropouts on macOS: the 1 s output-device check creates AudioUnits via cpal description() #928: The change replaces the recurring macOS device-description lookup with a device-ID lookup, addressing the reported source of audio dropouts.
Merge Risk: 
ee582
-
The macOS output check compares device IDs without an identified regression. No issue remains that should delay merging after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title uses the permitted conventional-commit prefix, includes the audio scope, and accurately describes the macOS device ID change. The subject is concise and imperative. Linked Issues check Issue [ #928] requires the macOS default-output comparison to use the device ID instead ofdescription().src/infra/audio/player.rsusesdevice.id()on macOS for both the opened sink and current …Out of Scope Changes check The changes stay within issue [ #928]. The source changes implement the macOS device-key comparison and preserve non-macOS behavior. The test updates document the required behavior. The Unreleased `CHA…Full details: Linked Issues check
-
Explanation
Issue [
#928] requires the macOS default-output comparison to use the device ID instead ofdescription().src/infra/audio/player.rsusesdevice.id()on macOS for both the opened sink and current default output, while other platforms retain the description path. TheOptionhandling preserves the unreadable-key behavior, so an unreadable key does not indicate a device change. The reported audio-player tests, clippy check, and format check support the implementation.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue [
#928]. The source changes implement the macOS device-key comparison and preserve non-macOS behavior. The test updates document the required behavior. The UnreleasedCHANGELOG.mdentry documents the fix. No unrelated implementation change is shown.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
✨ Simplify code
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
The once-a-second default-output check went through cpal's description(), which builds two AudioUnits per call on macOS (one per supported-config query). That churn glitches USB outputs. The id is a plain property read with no side effects, so it is what the comparison uses on macOS; other platforms keep the description, because cpal's WASAPI id() leaks the COM string it copies on every call and a poll would turn that into a steady leak. Fixes LargeModGames#928
faffde8 to
ee582b2
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
What was wrong
On macOS, decoded sources (Local Files, Subsonic, YouTube) played with short random dropouts on USB output devices, as reported in #928. The once-a-second default-output check in
LocalPlayer::device_lostwent through cpal'sdescription(), and in cpal 0.17.1 that call instantiates an AudioUnit throughsupported_input_configs()and another throughsupported_output_configs()just to read the buffer-size range. Every second the check re-created and connected two AudioUnits to the device that was playing, and that churn made USB outputs glitch. Spotify playback was clean because librespot keeps its own portaudio sink.What changed
The comparison now reads the device's id on macOS (
kAudioDevicePropertyDeviceUID, a plain property read with no side effects) instead of its description. Other platforms keep the description: cpal's WASAPIid()implementation leaks the COM string it copies on every call, and a once-a-second poll would turn that into a steady leak, so the choice is behind#[cfg(target_os = "macos")]and behavior is unchanged everywhere else. A CHANGELOG entry was added under Unreleased.The
None-means-cannot-tell semantics of the check are preserved: an unreadable key only disables the comparison, never reads as a change.Testing
cargo test --features local-files --lib audio::player- 3 passed, 7 ignored (the ignored ones need a real audio output device)cargo clippy --features local-files --lib -- -D warnings- cleancargo fmt --check- cleanThe dropouts themselves were confirmed gone by the issue reporter listening to a patched build on the same USB setup (see the comment on #928); the hardware-gated tests here cannot capture that.
Fixes #928
Summary by CodeRabbit