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 3 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.
2 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.
Oversampling in non-linear processors was explicitly left out of this PR in order to minimize blast radius and failure modes. This improvement 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