refactor(audio): negotiate QuindarLocalSink format via the factory (Phase 6, #3306)#3631
refactor(audio): negotiate QuindarLocalSink format via the factory (Phase 6, #3306)#3631jensenpat wants to merge 2 commits into
Conversation
…ethersdr#3306) Replace the hand-wired "connect each external playback sink to outputDeviceChanged" lambda in MainWindow with a single registry. External sinks that own their own QAudioSink (the post-DSP Pudu monitor, QSO playback) already followed the user-selected output device correctly — every device-change path (user selection, device removal, OS-default change) flows through AudioEngine::setOutputDevice() -> outputDeviceChanged. The risk was purely structural: each NEW external sink had to remember to join that lambda or it would "uncouple" and keep playing on the old/default endpoint (the recurring class behind aethersdr#2899 / aethersdr#3361 / aethersdr#3378). AudioOutputRouter (src/core/AudioOutputRouter.{h,cpp}) makes following a registration instead of hand-wiring: - addFollower(sink) takes any object with setOutputDevice(QAudioDevice) (or a std::function); the follower is seeded immediately and re-seeded on every change, QPointer-guarded against early destruction. - One QueuedConnection forwards outputDeviceChanged to setCurrentDevice(), which fans out to all followers on the GUI thread (matching prior behaviour) over a snapshot so a follower mutating the list mid-fan-out can't invalidate it. - Depends only on Qt Core/Multimedia, so the registry/fan-out is unit-tested headless (tests/audio_output_router_test, 11/11) without a live AudioEngine, including the QPointer-guard and reentrancy paths. MainWindow registers m_finalMonitor + m_qsoRecorder; a future external sink follows correctly just by registering — no new connect to forget. Behaviour- identical hardening. Intentionally NOT routed (documented): the AudioEngine- internal sinks (RX speaker, CW sidetone, Quindar — follow via the RX restart), and the WFM demodulator's WaveOutWriter (plays to its own WfmDeviceDialog device by design, aethersdr#3407). Builds & links clean into the full app; all three audio tests pass under CTest. Refs aethersdr#3306 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ethersdr#3306) Phase 6, first sink. QuindarLocalSink previously tried only 48 kHz Float then the device preferredFormat — no 44.1 kHz rung and no graceful fallback — so it failed outright on a 44.1k-only output (the class aethersdr#3306 set out to close; Quindar was the worst offender, with effectively no ladder). It now walks the shared AudioDeviceNegotiator's Float rungs (the tone is generated in Float, so Int16 rungs are skipped, mirroring the RX-speaker migration). This gives it, in one place, the per-OS preferred rate plus the 44.1 kHz and preferredFormat fallbacks. The generator retunes to the negotiated rate (RegenerateAtRate — no resampler), and the "every transmitted Quindar sample also plays locally" contract is unchanged. Diagnostics/summary logging preserved. Builds & links clean into the full app. Refs aethersdr#3306 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8497228 to
37f99cf
Compare
There was a problem hiding this comment.
Reviewed the Quindar migration commit (the AudioOutputRouter content belongs to the stacked #3630). The factory wiring is clean and the contract handling is careful — thanks for the thorough write-up. CI is green across build/macOS/Windows/CodeQL.
One substantive issue: the Quindar ladder is gated on isFormatSupported, not walked via start()
The PR says this "mirrors the RX-speaker migration," but the consumption pattern differs in a way that matters. The RX speaker (AudioEngine.cpp:1034-1054) walks the Float rungs and tries each with a real sink->start(), falling through to the next rung on failure — which is exactly what formatLadder() is built for. Its own doc comment is explicit:
The full ordered Qt-format ladder, for backends where
isFormatSupported()is unreliable and the caller must try-at-open (Windows WASAPI). … Each rung is tried with a realstart(), so reliable backends and WASAPI's probe-at-open are handled identically.
The new Quindar code instead does:
for (const QAudioFormat& cand : ladder) {
if (cand.sampleFormat() != QAudioFormat::Float) continue;
if (dev.isFormatSupported(cand)) { fmt = cand; haveFormat = true; break; }
}
if (!haveFormat) { /* log + return false */ }i.e. it picks the first isFormatSupported-true rung and then opens it once. Two consequences:
- Windows/WASAPI regression risk.
isFormatSupportedis documented-unreliable there — bypassing it is the whole reasonformatLadderexists. If it returns false for every Float rung, Quindar now bails withreturn falsewithout ever attempting an open, where the old code at least triedpreferredFormatviastart(). So on a WASAPI box this can turn "plays" into "silently never starts." (Windows wasn't part of the test matrix per the description, so this wouldn't have surfaced.) - No open-time fallback. Even on reliable backends, if the chosen rung's
start()fails there's no fall-through to the next rung — undercutting the "graceful fallback ladder" this PR is meant to add.
Suggested fix is to mirror RX literally: drop the isFormatSupported gate and walk the ladder constructing+start()ing each Float rung, keeping the first that yields a non-null QIODevice, and only emit the open-failure summary if all rungs fail. That also lets you delete the separate isFormatSupported check and fold the existing start()-failure logging into the loop.
Smaller notes (non-blocking)
formatLadderalways emitschannels = 2(AudioFormatNegotiator.cpp:96), so the hardcoded stereo write inonTimerTick(frameBytes = 2 * sizeof(float)) stays consistent — good. Worth a one-line comment noting the sink relies on the ladder being stereo, since a future ladder change to honor mono devices would silently corrupt the interleaving here.- Diagnostics/summary logging and the
RegenerateAtRateretune contract look correctly preserved.
Everything else (the router registry, QPointer guard, snapshot-during-fan-out, MainWindow seed-then-register ordering) reads cleanly. The only thing I'd want addressed before merge is the ladder-walk vs. isFormatSupported gating above.
🤖 aethersdr-agent · cost: $6.0724 · model: claude-opus-4-8
Phase 6, first sink — QuindarLocalSink → AudioDeviceNegotiator (#3306)
QuindarLocalSinkpreviously tried only 48 kHz Float then the device preferredFormat — no 44.1 kHz rung and no graceful fallback — so it failed outright on a 44.1k-only output. It was the worst offender in #3306 (effectively no ladder).It now walks the shared
AudioDeviceNegotiator's Float rungs (the tone is generated in Float, so Int16 rungs are skipped, mirroring the RX-speaker migration), gaining the per-OS preferred rate plus the 44.1 kHz and preferredFormat fallbacks in one place. RegenerateAtRate (the generator retunes — no resampler); the "every transmitted Quindar sample also plays locally" contract is unchanged. Diagnostics/summary logging preserved.Phase 6 scope
Quindar was the high-value sink (it had an actual no-fallback bug). The other three aux sinks are deferred with reasons: CW sidetone's QAudio path is only the fallback backend (PortAudio is primary and can't use the Qt negotiator) and is started at a constant 48 kHz;
ClientPuduMonitor+QsoRecorderplayback are Int16-native and already robust, so migrating them cleanly wants a small negotiator format-preference (Int16-first) enabler — its own follow-up.Test / build
All three audio tests green under CTest (negotiation 25/25, device wrapper 8/8, router 9/9). Full macOS app builds & links clean (RelWithDebInfo).
Refs #3306
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat