Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/core/AudioEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9001,18 +9001,22 @@ void AudioEngine::setRadioTransmitting(bool tx)
// adapt its internal state to TX silence, #367/#1505). But that leaves NR2
// holding pre-TX state when RX resumes: a stale overlap-add ring (read out
// as a faint whistle, #3340) and a maxed-out startup-ramp counter, so
// suppression slams to full-wet on a stale noise estimate that then takes
// ~3-4s to reconverge — the audio "gap" users hear with NR2 engaged
// (#1863). reset() flushes the OA ring, re-seeds the noise floor high
// (gentle suppression), and re-arms the ~1s dry→wet ramp so audio returns
// immediately on the dry signal and NR2 fades back in cleanly.
// suppression slams to full-wet the instant RX resumes. resetTransient()
// flushes exactly that — the OA ring, the gain masks, the AGC common-mode
// references — and re-arms the ~1s dry→wet ramp, while RETAINING the
// converged noise estimate. The full reset() used here previously also
// re-seeded the noise floor, forcing a fresh multi-second estimator
// convergence on every over, heard as un-suppressed band noise after
// unkey (#3821); with the profile retained, suppression is back at full
// depth as the ramp completes, and the estimator keeps adapting from
// there if the band moved during TX.
//
// Scoped to NR2 for now: it's the reported filter and this keeps testing
// localized. RN2/NR4/DFNR/MNR share the same bypass + stale-state path and
// can get the same flush as a follow-up once this is validated in the field.
if (previous && !tx) {
std::lock_guard<std::recursive_mutex> dspLock(m_dspMutex);
if (m_nr2Enabled && m_nr2) m_nr2->reset();
if (m_nr2Enabled && m_nr2) m_nr2->resetTransient();
}

emit radioTransmittingChanged(tx);
Expand Down
81 changes: 52 additions & 29 deletions src/core/SpectralNR.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,17 @@ void SpectralNR::setNpeMethod(int method)
}

void SpectralNR::reset()
{
resetTransientState();
resetNoiseEstimate();
}

void SpectralNR::resetTransient()
{
resetTransientState();
}

void SpectralNR::resetTransientState()
{
std::fill(m_inAccum.begin(), m_inAccum.end(), 0.0);
std::fill(m_outAccum.begin(), m_outAccum.end(), 0.0);
Expand All @@ -542,6 +553,47 @@ void SpectralNR::reset()
m_outReadPos = 0;
m_outputAvailable = m_fftSize;

// The AGC common-mode references flush with the transients rather than

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Risk to confirm (non-blocking) — retained m_noisePsd vs. an AGC level change. This comment correctly justifies flushing the common-mode references (their corrector is dormant during the ramp). But the retained m_noisePsd is an absolute level: if receiver AGC gain stepped during the over, gamma = lambdaY/noisePsd runs against a stale floor for the ~1 s ramp with the scale corrector returning early — a possible post-unkey over-suppression transient in the opposite direction from the old reset(). The dry→wet ramp and min-statistics tracking likely mask it. Confirm with a bench row that steps AGC across the over, or note that NR2's input is pre-AGC (then this is a non-issue).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Confirmed rather than a non-issue: NR2's input is post-AGC on every path (the radio's AGC for a Flex, WDSP's inside Hl2RxDsp for an HL2). The comment now records why the corrector cannot observe a step that straddles the gap and what bounds the fallout (min-statistics re-levels within one m_U * m_V window, ~1.5 s — no slower than the full reset() this replaced). Pinned by the new ±6 dB rows in spectral_nr_test; numbers in the PR thread. b2d90ce.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirming @ten9876's flagged risk with a specific mechanism, and then arguing why it is still non-blocking.

This comment says the references' "calibration re-runs inside the re-armed ramp window," which is true but understates it. resetTransientState() sets m_commonReferenceInitialized = false, so the first post-TX frame hits the early return at SpectralNR.cpp:1350-1354 and copies the current m_lambdaY straight into m_commonReferencePsd. The reference is re-anchored to the post-TX level. For the rest of the ramp (m_frameCount < m_rampFrames, :1418) it only EMA-smooths that reference and returns without any scale detection.

Net effect: if the receiver AGC stepped during the over, the step is invisible to detectCommonModeScale() — not merely deferred past the ramp, but erased, because the reference now describes the new level. scalePowerHistory() will therefore never fire to rescale the retained m_noisePsd, which is an absolute level and is now inconsistent with the input by exactly that step.

Why I still don't think this blocks:

  • Gain stepped down → retained floor too high → gamma = lambdaY/noisePsd too low → over-suppression. Min-statistics tracks downward fast, so this self-corrects quickly, and m_currentWet is ramping from 0 through the first ~1 s anyway.
  • Gain stepped up → retained floor too low → under-suppression. Min-statistics is slow upward here, so this is the direction that could linger — but it lands the user in roughly the pre-PR behavior (audible band noise for a beat), not somewhere worse.

So the failure mode is bounded on both sides and the ramp covers the loud half. Two things would settle it:

  1. If NR2's input is pre-AGC, this is a non-issue outright — worth stating in the comment either way.
  2. Otherwise, a bench row that steps AGC-T across an over would close it. Not something I can run headless.

Either way, please leave a sentence here recording that the corrector cannot observe a step straddling the gap, so the next reader doesn't have to re-derive it from :1350.

// surviving alongside the noise estimate: their calibration re-runs
// inside the re-armed ramp window (m_frameCount < m_rampFrames), and its
// first frame overwrites the level reference with weight 1/(0+1) anyway,
// so a retained value could only live for one frame. Flushing keeps the
// recalibration deterministic — and post-TX the receiver AGC state that
// these references describe is exactly what may have changed.
std::fill(m_commonWantedProtected.begin(),
m_commonWantedProtected.end(), 0);
std::fill(m_commonReferencePsd.begin(),
m_commonReferencePsd.end(), 0.0);
std::fill(m_residualReferencePsd.begin(),
m_residualReferencePsd.end(), 0.0);
std::fill(m_residualReferenceGainRatio.begin(),
m_residualReferenceGainRatio.end(), 1.0);
std::fill(m_residualReferenceValid.begin(),
m_residualReferenceValid.end(), 0);
std::fill(m_commonNoiseLike.begin(), m_commonNoiseLike.end(), 0);

std::fill(m_prevMask.begin(), m_prevMask.end(), 1.0);
std::fill(m_prevGamma.begin(), m_prevGamma.end(), 1.0);
std::fill(m_mask.begin(), m_mask.end(), 1.0);
std::fill(m_smoothMask.begin(), m_smoothMask.end(), 1.0);
std::fill(m_aeMask.begin(), m_aeMask.end(), 1.0);
std::fill(m_aePrefix.begin(), m_aePrefix.end(), 0.0);

m_commonReferenceInitialized = false;
m_commonReferenceReacquiring = false;
m_commonSilenceRecoveryContext = false;
m_commonLevelReferenceInitialized = false;
m_commonLevelReferencePower = 0.0;
m_commonScaleLog = 0.0;
m_commonAppliedScale = 1.0;
m_commonReturnScale = 1.0;
m_commonDetectedScale = 1.0;
m_frameCount = 0;
m_currentWet = 0.0;
}

void SpectralNR::resetNoiseEstimate()
{
// Start with a HIGH noise estimate — gains will be < 1 during convergence,
// producing gentle suppression rather than amplification spikes.
// The OSMS tracker will converge downward to the true noise floor in ~2s.
Expand All @@ -568,45 +620,16 @@ void SpectralNR::reset()
m_nstatTonalProbability.end(), 0.0);
std::fill(m_nstatTonalIndicator.begin(),
m_nstatTonalIndicator.end(), 0);
std::fill(m_commonWantedProtected.begin(),
m_commonWantedProtected.end(), 0);
std::fill(m_nstatNoisePsd.begin(), m_nstatNoisePsd.end(), 0.0);
std::fill(m_commonReferencePsd.begin(),
m_commonReferencePsd.end(), 0.0);
std::fill(m_residualReferencePsd.begin(),
m_residualReferencePsd.end(), 0.0);
std::fill(m_residualReferenceGainRatio.begin(),
m_residualReferenceGainRatio.end(), 1.0);
std::fill(m_residualReferenceValid.begin(),
m_residualReferenceValid.end(), 0);
std::fill(m_commonNoiseLike.begin(), m_commonNoiseLike.end(), 0);

for (auto& v : m_actMinBuf)
std::fill(v.begin(), v.end(), 1e30);

