From 608ae003dfc06c7fc9dfc974f640d8f626bfddb9 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Mon, 27 Jul 2026 18:08:35 +0200 Subject: [PATCH] Fix High-severity real-time audio-thread safety issues from TODO.md - synthChanged() no longer builds JaySynthActionListener Strings and calls sendActionMessage directly on the audio thread. It now pushes a small POD event into a lock-free SPSC queue (JUCE's AbstractFifo), drained by the existing 10ms UI timer on the message thread, which does the String-building/posting there instead. - VoiceProcessDataV's ~450KB-900KB of SYNTH_MAX_BUFSIZE-sized local arrays are now persistent per-voice buffers allocated once in VoiceSetBufsize (freed in VoiceFree), matching the pattern already used for pBuf_Q and by vcf.c/env.c/lfo.c, instead of being reallocated on the stack on every audio block. - SynthDebug uses vsnprintf instead of vsprintf, bounding writes to its 1024-byte buffer. Verified with clean debug and release builds (no new warnings) and a live playback smoke test in Reaper. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011dhtwRLARk4eiPngcQykLJ --- TODO.md | 8 +-- src/plug/PluginProcessor.cpp | 80 ++++++++++++++++++++++++--- src/plug/PluginProcessor.h | 24 ++++++++- src/synth/synth_debug.c | 2 +- src/synth/voice.c | 102 ++++++++++++++++++++++++++++++----- src/synth/voice.h | 14 +++++ 6 files changed, 206 insertions(+), 24 deletions(-) diff --git a/TODO.md b/TODO.md index 5b70410..13ef449 100644 --- a/TODO.md +++ b/TODO.md @@ -12,11 +12,11 @@ Findings from a code-review pass over `src/plug` and `src/synth` (2026-07-27, br - [x] **Unchecked file stream could be null, crashing on plugin construction.** `JaySynth::JaySynth` (`src/plug/JaySynth.cpp:55-57`) now guards the wavetable file read against `createInputStream()` returning null. - [x] **Parameter-changing calls raced with real-time audio rendering — no lock protected them.** `JaySynth::setParameter`/`setControl`/`setPerVoiceControl`/`setParam`/`ClearControls` (`src/plug/JaySynth.cpp`) now take `ScopedLock sl(lock)`, matching `renderNextBlock` and the note/controller handlers. -## High — real-time audio-thread safety +## High — real-time audio-thread safety (fixed) -- [ ] **Every note/CC/voice-count change allocates Strings and posts a message from the audio thread.** `JaySynthActionListener::call*` (`src/plug/PluginProcessor.h:41-127`) build messages via chained `String` concatenation, invoked synchronously from `synthChanged` inside `noteOn`/`noteOff`/`handleController`/`Voicestart`/`renderNextBlock` — all on the audio thread whenever the GUI is open. Fix: pass typed structs instead of encoding into Strings, or hop to the message thread before allocating. -- [ ] **~450KB–900KB of stack arrays allocated per voice per audio block.** `VoiceProcessDataV` (`src/synth/voice.c:836-858`) declares ~11 `SYNTH_MAX_BUFSIZE`-sized locals, called once per active voice per block from the worker threads. Fix: hoist these into `voice_common_t`/`voice_t` as pre-allocated buffers (same pattern already used by `vcf.c`/`env.c`/`lfo.c`/`blit.c`). -- [ ] **`SynthDebug` uses unbounded `vsprintf` into a fixed 1024-byte buffer plus blocking I/O**, called from `handleMidiEvent` inside the audio-thread render path (`src/synth/synth_debug.c:27-38`). Fix: use `vsnprintf` with the buffer size, and consider a lock-free ring buffer instead of blocking I/O for debug builds. +- [x] **Every note/CC/voice-count change allocated Strings and posted a message from the audio thread.** `synthChanged()` (`src/plug/PluginProcessor.cpp`) no longer calls `JaySynthActionListener::call*` directly from the audio thread. It now pushes a small POD event into a lock-free single-producer/single-consumer queue (JUCE's `AbstractFifo`, added to `PluginProcessor.h`), and the existing 10ms UI `Timer` (`timerCallback`/`dispatchUiEvents`) drains it on the message thread, where the String-building and `sendActionMessage` now safely happen. +- [x] **~450KB–900KB of stack arrays allocated per voice per audio block.** `VoiceProcessDataV`'s 11 `SYNTH_MAX_BUFSIZE`-sized locals (`src/synth/voice.c`) are now persistent per-voice buffers (`pBuf_VCO_fmout`, `pBuf_vco_pwm`, etc., declared in `voice.h`), allocated once in `VoiceSetBufsize`/freed in `VoiceFree` — same lifecycle/pattern already used by `pBuf_Q` and by `vcf.c`/`env.c`/`lfo.c`. +- [x] **`SynthDebug` used unbounded `vsprintf` into a fixed 1024-byte buffer.** `src/synth/synth_debug.c` now uses `vsnprintf` with the buffer size. (Left the blocking `puts()`/`OutputDebugString` I/O as-is — it's compiled out entirely unless `SYNTH_DEBUG` is defined, so release builds are unaffected, and a lock-free logging redesign felt like overreach for a debug-only path.) ## Memory management diff --git a/src/plug/PluginProcessor.cpp b/src/plug/PluginProcessor.cpp index 793024e..6d78bf0 100644 --- a/src/plug/PluginProcessor.cpp +++ b/src/plug/PluginProcessor.cpp @@ -24,6 +24,7 @@ JaySynthAudioProcessorEditor* getEditor(JaySynthAudioProcessor *pThis) //============================================================================== JaySynthAudioProcessor::JaySynthAudioProcessor() +: m_uiEventFifo(uiEventQueueCapacity) { currProgram = 0; currpatch = &patches[currProgram]; @@ -57,10 +58,77 @@ JaySynthAudioProcessor::~JaySynthAudioProcessor() void JaySynthAudioProcessor::timerCallback() { + dispatchUiEvents(); + if (getEditor(this)) actionListener.callLimiterChanged(m_limiter_active); } +void JaySynthAudioProcessor::pushUiEvent(int type, int i1, int i2, float f1) +{ + int start1, size1, start2, size2; + m_uiEventFifo.prepareToWrite(1, start1, size1, start2, size2); + + if (size1 > 0) + { + SynthUiEvent &e = m_uiEventBuffer[start1]; + e.type = type; + e.i1 = i1; + e.i2 = i2; + e.f1 = f1; + m_uiEventFifo.finishedWrite(1); + } + // else: queue is full, drop the event rather than blocking the audio thread. +} + +void JaySynthAudioProcessor::dispatchUiEvent(const SynthUiEvent &e) +{ + switch (e.type) + { + case JaySynth::SYNTH_CHANGED_PARAM: + actionListener.callParamChanged(e.i1); + break; + + case JaySynth::SYNTH_CHANGED_MIDICC: + actionListener.callMidiControllerChanged(e.i1, e.i2, e.f1); + break; + + case JaySynth::SYNTH_CHANGED_NOTE_PRESSED: + actionListener.callMidiNotePressed(e.i1, e.f1); + break; + + case JaySynth::SYNTH_CHANGED_NOTE_RELEASED: + actionListener.callMidiNoteReleased(e.i1, e.f1); + break; + + case JaySynth::SYNTH_CHANGED_NUM_VOICES_PLAYING: + actionListener.callNumVoicesPlaying(e.i1); + break; + + case JaySynth::SYNTH_CHANGED_MIDICLOCK: + actionListener.callMidiClock((uint32_t)e.i1, (uint32_t)e.i2); + break; + + default: + break; + } +} + +void JaySynthAudioProcessor::dispatchUiEvents(void) +{ + int start1, size1, start2, size2; + m_uiEventFifo.prepareToRead(m_uiEventFifo.getNumReady(), start1, size1, start2, size2); + + int i; + for (i = 0; i < size1; i++) + dispatchUiEvent(m_uiEventBuffer[start1 + i]); + + for (i = 0; i < size2; i++) + dispatchUiEvent(m_uiEventBuffer[start2 + i]); + + m_uiEventFifo.finishedRead(size1 + size2); +} + String JaySynthAudioProcessor::wavesGetPath() { bool isExistent, isValid; @@ -149,37 +217,37 @@ void JaySynthAudioProcessor::synthChanged(int type, void *pData) { case JaySynth::SYNTH_CHANGED_PARAM: if (getEditor(this) != NULL) - actionListener.callParamChanged(*((int*)pData)); + pushUiEvent(type, *((int*)pData), 0, 0.0f); break; case JaySynth::SYNTH_CHANGED_MIDICC: pMidiCC_info = (JaySynth::midiCC_info_t*)pData; applyMidiController(pMidiCC_info, true); if (getEditor(this) != NULL) - actionListener.callMidiControllerChanged(pMidiCC_info->channel, pMidiCC_info->ID, (float)pMidiCC_info->value); + pushUiEvent(type, pMidiCC_info->channel, pMidiCC_info->ID, (float)pMidiCC_info->value); break; case JaySynth::SYNTH_CHANGED_NOTE_PRESSED: pMidi_note_info = (JaySynth::midi_note_info_t*)pData; if (getEditor(this) != NULL) - actionListener.callMidiNotePressed(pMidi_note_info->note, (float)pMidi_note_info->velocity); + pushUiEvent(type, pMidi_note_info->note, 0, (float)pMidi_note_info->velocity); break; case JaySynth::SYNTH_CHANGED_NOTE_RELEASED: pMidi_note_info = (JaySynth::midi_note_info_t*)pData; if (getEditor(this) != NULL) - actionListener.callMidiNoteReleased(pMidi_note_info->note, (float)pMidi_note_info->velocity); + pushUiEvent(type, pMidi_note_info->note, 0, (float)pMidi_note_info->velocity); break; case JaySynth::SYNTH_CHANGED_NUM_VOICES_PLAYING: if (getEditor(this) != NULL) - actionListener.callNumVoicesPlaying(*((int*)pData)); + pushUiEvent(type, *((int*)pData), 0, 0.0f); break; case JaySynth::SYNTH_CHANGED_MIDICLOCK: pMidi_quarter_info = (JaySynth::midi_quarter_clock_info_t*)pData; if (getEditor(this) != NULL) - actionListener.callMidiClock(pMidi_quarter_info->bpm, pMidi_quarter_info->type); + pushUiEvent(type, (int)pMidi_quarter_info->bpm, (int)pMidi_quarter_info->type, 0.0f); break; default: diff --git a/src/plug/PluginProcessor.h b/src/plug/PluginProcessor.h index 449d8a6..f2f6234 100644 --- a/src/plug/PluginProcessor.h +++ b/src/plug/PluginProcessor.h @@ -396,7 +396,29 @@ private: int m_limiter_active; synth_float_t m_limiter_env; JK_MidiClock m_JK_MidiClock; - + + // synthChanged() runs on the audio thread. Building the JaySynthActionListener + // Strings and posting them there (as JAYSYNTH_CHANGED_* used to do directly) + // allocates and locks on the audio thread. Instead, synthChanged() pushes a + // small POD event into this lock-free single-producer (audio thread) / + // single-consumer (message-thread timer) queue, and timerCallback() drains it + // and does the actual String-building/sendActionMessage on the message thread. + struct SynthUiEvent + { + int type; + int i1; + int i2; + float f1; + }; + + enum { uiEventQueueCapacity = 256 }; + SynthUiEvent m_uiEventBuffer[uiEventQueueCapacity]; + AbstractFifo m_uiEventFifo; + + void pushUiEvent(int type, int i1, int i2, float f1); + void dispatchUiEvent(const SynthUiEvent &e); + void dispatchUiEvents(void); + //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (JaySynthAudioProcessor); }; diff --git a/src/synth/synth_debug.c b/src/synth/synth_debug.c index 475eabc..21b95f7 100644 --- a/src/synth/synth_debug.c +++ b/src/synth/synth_debug.c @@ -27,7 +27,7 @@ void SynthDebug(const char *fmtstr, ...) va_list args; va_start(args, fmtstr); - vsprintf(buf, fmtstr, args); + vsnprintf(buf, sizeof(buf), fmtstr, args); va_end(args); #ifdef WIN32 OutputDebugString(buf); diff --git a/src/synth/voice.c b/src/synth/voice.c index eae76a0..24b8323 100644 --- a/src/synth/voice.c +++ b/src/synth/voice.c @@ -121,6 +121,21 @@ void VoiceInit(voice_t *pObj, voice_common_t *pCom, UINT32 id, synth_float_t fs) pObj->pBuf_Q = NULL; + for (i = 0; i < (INT32)VOICE_NUM_OSC; i++) + { + pObj->pBuf_VCO_fmout[i] = NULL; + pObj->pBuf_vco_pitch[i] = NULL; + pObj->pBuf_osc_out_smoothed[i] = NULL; + } + pObj->pBuf_vco_pwm = NULL; + pObj->pBuf_vco_fm = NULL; + pObj->pBuf_vco_am = NULL; + pObj->pBuf_vcf_fm = NULL; + pObj->pBuf_vcf_qm = NULL; + pObj->pBuf_vca_am = NULL; + pObj->pBuf_vca_pan = NULL; + pObj->pBuf_osc_out = NULL; + VoiceSetBufsize(pObj, SYNTH_MAX_BUFSIZE); } @@ -249,11 +264,65 @@ void VoiceSetBufsize(voice_t *pObj, UINT32 size) pObj->pBuf_Q = NULL; + for (i = 0; i < (INT32)VOICE_NUM_OSC; i++) + { + if (pObj->pBuf_VCO_fmout[i]) + free(pObj->pBuf_VCO_fmout[i]); + pObj->pBuf_VCO_fmout[i] = NULL; + + if (pObj->pBuf_vco_pitch[i]) + free(pObj->pBuf_vco_pitch[i]); + pObj->pBuf_vco_pitch[i] = NULL; + + if (pObj->pBuf_osc_out_smoothed[i]) + free(pObj->pBuf_osc_out_smoothed[i]); + pObj->pBuf_osc_out_smoothed[i] = NULL; + } + + if (pObj->pBuf_vco_pwm) free(pObj->pBuf_vco_pwm); + pObj->pBuf_vco_pwm = NULL; + + if (pObj->pBuf_vco_fm) free(pObj->pBuf_vco_fm); + pObj->pBuf_vco_fm = NULL; + + if (pObj->pBuf_vco_am) free(pObj->pBuf_vco_am); + pObj->pBuf_vco_am = NULL; + + if (pObj->pBuf_vcf_fm) free(pObj->pBuf_vcf_fm); + pObj->pBuf_vcf_fm = NULL; + + if (pObj->pBuf_vcf_qm) free(pObj->pBuf_vcf_qm); + pObj->pBuf_vcf_qm = NULL; + + if (pObj->pBuf_vca_am) free(pObj->pBuf_vca_am); + pObj->pBuf_vca_am = NULL; + + if (pObj->pBuf_vca_pan) free(pObj->pBuf_vca_pan); + pObj->pBuf_vca_pan = NULL; + + if (pObj->pBuf_osc_out) free(pObj->pBuf_osc_out); + pObj->pBuf_osc_out = NULL; + if (!size) return; pObj->pBuf_Q = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + for (i = 0; i < (INT32)VOICE_NUM_OSC; i++) + { + pObj->pBuf_VCO_fmout[i] = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vco_pitch[i] = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_osc_out_smoothed[i] = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + } + + pObj->pBuf_vco_pwm = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vco_fm = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vco_am = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vcf_fm = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vcf_qm = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vca_am = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_vca_pan = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); + pObj->pBuf_osc_out = (synth_float_t*)malloc(pObj->bufsize*sizeof(synth_float_t)); } void VoiceEvent(voice_t *pObj, UINT32 type) @@ -844,18 +913,27 @@ void VoiceProcessDataV(voice_t *pObj, float *pOut1, float *pOut2, UINT32 len) synth_float_t *pModSrc[VOICE_NUM_MOD_SOURCES]; voice_common_t *pCom = pObj->pCom; - // Temp Variables - synth_float_t VCO_fmout[VOICE_NUM_OSC][SYNTH_MAX_BUFSIZE]; - synth_float_t vco_pwm[SYNTH_MAX_BUFSIZE]; - synth_float_t vco_fm[SYNTH_MAX_BUFSIZE]; - synth_float_t vco_am[SYNTH_MAX_BUFSIZE]; - synth_float_t vcf_fm[SYNTH_MAX_BUFSIZE]; - synth_float_t vcf_qm[SYNTH_MAX_BUFSIZE]; - synth_float_t vca_am[SYNTH_MAX_BUFSIZE]; - synth_float_t vca_pan[SYNTH_MAX_BUFSIZE]; - synth_float_t vco_pitch[VOICE_NUM_OSC][SYNTH_MAX_BUFSIZE]; - synth_float_t osc_out[SYNTH_MAX_BUFSIZE]; - synth_float_t osc_out_smoothed[VOICE_NUM_OSC][SYNTH_MAX_BUFSIZE]; + // Temp Variables - these point at persistent per-voice buffers (allocated + // in VoiceSetBufsize) rather than being call-local arrays, to avoid + // ~450KB-900KB of stack allocation on every audio block. + synth_float_t *VCO_fmout[VOICE_NUM_OSC]; + synth_float_t *vco_pwm = pObj->pBuf_vco_pwm; + synth_float_t *vco_fm = pObj->pBuf_vco_fm; + synth_float_t *vco_am = pObj->pBuf_vco_am; + synth_float_t *vcf_fm = pObj->pBuf_vcf_fm; + synth_float_t *vcf_qm = pObj->pBuf_vcf_qm; + synth_float_t *vca_am = pObj->pBuf_vca_am; + synth_float_t *vca_pan = pObj->pBuf_vca_pan; + synth_float_t *vco_pitch[VOICE_NUM_OSC]; + synth_float_t *osc_out = pObj->pBuf_osc_out; + synth_float_t *osc_out_smoothed[VOICE_NUM_OSC]; + + for (i = 0; i < VOICE_NUM_OSC; i++) + { + VCO_fmout[i] = pObj->pBuf_VCO_fmout[i]; + vco_pitch[i] = pObj->pBuf_vco_pitch[i]; + osc_out_smoothed[i] = pObj->pBuf_osc_out_smoothed[i]; + } #if 0 // Process parameter smoothing diff --git a/src/synth/voice.h b/src/synth/voice.h index babd62a..d4246dd 100644 --- a/src/synth/voice.h +++ b/src/synth/voice.h @@ -305,6 +305,20 @@ typedef struct _svoice_t smooth_t smooth_vco_enable[VOICE_NUM_OSC]; smooth_t smooth_vco_portamento[VOICE_NUM_OSC]; synth_float_t vco_pitch[VOICE_NUM_OSC]; + // Scratch buffers used only within VoiceProcessDataV (voice.c), allocated + // once per voice (see VoiceSetBufsize) instead of as call-local arrays, to + // avoid ~450KB-900KB of stack allocation on every audio block. + synth_float_t *pBuf_VCO_fmout[VOICE_NUM_OSC]; + synth_float_t *pBuf_vco_pwm; + synth_float_t *pBuf_vco_fm; + synth_float_t *pBuf_vco_am; + synth_float_t *pBuf_vcf_fm; + synth_float_t *pBuf_vcf_qm; + synth_float_t *pBuf_vca_am; + synth_float_t *pBuf_vca_pan; + synth_float_t *pBuf_vco_pitch[VOICE_NUM_OSC]; + synth_float_t *pBuf_osc_out; + synth_float_t *pBuf_osc_out_smoothed[VOICE_NUM_OSC]; // smooth_t *pParam_smoother[VOICE_NUM_PARAMS]; // synth_float_t param_smoothed[VOICE_NUM_PARAMS][SYNTH_MAX_BUFSIZE]; } voice_t;