feat(audio): consolidated sink/rate negotiation foundation (#3306)#3397
Open
jensenpat wants to merge 1 commit into
Open
feat(audio): consolidated sink/rate negotiation foundation (#3306)#3397jensenpat wants to merge 1 commit into
jensenpat wants to merge 1 commit into
Conversation
…#3306) Introduce the pure, OS-parameterized audio format/rate negotiation policy that every audio sink and source will share, replacing the ~9 divergent per-sink fallback ladders and per-OS #ifdef branches that are the root of a recurring class of platform audio bugs. This first step is a pure addition with zero behaviour change — the maintainer-endorsed "land the helper first" approach (aethersdr#3306, aethersdr#3194 thread). - src/core/AudioFormatNegotiator.{h,cpp}: dependency-free policy (links only Qt6::Core) over an injected DeviceCaps snapshot with TargetOs as a PARAMETER, not an #ifdef. One ladder owns the Windows-48k-prefer (aethersdr#2120), macOS-48k/A2DP (aethersdr#1705), Linux-24k-native divergences, the universal 44.1k rung (aethersdr#3385), Int16<->Float32 + preferredFormat fallbacks (aethersdr#2669/aethersdr#1090/ aethersdr#3231), macOS mic preferred-first (aethersdr#2930) and Bluetooth-HFP native rate (aethersdr#2615), and the Windows probe-at-open rule (aethersdr#2929). The two stereo resampler strategies (PreservePan vs MonoCollapse) are kept deliberately distinct (aethersdr#2403/aethersdr#2459). - tests/audio_format_negotiation_test.cpp: headless, hardware-free golden matrix. Because TargetOs is data, one binary exercises all three OS ladders on any CI runner — the reason the historical bugs escaped CI. 25/25 pass, including the 44.1k-only-device regression guard that RX/Quindar fail today. - docs/audio-sink-factory.md: the full three-layer factory design (pure policy / live Qt wrapper / device-ownership router), the device-following "uncoupling" fix, the regression-guard invariants, and the incremental one-sink-per-PR migration plan. Refs aethersdr#3306 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Thanks @jensenpat — this is a really clean foundation PR.
What I checked
- Scope. The five files (CMake test target, design doc,
AudioFormatNegotiator.{h,cpp}, golden-matrix test) are all in scope; no drive-by changes touch live sinks. Behaviour-change is genuinely zero because nothing consumes the new module yet. - Conventions. No
QSettingsuse (irrelevant here — pure policy module), no raw owning pointers, value-semantics throughout. The single#if defined(Q_OS_*)lives inhostTargetOs()exactly as the design doc commits to. - Bugs / leaks / null-deref. Nothing. Pure value-based code over
QList/QString, no allocation, no I/O. - Error handling at boundaries. No system boundaries in this layer (the live wrapper lands in a follow-up PR). The total-failure case is correctly surfaced via
NegotiatedFormat::ok = falsewith areasonstring — the test exercises it ("empty reliable device -> negotiation fails").
Things I particularly liked
TargetOsas a parameter rather than#ifdefmakes the golden matrix actually able to assert all three OS ladders from any runner — the right primitive for catching the historical bug class.- The
preferredFirstrung for macOS input correctly precedes the per-OS ladder so BT-HFP and 16k-native mics aren't forced to 48k, and the test covers both #2615 and #2930 explicitly. - The
nothingProbeableheuristic for Windows probe-at-open is well-contained innegotiate()and has a dedicated test row. resamplerKindFor()keepsPreservePanandMonoCollapsedeliberately distinct — the unit checks at the bottom of the test guard #2403 from coming back.
Tiny nit (non-blocking, no need to spin)
primaryRateOrder() falls through to return {internalRate} after the switches; that's only reachable if a new TargetOs value is added later. A Q_UNREACHABLE() or default: would document the invariant for whoever touches the enum next, but the current shape is fine.
Looking forward to the live wrapper. The migration plan in the doc is exactly the cadence the audio stack needs.
🤖 aethersdr-agent · cost: $13.4278 · model: claude-opus-4-7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
First, pure-addition step of the consolidated audio sink factory (#3306): a single shared, OS-parameterized format/rate negotiation policy that every audio sink and source will migrate onto, replacing the ~9 divergent per-sink fallback ladders and per-OS
#ifdefbranches that are the root of a recurring class of platform audio bugs.Zero behaviour change — this is the maintainer-endorsed "land the helper first, migrate sinks incrementally" approach (#3306, and the #3194 thread's "sensitive audio-stack changes must be separate PRs with soak time").
Why this shape
The historical bugs escaped CI because the per-OS rate ladders were compiled behind
Q_OS_*, so a Linux runner could only ever exercise the Linux path. The new policy is a pure function over an injectedDeviceCapssnapshot withTargetOsas a parameter, not an#ifdef— so one headless, hardware-free test binary exercises the Windows, macOS and Linux ladders on any runner.Contents
src/core/AudioFormatNegotiator.{h,cpp}— dependency-free policy (links onlyQt6::Core). One ladder owns:#ifdef.Int16↔Float32+preferredFormat()catch-all (fix(cw): Int16 fallback for sidetone sink on Int16-only WASAPI devices (#2668). Principle I. #2669 / Add CommonRadioAudio / Float32 virtual driver support (#1070) #1090 / fix(audio): add preferredFormat() fallback for WASAPI Float32 output devices #3231).tests/audio_format_negotiation_test.cpp— table-driven golden matrix, registered under CTest. 25/25 pass, including the 44.1k-only-device regression guard.docs/audio-sink-factory.md— the full three-layer design (pure policy → live Qt wrapper → device-ownership router), the device-following "uncoupling" fix (CW / RADE / Aetherial-Audio "Pudu" recorder following the selected output), the regression-guard invariants, and the one-sink-per-PR migration plan.Test
Follow-ups (per the migration plan in the doc)
Live Qt wrapper → migrate RX speaker → migrate PC mic →
AudioOutputRouter(closes the uncoupling class) → TCI-TX client-rate → PipeWire DAX shared resampler. Each a separate, soakable PR.Refs #3306
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat