Skip to content

feat(audio): live wrapper + RX & TX-mic migration onto the sink factory (#3306)#3398

Merged
ten9876 merged 5 commits into
aethersdr:mainfrom
jensenpat:aether/audio-sink-factory-live
Jun 6, 2026
Merged

feat(audio): live wrapper + RX & TX-mic migration onto the sink factory (#3306)#3398
ten9876 merged 5 commits into
aethersdr:mainfrom
jensenpat:aether/audio-sink-factory-live

Conversation

@jensenpat
Copy link
Copy Markdown
Collaborator

@jensenpat jensenpat commented Jun 5, 2026

Stacks on #3397 (the pure foundation). The first two commits here are #3397's (foundation + live wrapper); the last two are the RX and TX-mic migrations. Merge #3397 first and this collapses to just those deltas. Kept as separate commits per the maintainer's one-change-at-a-time / soak rule (#3194 thread).

What

Layer 2 + the first two sink migrations of the consolidated audio sink factory (#3306):

  1. Live Qt-Multimedia wrapper AudioDeviceNegotiator — the thin, and only, platform-specific bridge. probe(QAudioDevice) → pure DeviceCaps; negotiate()/formatLadder() → openable QAudioFormats. Smoke-tested against the real default devices (audio_device_negotiator_test, 8/8).
  2. RX speakerstartRxStream() walks the factory's Float ladder with real start() attempts instead of two forked per-OS #ifdef blocks (net −87 lines). m_resampleTo48k (bool) → m_rxOutputRate (int).
  3. PC mic (TX), macOS + LinuxstartTxStream() drives mic rate/format from the factory's Int16 Input ladder (stereo-then-mono), removing the per-OS rate lists and the bespoke macTxInputRateCandidates() helper.

Behaviour (preserved)

Scope notes / invariants

Test / build

  • audio_format_negotiation_test 25/25, audio_device_negotiator_test 8/8.
  • Full macOS app builds & links clean (RelWithDebInfo); deployed to the macOS test box. Soak on Windows/Linux requested before un-drafting.

Refs #3306

💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat

@jensenpat jensenpat changed the title feat(audio): live wrapper + RX-speaker migration onto the sink factory (#3306) feat(audio): live wrapper + RX & TX-mic migration onto the sink factory (#3306) Jun 5, 2026
@jensenpat jensenpat marked this pull request as ready for review June 5, 2026 14:44
@jensenpat jensenpat requested review from a team as code owners June 5, 2026 14:44
Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the careful staging here, @jensenpat — the OS-as-data design carries through cleanly to the wrapper, and isolating it to one live-Qt-Multimedia layer makes the regression matrix a real artifact rather than aspirational. A few notes from a focused read:

Real findings

  1. PR title contains an unsubstituted ${PR_TITLE} / ${PR_DIFF} placeholder ("live wrapper + RX ${PR_TITLE} TX-mic migration…", and similar in the body). Looks like a templating substitution that didn't run before push. Worth editing the title and the "Full macOS app builds ${PR_DIFF} links clean" line in the body before merge so the squashed commit message lands clean. (Not a code issue, but it propagates to the commit log.)

  2. m_rxOutputRate is plain int, read from the m_rxTimer lambda and written from startRxStream() (AudioEngine.cpp:676 and :1044). Identical hazard to the prior bool m_resampleTo48k, so not a regression introduced here — but as long as the field is changing shape, it'd be cheap to make it std::atomic<int> to match the surrounding m_rxPan / m_rxBufferCapMs / m_rxBufferSampleRate (which are all std::atomic). Drive-by, not blocking.

  3. Windows Quindar-on-RX is a real, scope-creep-y behavior delta worth flagging on the soak list explicitly — the PR description does mention it, but the body bullet calls it a "strictly-additive win." It's additive in intent, but it does mean Windows users get an additional QAudioSink opening alongside RX on a code path that previously short-circuited. Probably fine, but it deserves its own line item in the Windows soak ("verify Quindar local sink opens cleanly, no extra device contention, no Bluetooth-route flip").

Smaller observations

  1. startRxStream() skips Int16 ladder rungs (AudioEngine.cpp:1037) because the RX drain is Float PCM. Consistent with the old makeFormat() (Float-only), so no regression — but the policy generates Int16 candidates that you then filter out. A Direction::OutputFloatOnly variant or a policy.formatMask would let buildLadder() skip them at source. Pure cleanup, doesn't have to happen now.

  2. The new RX loop captures lastRxError = audioErrorName(sink->error()) before delete sink — good. But on the success path, triedFloatRung is left false on the first rung succeeding, so noteRxFallback("preferred RX format unavailable -> N Hz") only fires after a real fallback. That reads correctly.

  3. m_audioSink = nullptr; m_audioDevice = nullptr; at :1030 is redundant given the early-return guard at :932, but harmless.

Correctness spot-checks that came back clean

  • Resampler::dstRate() exists (src/core/Resampler.h:40), so the rebuild check on device-rate change is well-formed.
  • macOS telephony substitution stays pre-ladder (intentional, matches #1705).
  • formatLadder() falls back to optimistic first-rung-only on a null/unprobeable device (AudioDeviceNegotiator.cpp:39), which is the right move for WASAPI probe-at-open.
  • The two stereo resampler strategies (PreservePan vs MonoCollapse) stay distinct in ResamplerPolicy, with the policy header explicitly calling out the #2403/#2459 regression they must not re-introduce.

Architecture is sound and the golden matrix is the right shape for catching this class of bug on a Linux runner. Mostly housekeeping comments above — please fix the title placeholder, and consider the atomic on m_rxOutputRate while you're in there.


🤖 aethersdr-agent · cost: $17.0716 · model: claude-opus-4-7

@ten9876 ten9876 force-pushed the aether/audio-sink-factory-live branch from 583067c to 0b4a85b Compare June 6, 2026 16:40
ten9876 added a commit to jensenpat/AetherSDR that referenced this pull request Jun 6, 2026
m_rxOutputRate is written from the GUI thread (startRxStream() at
AudioEngine.cpp:1044) and read from the RX drain timer lambda at :676,
plus the status JSON path at :851, the scope/status read sites at
:1471/3849/5108, and the cross-thread .store() into m_rxBufferSampleRate
at :1106. All four surrounding fields (m_rxPan, m_rxBufferCapMs,
m_rxBufferSampleRate, m_muted) are already std::atomic<int>/<bool>; the
previous bool m_resampleTo48k had the same hazard, so this is not a
regression introduced by aethersdr#3398 — but the migration is the cheapest
moment to close it.

Flagged in the @aethersdr-agent bot review on aethersdr#3398 as a non-blocking
drive-by; folding it in here rather than as a follow-up issue keeps the
field-shape change and the atomic upgrade in one commit so git blame
tells the right story.

Mechanical: 16 read sites take .load(), 1 write at :1044 takes .store(),
the inline rxOutputResamplingActive() getter in AudioEngine.h gets a
.load() too. <atomic> is already pulled in transitively.

Verified locally:
- audio_format_negotiation_test 25/25
- audio_device_negotiator_test 8/8
- Full AetherSDR build clean (626/626)

Principle XI.
@ten9876 ten9876 self-assigned this Jun 6, 2026
@ten9876
Copy link
Copy Markdown
Collaborator

ten9876 commented Jun 6, 2026

Rebased against current main (post-#3397 merge) and pushed `0b4a85b3` addressing the one substantive item from @aethersdr-agent's review.

What changed in the force-push:

  1. Rebased onto current main. Git's cherry-pick logic dropped the squashed-and-merged foundation commit (`5e3fb831` → `204585bc`) cleanly, replayed only the three real commits: live wrapper + RX migration + TX-mic migration. The PR now shows its true delta of +439/-234 across 7 files, not the +1223/-226 it showed when stacked on the unsquashed foundation.

  2. `m_rxOutputRate` upgraded to `std::atomic` to match its neighbours (`m_rxPan`, `m_rxBufferCapMs`, `m_rxBufferSampleRate`, `m_muted` are all atomic). Mechanical: 16 read sites take `.load()`, the single write at `startRxStream():1044` takes `.store()`, the inline `rxOutputResamplingActive()` getter in the header takes `.load()`. The previous `bool m_resampleTo48k` had the same latent hazard, so this isn't a regression introduced by the migration — but @aethersdr-agent was right that the field-shape change is the cheapest moment to close it.

On the other two bot findings:

  • The `${PR_TITLE}` / `${PR_DIFF}` placeholder concern was already stale by the time I looked — the title and body both read clean now.
  • The Windows Quindar behaviour delta is correctly documented in code at `AudioEngine.cpp:1117-1119` (the comment explicitly calls out "the old Windows branch returned before `startQuindarLocalSink()`, so the Quindar local monitor never opened on Windows — unifying the path fixes that"). Belongs on the Windows soak checklist.

Verified locally (Arch Linux x86):

Same maintainer-fix-up pattern as #3279/#3286/#3289/#3381/#3391/#3397. Soak on Linux from my side is clean; Windows/macOS soak still wanted before final merge per your description, but CI is green and the code is in a state where soak is the bottleneck, not review.

@jensenpat — happy to revert the rebase or the atomic upgrade if you'd rather take a different cut at either.

Principle XI.

jensenpat and others added 5 commits June 6, 2026 10:08
Layer 2 of the audio sink factory: the thin, and ONLY, platform-specific
bridge between a real QAudioDevice and the pure AudioFormatNegotiator policy.

- src/core/AudioDeviceNegotiator.{h,cpp}: probe(QAudioDevice) builds a
  DeviceCaps snapshot (rate probing via isFormatSupported, preferredFormat,
  per-OS isFormatSupportedReliable so Windows WASAPI probes-at-open aethersdr#2120/
  aethersdr#2929, caller-fed Bluetooth-HFP hint aethersdr#2615); negotiate() returns a ready-to-
  open QAudioFormat + ResamplerKind; formatLadder() returns the ordered Qt
  format ladder for try-at-open backends. SampleFmt <-> QAudioFormat mapping
  lives here so the policy layer stays Qt-Multimedia-free and headless-testable.

- tests/audio_device_negotiator_test.cpp: smoke test that probes the real
  default output/input devices and round-trips to an openable format; tolerant
  of headless CI runners with no audio hardware. 8/8 pass on macOS (default
  output negotiates 48000/PreservePan, matching the current RX behaviour).

No sink consumes the wrapper yet — the RX/mic migrations land next, per the
incremental plan in docs/audio-sink-factory.md.

Refs aethersdr#3306

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-authored-by: Codex <noreply@openai.com>
…dr#3306)

Replace the two forked per-OS #ifdef negotiation blocks in
AudioEngine::startRxStream() with a single walk of the factory's Float format
ladder (AudioDeviceNegotiator), trying each rung with a real start() so
reliable backends and WASAPI probe-at-open behave identically. Net -87 lines.

Behaviour is identical for normal devices: Windows/macOS still open 48k (dodging
the WASAPI 24k resampler artifacts aethersdr#2120 and keeping macOS A2DP devices off the
HFP/telephony route), Linux still opens native 24k. New, strictly-additive wins:
- a 44.1 kHz-only output now works (24k->44.1k resample) instead of failing —
  the aethersdr#3306 / aethersdr#3385 regression the golden matrix guards;
- the Quindar local monitor now opens on Windows too (the old Windows branch
  returned before startQuindarLocalSink()).

To support a non-48k device rate, the m_resampleTo48k bool is generalized to an
m_rxOutputRate int. resampleStereo() now targets that rate with its two
independent L/R r8brain instances (PreservePan — VITA-49 pan preserved, never
mono-collapsed here, aethersdr#2403/aethersdr#2459); the RADE decoded-speech path (MonoCollapse)
and both TX-scope rate sites follow suit. Cached resamplers rebuild when the
device rate changes (e.g. a 48k->44.1k device swap).

The macOS telephony/HFP output substitution (aethersdr#1705) is unchanged — it is
device selection, applied before the format ladder.

Builds clean into the full app; audio_format_negotiation_test (25/25) and
audio_device_negotiator_test (8/8, real macOS devices -> 48000/PreservePan) pass.

Refs aethersdr#3306

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-authored-by: Codex <noreply@openai.com>
…ethersdr#3306)

macOS + Linux: AudioEngine::startTxStream() now drives the mic rate/format
selection from the factory's Int16 Input ladder (AudioDeviceNegotiator),
walking stereo-then-mono to preserve the existing channel fallback. This
removes the per-OS #ifdef rate lists and the bespoke macTxInputRateCandidates()
helper — its policy (preferred-rate-first to dodge the silent-48k-open trap
aethersdr#2930; Bluetooth-HFP native rate first aethersdr#2615) now lives in the one factory
ladder. The macOS CoreAudio-HAL detection the factory can't derive
(macBluetoothNativeInputRate) is fed in via a new preferredRateOverride on the
wrapper, so a BT-HFP mic still opens at its native low rate.

Behaviour preserved: standard macOS mic -> preferred 48k (then 44.1k/24k/16k);
Linux -> native 24k (then 48k/44.1k); BT-HFP mic -> native low rate first. The
existing TX resampler (m_txInputRate -> 24k) and the push/pull open modes,
watchdog, and metering are unchanged.

Windows mic is intentionally left as-is: it already matches the factory's
Windows policy (force 48k + WASAPI probe-at-open) and carries the mono-only
USB-mic channel clamp (aethersdr#2929) that warrants its own soak — the remaining mic
increment.

Builds & links clean into the full app; both negotiation tests still pass.

Refs aethersdr#3306

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-authored-by: Codex <noreply@openai.com>
m_rxOutputRate is written from the GUI thread (startRxStream() at
AudioEngine.cpp:1044) and read from the RX drain timer lambda at :676,
plus the status JSON path at :851, the scope/status read sites at
:1471/3849/5108, and the cross-thread .store() into m_rxBufferSampleRate
at :1106. All four surrounding fields (m_rxPan, m_rxBufferCapMs,
m_rxBufferSampleRate, m_muted) are already std::atomic<int>/<bool>; the
previous bool m_resampleTo48k had the same hazard, so this is not a
regression introduced by aethersdr#3398 — but the migration is the cheapest
moment to close it.

Flagged in the @aethersdr-agent bot review on aethersdr#3398 as a non-blocking
drive-by; folding it in here rather than as a follow-up issue keeps the
field-shape change and the atomic upgrade in one commit so git blame
tells the right story.

Mechanical: 16 read sites take .load(), 1 write at :1044 takes .store(),
the inline rxOutputResamplingActive() getter in AudioEngine.h gets a
.load() too. <atomic> is already pulled in transitively.

Verified locally:
- audio_format_negotiation_test 25/25
- audio_device_negotiator_test 8/8
- Full AetherSDR build clean (626/626)

Principle XI.
… log

Final-pass cleanup: the RX-speaker migration entry in the design doc said
"generalized to an m_rxOutputRate int", but the atomic upgrade earlier
in this PR (0b4a85b) makes the final shape std::atomic<int>. Future
readers of the migration log shouldn't have to chase the header to find
out the field is atomic.

Principle XI.
@ten9876 ten9876 force-pushed the aether/audio-sink-factory-live branch from 0b4a85b to 908f8a1 Compare June 6, 2026 17:22
@ten9876 ten9876 merged commit 9d91206 into aethersdr:main Jun 6, 2026
5 checks passed
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