Fix MNR noise tracking and redundant activation control - #4807
Conversation
Use smoothed minimum-statistics noise estimation, dimensionally correct decision-directed SNR state, and synthesis-only strength blending. Ignore the synthetic latency-prefill frame when initializing the estimator. Remove the redundant MNR enable checkbox so the method selector is the sole activation control, and add deterministic attenuation, signal-preservation, stereo, startup, and reset coverage. Refs aethersdr#4806
There was a problem hiding this comment.
Pull request overview
This pull request addresses weak stationary-noise suppression in the macOS minimum-statistics noise reducer (MNR) by correcting the estimator and decision-directed SNR recurrence, and simplifies the UI by removing the redundant “Enable MNR” checkbox so the DSP method selector is the sole enable/disable control.
Changes:
- Fixes MNR noise-floor tracking and decision-directed update logic (smoothed periodograms, calibrated min-statistics bias, controlled upward noise rise, correct posterior-SNR recurrence, and strength blend applied only at synthesis).
- Removes the legacy “Enable MNR” checkbox and its dead settings/signal wiring; retains the strength slider with clarified “0 = bypass”.
- Adds a deterministic macOS-only regression/quality test target and wires it into CTest.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mac_nr_filter_test.cpp | Adds deterministic offline DSP coverage for MNR attenuation, scale invariance, bypass correctness, stereo balance, startup/prefill, and reset behavior. |
| src/gui/MainWindow_Wiring.cpp | Removes redundant wiring for the deleted MNR enable checkbox signal. |
| src/gui/AetherDspWidget.h | Removes the MNR enable checkbox member and its signal declaration. |
| src/gui/AetherDspWidget.cpp | Removes the checkbox UI/settings write; improves strength slider tooltip and a11y metadata; keeps strength synchronization. |
| src/gui/AetherDspDialog.h | Removes the dialog-level relay signal for the deleted MNR enable checkbox path. |
| src/gui/AetherDspDialog.cpp | Removes the connection relaying the deleted widget signal. |
| src/core/MacNRFilter.h | Updates the filter interface/state for corrected estimator behavior and synthesis-time strength blending. |
| src/core/MacNRFilter.cpp | Implements smoothed minimum-statistics tracking, corrected decision-directed recurrence, prefill-safe initialization, and strength-independent adaptive state. |
| CMakeLists.txt | Adds mac_nr_filter_test (Apple-only) executable and registers it with CTest. |
There was a problem hiding this comment.
Solid work, and the diagnosis is right on all three counts — the raw 25-frame minimum really was under-reporting stationary noise, m_prevPow really was feeding raw FFT power into a dimensionless recurrence, and folding the strength blend back into m_prevGain really did contaminate the adaptive state. Storing the previous posterior SNR and keeping m_filterGain strength-free are both textbook-correct now, and moving the blend into synthesizeFrameWithCurrentGain() is the right seam. I traced the UI removal through the tree: nothing reads the flat MnrEnabled key (persistence goes through ClientMnrEnabled), the method selector at AetherDspWidget.cpp:378 already calls setMnrEnabled(), the non-macOS gate at line 239 still carries the "macOS only" tooltip, and the new resetCurrentTab behaviour now matches every other tab. Clean removal. The new deterministic test target is a genuinely good addition — CMake placement matches its if(APPLE) siblings, and all four checks are green.
My one substantive question is about tuning rather than structure: OVER and FLOOR were almost certainly chosen to compensate for the estimator that this PR just fixed, and the numbers in your own test plan suggest they're now double-correcting.
Would like addressed before merge
MacNRFilter.h:71—OVER = 2.0on top of a now-calibrated noise floor pins the mask nearFLOORfor any stationary noise; worth a deliberate answer rather than leaving it implicit.
Polish
tests/mac_nr_filter_test.cpp:199— the desired-signal-loss assertion has 0.18 dB of headroom against your own measurement; likely to flake on a different Accelerate build.
Non-blocking notes
src/core/MacNRFilter.cpp:144— the zero-frame poisoning you fixed at startup can still recur mid-stream after a silence gap; them_noiseInitializedseed only covers frame zero.- The class doc block at the top of
MacNRFilter.h(outside the diff) still describes the old design — "25-frame noise history … vs 10 frames before", no mention of the smoothed periodogram, bias calibration, or rise limiting. Worth a refresh while you're in there, since that comment is the first thing the next reader hits.
🤖 aethersdr-agent · cost: $8.2592 · model: claude-opus-5
There was a problem hiding this comment.
Nice piece of DSP work — the core diagnosis holds up on inspection. The decision-directed recurrence is now the textbook form (ξ = α·G²·γ_prev + (1-α)·max(γ-1,0)), where the old code was feeding raw FFT power in for γ_prev, which made the estimator scale-dependent; and pulling the strength blend out of m_filterGain so it only lands in synthesis is the right separation. I also confirmed the claim about the dead setting: nothing reads the flat MnrEnabled key — the persisted state is ClientMnrEnabled, written by MainWindow.cpp:3630 and read at MainWindow_Wiring.cpp:1677 — and the method selector (AetherDspWidget.cpp:378) already owns enable/disable, so the checkbox really was redundant. No stale Enable MNR references remain anywhere in the tree, and the new <algorithm> include in the header fixes a genuine transitive-include dependency for std::clamp in setStrength().
Nothing here blocks merge. Two estimator-seeding edge cases and a CI-coverage question are worth a look.
Polish
- Seeding
m_noiseEstat the full power of the first energetic frame means enabling MNR (or areset()on band change) mid-signal briefly suppresses wanted audio rather than passing it through — inline. - The
> 0.0fenergy test accepts a single denormal, so a near-silent-but-dithered first frame can seed a ~zero floor that takes the history flush plus theNOISE_RISEramp to recover from — inline, with a suggestion.
Non-blocking notes
mac_nr_filter_testis registered only underif(APPLE)— inline; worth confirming the macOS CI job actually builds it, given the local linker crash noted in the test plan.m_frameCount(MacNRFilter.cpp:275,.h:97) is incremented and reset but never read — pre-existing dead state, and this PR is already tidying the estimator fields, so it'd be a natural thing to drop while you're in here. Outside the diff, hence not inline.
🤖 aethersdr-agent · cost: $4.9903 · model: claude-opus-5
Freeze minimum-statistics tracking for near-silent frames so TX, mute, and squelch gaps cannot collapse the learned floor. Seed new filters conservatively to avoid ducking wanted audio when MNR is enabled mid-speech. Add regressions for silence recovery, dithered startup, and mid-speech activation; loosen the portable signal-loss margin and execute the MNR test in macOS CI.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/mac_nr_filter_test.cpp:299
- In the reset/noise-history test loop,
processedis reinterpreted asfloat*and appended without validating thatMacNRFilter::process()returned the expected byte count. Ifprocess()ever regresses and returns a shorter/longer buffer, this will read past the end ofprocessedand can crash the test instead of failing cleanly.
for (int frame = 0; frame < kResetFrames; frame += kBlockFrames) {
QByteArray block(kBlockFrames * 2 * static_cast<int>(sizeof(float)),
Qt::Uninitialized);
std::copy_n(input.data() + 2 * frame, kBlockFrames * 2,
reinterpret_cast<float*>(block.data()));
const QByteArray processed = filter.process(block);
const auto* values = reinterpret_cast<const float*>(processed.constData());
output.insert(output.end(), values,
values + processed.size() / static_cast<int>(sizeof(float)));
}
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed final state at 590c06e: all prior review points addressed in code (documented OVER rationale, -4 dB loss guard, MIN_FRAME_POWER silence freeze, 0.25 conservative seed, macOS CI runs mac_nr_filter_test, dead m_frameCount dropped). DSP recurrence and strength/mask separation verified; no stale MNR checkbox references remain. All checks green.
Summary
Fixes #4806.
MNR's minimum-statistics estimator took a raw minimum across 25 periodograms, which systematically underestimated stationary noise power and left the Wiener gain near unity. Its decision-directed recurrence also mixed raw FFT power into a dimensionless SNR term, and its strength-blended output gain fed back into the adaptive state. Together those defects limited maximum-strength stationary-noise attenuation to about 4.54 dB.
This change smooths the periodogram before minimum tracking, applies a calibrated minimum-statistics bias, gives upward noise updates a controlled rise, stores the previous posterior SNR in the decision-directed recurrence, and keeps the adaptive gain independent of the user strength blend. The first real-energy frame seeds the estimator so the synthetic latency-prefill frame cannot poison startup or reset.
The redundant
Enable MNRcheckbox and its deadMnrEnabledsettings/signal wiring are removed. The MNR method selector is now the sole enable/disable control, consistent with the other AetherDSP methods. The strength slider remains and now documents 0 as bypass.Constitution principle honored
Principle XI — Fixes Are Demonstrated. The correction includes deterministic quantitative DSP coverage plus authenticated, RX-only agent automation bridge proof of the production UI and audio path.
Root cause and correction
Test plan
/opt/homebrew/bin/cmake --build build-codex-arm64 -j8; one Apple linker process crashed under parallel test linking, and the incremental serial retry completed successfully)Local verification:
ctest -R 'mac_nr_filter_test|mono_dsp_stereo_adapter_test|aether_dsp_mode_policy_test': 3/3 passed.tools/check_engine_boundary.py --strict: 0 blockers (known baseline warnings only).tools/check_a11y.py: no new findings (existing repository warnings only).git diff --check: clean.Agent automation bridge proof
The final ARM64 app was launched with authenticated automation in an exclusive radio-lock session; no TX controls were invoked.
get dsp: active methodNR2;MNR.available=true,MNR.enabled=false.dumpTree: visibledspMethodBtnMNRandmnrStrengthSlider; noEnable MNRcheckbox. Slider tooltip reports0 = bypass, 100 = maximum.invoke dspMethodBtnMNR click, followed byget dsp:active=MNR,MNR.enabled=true, and NR2 disabled.get dsp:active=none,MNR.enabled=false.audioCapture probeDspStereo MNR:ok=true,audible=true,fullCoverage=true, exact 4:1 stereo ratio preserved, and both channels measured the same 0.0587033 output/input level ratio.Proof
MNR selected at maximum strength with the redundant checkbox removed:
Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls — the deadMnrEnabledwrite is removedMeterSmoother(no meter UI changed)CHANGELOG.mdis untouchedGenerated with OpenAI Codex.