diff --git a/src/vst/editor_geometry.cpp b/src/vst/editor_geometry.cpp index 39919bb..83a9b7d 100644 --- a/src/vst/editor_geometry.cpp +++ b/src/vst/editor_geometry.cpp @@ -64,9 +64,12 @@ int sampleRowHitTest(const EditorLayout& layout, int rowCount, int x, int y) { // Must be within the canvas horizontally and at/below its top. if (x < layout.canvas.left || x >= layout.canvas.right) return -1; if (y < layout.canvas.top) return -1; + // Clip at the canvas bottom: clicks in the canvas's dead-zone below the last + // visible row agree with sampleRowRect, which does not clamp rows to canvas.bottom. + if (y >= layout.canvas.bottom) return -1; const int index = (y - layout.canvas.top) / kSampleRowHeight; if (index < 0 || index >= rowCount) return -1; - // Guard the bottom edge: a click below the last row's canvas bottom is outside. + // Guard the bottom edge: a click below the last row's bottom is outside. const Rect r = sampleRowRect(layout, index); if (y >= r.bottom) return -1; return index; diff --git a/src/vst/reasampler_processor.cpp b/src/vst/reasampler_processor.cpp index 6346b83..ed3c337 100644 --- a/src/vst/reasampler_processor.cpp +++ b/src/vst/reasampler_processor.cpp @@ -2,6 +2,7 @@ #include "reasampler_processor.h" +#include #include #include #include @@ -121,7 +122,6 @@ tresult PLUGIN_API ReaSamplerProcessor::setState(IBStream* state) { int32 got = 0; while (state->read(chunk, sizeof(chunk), &got) == kResultOk && got > 0) { bytes.insert(bytes.end(), chunk, chunk + got); - if (got < static_cast(sizeof(chunk))) break; } setSelectedSampleId(deserializeSelection(bytes)); // Rebuild from the restored selection (off-thread — setState is a load-time call). @@ -133,8 +133,9 @@ tresult PLUGIN_API ReaSamplerProcessor::getState(IBStream* state) { if (!state) return kResultFalse; const std::vector bytes = serializeSelection(selectedSampleId()); if (!bytes.empty()) { - state->write(const_cast(bytes.data()), - static_cast(bytes.size()), nullptr); + const tresult wr = state->write(const_cast(bytes.data()), + static_cast(bytes.size()), nullptr); + if (wr != kResultOk) return wr; } return kResultOk; } @@ -155,6 +156,10 @@ std::string ReaSamplerProcessor::reloadFromBank() { // thread — process() only touches the atomic. std::lock_guard lock(reloadMutex_); + // Mint this reload's generation number first so we can stamp the built instrument + // with it before publishing. Under reloadMutex_ no other reload races here. + const std::uint64_t gen = reloadGeneration_.fetch_add(1, std::memory_order_relaxed) + 1; + // 1. Read the live bank + resolve the project dir over the bridge (allocates, // calls REAPER — fine here, off-thread). std::optional banksJson = @@ -187,7 +192,7 @@ std::string ReaSamplerProcessor::reloadFromBank() { static_cast(layout.sampleRate), sel->rootNote, sel->loop); built = std::make_unique( - std::move(km), kMaxVoices, tier0Adsr(sampleRate_)); + std::move(km), kMaxVoices, tier0Adsr(sampleRate_), gen); // Record which id actually resolved so a first-sample fallback // (empty stored id) becomes the concrete selection. resolvedId = selectedSampleId(); @@ -198,19 +203,43 @@ std::string ReaSamplerProcessor::reloadFromBank() { } // 4. Publish. Atomically install the new instrument; the DISPLACED one goes to the - // graveyard (process may still be reading it this block — it is reclaimed only when - // process is stopped, in setActive(false)/terminate). A null `built` (no bank / - // unreadable WAV) installs silence. `built` is heap-owned; release() hands - // ownership to the atomic, and the exchanged pointer is re-owned by the graveyard. + // graveyard tagged with this generation (process may still be mid-block reading + // it). A null `built` (no bank / unreadable WAV) installs silence. + // `built` is heap-owned; release() hands ownership to the atomic, and the + // exchanged pointer is re-owned by the graveyard. + // + // Bounded reclaim: prune graveyard entries where displacedAt <= seen, where seen + // is the last generation process() published. process() publishes inst->installedAt + // (not a re-read of reloadGeneration_), so seen == D means process holds the + // instrument installed at gen D. An entry with displacedAt == D was displaced by + // reload D, which installed that very successor — process cannot be holding the + // displaced entry. The pruning condition is therefore <= (see header for the full + // proof). Remaining entries drain at setActive(false) / terminate() when process + // is guaranteed stopped. + const std::uint64_t seen = processGeneration_.load(std::memory_order_acquire); + graveyard_.erase( + std::remove_if(graveyard_.begin(), graveyard_.end(), + [seen](const GraveyardEntry& e) { return e.displacedAt <= seen; }), + graveyard_.end()); LoadedInstrument* prev = live_.exchange(built.release()); - if (prev) graveyard_.emplace_back(prev); + if (prev) graveyard_.push_back({gen, std::unique_ptr(prev)}); return resolvedId; } tresult PLUGIN_API ReaSamplerProcessor::process(ProcessData& data) { // REAL-TIME: no allocation, no IO, no locks. Load the live instrument once for the - // whole block (a single atomic acquire). + // whole block (a single atomic acquire), then publish inst->installedAt so the off- + // thread graveyard pruner knows exactly which generation this block is holding. + // + // We publish installedAt — not a fresh re-read of reloadGeneration_ — to close an + // ordering race: reading reloadGeneration_ after live_ could observe a generation + // newer than the pointer we actually hold, causing the pruner to free an instrument + // process is still reading. installedAt was set on the reload path before the atomic + // exchange that made the instrument visible, so it is always <= the generation of any + // instrument that could have been loaded after our acquire above. LoadedInstrument* inst = live_.load(std::memory_order_acquire); + const std::uint64_t heldGen = inst ? inst->installedAt : 0; + processGeneration_.store(heldGen, std::memory_order_release); // Marshal MIDI note-on/off from the event input into the voice engine. Tier 0 maps // events at block granularity (no per-event sample-offset split) — audible timing is diff --git a/src/vst/reasampler_processor.h b/src/vst/reasampler_processor.h index a4bad33..c0bffbc 100644 --- a/src/vst/reasampler_processor.h +++ b/src/vst/reasampler_processor.h @@ -37,12 +37,19 @@ namespace reasampler::vst { // live and die together at a STABLE address — hence this is heap-allocated and neither // copyable nor movable. The audio thread only ever reads it through an atomic pointer; // it is built and destroyed off the audio thread. +// +// installedAt: the reloadGeneration_ value at which this instrument was atomically +// installed into live_. Set on the reload path before the exchange. process() publishes +// this field (not a fresh re-read of reloadGeneration_) so the published generation is +// exactly the generation of the instrument actually in hand for the block. struct LoadedInstrument { Keymap keymap; VoiceEngine engine; + std::uint64_t installedAt = 0; // reload generation at which this was installed - LoadedInstrument(Keymap km, std::size_t maxVoices, const AdsrParams& adsr) - : keymap(std::move(km)), engine(maxVoices, keymap, adsr) {} + LoadedInstrument(Keymap km, std::size_t maxVoices, const AdsrParams& adsr, + std::uint64_t gen) + : keymap(std::move(km)), engine(maxVoices, keymap, adsr), installedAt(gen) {} LoadedInstrument(const LoadedInstrument&) = delete; LoadedInstrument& operator=(const LoadedInstrument&) = delete; @@ -105,13 +112,31 @@ private: // LoadedInstrument and atomically swaps it into `live_`. The DISPLACED instrument is // NOT freed on the reload path: process() may still be mid-block reading it, and two // rapid reloads could otherwise free a pointer process is using. Instead it is parked - // in `graveyard_` and reclaimed only when process is GUARANTEED stopped — at - // setActive(false) / terminate(), which the host never runs concurrently with - // process. The graveyard grows by one engine per reload during a session (bounded by - // user sample switches — a few objects), a deliberate leak-until-deactivate trade for - // a lock-free, race-free audio thread. Tier 2 can add epoch-based reclaim if needed. + // in `graveyard_` tagged with the reload generation at which it was displaced. + // + // Bounded reclaim: process() publishes inst->installedAt (the generation at which the + // held instrument was installed) via processGeneration_ — a single atomic store, RT- + // safe. The reload path prunes graveyard entries where displacedAt <= seen (where seen + // is the last published processGeneration_). + // + // Safety argument: an entry with displacedAt == D was displaced by reload D, which + // simultaneously installed its successor with installedAt == D. process() publishing + // seen == D means it holds that successor (or a later one). In either case, the + // displaced entry is not the pointer process is using, so freeing it is safe. The + // pruning condition is therefore <= (not strict <): an entry displaced at exactly the + // published generation is also provably unreachable. + // + // The graveyard's upper bound is the number of reloads since process last ran + // (typically 0–1 in normal use). Remaining entries drain at setActive(false) / + // terminate(), when the host guarantees process is stopped. std::atomic live_{nullptr}; - std::vector> graveyard_; // freed only when stopped + std::atomic reloadGeneration_{0}; // incremented by each reload (off-thread, under reloadMutex_; read atomically by process) + std::atomic processGeneration_{0}; // generation last seen by process (written on audio thread, read off-thread) + struct GraveyardEntry { + std::uint64_t displacedAt = 0; // reloadGeneration_ value when this was displaced + std::unique_ptr instrument; + }; + std::vector graveyard_; // drained on reclaim + setActive(false) + terminate std::mutex reloadMutex_; // serializes off-thread reloads + graveyard access // The selected sample id (instance state). Off-thread only; a small mutex guards the diff --git a/tests/test_editor_geometry.cpp b/tests/test_editor_geometry.cpp index b19db13..d808584 100644 --- a/tests/test_editor_geometry.cpp +++ b/tests/test_editor_geometry.cpp @@ -172,6 +172,16 @@ static void testSampleRowHitTestMisses() { CHECK(sampleRowHitTest(L, rows, L.canvas.left - 1, last.top) == -1); // Zero rows -> always -1. CHECK(sampleRowHitTest(L, 0, 200, L.canvas.top + 1) == -1); + // At or below canvas.bottom -> always -1, even if rowCount would cover that y. + // This guards paint<->hit-test agreement: sampleRowRect does not clamp to canvas, + // so without this clip a row that extends past canvas.bottom would hit-test but + // never be drawn (or vice versa). + CHECK(sampleRowHitTest(L, rows, 200, L.canvas.bottom) == -1); + // Use a large rowCount so index arithmetic would return a valid row without the + // canvas.bottom guard — proving the guard fires independently of rowCount. + const int bigRows = 1000; + CHECK(sampleRowHitTest(L, bigRows, 200, L.canvas.bottom) == -1); + CHECK(sampleRowHitTest(L, bigRows, 200, L.canvas.bottom + 5) == -1); } // The drawn-row <-> hit-test agreement: every pixel inside a row rect must resolve to diff --git a/tests/test_sample_map.cpp b/tests/test_sample_map.cpp index 130dc05..70e2bc5 100644 --- a/tests/test_sample_map.cpp +++ b/tests/test_sample_map.cpp @@ -16,12 +16,17 @@ // average / zero-stride / empty; buildTier0Keymap single full-keyboard zone with the // root + loop + rate threaded and rate defaulting; selection state round-trip + empty id // + wrong-version / truncated -> "". +// wav_trim -> extractFloatFrames -> downmixToMono integration: locks the interleave- +// stride contract across the seam (that the byte stride wav_trim reports matches the +// channel-count stride downmixToMono divides by). #include "../src/vst/sample_map.h" #include #include +#include #include +#include #include "../src/bank_book.h" #include "../src/bank_model.h" @@ -247,6 +252,104 @@ static void testSelectionStateTruncated() { CHECK(deserializeSelection({1, 0, 0}) == ""); // fewer than 4 bytes (no tag) } +// --- wav_trim -> extractFloatFrames -> downmixToMono integration --------------- +// +// Locks the interleave-stride contract at the seam between wav_trim and sample_map: +// wav_trim reports channelCount, extractFloatFrames yields interleaved samples with +// that stride, and downmixToMono divides by that same stride. If either module +// changed its understanding of the layout (e.g. extractFloatFrames started packing +// differently, or downmixToMono changed its stride divisor), this test catches it. + +static void putU16sm(std::vector& b, std::uint16_t v) { + b.push_back(static_cast(v & 0xFF)); + b.push_back(static_cast((v >> 8) & 0xFF)); +} +static void putU32sm(std::vector& b, std::uint32_t v) { + b.push_back(static_cast(v & 0xFF)); + b.push_back(static_cast((v >> 8) & 0xFF)); + b.push_back(static_cast((v >> 16) & 0xFF)); + b.push_back(static_cast((v >> 24) & 0xFF)); +} +static void putTagsm(std::vector& b, const char* t) { + for (int i = 0; i < 4; ++i) b.push_back(static_cast(t[i])); +} +static void putFloatsm(std::vector& b, float f) { + std::uint8_t tmp[4]; + std::memcpy(tmp, &f, 4); + for (int i = 0; i < 4; ++i) b.push_back(tmp[i]); +} + +// Build a 32-bit-float WAV byte buffer. Samples: frame f, channel c = value(f, c). +template +static std::vector buildWav(std::uint16_t channels, + std::uint32_t sampleRate, + std::size_t frames, + Fn value) { + const std::uint32_t dataBytes = + static_cast(frames * channels * 4u); + std::vector chunks; + putTagsm(chunks, "fmt "); + putU32sm(chunks, 16); + putU16sm(chunks, 3); // IEEE float + putU16sm(chunks, channels); + putU32sm(chunks, sampleRate); + putU32sm(chunks, sampleRate * channels * 4u); + putU16sm(chunks, static_cast(channels * 4)); + putU16sm(chunks, 32); + putTagsm(chunks, "data"); + putU32sm(chunks, dataBytes); + for (std::size_t f = 0; f < frames; ++f) + for (std::uint16_t c = 0; c < channels; ++c) + putFloatsm(chunks, value(f, c)); + std::vector wav; + putTagsm(wav, "RIFF"); + putU32sm(wav, static_cast(4 + chunks.size())); + putTagsm(wav, "WAVE"); + wav.insert(wav.end(), chunks.begin(), chunks.end()); + return wav; +} + +static void testWavTrimToDownmixPipelineStereo() { + // Stereo WAV: frame f, L = f * 0.1f, R = f * 0.1f + 0.5f. Expected mono average: + // (f * 0.1f + f * 0.1f + 0.5f) / 2 = f * 0.1f + 0.25f. + const std::size_t kFrames = 4; + auto wav = buildWav(2, 48000, kFrames, + [](std::size_t f, std::uint16_t c) { + return static_cast(f) * 0.1f + (c == 1 ? 0.5f : 0.0f); + }); + WavLayout layout = parseWavLayout(wav); + CHECK(layout.valid); + CHECK(layout.channelCount == 2); + CHECK(layout.frameCount() == kFrames); + const std::vector interleaved = + extractFloatFrames(wav, layout, 0, layout.frameCount()); + CHECK(interleaved.size() == kFrames * 2); + const std::vector mono = downmixToMono(interleaved, layout.channelCount); + CHECK(mono.size() == kFrames); + for (std::size_t f = 0; f < kFrames; ++f) { + const float expected = static_cast(f) * 0.1f + 0.25f; + CHECK(approx(mono[f], expected)); + } +} + +static void testWavTrimToDownmixPipelineMono() { + // Mono WAV: extractFloatFrames -> downmixToMono with channelCount==1 is a passthrough. + const std::size_t kFrames = 3; + auto wav = buildWav(1, 44100, kFrames, + [](std::size_t f, std::uint16_t) { + return static_cast(f) * 0.5f; + }); + WavLayout layout = parseWavLayout(wav); + CHECK(layout.valid); + CHECK(layout.channelCount == 1); + const std::vector interleaved = + extractFloatFrames(wav, layout, 0, layout.frameCount()); + CHECK(interleaved.size() == kFrames); + const std::vector mono = downmixToMono(interleaved, layout.channelCount); + CHECK(mono.size() == kFrames); + CHECK(approx(mono[0], 0.0) && approx(mono[1], 0.5) && approx(mono[2], 1.0)); +} + int main() { testSelectByIdHit(); testSelectFirstSampleFallbackOnEmptyId(); @@ -269,6 +372,8 @@ int main() { testSelectionStateEmptyId(); testSelectionStateWrongVersion(); testSelectionStateTruncated(); + testWavTrimToDownmixPipelineStereo(); + testWavTrimToDownmixPipelineMono(); if (g_fail == 0) std::printf("sample_map: all tests passed\n"); return g_fail != 0;