Refactor(audio): Move TX Voice DSP to 48 kHz Float — Principle VIII. - #4875
Refactor(audio): Move TX Voice DSP to 48 kHz Float — Principle VIII.#4875Silent-Gloves wants to merge 25 commits into
Conversation
Introduce a backend-independent TxVoiceProcessor that normalizes captured voice audio to a fixed 48 kHz float domain, runs RNNoise and the user-ordered channel strip without per-stage integer round trips, then performs one 48-to-24 kHz egress conversion and one transport-boundary quantization. Preserve the existing Opus/VITA framing and the separate DAX and RADE paths. Add deterministic coverage for rate contracts, block continuity, reset behavior, measurement taps, native-float input, RNNoise framing, finite output, and latency accounting.
…identical dither values are applied to both channels to preserve duplicate mono.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nicely structured refactor — extracting the voice strip into a headless TxVoiceProcessor with an explicit 48 kHz float island, one SRC, and one dithered quantization at the transport boundary is the right shape, and the doc rewrite is unusually thorough. I built tx_voice_processor_test against this branch and ran it: 39/39 pass, including the block-boundary invariance and TPDF determinism checks. TxVoiceProcessor.cpp, RNNoiseFilter.cpp and the test compile clean.
The one thing I'd want resolved before merge is added TX voice latency. The 48→24 egress SRC is not free: I measured this branch's end-to-end mic→transport delay with an impulse and got 70.6 ms at 48 kHz capture, 109 ms at 44.1 kHz, and 141 ms at 24 kHz capture. On main those same rates are ~71 ms / ~78 ms / 0 ms — because at 24 kHz m_txNeedsResample was false and the int16 strip ran with no SRC at all. AudioFormatNegotiator's Linux input ladder is {24000, 48000, 44100}, so 24 kHz is the first thing tried on the platform this client targets. Details and numbers in the inline comment.
Would like fixed before merge
- ~141 ms of new one-way TX voice delay at 24 kHz capture (0 ms on
main); +31 ms at 44.1 kHz —src/core/TxVoiceProcessor.cpp latencyFrames()reports 0 for the SRCs on the strength of a "startup latency is consumed" claim that doesn't hold — measured group delay is ~3388 DSP frames per instance
Polish
QByteArray::clear()frees the reservation, so theprocess(in, n, output)"allocation-free" contract doesn't hold andprepare()'s tenreserve()calls are undone by its own trailingreset()- Capture blocks larger than
m_maxInputFramesare dropped whole and silently — no log, no meter, no packet m_txResampleris now only advanced whilem_radeModeis set, so toggling RADE mid-stream feeds it stale filter state
Non-blocking notes
txInputNormalizationTo48k()hardcodes48000rather thanTxVoiceProcessor::kDspRate(AudioEngine.h:182).AudioEngine.honly forward-declares the class so you can't reference the constant there directly, but astatic_assertin the .cpp would keep the two from drifting.- The pre-tail
m_txPostDspMonitortap moves from post-strip/pre-gain to post-limiter, making it identical tom_txFinalMonitor. I checked and your "no active GUI owner" comment is correct —setTxPostDspMonitor()has no callers in the tree — so this is a documented no-op today, just worth knowing it's a real semantic change if that monitor is ever wired up. - Removing
applyClientTxDspFloat32()and friends is in scope: they were defined-but-never-called onmain. - CodeGuard: all 21 findings are in
src/gui/MainWindow.cppandthird_party/liquid-dsp/, neither of which this PR touches. False positives for this diff — no action needed.
🤖 aethersdr-agent · cost: $11.5100 · model: claude-opus-5
| if (inputRate != kDspRate) { | ||
| m_inputResampler = std::make_unique<Resampler>( | ||
| inputRate, kDspRate, maxInputFrames); | ||
| } else { | ||
| m_inputResampler.reset(); | ||
| } | ||
|
|
||
| m_maxDspFrames = static_cast<int>( | ||
| std::ceil(static_cast<double>(maxInputFrames) * kDspRate / inputRate)) + 32; | ||
| m_outputLeftResampler = std::make_unique<Resampler>( | ||
| kDspRate, kTransportRate, m_maxDspFrames); | ||
| m_outputRightResampler = std::make_unique<Resampler>( | ||
| kDspRate, kTransportRate, m_maxDspFrames); |
There was a problem hiding this comment.
This is the one blocking item. The unconditional 48 kHz island means every capture rate except 48 kHz now pays for two r8brain conversions where main paid for one or zero.
I measured a single-sample impulse through this branch's processCapturedInt16() end to end (10 ms blocks, primed with silence first), and separately measured each Resampler instance's group delay:
| capture rate | main TX voice delay |
this branch | delta |
|---|---|---|---|
| 48 kHz | 70.6 ms (48→24) | 70.6 ms | 0 |
| 44.1 kHz | 78.2 ms (44.1→24) | 109.1 ms (38.5 + 70.6) | +31 ms |
| 24 kHz | 0 ms — no SRC at all | 141.2 ms (70.6 + 70.6) | +141 ms |
Each Resampler (CDSPResampler24, default transition band) has ~70.6 ms of group delay; prewarm() fills the pipeline so output starts immediately, but it does not remove the filter delay.
Why 24 kHz matters specifically: AudioFormatNegotiator's input ladder is {internalRate, 48000, 44100} on Linux — i.e. 24 kHz is tried first on the platform this client is built around. On main those users had a zero-SRC TX voice path. 141 ms of one-way delay is enough to clip the tail of the last syllable when PTT drops, and it desynchronises voice from the MON/sidetone monitor and from anything timed off PTT.
A couple of ways out, in rough order of preference:
- Keep the 24 kHz capture case SRC-free — process the strip at 24 kHz when the device is already there, or upsample with a much shorter filter for the voice path only. The stated goal (one float domain, one quantization) survives; only the rate of the island becomes device-dependent.
- Give the voice SRCs their own low-latency
Resamplerprofile (widerreqTransBand/ lower-order class). TheResamplerctor already takesreqTransBand; the voice band is 300–3000 Hz, so a much cheaper filter is defensible here in a way it isn't on the RX panadapter path. - If the latency is considered acceptable, it needs to be a stated, measured decision in the PR body and in
audio-pipeline.mdrather than an unmentioned side effect — the doc currently discusses the 48→24 filter only as an anti-alias filter, not as a delay.
Happy to be told option 3 is the call, but it shouldn't land unmeasured.
| // Deterministic end-to-end delay expressed in 48 kHz DSP frames. Includes | ||
| // RNNoise's one-frame WOLA delay and enabled gate lookahead. r8brain is | ||
| // configured to consume its integer startup latency; reverb pre-delay is | ||
| // an artistic wet-path parameter rather than whole-signal latency. |
There was a problem hiding this comment.
This comment is the reason the latency above went unnoticed: "r8brain is configured to consume its integer startup latency" is true of the startup transient (that's what Resampler::prewarm() does) but not of the filter's group delay, which stays in the signal.
Measured: each Resampler instance in this chain contributes ~3388 frames at 48 kHz (70.6 ms). So latencyFrames() currently returns 576 for the RNNoise + 2 ms-gate case in your own test, when the real end-to-end figure at 24 kHz capture is closer to 576 + 6776.
Since this accessor exists precisely so callers can reason about delay, it should either add the SRC group delay (r8brain exposes it via getLatencyFrac() / getInLenBeforeOutPos()) or the comment should say plainly that SRC delay is excluded and roughly how large it is.
| return result; | ||
| int Resampler::process(const float* in, int numSamples, QByteArray& output) | ||
| { | ||
| output.clear(); |
There was a problem hiding this comment.
QByteArray::clear() releases the buffer, it doesn't just set the size to 0. Verified against the Qt 6.11.1 in this environment:
after reserve(4096) cap=4096
after clear() cap=0
after resize(0) cap=4096
So the contract documented at Resampler.h:31 — "Reserving output before the first call keeps steady-state conversion allocation-free" — does not hold: every call frees the caller's reservation and then reallocates as it grows, on the audio thread.
| output.clear(); | |
| output.resize(0); |
Same fix applies to RNNoiseFilter::process48kStereo()'s opening output.clear().
The sharpest instance is in TxVoiceProcessor::prepare(): it makes ten reserve() calls and then ends with reset(), which clear()s every one of them — so the reservations are dead on arrival and the first several blocks of every TX stream allocate. Switching those reset() clears to resize(0) fixes it without changing any observable state.
| } | ||
| const int inputFrames = canonicalInput.size() | ||
| / (kChannels * static_cast<int>(sizeof(int16_t))); | ||
| if (inputFrames <= 0 || inputFrames > m_maxInputFrames) { |
There was a problem hiding this comment.
An oversized block is dropped in full and silently — onTxAudioReady() returns on the false, so that audio never reaches the monitor taps, the PC mic meter, the scopes, or the Opus accumulator. Confirmed: prepare(48000, 1024) then a 2000-frame block returns false with zero output frames.
The old int16 strip had no such cap — it processed whatever the device handed it. m_maxInputFrames is 16384 in practice (341 ms at 48 kHz), so this only bites after a scheduling stall or a long macOS push-buffer accumulation, but that's exactly when you least want a silent hole in the transmitted audio.
Resampler::process() already chunks internally against m_maxBlockSamples, so the safer shape is to loop over the input in m_maxInputFrames chunks rather than reject. Failing that, a rate-limited qCWarning would at least make it diagnosable.
| // TxVoiceProcessor at the negotiated device rate. Do not call | ||
| // processStereoToStereo() here: that helper would average raw mic L/R and | ||
| // reintroduce the one-sided-channel 6.02 dB loss. | ||
| if (m_radeMode && m_radeTxNeedsResample && m_txResampler) { |
There was a problem hiding this comment.
Gating on m_radeMode means m_txResampler is now only advanced while RADE is active. Toggling RADE on mid-stream hands the r8brain instance a discontinuity — its internal filter history is whatever was left from the last RADE session (or the prewarm zeros), which will produce a short transient on the first RADE block.
On main it ran continuously so it was always warm. Cheap fix: call m_txResampler->reset() from wherever m_radeMode transitions to true — the new Resampler::reset() you added does exactly the right thing (clear + re-prewarm), and RADE enable is not a realtime callback.
|
Cisco CodeGuard — static analysis of this PR (21 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $11.7671 · model: claude-opus-5 |
There was a problem hiding this comment.
Thanks for this — it's a well-executed refactor, and the parts that usually go wrong in a rate-domain move are right here. I checked the stage ordering against main (RN2 → test tone → strip → mic gain → Quindar → limiter) and it's preserved exactly; the Comp's four-way enable condition (isEnabled || driveDb>0 || phaseRotatorStages>0 || limiterEnabled) is carried over verbatim; the EQ analyzer tap still fires on the bypassed-EQ path; and both analyzer call sites read eq->sampleRate(), so the FFT axis follows the move to 48 kHz on its own instead of silently halving every displayed frequency. The static_assert block pinning TxChainStage to TxVoiceProcessor::Stage is the right way to protect a persisted numeric contract, and the test file is the strongest part — block-boundary SRC invariance, TPDF DC bias and rail saturation, the non-finite poison guard, and determinism after reset() are exactly the properties that make this reviewable.
Two things I'd like fixed before merge, both about lifetime and edge handling rather than the DSP itself. The m_txPostDspMonitor semantic change is fine — I confirmed setTxPostDspMonitor() has no caller anywhere in the tree, so that pointer is always null and the comment's claim holds.
Would like fixed before merge
TxVoiceProcessorcaches a rawRNNoiseFilter*that can outlivem_rn2Tx;prepare()→reset()then dereferences freed memory (AudioEngine.cpp:7851).- An over-long capture block is rejected and the whole buffer silently discarded — no log, no health event (
TxVoiceProcessor.cpp:204).
Polish
std::min(leftOutputFrames, rightOutputFrames)would permanently desync L/R rather than surface a mismatch (TxVoiceProcessor.cpp:301).process48kStereo()has noRateDomainguard, unlike the one you added toprocess()(RNNoiseFilter.cpp:240).
Non-blocking notes
src/gui/MainWindow_DspApplets.cpp:741still carries a comment referencingAudioEngine::applyClientTxDspInt16, which this PR deletes. Worth a one-line touch-up so the last reference to the removed symbol doesn't linger.- The whole voice strip now runs on 2× the sample count, and non-48 kHz devices gain a second SRC. That's the deliberate trade here, not a defect — but since the test plan notes no Flex hardware was available, a CPU-per-block figure on the slowest supported target would be a useful thing to add to the PR body before this goes near a release.
- CodeGuard's findings on this PR are all in
src/gui/MainWindow.cppandthird_party/liquid-dsp/, neither of which this PR touches — nothing to action.
🤖 aethersdr-agent · cost: $8.4060 · model: claude-opus-5
| m_txVoiceProcessor->setStageOrder( | ||
| m_txChainPacked.load(std::memory_order_acquire)); | ||
| m_txVoiceProcessor->setMicGain(m_pcMicGain.load()); | ||
| m_txVoiceProcessor->setRnnoise(m_rn2Tx.get()); |
There was a problem hiding this comment.
TxVoiceProcessor stores this raw pointer persistently in m_processors.rnnoise, and TxVoiceProcessor::reset() dereferences it unconditionally (TxVoiceProcessor.cpp:147) — with no m_rnnoiseEnabled guard, unlike the use site in processWorkBuffer(). Since prepare() ends by calling reset(), that's a use-after-free:
- Enable RN2 TX →
m_rn2Txallocated. - Transmit once → this line caches
m_rn2Tx.get()inside the processor. - Disable RN2 TX (or toggle TX master bypass, which calls
setRn2TxEnabled(false)atAudioEngine.cpp:5119) →m_rn2Tx.reset()frees the filter; nothing clears the cached pointer. - Change the mic device / restart the TX stream →
startTxStream()callsm_txVoiceProcessor->prepare(...)→reset()→m_processors.rnnoise->reset()on freed memory.
Every other pointer in Processors is owned by AudioEngine for its whole lifetime, so m_rn2Tx is the only one with this exposure. Cheapest fix is to clear it where the object dies — in setRn2TxEnabled()'s else branch, m_txVoiceProcessor->setRnnoise(nullptr); immediately before m_rn2Tx.reset();. Belt-and-braces, gating reset()'s rnnoise call on m_rnnoiseEnabled would also help, but the ownership fix is the one that matters.
| } | ||
| const int inputFrames = canonicalInput.size() | ||
| / (kChannels * static_cast<int>(sizeof(int16_t))); | ||
| if (inputFrames <= 0 || inputFrames > m_maxInputFrames) { |
There was a problem hiding this comment.
Returning false here makes onTxAudioReady() return without processing, so an over-long block is discarded whole and silently. prepare() is called with maxInputFrames = 16384, which at 48 kHz is ~341 ms of capture; the macOS poll timer is 5 ms and Linux uses readAll(), so any stall longer than that (the exact condition TxCaptureHealthTracker exists to observe) drops every buffered sample instead of glitching. main had no such ceiling — Resampler::process() chunked internally, so long blocks were processed, just late.
Two options: loop the input in m_maxInputFrames slices and append the transport results, or keep the rejection but make it visible — a rate-limited qCWarning plus a TxCaptureHealthTracker event, so a "my mic cuts out" report has something to point at. Right now this failure is indistinguishable from a dead capture device.
| m_outputLeft.data(), frames48, m_resampledLeft24); | ||
| const int rightOutputFrames = m_outputRightResampler->process( | ||
| m_outputRight.data(), frames48, m_resampledRight24); | ||
| const int outputFrames = std::min( |
There was a problem hiding this comment.
If the two egress resamplers ever disagreed, taking the min doesn't just trim this block — the longer channel's extra samples are dropped for good (both byte arrays are fully rewritten next call), so L and R would be permanently offset by that many samples with no way to resync, and the duplicatedStereo() invariant the tests pin would quietly stop holding in the field.
They're matched instances fed identical frame counts, so this should be unreachable. That's the argument for making it loud rather than lenient: compare for equality and, on mismatch, reset() both resamplers (or log once) instead of silently absorbing the drift.
| const QByteArray& pcm48kStereo, QByteArray& output) | ||
| { | ||
| output.clear(); | ||
| if (!isValid() || pcm48kStereo.isEmpty()) { |
There was a problem hiding this comment.
Nice touch adding the RateDomain guard to process(), but this entry point has no matching one — a Legacy24k instance (the RX m_rn2) passed to TxVoiceProcessor::setRnnoise() would run its 24 kHz-domain state at 48 kHz with no complaint. A symmetric qCWarning + passthrough when m_rateDomain != RateDomain::Native48k would make the pairing self-enforcing.
Also worth updating the m_outAccum declaration comment in the header — it still says "24kHz stereo float output", which is no longer true in the Native48k domain.
|
Cisco CodeGuard — static analysis of this PR (21 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $8.5395 · model: claude-opus-5 |
|
Converted to draft while resolving review comments. |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Context first: I went and read the last float32 attempt
Before reviewing this I dug up what happened in v0.8.9 / v0.8.10 (2026-04-11), when the audio pipeline was moved to float32 end-to-end and had to be partly rolled back the same day. The conclusion matters for this PR, and it is favourable — so I want to state it up front rather than let this change be judged by association.
What actually broke back then:
| Commit | What happened |
|---|---|
502b9342 |
"Float32 audio pipeline: end-to-end from radio to speaker" — RX and TX converted to float32 |
| #1175 | Level meter pegged and oscillating, RF on key with no modulation, PC Audio TX dead. Flex 6300/8400, Win 10 + Ubuntu + Debian. Bisected by NF0T |
03c509f6 |
First "root cause" fix — mic gain/metering made float-native, f32→i16 moved to the Opus boundary. Shipped as a hotfix and did not work |
cfab9ea2 |
The real root cause: makeFormat() returned Float for the RX sink and the TX mic QAudioSource used the same helper. Mic hardware delivers Int16, so opening with Float double-converted → garbage |
661627a3 |
TX path restored to int16 end-to-end, on the reasoning that mic captures int16 and Opus encodes int16, so float was pure overhead |
| #1191 | Separately: CW decoder broken until an explicit f32→i16 conversion was added at the ggmorse boundary |
The April failure was a capture/transport format bug, not a DSP-rate bug. Every one of those boundaries is intact in this PR, and I checked each one specifically:
| April failure | Reintroduced here? |
|---|---|
Mic QAudioSource opened Float via the shared makeFormat() |
No — the explicit Int16 block and cfab9ea2's comment survive verbatim (AudioEngine.cpp:7222-7227) |
| Mic gain / metering reinterpreting float as int16 | No — both accumulatePcMicMeterInt16Stereo() taps are byte-identical to main and still read the int16 transport buffer |
| Opus fed the wrong format | No — transport is still int16 @ 24 kHz; data = transportInt16Stereo() |
| CW decoder / ggmorse (#1191) | No — untouched by the TX path |
| RADE / LPCNet | No — kept as an explicit fixed 24 kHz island |
| DAX / TCI clients | No — m_daxTxMode returns before the strip |
I also checked for a double mic-gain application (RADE branch vs TxVoiceProcessor::setMicGain) — applied once per path, correct.
So the shape is right: this floats the DSP island, not the capture/transport boundary, which is exactly the distinction April got wrong. 661627a3's reasoning ("nothing between mic and Opus, so float is overhead") was true in April and is no longer true — there is now EQ, Comp, Gate, DeEss, Tube, PUDU, Reverb and a limiter in between. I'm not going to hold a stale revert against this.
The one April lesson that does transfer is procedural, and it's Principle VIII, the principle this PR's title cites: the first root-cause fix was confidently wrong and shipped anyway, and it took a user bisect on real hardware to find the truth. This PR's test plan has "Behavior verified on a real radio" unchecked. That's the box that mattered last time.
Issue fit
No linked issue and no RFC. Reviewed against the PR's own stated intent: remove compounding SRCs and bit-depth truncations from the TX channel strip by pinning it to 48 kHz float with one high-quality egress SRC and one dithered quantization.
It does that, and the execution is good. The TxVoiceProcessor extraction is clean, stage ordering matches main exactly, the static_assert block pinning TxChainStage to Stage is the right way to protect a persisted numeric contract, RNNoiseFilter's new native-48k mode genuinely removes two SRCs from the RN2 path, and the test file is strong. I ran it: tx_voice_processor_test passes, as do the other 8 audio-suite tests.
I also refuted one thing that looked alarming on first read: m_work48.swap(m_rnnoiseOutput48) does not invalidate the stale frames48 used by every loop below it, because process48kStereo() always emits exactly pcm48kStereo.size() bytes (a zero-primed delay line). No overflow. Worth recording so nobody else re-raises it.
But the rate-domain move has a cost the PR does not account for, and one lifetime bug.
Blockers
1. +141 ms of one-way TX voice latency on Linux, where main has zero
Inline on TxVoiceProcessor.cpp:297. I measured this rather than reasoning about it — impulse through the real chain, no DSP stages enabled, so this is the floor cost of the rate domain itself:
| Mic capture rate | main |
this PR | Δ |
|---|---|---|---|
| 48 kHz (Windows/macOS try first) | 70.58 ms | 70.58 ms | 0 |
| 24 kHz (Linux tries first) | 0.00 ms (no SRC at all) | 141.17 ms | +141.17 ms |
| 44.1 kHz | 78.25 ms | 109.12 ms | +30.87 ms |
Block-size independent (240 / 512 / 1024 frames all identical). AudioFormatNegotiator::primaryRateOrder() returns {internalRate, 48000, 44100} for Linux input — 24 kHz first — so this is the default path on Linux, not a corner case.
2. latencyFrames() returns 0 for the SRCs, and the reasoning behind it is wrong
Inline on TxVoiceProcessor.cpp:434. Each CDSPResampler24 instance has 3388 samples of measured group delay, not zero. prewarm() removes the startup transient; it does not remove group delay, and the header comment conflates the two.
3. Use-after-free on the cached RNNoiseFilter*
Inline on TxVoiceProcessor.cpp:147. Deterministic, no thread race needed. This is the same shape as the SIGSEGV that 3a00e085 ("stop freeing the RN2 TX filter under the audio thread", Principle XII) was written to fix — and I should flag that that fix never landed: it lives only on the unmerged feat/eq-autofit-refcurves branch, so main still destroys m_rn2Tx on disable. Worth raising separately with the maintainer; this PR just needs to not add a second, easier-to-hit dereference.
4. Governance — this is an audio-pipeline architecture change with no RFC
GOVERNANCE.md, "What requires an RFC":
- Architecture changes — new threads, new signal routing patterns, changes to the audio pipeline
and:
Do not open a PR until the RFC issue is approved.
I can't find an RFC or issue for this, and "Refactor(audio): Move TX Voice DSP to 48 kHz Float" is about as squarely inside that clause as a change can be. I'm flagging it as a blocker because the rule is explicit, not because I think the idea is wrong — see the recommendation below. This is a maintainer call, not mine.
The RFC is also where the sequencing question belongs. The PR's stated benefit is avoiding "compounding aliasing artifacts and quantization noise", but the main payoff of a 48 kHz island is alias-free non-linear processing (tube, comp, drive), and the PR explicitly defers oversampling to a later change. So as it stands the full latency cost is paid now for a benefit that arrives later. That's a legitimate way to stage the work — it just needs to be a decision someone made on purpose. #4836 (WDSP-backed TX stages: 12-band EQ, CFC, Leveler) is open in the same area and should probably share the rate-domain decision.
Nits (non-blocking)
- The final limiter no longer bounds what reaches the radio. It now runs before the 48→24 SRC, so decimation overshoot escapes it and the only backstop is the hard clip in
quantizeTransportSample(). Measured worst case (full-scale square, pathological but bounding): post-SRC peak 1.2428 = +1.89 dBFS, 33% of samples clipped. Real limited speech will overshoot far less, but the guarantee is structurally gone. Conventional fixes are a small true-peak headroom on the limiter ceiling, or moving it after the SRC. QByteArray::clear()releases the allocation (I checked:reserve(65536)→clear()→capacity() == 0, whereasresize(0)keeps 65536). Soprepare()'s tenreserve()calls are undone by its own trailingreset(), and the "allocation-free" contract doesn't hold for the first block after every prepare/reset.resize(0)atResampler.cpp:28and inprocess48kStereo()fixes it. Inline onResampler.cpp:28.- Over-long capture blocks are dropped whole and silently — inline on
TxVoiceProcessor.cpp:204. m_txResampleris now only advanced in RADE mode — inline onAudioEngine.cpp:7786.process48kStereo()has noRateDomainguard while itsprocess()sibling just gained one (RNNoiseFilter.cpp:236). Unreachable today, cheap to make symmetric.src/gui/MainWindow_DspApplets.cpp:741still referencesAudioEngine::applyClientTxDspInt16, which this PR deletes.txInputNormalizationTo48k()hardcodes48000rather thanTxVoiceProcessor::kDspRate(AudioEngine.h:182). Astatic_assertin the .cpp would stop the two drifting.- Removing the
applyClient*TxInt16/Float32family is in scope — the Float32 variants had no callers onmain.
What I verified empirically vs. only read
Measured on this branch, built clean (Ninja, RelWithDebInfo, full all):
- Impulse-response latency through
TxVoiceProcessorat 24 / 44.1 / 48 kHz capture, three block sizes each — the table in blocker 1. - Bare
Resamplergroup delay per instance: 24→48 = 70.58 ms, 48→24 = 70.58 ms, 44.1→24 = 78.25 ms, 24→24 = 0.00 ms (confirmingmain's 24 kHz baseline really is SRC-free). - Post-SRC overshoot above the limiter ceiling: +1.89 dBFS worst case.
QByteArray::clear()vsresize(0)capacity behaviour on this Qt build.tx_voice_processor_testplus the audio suite: 9/9 pass.
Read, not run: the use-after-free in blocker 3 (traced through the call graph — I did not build an ASan repro), the April history (reconstructed from 502b9342 / 03c509f6 / cfab9ea2 / 661627a3, issues #1175 and #1191, and the v0.8.9/v0.8.10 CHANGELOG entries), and all governance checks.
Not verified by anyone yet: on-air behaviour. No Flex hardware was available to the author, and I did not key a transmitter for this review either.
| m_outputLeft[static_cast<size_t>(frame)] = work[frame * 2]; | ||
| m_outputRight[static_cast<size_t>(frame)] = work[frame * 2 + 1]; | ||
| } | ||
| const int leftOutputFrames = m_outputLeftResampler->process( |
There was a problem hiding this comment.
Blocker: this SRC pair adds 141 ms of one-way TX voice delay on Linux, where main has none.
I measured it rather than estimating — an impulse through the real chain with no DSP stages enabled, so this is the floor cost of the rate domain itself:
=== bare Resampler group delay (r8brain CDSPResampler24) ===
Resampler 24000 -> 48000 : peak at out[3388] = 1694.0 input samples = 70.58 ms
Resampler 48000 -> 24000 : peak at out[1694] = 3388.0 input samples = 70.58 ms
Resampler 44100 -> 24000 : peak at out[1878] = 3450.8 input samples = 78.25 ms
Resampler 24000 -> 24000 : peak at out[0] = 0.0 input samples = 0.00 ms
=== TxVoiceProcessor end-to-end, no DSP stages enabled ===
input 48000 Hz, block 512 fr : peak at transport frame 1694 = 70.58 ms
input 24000 Hz, block 512 fr : peak at transport frame 3388 = 141.17 ms
input 44100 Hz, block 512 fr : peak at transport frame 2619 = 109.12 ms
Identical at 240, 512 and 1024-frame blocks, so it is group delay and not buffering.
Against main:
| Mic capture rate | main |
this PR | Δ |
|---|---|---|---|
| 48 kHz | 70.58 ms (one 48→24) | 70.58 ms | 0 |
| 24 kHz | 0.00 ms | 141.17 ms | +141.17 ms |
| 44.1 kHz | 78.25 ms | 109.12 ms | +30.87 ms |
The 24 kHz row is the one that matters. On main, m_txNeedsResample = (m_txInputRate != 24000) (AudioEngine.cpp:7681) is false at 24 kHz, so m_txResampler is never even constructed and the int16 strip runs with no SRC in the path at all. And AudioFormatNegotiator::primaryRateOrder() returns {internalRate, 48000, 44100} for Linux input — 24 kHz is the first rate tried. So on Linux this is the default path, and it goes from the best case on any platform to the worst.
141 ms one-way is past where operators notice: it is audible as monitor/sidetone lag, it stacks on top of radio and network delay for SmartLink, and it changes the feel of VOX and quick-break SSB work.
Worth noting the flip side, because it points at a fix that helps everywhere: main already pays 70.58 ms on Windows and macOS for its single 48→24 SRC. That is a lot for a voice path, and it comes from CDSPResampler24's filter — linear phase, 180.15 dB attenuation (third_party/r8brain/CDSPResampler.h:804). A TX voice path bound for a 3 kHz SSB channel does not need 180 dB of stopband or strict linear phase. Options, roughly in increasing order of effort:
- construct these two SRCs with a wider
ReqTransBand/ lowerReqAtten(both areCDSPResamplerctor parameters), or usefprMinPhase, which is specifically documented there as the low-delay option; - skip the 48 kHz island entirely when the capture rate is already 24 kHz and no stage that benefits from the higher rate is enabled — at which point the strip runs where it does today;
- keep the island but make the rate a property of the transport rather than pinning 48 → if the radio takes 24 kHz, the extra octave only pays off once oversampling lands, which this PR defers.
Whichever way it goes, the number belongs in the PR body — this is the trade the change is actually making.
| return m_postStrip48; | ||
| } | ||
|
|
||
| int TxVoiceProcessor::latencyFrames() const noexcept |
There was a problem hiding this comment.
Blocker: this reports 0 for the SRCs, and the justification in the header comment doesn't hold.
The header says:
r8brain is configured to consume its integer startup latency; reverb pre-delay is an artistic wet-path parameter rather than whole-signal latency.
prewarm() (Resampler.cpp:163) does consume the startup latency — but that removes the transient at the start of a stream, not the filter's group delay. Those are different things, and the comment in prewarm() is only claiming the former:
Feeding zeros here consumes that startup latency so the first real audio sample produces output immediately, removing the transient that would otherwise appear at the start of every audio session.
Measured, each CDSPResampler24 instance carries 3388 samples of group delay (70.58 ms at 48 kHz), so latencyFrames() under-reports by 3388 frames per SRC instance — 3388 with a 48 kHz mic, 6776 with a 24 kHz one. It correctly accounts for RNNoise's 480 and the gate lookahead, which makes the omission easy to miss: the function looks like it is doing careful accounting.
Nothing consumes this today, so it isn't currently visible — but it is a public accessor whose whole purpose is to be trusted by something later (monitor alignment, QSO-recorder sync, an ALC or VOX timing budget), and a latency accessor that silently omits the largest term in the chain is worse than no accessor.
Either add the SRC delay, or make the omission explicit rather than implicit:
int TxVoiceProcessor::latencyFrames() const noexcept
{
// Egress SRC first: it is the LARGEST term, not a free one. r8brain's
// prewarm() consumes the startup TRANSIENT (see Resampler::prewarm),
// which is a different thing from the linear-phase filter's GROUP DELAY
// — measured at 3388 samples per CDSPResampler24 instance, i.e. 70.58 ms
// at 48 kHz. Counting it as zero under-reported the chain by 3388 frames
// with a 48 kHz mic and 6776 with a 24 kHz one.
int frames = m_outputLeftResampler
? m_outputLeftResampler->latencyFrames() : 0;
if (m_inputResampler) {
// Input SRC delay is in input samples; express it in DSP frames.
frames += static_cast<int>(std::lround(
m_inputResampler->latencyFrames()
* static_cast<double>(kDspRate) / m_inputRate));
}
frames += m_rnnoiseEnabled && m_processors.rnnoise
&& m_processors.rnnoise->isValid()
? 480
: 0;…keeping the existing gate-lookahead loop below unchanged. That needs a small Resampler::latencyFrames() accessor; r8brain exposes what you need via getLatency() / getLatencyFrac() on CDSPProcessor, and my measurement above is a good cross-check for whatever it returns.
| if (m_processors.rnnoise) { | ||
| m_processors.rnnoise->reset(); | ||
| } |
There was a problem hiding this comment.
Blocker: this dereferences a raw RNNoiseFilter* that can already be freed.
m_processors.rnnoise is refreshed only from the audio callback (AudioEngine.cpp:7851, setRnnoise(m_rn2Tx.get())), but AudioEngine owns the filter in a unique_ptr and destroys it on disable (AudioEngine.cpp:6786, m_rn2Tx.reset()). reset() here is reached from prepare(), which runs on the caller thread from startTxStream() (AudioEngine.cpp:7357 and :7489).
No thread race required — this is a deterministic sequence:
- Enable RN2 TX →
m_rn2Txconstructed. - Start TX → first audio callback caches the pointer here.
- Stop TX → no more callbacks, so nothing will ever clear the cached pointer.
- Disable RN2 TX →
m_rn2Tx.reset()frees the filter.m_processors.rnnoisenow dangles. - Start TX again →
startTxStream()→prepare()→reset()→m_processors.rnnoise->reset()on freed memory.
The guard above (if (m_processors.rnnoise)) doesn't help; the pointer is non-null and stale. Note the isValid() checks elsewhere don't protect this path either — isValid() is itself a call on the freed object.
This is the same shape as the SIGSEGV that 3a00e085 ("fix(audio): stop freeing the RN2 TX filter under the audio thread. Principle XII.") was written to fix. Its remedy was to retain the filter for the process lifetime so the published pointer can never dangle — but that commit never landed; it only exists on the unmerged feat/eq-autofit-refcurves branch, so main still frees on disable. That pre-existing hazard isn't this PR's to fix, but this PR shouldn't add a second dereference site that doesn't even need a race to hit.
Cheapest correct fix here is to stop caching an owned pointer across calls — clear it whenever the owner might have dropped it:
| if (m_processors.rnnoise) { | |
| m_processors.rnnoise->reset(); | |
| } | |
| if (m_processors.rnnoise) { | |
| m_processors.rnnoise->reset(); | |
| } | |
| // Do not keep the pointer across a prepare/reset boundary. AudioEngine | |
| // owns the filter in a unique_ptr and destroys it on RN2 disable, while | |
| // this pointer is only refreshed from the audio callback — so after | |
| // stop-TX / disable-RN2 / start-TX it would be stale here with nothing | |
| // to clear it. The next callback republishes it before first use. | |
| m_processors.rnnoise = nullptr; |
A std::weak_ptr, or adopting 3a00e085's retain-for-lifetime approach, would both also work — but this one is local to the class and needs no ownership change.
| if (inputFrames <= 0 || inputFrames > m_maxInputFrames) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Non-blocking, but a bad failure shape: an over-long capture block is rejected here and AudioEngine.cpp:7854 turns that into a bare return — so the whole burst is dropped with no log, no health event, no meter movement. Silent loss of transmit audio is exactly the kind of thing that gets diagnosed as "the radio is broken".
It's also a behaviour change rather than a new limit: main had no cap, because Resampler::process() chunks internally against m_maxBlockSamples. 16384 frames is 341 ms at 48 kHz, so this needs a real capture stall to reach — unlikely, not impossible.
Either chunk (matching what Resampler already does), or at minimum make it visible:
| if (inputFrames <= 0 || inputFrames > m_maxInputFrames) { | |
| return false; | |
| } | |
| if (inputFrames <= 0 || inputFrames > m_maxInputFrames) { | |
| // Dropping transmit audio silently is worse than the overrun itself: | |
| // main chunked instead of rejecting (Resampler::process bounds itself | |
| // against m_maxBlockSamples), so this is a new way to lose a burst. | |
| // Rate-limited by the caller's own block cadence being the only way | |
| // to get here at all. | |
| qCWarning(lcAudio) | |
| << "TxVoiceProcessor: capture block of" << inputFrames | |
| << "frames exceeds the prepared maximum" << m_maxInputFrames | |
| << "- dropping" << (1000.0 * inputFrames / m_inputRate) << "ms of TX audio"; | |
| return false; | |
| } |
(needs the lcAudio category include; if you'd rather keep this class free of Qt logging, returning a distinct status the caller can log is equally good.)
| return result; | ||
| int Resampler::process(const float* in, int numSamples, QByteArray& output) | ||
| { | ||
| output.clear(); |
There was a problem hiding this comment.
Non-blocking: clear() releases the allocation, so the reservations TxVoiceProcessor::prepare() makes are dead on arrival.
Verified on this Qt build:
after reserve(65536): capacity=65536
after clear(): capacity=0
after resize(0): capacity=65536
So prepare()'s ten reserve() calls are undone by its own trailing reset() (which clear()s the same buffers), and the first block after every prepare/reset re-allocates on the realtime path. Steady state is fine afterwards because the later resize()s fit inside the regrown capacity — so the cost is bounded, not per-block. But the header's "keeps steady-state conversion allocation-free" only becomes true with:
| output.clear(); | |
| output.resize(0); |
Same swap is worth making at the head of RNNoiseFilter::process48kStereo(), and in TxVoiceProcessor::reset() for the buffers prepare() reserves — otherwise reset() keeps quietly undoing prepare().
| // TxVoiceProcessor at the negotiated device rate. Do not call | ||
| // processStereoToStereo() here: that helper would average raw mic L/R and | ||
| // reintroduce the one-sided-channel 6.02 dB loss. | ||
| if (m_radeMode && m_radeTxNeedsResample && m_txResampler) { |
There was a problem hiding this comment.
Non-blocking: gating on m_radeMode means m_txResampler is now only advanced while RADE is active, where on main (m_txNeedsResample && m_txResampler) it was fed continuously for the whole TX session.
r8brain is a stateful streaming filter, so switching into RADE mid-transmission now resumes it with filter history from whenever RADE was last active — or from prewarm() on the first switch. The audible result is a discontinuity at the mode change rather than anything persistent, and on main there was none because the filter never went idle.
This PR conveniently adds the fix: call m_txResampler->reset() on the RADE-entry edge so the filter starts each RADE burst from a defined state. (Resampler::reset() is explicitly documented as "call between independent streams, never from the realtime process callback" — the mode edge is exactly that.)
| return output; | ||
| } | ||
|
|
||
| int RNNoiseFilter::process48kStereo( |
There was a problem hiding this comment.
Non-blocking, symmetry: process() gained a RateDomain guard in this PR —
if (m_rateDomain != RateDomain::Legacy24k) {
qCWarning(lcRn2) << "RNNoiseFilter: 24 kHz process() called on native-48 kHz instance";
return pcm24kStereo;
}— but process48kStereo() has no matching check. A Legacy24k instance fed 48 kHz audio here would silently mix rate domains in the shared m_inAccum / m_outAccum buffers, which is the harder direction to debug of the two.
Unreachable today (the only 48 kHz caller constructs with Native48k), so purely defensive — but the guard you already wrote is one line and this is the side that fails quietly.
…ed corrections (including additional tests)
…t Mismatches. Principle XI.
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Request changes — 8-angle audio-stack review (Flex / Hermes-Lite 2 / Icom CI-V)
We ran an eight-angle review against the audio-regression history of all three radio stacks (line-by-line, removed-behavior audit of the ~480 deleted AudioEngine lines, cross-file trace into every backend consumer, plus reuse/simplification/efficiency/altitude/conventions passes; every finding survived adversarial verification). The review ran against 365d275e; mid-review the fix push through 3b870e60 landed, so we re-verified everything against the new head — assessment of those fixes below, then the findings that still stand.
The fix push (114fcae1..3b870e60) — verified sound
We independently confirmed all of the previously-reported blockers before reading the existing reviews, and we've now read each fix commit: the RN2 lifetime fix is the right shape (association cleared at the ownership seam in setRn2TxEnabled(), per-block re-injection removed, reset() guarded), the oversized-block cap is gone (buffers grow for the exceptional case instead of dropping audio), resize(0) restores the reservation contract, the RADE resampler reset-pending handoff is correctly ordered, reconcileEgressFrameCounts() warns once and resets both channels, and latencyFrames() now includes SRC group delay with the 12% voice transition band. Nice turnaround.
Findings that still stand on 3b870e60 (inline comments have details)
kTransportRate = 24000hard-codes the Flex wire into the shared processor (TxVoiceProcessor.h:32).Hl2Backend::submitTxAudiohard-rejects anything but 24000 andHl2TxDspupsamples ×2 back to 48 kHz;IcomCivBackendresamples the seam feed back tokRadioAudioRateHz = 48000. For two of three backends the chain is 48 k float → SRC 24 k → dither+int16 → dequantize → SRC 48 k: two avoidable SRCs and a quantization at a rate no seam wire uses. Parity withmain, so not a blocker — butdocs/architecture/audio-pipeline.md's "apply dither and quantize only once" claim only holds for Flex, andRadioCapabilities(which already hastakesTxAudioOverSeam) has notxAudioRateHzto carry the right fix. The doc should scope its claim, and the follow-up this PR's summary anticipates should parameterize the transport rate per consumer.- Unconditional TPDF dither ends digital silence (
TxVoiceProcessor.cpp:342) — deliberate and tested, but it needs an explicit maintainer sign-off: a muted mic now emits ±1-LSB samples ~25 % of the time, the PC-mic meter floor rises from −150 to −90.3 dBFS, and the noise ridesremote_audio_txwhenever the mic stream is open. resampling_activetelemetry key silently inverted (AudioEngine.cpp:2757) — same JSON key, opposite meaning for both common mic rates. Support-bundle diffs across versions will misclassify the TX route;DeviceDiagnosticsrenamed its sibling key, this one should be renamed too.- ~100-line duplicate of the RNNoise frame-accumulation core (
RNNoiseFilter.cpp:236) — the RateDomain guards are now symmetric (thanks), but the stateful FIFO machinery still exists twice and has already drifted (the legacy path's L/R length-mismatch warning is absent from the new copy). - Remaining per-block mirroring (
AudioEngine.cpp:7866) — the rnnoise pointer is fixed, but stage order, mic gain, rnnoise-enable, and a constantsetMeasurementCaptureEnabled(false)are still re-pushed every 10 ms block; sync-by-convention at one call site is the silent-config-drift shape. - Second egress resampler is pure waste on the production path (
TxVoiceProcessor.cpp:323) — L/R are byte-identical by construction (verified:ClientReverbdoes not actually decorrelate; its 23-sample stereo spread is allocation headroom only, and the tests pin the invariant), so the dual instances double the most expensive DSP in the block to produce identical outputs. CPU/memory only — it won't reduce latency.
Docs left inconsistent (files not in this diff, so no inline anchor)
docs/architecture/pipelines.md:92still describes the old TX chain (monitor tap pre-gain, no 48 kHz domain) and now contradicts this PR's ownaudio-pipeline.md.ClientQuindarTone.h:131still says TXprocess()"runs at the radio's 24 kHz TX rate" (now 48 k);ClientReverb.h:22's ~12 kB budget doubled at 48 kHz. (TheMainWindow_DspApplets.cpppointer to the deleted function was already fixed in3b870e60.)
Verified as safe across all three stacks (for the record)
The removed-behavior audit confirmed no regression in: TCI TX client-declared-rate resampling (#3892 preserved), HL2/Icom seam feeds (byte-parity 24 kHz int16 via submitTxAudio), mic suppression during TCI-client TX (#2982), mode gating (RADE/DAX/WSPR/modem still bypass the voice strip — the #1279 class), every tap (QsoRecorder, WAVE scope, meters, Opus), r8brain MaxInLen chunking (#2114), and arbitrary/Bluetooth mic capture rates via the ingress SRC. The deleted float32 helpers had zero callers; stage ordering, EQ analyzer rate-following, and Opus settings match the docs.
This is a well-built refactor headed the right way, and the fix push addressed the hard blockers properly. The request-changes is for the dither sign-off (2), the telemetry key (3), and scoping the docs' single-quantization claim to Flex (1) — the rest are strong-recommendation follow-ups.
| class TxVoiceProcessor { | ||
| public: | ||
| static constexpr int kDspRate = 48000; | ||
| static constexpr int kTransportRate = 24000; |
There was a problem hiding this comment.
Transport rate is a per-consumer property, not a pipeline constant. Hl2Backend::submitTxAudio hard-rejects sampleRateHz != 24000 (Hl2Backend.cpp:3025) and Hl2TxDsp upsamples ×2 back to 48 kHz for the Protocol-1 wire; IcomCivBackend resamples the seam feed back to kRadioAudioRateHz = 48000 (IcomCivBackend.cpp:1186). With kTransportRate pinned here, both non-Flex stacks run 48 k float → SRC 24 k → TPDF dither + int16 → dequantize → SRC 48 k: two avoidable SRCs and a mid-chain quantization at a rate neither wire uses.
This is parity with main (the seam already fed 24 k), so not a blocker for this PR — but audio-pipeline.md's "apply dither and quantize only once after the final 48-to-24 kHz SRC" claim only holds for Flex remote_audio_tx, and the doc should say so. The durable shape for the follow-up: declare txAudioRateHz in RadioCapabilities (next to the existing takesTxAudioOverSeam, RadioCapabilities.h:279) and parameterize this egress SRC + quantizer per consumer, so HL2/Icom can stay 48 k float end-to-end.
There was a problem hiding this comment.
This is an accurate assessment, but is largely a condition that pre-exists this PR as stated, since the previous pipeline was 24kHz-48kHz. In other words, this isn't a regression introduced by this PR, it just surfaces an existing condition and makes it more relevant. Transport efficiency improvements are likely outside of the scope of this PR, though probably should be addressed in a discrete, quick follow-on PR.
| for (int frame = 0; frame < outputFrames; ++frame) { | ||
| outputFloat[frame * 2] = left[frame]; | ||
| outputFloat[frame * 2 + 1] = right[frame]; | ||
| const float ditherLsb = nextTpdfDitherLsb(); |
There was a problem hiding this comment.
Behavior change that needs an explicit sign-off: digital silence no longer exists on any int16 egress. With unconditional TPDF in (−1, 1), zero input quantizes to ±1 on ~25 % of samples (|dither| ≥ 0.5 rounds away from zero). main truncated to exact 0 for silence. Downstream there is no silence gate: the same dithered buffer feeds accumulatePcMicMeterInt16Stereo — the PC-mic meter floor rises from −150 dBFS to −90.3 dBFS peak — and the always-on Opus remote_audio_tx accumulator, so a muted mic now puts continuous ±1-LSB noise on the wire whenever the stream is open.
We can see it's deliberate (the TPDF determinism tests pin it), and dither-below-quantization is defensible — but it silently changes silence-detection semantics for meters, squelch-style tooling, and any future automation assertion of "mic muted ⇒ silence". Worth either gating dither on a true-zero block (cheap: skip when the block's peak is exactly 0.0f) or an explicit maintainer ACK in the PR description.
There was a problem hiding this comment.
I don't think a whole-block peak test is necessary. We can continue advancing PRNG once per output frame, but if both float values for that frame are zero, we just output exact int16 zero, otherwise, we can apply the TPDF.
| tx["channel_count"] = txRunning ? QJsonValue(m_txInputChannels) : QJsonValue(); | ||
| tx["sample_format"] = txRunning ? QStringLiteral("Int16") : QString(); | ||
| tx["resampling_active"] = txRunning ? QJsonValue(m_txNeedsResample) : QJsonValue(); | ||
| tx["resampling_active"] = txRunning |
There was a problem hiding this comment.
resampling_active keeps its JSON key but inverts its meaning for both common mic rates. A 48 kHz mic previously reported true (48→24 SRC ran) and now reports false — even though the 48→24 egress SRC always runs. A 24 kHz mic previously reported false (no SRC at all) and now reports true. Anyone diffing support bundles across versions, or tooling keyed on this field, misclassifies the TX route — the #3914 telemetry-mislabel class this codebase has paid for before. DeviceDiagnostics.cpp did the right thing and renamed its sibling key to voice_normalizing_to_48k; this one should be renamed too (e.g. voice_input_normalizing_to_48k), with a note that the egress SRC is unconditional.
| return output; | ||
| } | ||
|
|
||
| int RNNoiseFilter::process48kStereo( |
There was a problem hiding this comment.
The RateDomain guards are now symmetric after e83a4b22 — good. The remaining maintenance cost is that process48kStereo() duplicates ~100 lines of process()'s stateful frame-accumulation core (deinterleave/×32768 scale, per-channel m_inAccum fill, 480-frame loop with the dry-mix branch, leftover carry, m_outAccum drain) rather than sharing it. The copies have already drifted: the legacy path's L/R length-mismatch warning (documented at the top of process() as "say so once instead of drifting quietly") is absent from this one. Suggested shape: one native-48 k accumulation core, with the legacy 24 k RX entry point becoming a thin resampling adapter around it — then a future buffering fix can't miss one copy.
Small things while here: the one-arg QByteArray wrapper above has no callers, and the inner float* output shadows the QByteArray& output parameter.
There was a problem hiding this comment.
This would require an RNNoise refactor that is outside of the scope of this PR. It would require careful latency, arbitrary-block size, and stereo testing. Recommended as subsequent RNNoise refactor PR.
| // stereo r8brain pair performs the sole 48 -> 24 kHz egress conversion; | ||
| // quantization happens once at the unchanged Opus/VITA boundary. | ||
| // Radio-authoritative mode/passband filtering remains downstream. | ||
| m_txVoiceProcessor->setStageOrder( |
There was a problem hiding this comment.
Moving setRnnoise() to the ownership seam in ba804a6e was the right fix — but the same shape argues for finishing the job on the rest of this block: stage order, mic gain, rnnoise-enable, and setMeasurementCaptureEnabled(false) are still re-pushed into the processor on every 10 ms block. Sync-by-convention at one call site means any future TxVoiceProcessor setting not remembered here silently stays at its construction default (the stale-state class this codebase knows from #3459), and the measurement flag is a constant in production. Suggested: pass stage order + mic gain as parameters to processCapturedInt16() (they're per-block inputs, not state), sync setRnnoiseEnabled where m_rn2TxEnabled changes, and drop the per-block measurement-flag call.
| m_outputLeft.data(), frames48, m_resampledLeft24); | ||
| const int rightOutputFrames = m_outputRightResampler->process( | ||
| m_outputRight.data(), frames48, m_resampledRight24); | ||
| const int outputFrames = reconcileEgressFrameCounts( |
There was a problem hiding this comment.
reconcileEgressFrameCounts() resolves the silent-desync concern — but on the production path the second resampler is pure waste. L and R are byte-identical by construction: processCapturedInt16 duplicates mono into both channels, every strip stage is symmetric — we verified ClientReverb does not decorrelate (recacheIfDirty computes one shared combLen[i]/allpassLen[i] for both m_chL and m_chR; the header's "stereo-spread by 23 samples" is allocation headroom only) — and the tests pin "mono voice remains duplicated stereo through egress". So the dual instances double the most expensive DSP in the block (plus r8brain state memory) to produce two identical outputs, and the mismatch branch becomes structurally unreachable.
Resample once mono and duplicate, falling back to the pair only if a genuinely decorrelating stage ever ships. (CPU/memory only — to be clear, this doesn't reduce group-delay latency.) Also worth sharing one helper with AudioEngine::resampleStereo, which is the same PreservePan scaffolding hand-rolled a second time.
There was a problem hiding this comment.
I believe this to also be outside of the scope of this PR. This should be a broader discussion, as it would probably require narrowing the public contract.
|
Regarding the Limiter -> SRC -> Quantization pipeline, and overshoots no longer being hard-limited (which by the way, they weren't before, precisely, because the final limiter isn't a guaranteed brickwall limiter), it is preferable to simply allow overshoots into the quantization (and subsequently, the encoder), because the alternative; even 1dB of headroom, absent any radio-side compensation, results in a 21% decrease in PEP. Occasional speech overshoots are preferable to drastically reduced transmitter power output (especially for the tightly integrated Aurora). |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Re-review: two of four blockers closed. Two remain, and one of them is the big one.
Checked each at this head rather than against commit subjects.
| # | Blocker | Status |
|---|---|---|
| 1 | +141 ms one-way TX voice latency on Linux (24 kHz capture) | Open |
| 2 | latencyFrames() returns 0 for the SRCs |
Fixed |
| 3 | Use-after-free on the cached RNNoiseFilter* |
Fixed (deterministic case; see the residual below) |
| 4 | Audio-pipeline architecture change with no RFC | Open — maintainer call |
Blocker 2 — closed properly
latencyFrames() now sums real group delay for both directions instead of asserting zero:
frames += lround(m_inputResampler->groupDelayInputFrames() * kDspRate / m_inputRate);
frames += lround(m_outputLeftResampler->groupDelayInputFrames() * kDspRate
/ m_outputLeftResampler->srcRate());Querying the resampler for its own delay rather than hardcoding the 3388 I measured is the better fix — it stays correct if r8brain's filter design changes. And rate-scaling each term into DSP-domain frames is right, since the two resamplers run at different input rates.
Blocker 3 — closed for the case that actually bites
AudioEngine.cpp:6811-6812 now nulls before destroying:
m_txVoiceProcessor->setRnnoise(nullptr);
m_rn2Tx.reset();and the invalid-construction path at :6799-6800 does the same. That removes the deterministic dangling dereference, which was the shape of the SIGSEGV 3a00e085 was written for.
Residual, non-blocking: m_processors.rnnoise is a plain pointer written from the main thread and read from the audio thread with no synchronisation, and m_rnnoiseEnabled is a plain bool on the same footing. The guard at :276 is m_rnnoiseEnabled && m_processors.rnnoise && ->isValid(), so an audio block that reads a stale true/non-null pair just before reset() still has a window. Narrow — and much narrower than before, since the null now lands first — but it is the same class of bug as the original, and std::atomic<RNNoiseFilter*> with release/acquire would close it outright. Worth doing given this is the second time this pointer has caused a crash.
Separately, the finding still stands that 3a00e085 never landed on main — it lives only on the unmerged feat/eq-autofit-refcurves branch, so main still destroys m_rn2Tx on disable with no equivalent guard. That is worth its own issue regardless of this PR.
The other four commits
"Removes unused process48kStereo() overload" closes the RateDomain-guard-symmetry nit by deletion, which is the better answer. "Defer Stereo Egress SRC Recover Outside Realtime Callback. Principle XI" is the right instinct — SRC re-init is not realtime-safe work. "Adds Coverage for Lower Sample Rate Edge Cases" is welcome, and note it is exercising exactly the rate domain blocker 1 is about.
Still blocking
1. The +141 ms on Linux is unchanged. TxVoiceProcessor::prepare() still creates the input resampler for any inputRate != kDspRate (:38), and AudioFormatNegotiator::primaryRateOrder() still returns {internalRate, 48000, 44100} for Linux input — 24 kHz first. So the default Linux path still pays 24→48→…→48→24 where main pays nothing at all. My measurements stand:
| Mic capture rate | main |
this PR | Δ |
|---|---|---|---|
| 48 kHz | 70.58 ms | 70.58 ms | 0 |
| 24 kHz (Linux default) | 0.00 ms | 141.17 ms | +141.17 ms |
| 44.1 kHz | 78.25 ms | 109.12 ms | +30.87 ms |
For context on why 141 ms is the number that matters rather than a number: it is more than the entire 30–100 ms round trip that CwSidetoneGenerator exists to beat, added to the operator's own voice path. Options are inline.
4. Still no RFC or linked issue. closingIssuesReferences is empty and the body references neither. GOVERNANCE.md names "changes to the audio pipeline" explicitly under "What requires an RFC", followed by "Do not open a PR until the RFC issue is approved."
I want to be precise that this is not me re-litigating the engineering — the design is good and I said so at length last time. It is that the rule is explicit, this change is squarely inside it, and the sequencing question the RFC exists to settle is real and unanswered: the full latency cost is paid now, while the main payoff of a 48 kHz island (alias-free non-linear processing) arrives with the oversampling change this PR defers. #4836 is open in the same area and should share the rate-domain decision.
@ten9876 — this is your call, and it is the one that decides whether the rest of the review matters.
Still unchecked: "Behavior verified on a real radio"
Same box as last time. Given the April 2026 float32 rollback was found by a user bisect on real hardware after a confidently-wrong root-cause fix shipped, and given this PR's title cites Principle VIII, this is the box I would want ticked before merge more than any other.
Recommendation
Request changes. Blockers 2 and 3 are properly closed and the fixes are better than what I asked for. Blocker 1 is a real latency regression on the default Linux path with no mitigation yet, and blocker 4 is a governance decision only @ten9876 can make — and it should probably be made first, since an RFC is also where the blocker-1 trade-off gets decided rather than reviewed line by line.
(Automated code-review pass was not run for this one; the audit above is a re-check of the four blockers at this head. The latency table is from my earlier impulse measurements through the real chain, re-confirmed as still applicable because the code path that produced it is unchanged.)
|
|
||
| m_inputRate = inputRate; | ||
| m_maxInputFrames = maxInputFrames; | ||
| if (inputRate != kDspRate) { |
There was a problem hiding this comment.
Blocker 1, unchanged. This is the line that costs Linux +141 ms.
AudioFormatNegotiator::primaryRateOrder() returns {internalRate, 48000, 44100} for Linux input, so a Linux mic negotiates 24 kHz first. On main that means no SRC anywhere in the TX voice path — capture rate equals transport rate equals DSP rate, and the measured one-way latency is 0.00 ms. Here it becomes 24→48 in, 48→24 out, two CDSPResampler24 instances at ~3388 samples group delay each: 141.17 ms, block-size independent.
That is not a corner case, it is the default configuration on one of three supported platforms, and 141 ms of added latency in the operator's own voice path is more than the entire radio round trip the local CW sidetone exists to avoid.
Three ways out, roughly in order of how well they fit the PR's goal:
- Bypass the rate domain when nothing needs it. If no non-linear stage is enabled, 48 kHz buys nothing — EQ/comp/gate are as correct at 24 kHz. Prepare at
inputRateand skip both SRCs:The cost is re-preparing the chain when the operator toggles a non-linear stage, which is already a non-realtime operation.// A 48 kHz island only pays for itself when a non-linear stage is running. // With none enabled, running at the capture rate is bit-for-bit as good and // avoids two CDSPResampler24 instances (~3388 samples group delay each — // 141 ms round trip at a 24 kHz Linux capture, where main pays zero). const bool needsOversampling = anyNonLinearStageEnabled(); const int dspRate = needsOversampling ? kDspRate : inputRate;
- Prefer 48 kHz at capture on Linux, so the negotiator lands where the DSP wants to be and the input SRC disappears. Changes
primaryRateOrder(), which is a wider blast radius and wants its own justification. - Accept it as a documented trade — legitimate, but then it belongs in the RFC (blocker 4) as an explicit decision with the number in it, not as an emergent property of the rate choice.
Worth noting your own "Adds Coverage for Lower Sample Rate Edge Cases" commit exercises exactly this domain — so the test scaffolding to prove a bypass is correct is already in place.
There was a problem hiding this comment.
This appears to be based on the earlier 2% SRC profile. Since 114fcae, the TX voice path uses a tested 12% profile preserving response through 10 kHz. Current configured-path SRC delay is 788 DSP frames / 16.4 ms at 24 kHz capture, not 6,776 frames / 141.17 ms; 48 and 44.1 kHz measure approximately 8.2 and 12.8 ms respectively. The Linux 24 kHz preference and resulting +16.4 ms relative to main are real and documented, but the quoted blocker magnitude is stale.
There was a problem hiding this comment.
Just to be certain, I performed an independent impulse probe through the repository's actual r8brain implementation, including its prewarming behavior and 10 ms streaming blocks:
Current 12% profile
24 kHz Capture: 16.42 ms
44.1kHz Capture: 12.83 ms
48kHz Capture: 8.21 ms
Previous 2% profile
24 kHz Capture: 141.17 ms
44.1kHz Capture: 109.12 ms
48kHz Capture: 70.58 ms
| return m_postStrip48; | ||
| } | ||
|
|
||
| int TxVoiceProcessor::latencyFrames() const noexcept |
There was a problem hiding this comment.
Blocker 2 closed, and better than the fix I suggested.
I expected the measured 3388 to be hardcoded with a comment. Asking each resampler for groupDelayInputFrames() instead means the number stays correct if r8brain's filter design or the transition-band settings change — the delay becomes a property of the object rather than a constant someone has to remember to re-measure.
The rate scaling is the part that would have been easy to get wrong: the input and output resamplers run at different input rates, so both terms are converted into DSP-domain frames (* kDspRate / m_inputRate and * kDspRate / srcRate()) rather than summed raw. Correct.
The header comment that conflated prewarm()'s removal of the startup transient with removal of group delay is gone with it, which is the part that would have propagated the misunderstanding.
| // TxVoiceProcessor does not own this raw association. Clear it while | ||
| // the filter is still alive so a later prepare()/reset() cannot | ||
| // dereference an RNNoiseFilter destroyed here. | ||
| m_txVoiceProcessor->setRnnoise(nullptr); |
There was a problem hiding this comment.
Blocker 3 closed for the deterministic case — nulling the processor's cached pointer before m_rn2Tx.reset() on the next line, and the same ordering on the invalid-construction path at :6799-6800. That was the dereference-after-free I could trigger without any thread race.
Residual worth one more pass, non-blocking: the handoff is still unsynchronised. m_processors.rnnoise is a plain pointer written here on the main thread and read at TxVoiceProcessor.cpp:276 on the audio thread, and m_rnnoiseEnabled is a plain bool in the same position. An audio block that has already evaluated m_rnnoiseEnabled && m_processors.rnnoise and is about to call ->process48kStereo() when this line runs still dereferences a pointer that reset() frees a moment later.
Much narrower than before — the null lands first, so the window is one instruction pair rather than the whole disable path — but it is the same failure mode, and this pointer has now caused a crash twice. std::atomic<RNNoiseFilter*> with a release store here and an acquire load at the use site closes it for the cost of one relaxed-vs-acquire read per block.
Separately, and not this PR's problem: 3a00e085 ("stop freeing the RN2 TX filter under the audio thread", Principle XII) still has not landed on main — it exists only on the unmerged feat/eq-autofit-refcurves branch. So main today still destroys m_rn2Tx on disable with no guard at all. That deserves its own issue whatever happens to this PR.
3 participants
Summary
The TX Audio Channel Strip was performing several SRCs and bit depth truncations; introducing compounding aliasing artifacts and quantization noise. This feature branch implements several changes, including migrating the entire channel strip and TX voice audio pipeline to pinned 48kHz/Float sample rate/bit-depth, with a single, high quality SRC to transport rate. Applies unshaped TPDF dither immediately before final transport bit-depth quantization.
This change also improves audio pipeline latency specifically for 44.1kHz and 48kHz, the two most common host sample rates, by implementing a 12% SRC profile, while maintaining flat frequency response for the entire available 10kHz baseband and significant alias rejection, though at a very slight latency cost to 24kHz and lower sample rates.
Oversampling in non-linear processors was explicitly left out of this PR in order to minimize blast radius and failure modes, though migrating from 24kHz to 48kHz is equivalent to 2x oversampling compared to main. More aggressive oversampling can be added in subsequent PRs.
Constitution principle honored
Test plan
cmake --build build)Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — use nested-JSON-under-one-key(Principle V)
AppSettingsadditions in this commitreverse-engineered from a proprietary binary (Principle IV)
MeterSmoother(AGENTS.md convention)docs/and theaffected READMEs. Not
CHANGELOG.md, which is a release-prep file aPR must not add to (AGENTS.md); describe it in the Summary above instead