diff --git a/TODO.md b/TODO.md index 42ab2a2..789e5ad 100644 --- a/TODO.md +++ b/TODO.md @@ -23,12 +23,12 @@ Findings from a code-review pass over `src/plug` and `src/synth` (2026-07-27, br - [x] **`new[]`/`delete` mismatches (UB) in `JaySynth`'s destructor** — `m_pVoices`, `pPer_voice_controls`, `pCurrNoteInfos`, `humanize_voice_param[i]`, `ppHumanizedSliders[i]`, `m_ppAudioThread`, `m_ppEventAudioThreadRdy` (`src/plug/JaySynth.cpp`) all now freed with `delete[]` to match their `new T[n]` allocations. Also found and fixed the same pattern via `ScopedPointer` in `PluginProcessor.cpp` (`setStateInformation`/`setCurrentProgramStateInformation`) — `ScopedPointer` always calls scalar `delete` (per JUCE's own doc comment), so `ScopedPointer pUncompressedData = new char[...]` was the same bug; replaced with `HeapBlock`, JUCE's array-owning smart pointer. - [x] **`malloc` was never null-checked** across the C DSP core. Added a shared `SynthCheckAlloc()` helper (`synth_defs.h`/`synth_debug.c`) that aborts with a clear diagnostic instead of returning NULL, and wrapped all 24 `malloc` call sites across `env.c`, `lfo.c`, `vcf.c`, `vco.c`, `blit.c`, `wavetable.c`, `voice.c`, `param_scale.c`. All are one-time init/bufsize-change calls, never in the per-block render path, so this adds no real-time overhead. -## Patch/Bank import-export +## Patch/Bank import-export (fixed) -- [ ] **Legacy patch import decodes the wrong XML node** — uses outer `xml` instead of loop variable `xml2` in the legacy branch (`src/plug/PluginProcessor.cpp:959-991`), unlike every other branch; currently masked but re-decodes redundantly and is a latent bug if the legacy format gains child elements. -- [ ] **"Regular" (non-chunk) `.fxb`/`.fxp` files are silently accepted but never loaded** — `is_regular` is computed and never used (`src/plug/PluginProcessor.cpp:834-852`, `930-948`). Fix: either implement the regular-format path or surface an error to the user. -- [ ] **`.fxp`/`.fxb` saving is entirely unimplemented** (`savePatchToFile`/`saveBankToFile`, `src/plug/PluginProcessor.cpp:1027-1067`, both marked `// ToDo`) while loading is implemented — asymmetric format support. -- [ ] **`patchDecodeXml` and `patchDecodeXml_legacy` are near-duplicated**, including a copy-pasted legacy frequency→cents conversion special case (`src/plug/PluginProcessor.cpp:749-765`, `767-783`). Fix: factor out the shared per-parameter decode loop. +- [x] **Legacy patch import decoded the wrong XML node.** `patchImportXml` now handles the legacy single-patch format (`JSYNTH_PATCH` root) as its own top-level branch instead of incorrectly nested inside the modern per-child loop, which checked/decoded the outer `xml` on every iteration instead of the loop variable. Also switched its CC-table import to `import_midi_cc` (matching the actually-exercised convention already used by `bankDecodeXml_legacy`, since this branch was previously unreachable dead code). `loadPatchFromFile`'s `.xmp` branch now also accepts legacy-rooted files, so single legacy patches can actually be loaded. Also fixed a related bug found in passing: `setCurrentProgramStateInformation` (VST2 "copy plugin state to another track") was passing `patchImportXml` an XML node one level too deep, making it a complete no-op. +- [x] **Found a much bigger bug while investigating "regular .fxb/.fxp silently ignored": the magic-number check itself was completely broken.** It compared `String((const char*)&bank->chunkMagic)` (raw file bytes as text, e.g. `"CcnK"`) against `String(cMagic)` (JUCE's `int`-to-*decimal-text* constructor, e.g. `"1130589771"`) — these can never be equal, so **the entire opaque `.fxb`/`.fxp` load path was dead code**, regardless of format. Fixed using `ByteOrder::bigEndianInt`, the correct byte-order-aware comparison (matches the pattern JUCE's own VST3 wrapper uses for the identical struct). "Regular" (non-chunk) format is still not supported for load, but now surfaces an `AlertWindow` explaining why instead of silently doing nothing. Verified with a standalone round-trip test of the binary format logic (magic detection, size/payload round-trip, safe rejection of truncated/bogus files). +- [x] **`.fxp`/`.fxb` saving was unimplemented.** Now implemented using the same opaque-chunk format the (now-fixed) load path expects, reusing `getCurrentProgramStateInformation`/`getStateInformation` for the payload. +- [x] **`patchDecodeXml`/`patchDecodeXml_legacy` duplication** — `_legacy` now just delegates to `patchDecodeXml`. ## Design diff --git a/src/plug/PluginProcessor.cpp b/src/plug/PluginProcessor.cpp index 20c6dc5..02fcbac 100644 --- a/src/plug/PluginProcessor.cpp +++ b/src/plug/PluginProcessor.cpp @@ -9,6 +9,7 @@ */ #include #include +#include #include "PluginProcessor.h" #include "JaySynthAudioProcessorEditor.h" @@ -843,20 +844,9 @@ void JaySynthAudioProcessor::patchDecodeXml(const XmlElement *xml, JaySynthSound void JaySynthAudioProcessor::patchDecodeXml_legacy(const XmlElement& xml, JaySynthSound *patch) { - // ok, now pull out our parameters.. - patch->setName(xml.getStringAttribute("NAME")); - for (int i=0; i < SYNTH_NUM_PARAMS; i++) - { - synth_float_t value = (synth_float_t) xml.getDoubleAttribute (getParameterNameXML(i), patch->getParameter(i)); - if (i==SYNTH_PARAM_TUNE) - { - if (value > 400.0) - { - value = 1200*log(value/440)/log(2.0); - } - } - patch->setParameter(i, value); - } + // Legacy format uses the same per-parameter attribute layout as the + // current format, just addressed via a reference instead of a pointer. + patchDecodeXml(&xml, patch); } void JaySynthAudioProcessor::bankEncodeXml(XmlElement *xml) @@ -921,18 +911,28 @@ void JaySynthAudioProcessor::loadBankFromFile (const File &file) return; fxBank *bank = (fxBank*)mem.getData(); - if (String((const char*)&bank->chunkMagic) == String(cMagic)) + // fxb/fxp magic numbers and sizes are stored big-endian on disk (see + // vstfxstore.h); bigEndianInt() reads them correctly regardless of + // host byte order. + if (ByteOrder::bigEndianInt(&bank->chunkMagic) == (uint32_t)cMagic) { - bool is_regular = String((const char*)&bank->fxMagic) == String(bankMagic); - bool is_opaque = String((const char*)&bank->fxMagic) == String(chunkBankMagic); + bool is_regular = ByteOrder::bigEndianInt(&bank->fxMagic) == (uint32_t)bankMagic; + bool is_opaque = ByteOrder::bigEndianInt(&bank->fxMagic) == (uint32_t)chunkBankMagic; if (is_opaque) { - uint32_t size = ByteOrder::swap((uint32_t)bank->content.data.size); + uint32_t size = ByteOrder::bigEndianInt(&bank->content.data.size); const size_t chunkOffset = offsetof(fxBank, content.data.chunk); if (chunkOffset > mem.getSize() || (size_t)size > mem.getSize() - chunkOffset) return; setStateInformation(bank->content.data.chunk, (int)size); } + else if (is_regular) + { + AlertWindow::showMessageBoxAsync(AlertWindow::WarningIcon, "Load bank", + "This .fxb file uses the \"regular\" (non-chunk) VST2 bank format, " + "which JaySynth doesn't support. Only chunk-based .fxb files " + "(as saved by JaySynth or other JUCE-based plugins) can be loaded."); + } } } else if (String(".xmb") == ext) @@ -1026,24 +1026,34 @@ void JaySynthAudioProcessor::loadPatchFromFile (const File &file, int index) return; fxProgram *program = (fxProgram*)mem.getData(); - if (String((const char*)&program->chunkMagic) == String(cMagic)) + // fxb/fxp magic numbers and sizes are stored big-endian on disk (see + // vstfxstore.h); bigEndianInt() reads them correctly regardless of + // host byte order. + if (ByteOrder::bigEndianInt(&program->chunkMagic) == (uint32_t)cMagic) { - bool is_regular = String((const char*)&program->fxMagic) == String(fMagic); - bool is_opaque = String((const char*)&program->fxMagic) == String(chunkPresetMagic); + bool is_regular = ByteOrder::bigEndianInt(&program->fxMagic) == (uint32_t)fMagic; + bool is_opaque = ByteOrder::bigEndianInt(&program->fxMagic) == (uint32_t)chunkPresetMagic; if (is_opaque) { - uint32_t size = ByteOrder::swap((uint32_t)program->content.data.size); + uint32_t size = ByteOrder::bigEndianInt(&program->content.data.size); const size_t chunkOffset = offsetof(fxProgram, content.data.chunk); if (chunkOffset > mem.getSize() || (size_t)size > mem.getSize() - chunkOffset) return; setCurrentProgramStateInformation(program->content.data.chunk, (int)size); } + else if (is_regular) + { + AlertWindow::showMessageBoxAsync(AlertWindow::WarningIcon, "Load patch", + "This .fxp file uses the \"regular\" (non-chunk) VST2 patch format, " + "which JaySynth doesn't support. Only chunk-based .fxp files " + "(as saved by JaySynth or other JUCE-based plugins) can be loaded."); + } } } else if (String(".xmp") == ext) { ScopedPointer xml = XmlDocument::parse(file); - if (xml != 0 && xml->hasTagName ("JSynth")) + if (xml != 0 && (xml->hasTagName ("JSynth") || xml->hasTagName ("JSYNTH_PATCH"))) { patchImportXml(xml, currpatch); } @@ -1052,6 +1062,21 @@ void JaySynthAudioProcessor::loadPatchFromFile (const File &file, int index) void JaySynthAudioProcessor::patchImportXml (const XmlElement *xml, JaySynthSound *patch) { + // Legacy single-patch format: parameters are attributes directly on the + // root element, not nested under a "Patch" child - handle it separately + // rather than inside the per-child loop below (xml has no children with + // tag "Patch"/"VelKey"/"MidiCC" in this format, so it doesn't fit that + // loop's shape). Mirrors how bankDecodeXml_legacy decodes each legacy + // bank entry. + if (xml->hasTagName ("JSYNTH_PATCH")) + { + patchDecodeXml_legacy(*xml, patch); + XmlElement *pXML_MidiCc = xml->getChildByName ("JSYNTH_MIDICONTROLLER_TABLE"); + ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_cc(pXML_MidiCc); + setCurrentProgram(getCurrentProgram()); + return; + } + XmlElement *xml2 = xml->getFirstChildElement(); while(xml2) { @@ -1064,20 +1089,13 @@ void JaySynthAudioProcessor::patchImportXml (const XmlElement *xml, JaySynthSoun ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_cc_legacy(pXML_MidiCc); } } - else if (xml->hasTagName ("JSYNTH_PATCH")) - { - patchDecodeXml_legacy(*xml, patch); - XmlElement *pXML_MidiCc = xml->getChildByName ("JSYNTH_MIDICONTROLLER_TABLE"); - ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_cc_legacy(pXML_MidiCc); - - } else if (xml2->hasTagName ("VelKey")) { - ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_velkey(xml->getChildByName ("VelKey")); + ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_velkey(xml2); } else if (xml2->hasTagName ("MidiCC")) { - ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_cc(xml->getChildByName ("MidiCC")); + ((JaySynthMidiCC*)patch->getMidiCC())->import_midi_cc(xml2); } xml2 = xml2->getNextElement(); }; @@ -1111,8 +1129,10 @@ void JaySynthAudioProcessor::setCurrentProgramStateInformation (const void* data { if (xml->hasTagName ("JSynth")) { - XmlElement *xml2 = xml->getFirstChildElement(); - patchImportXml(xml2, currpatch); + // xml is already the "JSynth" container (Patch/VelKey/MidiCC + // children) - patchImportXml expects that container, not one of + // its children. + patchImportXml(xml, currpatch); } } } @@ -1122,7 +1142,34 @@ void JaySynthAudioProcessor::savePatchToFile (const File &file, int index) String ext = file.getFileExtension(); if (String(".fxp") == ext) { - // ToDo: implement fxp save + commitEditBuffer(); + + // Reuse the same opaque chunk format used for the host's own VST2 + // "current program" state (and expected back by the .fxp opaque-chunk + // branch of loadPatchFromFile). + MemoryBlock chunk; + getCurrentProgramStateInformation(chunk); + + const size_t headerSize = offsetof(fxProgram, content) + sizeof(VstInt32); + MemoryBlock mem(headerSize + chunk.getSize(), true); + fxProgram *program = (fxProgram*)mem.getData(); + + program->chunkMagic = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)cMagic); + program->byteSize = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)(mem.getSize() - 8)); + program->fxMagic = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)chunkPresetMagic); + program->version = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)1); + program->fxID = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)JucePlugin_VSTUniqueID); + program->fxVersion = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)JucePlugin_VersionCode); + program->numParams = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)SYNTH_NUM_PARAMS); + + const String name = currpatch->getName(); + memcpy(program->prgName, name.toRawUTF8(), + jmin(sizeof(program->prgName) - 1, (size_t)name.getNumBytesAsUTF8())); + + program->content.data.size = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)chunk.getSize()); + memcpy(program->content.data.chunk, chunk.getData(), chunk.getSize()); + + file.replaceWithData(mem.getData(), mem.getSize()); } else if (String(".xmp") == ext) { @@ -1144,7 +1191,30 @@ void JaySynthAudioProcessor::saveBankToFile (const File &file) String ext = file.getFileExtension(); if (String(".fxb") == ext) { - // ToDo: implement fxp save + commitEditBuffer(); + + // Reuse the same opaque chunk format used for the host's own VST2 + // bank state (and expected back by the .fxb opaque-chunk branch of + // loadBankFromFile). + MemoryBlock chunk; + getStateInformation(chunk); + + const size_t headerSize = offsetof(fxBank, content) + sizeof(VstInt32); + MemoryBlock mem(headerSize + chunk.getSize(), true); + fxBank *bank = (fxBank*)mem.getData(); + + bank->chunkMagic = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)cMagic); + bank->byteSize = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)(mem.getSize() - 8)); + bank->fxMagic = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)chunkBankMagic); + bank->version = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)1); + bank->fxID = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)JucePlugin_VSTUniqueID); + bank->fxVersion = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)JucePlugin_VersionCode); + bank->numPrograms = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)NUM_PROGRAMS); + + bank->content.data.size = (VstInt32)ByteOrder::swapIfLittleEndian((uint32_t)chunk.getSize()); + memcpy(bank->content.data.chunk, chunk.getData(), chunk.getSize()); + + file.replaceWithData(mem.getData(), mem.getSize()); } else if (String(".xmb") == ext) {