Fix audio pan broken with NR active (#1460) and AF mute not persisted (#1560)#1685
Conversation
There was a problem hiding this comment.
Review — PR #1685
Thanks for the clean re-submission @Chaosuk97 — separating this from the MNR branch makes it much easier to review. Both fixes are well-scoped and the PR description is excellent.
Pan fix (#1460) — looks good
applyRxPanInPlace is clean: file-static, early-out at centre, safe nFrames <= 0 guard. The linear pan law is simple and correct, and calling it at the right point in each NR branch (after the mono→stereo expand for NR2, after process() for RN2/NR4/DFNR) is the right approach.
One thing worth noting for the maintainer: AudioEngine has a single m_rxPan atomic, but wireVfoWidget connects every slice's rxPanChanged to it. In a multi-slice scenario, the last slider moved wins for all NR-processed audio. This is probably fine in practice since only the active slice's audio routes through the single AudioEngine's NR path, but it's worth being aware of if multi-slice NR is ever supported.
Mute persistence (#1560) — looks good
Using the slice letter (QChar('A' + sliceId)) as the settings key is a good choice for surviving re-assigned numeric IDs across sessions. The m_updatingFromModel guard on audioMuteToggled correctly prevents save-on-restore feedback loops.
Minor question: the restore path calls s->setAudioMute(true) which sends audio_mute=1 to the radio. If this runs before the radio connection is fully established (i.e., before the slice is "ready" on the radio side), does the command get queued or silently dropped? If the radio might not be ready yet, the timing could be fragile. The PR author notes "on reconnect" in the description, so this likely works in practice — just flagging for the maintainer to verify during testing.
Convention checks
- AppSettings used correctly (not raw QSettings) ✓
- No resource leaks — no heap allocations, no manual memory management ✓
- All files within stated scope — AudioEngine, VfoWidget, MainWindow only ✓
m_rxPanisstd::atomic<int>— safe for cross-thread reads from the audio thread ✓
Summary
Clean, well-documented, tightly scoped. No blockers from my side. The test plan covers the key scenarios. Nice contribution. 👍
Credits NR2 gainMax cap + output clamp fix (aethersdr#1696, issue aethersdr#1507). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed (aethersdr#1560) discarding the radio-side stereo balance. The pan slider sends audio_pan to the radio which embeds the balance in the VITA-49 PCM, but NR collapses it. Fix: - AudioEngine gains std::atomic<int> m_rxPan (0-100, centre=50) and void setRxPan(int). - Static helper applyRxPanInPlace() re-applies a linear-pan law to stereo float32 in-place; is a true no-op when pan==50. - Called at the end of processNr2() and after every other NR process() call in feedAudioData (RN2/NR4/DFNR/MNR paths). The non-NR path is untouched — the radio still owns pan there. - VfoWidget emits rxPanChanged(int) when the user moves the pan slider; wireVfoWidget connects it to m_audio->setRxPan(). radio, but the radio does not persist this between client sessions. On reconnect the slice starts unmuted. Fix: - VfoWidget emits audioMuteToggled(bool) when the user clicks the mute button (guarded by m_updatingFromModel so model-driven syncs don't trigger a save). - wireVfoWidget saves SliceAudioMuted_{id} to AppSettings on every user toggle, then immediately restores the saved state by calling s->setAudioMute(true) if needed (radio is already connected at this point, so the command goes through cleanly). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rsdr#1676 review) - Use QChar('A' + sliceId) as the SliceAudioMuted settings key so the persisted mute state survives sessions where the radio assigns different numeric slice IDs (matches DaxChannel_SliceA naming convention) (aethersdr#1560) - Add nFrames <= 0 early-exit and explanatory comment to applyRxPanInPlace so zero-length / partial buffers on error paths never enter the loop (aethersdr#1460) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1493bb1 to
fcc5c81
Compare
|
Claude here on behalf of Jeremy — rebased your branch on current main to resolve the conflict with #1696 (NR2 gain clamp, which just merged). The conflict was in Waiting for CI, then merging. — Claude (AI dev partner) |
Summary
applyRxPanInPlace()re-applies a linear pan law to the stereo NR output so the slider works correctly regardless of which NR is active. At pan=50 the function returns immediately — no change to existing behaviour when centred.audio_mute) is not persisted by the radio between client sessions. On reconnect every slice starts unmuted.VfoWidgetnow emitsaudioMuteToggled(bool)(guarded bym_updatingFromModel) andwireVfoWidgetsavesSliceAudioMuted_A/B/C/DtoAppSettings, then restores the saved state on each connection.Scope
This branch is a clean cherry-pick from
upstream/main— contains only the pan and mute fixes, no MNR code. (Previous PR #1676 was closed because it was branched offfeat/mnr-and-quickfixesand the diff included unrelated MNR work.)Review notes
applyRxPanInPlaceisstatic(file-local), called only fromfeedAudioDataandprocessNr2. The MNR path (#ifdef __APPLE__) is intentionally absent here — it belongs in the MNR PR (Add macOS MMSE-Wiener spectral NR (MNR) + 5 dark-theme quick-wins #1672) where the caller exists.QChar('A' + sliceId)) not the numeric ID, so saved state survives sessions where the radio assigns different numeric IDs.nFrames <= 0guard inapplyRxPanInPlaceensures an empty/partial buffer on an error path never enters the loop.Test plan
🤖 Generated with Claude Code