Add macOS MMSE-Wiener spectral NR (MNR) + 5 dark-theme quick-wins#1672
Add macOS MMSE-Wiener spectral NR (MNR) + 5 dark-theme quick-wins#1672M7HNF-Ian wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Thanks for this contribution, @Chaosuk97 — the MNR algorithm is well-implemented (decision-directed MMSE-Wiener with minimum-statistics is solid DSP engineering), and the dark-theme quick-wins are welcome fixes for longstanding issues. A few items to address before merge:
1. Missing mutual exclusion in setMnrEnabled (bug)
setMnrEnabled() does not disable the other NR modes (NR2/NR4/RN2/BNR/DFNR) when enabling MNR, and none of the existing setNr2Enabled, setNr4Enabled, setRn2Enabled, setBnrEnabled, or setDfnrEnabled methods disable MNR either. Every other NR mode follows this pattern — e.g. setNr2Enabled(true) calls setRn2Enabled(false), setBnrEnabled(false), etc.
Without this, two NR engines can run simultaneously in feedAudioData(), which would produce undefined audio output (whichever if branch matches first wins, but both consume CPU).
Fix: Add the mutual-exclusion calls inside setMnrEnabled(true):
if (m_nr2Enabled) setNr2Enabled(false);
if (m_rn2Enabled) setRn2Enabled(false);
if (m_bnrEnabled) setBnrEnabled(false);
if (m_nr4Enabled) setNr4Enabled(false);
if (m_dfnrEnabled) setDfnrEnabled(false);And add if (m_mnrEnabled) setMnrEnabled(false); to each of the existing setNr2Enabled, setRn2Enabled, etc. (guarded with #ifdef __APPLE__ or checking the atomic directly since it exists on all platforms).
2. Missing RADE guard in setMnrEnabled
All other NR enable methods check if (on && m_radeMode) return; — RADE outputs decoded speech where client-side DSP has no effect. MNR should do the same.
3. Raw new/delete for m_mnr — use std::unique_ptr (convention)
All other NR filter members use std::unique_ptr (m_nr2, m_rn2, m_nr4, m_dfnr, m_bnr). m_mnr uses raw new/delete. This is a deviation from project conventions and creates a leak risk if setMnrEnabled(true) is called twice without an intervening false (the early return if (m_mnrEnabled == on) return; prevents this today, but unique_ptr makes it structurally safe).
4. m_inAccum.erase(begin, begin + H) — O(n) front-erase on std::vector
This runs once per hop inside the OLA loop. With H = 256 and typical buffer sizes this is fine for now, but a std::deque or ring buffer would be more idiomatic for an accumulator that only grows at the back and shrinks from the front. Not a blocker — just noting it for future optimization if profiling shows it matters.
5. Header comment says int16, implementation uses float32
MacNRFilter.h line 28:
// stereo int16 → mono float → FFT NR (512-pt OLA) → stereo int16
But process() treats the input as stereo float32 (reinterpret_cast<const float*>), matching the 0.8.x audio pipeline. The comment should say stereo float32 → mono float → FFT NR → stereo float32.
6. m_mnrStrength not protected by mutex or atomic
m_mnrStrength is a plain float read from the audio thread (feedAudioData → m_mnr->process() → reads m_strength) and written from the GUI thread (setMnrStrength). The MacNRFilter::setStrength() is also not synchronized. Since MacNRFilter::m_strength is only used as a gain scalar (a torn read would produce a momentarily wrong but not dangerous value), this is low-severity, but for consistency with the rest of the codebase consider making it std::atomic<float>.
7. third_party/deepfilter/lib/darwin-arm64/libdeepfilter.a — out of scope?
This file appears in the changed files list but doesn't seem related to MNR or the dark-theme fixes. If it's an unrelated update, it should ideally be in a separate commit/PR to keep the change set clean.
8. MNR strength not persisted via AppSettings
The NR2, NR4, DFNR, and BNR modes all save/restore their tuning parameters through AppSettings. MNR strength appears to only be set at runtime via the slider — if the user restarts the app, the strength resets to 1.0. Consider saving/loading MnrStrength in AppSettings for parity with the other modes.
Quick-wins look good
The dark-theme colour fixes (#1503, #1581, #1583, #1576, #506) are straightforward and well-scoped. The colour choices (#00e060, #8aa8c0, #d04040, etc.) are consistent with the existing palette.
Summary
Items 1 and 2 are functional bugs that should be fixed before merge. Items 3, 5, and 8 are convention/correctness issues worth addressing. The rest are minor observations. The DSP implementation itself is clean and well-structured — nice work on the algorithm side.
|
Re reviewer items #1–3, #5–6, #8: pushed in commit 05940b2 to the same branch (
Re #7 (libdeepfilter.a): the |
MNR uses a decision-directed MMSE-Wiener filter with minimum-statistics noise floor tracking, accelerated by Apple Accelerate (vDSP / AMX). 512-point FFT at 24 kHz, 50% overlap-add, per-bin gain with temporal smoothing to suppress musical noise. Hardware-accelerated on Apple Silicon. Right-click the MNR button to open strength controls in DSP Settings. Quick-wins: - aethersdr#1503: Diversity button missing on Slice A at startup - aethersdr#1581: TNF/FDX indicators invisible in dark theme - aethersdr#1583: Date/Time status bar labels too dim - aethersdr#1576: Record/Playback buttons invisible in dark theme - aethersdr#506: Secondary text colour consistency (#506070 → #8aa8c0) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
before enabling MNR; every other NR setter (NR2/RN2/NR4/BNR/DFNR) now
also calls setMnrEnabled(false) in its mutual-exclusion block.
matching the pattern used by all other NR setters.
forward declaration (#ifdef __APPLE__) and unique_ptr member; AudioEngine.cpp
already had the full include; new/delete replaced with make_unique/reset.
std::atomic<float> m_mnrStrength replaces plain float.
"stereo int16 → … → stereo int16" to "stereo float32 → … → stereo float32";
also fixes the process() doc-comment.
strength() use store/load; processFrame() reads m_strength.load().
Allows AudioEngine to write strength from the audio thread while the
DSP dialog reads it from the UI thread without a data race.
AppSettings("MnrStrength"); setMnrStrength() now saves to AppSettings
on every change, so the value survives restarts.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
05940b2 to
6c407da
Compare
…sdr#1672) HAVE_PIPEWIRE is always defined on Linux so PipeWire/PulseAudio handles FreeDV audio routing at the OS level — the toggle has no utility there. When DaxTxLowLatency=True it also sent transmit set dax=0 on TCI PTT, silently discarding all dax_tx VITA-49 packets and producing no RF output for WSJT-X/JTDX digital modes. Guard the menu item with #if !defined(HAVE_PIPEWIRE) so it remains available on macOS (Loopback) and Windows (VB-Cable) where an external FreeDV client over a virtual audio cable is a legitimate use case. On Linux, add a one-time migration that resets DaxTxLowLatency=True to False and forces setDaxTxUseRadioRoute(true) to ensure the audio engine is in the correct state even for users upgrading from older builds. Document the DAX TX routing design and the platform rationale in docs/tx-audio-signal-path.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sdr#1672) HAVE_PIPEWIRE is always defined on Linux so PipeWire/PulseAudio handles FreeDV audio routing at the OS level — the toggle has no utility there. When DaxTxLowLatency=True it also sent transmit set dax=0 on TCI PTT, silently discarding all dax_tx VITA-49 packets and producing no RF output for WSJT-X/JTDX digital modes. Guard the menu item with #if !defined(HAVE_PIPEWIRE) so it remains available on macOS (Loopback) and Windows (VB-Cable) where an external FreeDV client over a virtual audio cable is a legitimate use case. On Linux, add a one-time migration that resets DaxTxLowLatency=True to False and forces setDaxTxUseRadioRoute(true) to ensure the audio engine is in the correct state even for users upgrading from older builds. Document the DAX TX routing design and the platform rationale in docs/tx-audio-signal-path.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lter.a - SpectrumOverlayMenu: only widen DSP panel to 280px on Apple builds where the extra MNR toggle actually appears; non-Apple builds stay at 200px (aethersdr#1672 review) - Remove third_party/deepfilter/lib/darwin-arm64/libdeepfilter.a — MNR uses Apple Accelerate vDSP, not DeepFilter; the binary had no business in this PR Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sdr#1672) HAVE_PIPEWIRE is always defined on Linux so PipeWire/PulseAudio handles FreeDV audio routing at the OS level — the toggle has no utility there. When DaxTxLowLatency=True it also sent transmit set dax=0 on TCI PTT, silently discarding all dax_tx VITA-49 packets and producing no RF output for WSJT-X/JTDX digital modes. Guard the menu item with #if !defined(HAVE_PIPEWIRE) so it remains available on macOS (Loopback) and Windows (VB-Cable) where an external FreeDV client over a virtual audio cable is a legitimate use case. On Linux, add a one-time migration that resets DaxTxLowLatency=True to False and forces setDaxTxUseRadioRoute(true) to ensure the audio engine is in the correct state even for users upgrading from older builds. Document the DAX TX routing design and the platform rationale in docs/tx-audio-signal-path.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sdr#1672) HAVE_PIPEWIRE is always defined on Linux so PipeWire/PulseAudio handles FreeDV audio routing at the OS level — the toggle has no utility there. When DaxTxLowLatency=True it also sent transmit set dax=0 on TCI PTT, silently discarding all dax_tx VITA-49 packets and producing no RF output for WSJT-X/JTDX digital modes. Guard the menu item with #if !defined(HAVE_PIPEWIRE) so it remains available on macOS (Loopback) and Windows (VB-Cable) where an external FreeDV client over a virtual audio cable is a legitimate use case. On Linux, add a one-time migration that resets DaxTxLowLatency=True to False and forces setDaxTxUseRadioRoute(true) to ensure the audio engine is in the correct state even for users upgrading from older builds. Document the DAX TX routing design and the platform rationale in docs/tx-audio-signal-path.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Five small contrast/visibility fixes, applied from #1672 with the macOS MNR scope held back for separate review + on-platform testing. - #1576 — Record / Play buttons unreadable in dark theme. Record \`#804040\` → \`#d04040\`, Play \`#406040\` → \`#70b070\`, disabled state \`#303030\` → \`#505050\`. - #1581 — TNF and FDX status indicators invisible in active state on dark theme. Active color switched from \`rgba(255,255,255,128)\` (dim half-white) to \`#00e060\` (bright green), matching the TGXL/PGXL OPERATE indicator style. Inactive \`#404858\` unchanged. - #1583 — GPS date/time status labels and radio version label too dim. \`#607080\` / \`#506070\` → \`#8aa8c0\` to match the other status-bar labels. GPS date label was also at the wrong size (10 px vs the surrounding 12 px); now aligned. - #506 — Secondary-text colour consistency pass. \`#607080\` / \`#506070\` → \`#8aa8c0\` across MqttApplet, EqApplet, DxClusterDialog, WhatsNewDialog. - #1503 — Diversity button missing on Slice A at startup. Previous loop only touched the active pan's active VFO widget; now iterates every pan → every slice so every VFO widget gets the diversity- allowed flag set when the radio supports it. Scope creep held for a follow-up commit: the macOS MNR DSP path (MacNRFilter, AudioEngine integration, DSP-dialog tab, spectrum overlay MNR toggle, VFO MNR button). Will land separately after on-platform testing. One hunk from #1672 was skipped because its target no longer exists: the AppletPanel grip-handle label was retired with the old AppletTitleBar in ba61f48. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude here on behalf of Jeremy — scope B (the 5 dark-theme quick-wins) landed direct on main as `eba4f8f` with you preserved as git author. Following the same pattern we used on #1547: bypass the PR merge, hand-apply the scope we're ready to land, keep the rest of the PR open for separate review. What landed
Skipped when applying
QA findings (not your bugs — pre-existing)Tested on Linux x86_64. All 5 quick-wins passed cleanly, but the QA pass also surfaced two pre-existing layout/rendering bugs the brighter colours made visible:
Both opened as separate issues. Neither is a regression from this PR. Keeping this PR open for scope AThe macOS MNR scope is held until we can:
Will circle back. Thanks Ian — the quick-wins were all surgical and clean. 73, Jeremy KK7GWY & Claude (AI dev partner) |
Community contribution by Ian M7HNF (Chaosuk97). New macOS-exclusive client-side NR mode, hardware-accelerated via Apple Accelerate (vDSP / AMX on Apple Silicon). Algorithm: decision-directed MMSE-Wiener filter with 25-frame minimum- statistics noise-floor tracking. 512-point real FFT at 24 kHz (46.9 Hz per bin), 50% overlap-add with sqrt-Hann window, per-bin gain with temporal smoothing to suppress musical noise. ~0.3 ms per hop on M-series chips. One-hop startup latency (≈10.7 ms). Integration: - MacNRFilter owns the vDSP pipeline; AudioEngine has an #ifdef __APPLE__ unique_ptr<MacNRFilter> m_mnr. All audio-path calls and the filter object itself are Apple-gated; non-Apple builds compile with zero MNR overhead and the VFO/overlay MNR buttons are hidden. - Mutual exclusion with NR2 / RN2 / NR4 / BNR / DFNR — only one client-side NR runs at a time, same as existing behaviour. - Strength slider (0-100, AppSettings key "MnrStrength") with real-time parameter update from UI thread via atomic float. - Persists enable state as "ClientMnrEnabled" across app restart. - MNR button appears in both the VFO DSP tab and the spectrum overlay DSP panel. DSP panel width widened to 280 px on Apple builds only to fit the extra toggle; non-Apple stays at 200 px. - Right-click the MNR button → opens DSP Settings dialog on the MNR tab with the strength slider. Applied from PR #1672 with scope B (the 5 dark-theme quick-wins) previously landed as eba4f8f. Scope A is this commit. Two fixes during application: - \`syncMnr\` lambda in MainWindow was missing the VFO-button sync block that every other DSP has — only the overlay-menu button updated on mutual-exclusion events, so the VFO MNR button stayed visibly checked when a sibling NR turned MNR off. Fixed to match syncBnr / syncNr4 / syncDfnr pattern. - Intermediate commit's stray \`libdeepfilter.a\` binary was reverted in Ian's own review pass — final state has no third-party binary added. Tested on macOS 26.4 (mutual exclusion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Claude here on behalf of Jeremy — scope A (macOS MNR) landed on main as `71c0396` with you preserved as git author. Scope B landed earlier as `eba4f8f`. Both halves of the PR are now on main. What landed (this commit)
Two fixes applied during landing
Tested
Outstanding QA items from the original plan (activation, strength slider sweep, performance, dual-button sync, persistence, regression spot-check) weren't systematically walked through after the exclusion fix landed — we went straight to merge at Jeremy's call. If anything else surfaces, happy to iterate. Nice piece of work. The vDSP / Accelerate integration is clean, and the AppSettings-threading pattern for strength is exactly right. 73, Jeremy KK7GWY & Claude (AI dev partner) |
Summary
MNR — macOS MMSE-Wiener Spectral Noise Reduction
Adds a new client-side noise reduction mode exclusive to macOS, accelerated by Apple's Accelerate framework (vDSP / AMX on Apple Silicon).
MNR uses a decision-directed MMSE-Wiener filter with minimum-statistics noise floor tracking. Unlike the radio-side NR (which applies a single scalar to the whole spectrum) or RNNoise (which is speech-optimised), MNR works per-bin in the frequency domain — it estimates the noise power at each of the 257 spectral bins independently and applies a customised Wiener gain to suppress only the bins that contain noise, leaving signal-bearing bins untouched.
Key characteristics:
vDSP_fft_zripuses AMX co-processor on Apple Silicon; the whole pipeline runs in ~0.3 ms per hop on M-series chipsThe MNR button appears in both the VFO widget DSP tab and the spectrum overlay DSP panel. On non-Apple builds it is hidden at compile time — zero overhead for Windows/Linux users. Right-clicking the MNR button opens the DSP Settings dialog directly on the MNR tab.
Quick-wins bundled in this PR
#00e060(bright green), inactive from near-black to#404858#8aa8c0#d04040, playback green to#70b070, disabled grey to#505050#8aa8c0(was#506070/#607080— unreadably dark on the#0f0f1abackground)Test plan
🤖 Generated with Claude Code