std::fill(m_prevMask.begin(), m_prevMask.end(), 1.0);
std::fill(m_prevGamma.begin(), m_prevGamma.end(), 1.0);
std::fill(m_mask.begin(), m_mask.end(), 1.0);
std::fill(m_smoothMask.begin(), m_smoothMask.end(), 1.0);
std::fill(m_aeMask.begin(), m_aeMask.end(), 1.0);
std::fill(m_aePrefix.begin(), m_aePrefix.end(), 0.0);

m_alphaC = 1.0;
// WDSP rotates on the first complete frame so the estimator starts from
// observed audio rather than waiting a full sub-window on its seed value.
m_subwc = m_V;
m_ambIdx = 0;
m_commonReferenceInitialized = false;
m_commonReferenceReacquiring = false;
m_commonSilenceRecoveryContext = false;
m_commonLevelReferenceInitialized = false;
m_commonLevelReferencePower = 0.0;
m_commonScaleLog = 0.0;
m_commonAppliedScale = 1.0;
m_commonReturnScale = 1.0;
m_commonDetectedScale = 1.0;
m_frameCount = 0;
m_currentWet = 0.0;
}

void SpectralNR::initWindow()
Expand Down
13 changes: 13 additions & 0 deletions src/core/SpectralNR.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,17 @@ class SpectralNR {
// Reset all internal state (call when toggling on or stream restarts).
void reset();

// Flush only the transient state — overlap-add rings, gain masks, the
// AGC common-mode references, and the dry→wet startup ramp — while
// retaining the converged OSMS/MMSE/NSTAT noise estimates. For the
// TX→RX edge, where the stream resumes on the same band and the stale
// overlap-add ring is the hazard (#3340): a full reset() there re-seeds
// the noise floor and costs a fresh estimator convergence on every
// over, heard as un-suppressed band noise after unkey (#3821). Not a
// substitute for reset() on enable or source switches, where the old
// noise profile does not describe the new stream.
void resetTransient();

// User-adjustable parameters (thread-safe, called from main thread)
void setGainMax(float v);
void setGainFloor(float v);
Expand Down Expand Up @@ -301,6 +312,8 @@ class SpectralNR {

// ── Internal methods ───────────────────────────────────────────────
void initWindow();
void resetTransientState();
void resetNoiseEstimate();
void processFrame();
bool updateMaskFromCurrentFrame();
void synthesizeCurrentFrequencyBinsWithMask();
Expand Down
116 changes: 116 additions & 0 deletions tests/spectral_nr_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2392,6 +2392,119 @@ std::pair<double, double> stereoRmsAfter(const std::vector<float>& interleaved,
};
}

void test_transient_reset_retains_noise_profile()
{
// #3821: after the TX→RX edge, NR2 must re-engage within one dry→wet
// ramp, not a full estimator re-convergence. resetTransient() flushes
// the overlap-add ring (the #3340 stale-audio hazard), the gain masks,
// and the ramp, while retaining the converged noise estimate; a full
// reset() at the same point re-seeds the noise floor and leaves the
// band noise 6+ dB louder for roughly half a second after the ramp
// completes — the window in which the reporter's quick callbacks were
// being drowned out.
constexpr int sampleRate = 24000;
constexpr int fftSize = 1024;
constexpr int overlap = 4;
constexpr int blockSamples = 73;
constexpr int settleSamples = 6 * sampleRate;
constexpr int resumeSamples = 6 * sampleRate;

// One continuous synthetic noise timeline; the reset point models the TX
// gap, during which the bypassed filter sees no audio at all.
std::vector<float> settle(settleSamples);
std::vector<float> resume(resumeSamples);
std::uint32_t randomState = 0x33383231u;
auto nextWhite = [&randomState]() {
randomState = 1664525u * randomState + 1013904223u;
return 2.0 * (static_cast<double>(randomState) / 4294967295.0) - 1.0;
};
for (float& s : settle) {
s = static_cast<float>(0.25 * nextWhite());
}
for (float& s : resume) {
s = static_cast<float>(0.25 * nextWhite());
}

double settledDb = 0.0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

settledDb is captured by reference and written only on the transientReset == true branch inside runResumed, so both post-ramp assertions silently depend on runResumed(true) running before runResumed(false). As written that ordering holds, but if someone ever swaps the two calls (or evaluates them in one expression), settledDb stays 0.0 and std::abs(transientPostRampDb - settledDb) < 2.5 starts asserting against a meaningless reference rather than failing loudly.

Cheap hardening: have runResumed return the settled value alongside the resumed buffer, or assert settledDb != 0.0 before the comparisons. Non-blocking.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Taken — each run now measures and returns its own settled depth alongside the buffer, and the two settle phases are asserted identical, so no assertion depends on run order any more. b2d90ce.

const auto runResumed = [&](bool transientReset) {
SpectralNR nr(fftSize, sampleRate, overlap);
std::vector<float> out(settleSamples);
int offset = 0;
while (offset < settleSamples) {
const int count = std::min(blockSamples, settleSamples - offset);
nr.process(settle.data() + offset, out.data() + offset, count);
offset += count;
}
if (transientReset) {
settledDb = rmsGainDb(settle, out, 4 * sampleRate,
5 * sampleRate, fftSize);
nr.resetTransient();
} else {
nr.reset();
}
std::vector<float> resumed(resumeSamples);
offset = 0;
while (offset < resumeSamples) {
const int count = std::min(blockSamples, resumeSamples - offset);
nr.process(resume.data() + offset, resumed.data() + offset, count);
offset += count;
}
return resumed;
};

const std::vector<float> transientOut = runResumed(true);
const std::vector<float> fullOut = runResumed(false);

// #3340 guard: the flushed ring re-queues one frame of zero latency
// padding, so nothing recorded before the reset can leak out after it.
double stalePeak = 0.0;
bool allFinite = true;
for (int i = 0; i < fftSize; ++i) {
stalePeak = std::max(stalePeak,
static_cast<double>(std::abs(transientOut[i])));
}
for (const float sample : transientOut) {
allFinite = allFinite && std::isfinite(sample);
}

// Audio must return immediately on the dry signal (ramp re-armed) …
const double immediateDb = rmsGainDb(resume, transientOut, 0,
3 * sampleRate / 20, fftSize);
// … and with the retained profile, suppression must sit at the settled
// depth as soon as the ~1 s ramp completes. The full reset is still
// re-converging through this window (it does not reach depth until
// ~1.6-1.9 s post-reset), which is exactly the regression this test
// guards against.
const double transientPostRampDb = rmsGainDb(
resume, transientOut, 11 * sampleRate / 10, 8 * sampleRate / 5,
fftSize);
const double fullPostRampDb = rmsGainDb(
resume, fullOut, 11 * sampleRate / 10, 8 * sampleRate / 5, fftSize);
// Well after the edge both variants must agree again.
const double transientConvergedDb = rmsGainDb(
resume, transientOut, 5 * sampleRate / 2, 3 * sampleRate, fftSize);
const double fullConvergedDb = rmsGainDb(
resume, fullOut, 5 * sampleRate / 2, 3 * sampleRate, fftSize);

std::printf(" settled %+.2f dB, post-reset 0-0.15 s %+.2f dB\n"
" 1.1-1.6 s transient %+.2f dB vs full-reset %+.2f dB, "
"2.5-3.0 s %+.2f vs %+.2f dB, stale peak %.3g\n",
settledDb, immediateDb, transientPostRampDb, fullPostRampDb,
transientConvergedDb, fullConvergedDb, stalePeak);
report("transient_reset: output is finite", allFinite);
report("transient_reset: no stale ring audio leaks through the reset",
stalePeak < 1e-9);
report("transient_reset: audio returns immediately on the dry signal",
immediateDb > -3.0);
report("transient_reset: settled depth restored right at ramp end",
std::abs(transientPostRampDb - settledDb) < 2.5);
report("transient_reset: retained profile beats a full reset post-ramp",
fullPostRampDb - transientPostRampDb > 6.0);
report("transient_reset: both variants converge again well after the edge",
std::abs(transientConvergedDb - settledDb) < 2.5
&& std::abs(fullConvergedDb - settledDb) < 2.5);
}

void test_block_size_invariance()
{
// KiwiSDR audio arrives in packet-sized bursts, while native Flex RX audio
Expand Down Expand Up @@ -2629,6 +2742,9 @@ int main()
std::printf("\n-- NR2 live NPE switching --\n");
test_npe_switch_transients();

std::printf("\n-- NR2 TX->RX transient reset (#3821) --\n");
test_transient_reset_retains_noise_profile();

std::printf("\n-- NR2 quick reply after speech release --\n");
test_quick_reply_after_speech_release();
test_weak_reply_after_speech_release();
Expand Down
Loading