fix(S4): close graveyard epoch ordering race; add canvas-clip + stride integration tests
Tag LoadedInstrument with installedAt; process() publishes that field (not a re-read of reloadGeneration_) so the pruner's displacedAt <= seen bound is airtight. Also fixes sampleRowHitTest canvas.bottom over-read and adds interleave-stride + canvas-clip tests.
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
|
||||
#include "reasampler_processor.h"
|
||||
|
||||
#include <algorithm>
|
||||
#include <cstdint>
|
||||
#include <fstream>
|
||||
#include <vector>
|
||||
@@ -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<int32>(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<std::uint8_t> bytes = serializeSelection(selectedSampleId());
|
||||
if (!bytes.empty()) {
|
||||
state->write(const_cast<std::uint8_t*>(bytes.data()),
|
||||
static_cast<int32>(bytes.size()), nullptr);
|
||||
const tresult wr = state->write(const_cast<std::uint8_t*>(bytes.data()),
|
||||
static_cast<int32>(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<std::mutex> 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<std::string> banksJson =
|
||||
@@ -187,7 +192,7 @@ std::string ReaSamplerProcessor::reloadFromBank() {
|
||||
static_cast<int>(layout.sampleRate), sel->rootNote,
|
||||
sel->loop);
|
||||
built = std::make_unique<LoadedInstrument>(
|
||||
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<LoadedInstrument>(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
|
||||
|
||||
@@ -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<LoadedInstrument*> live_{nullptr};
|
||||
std::vector<std::unique_ptr<LoadedInstrument>> graveyard_; // freed only when stopped
|
||||
std::atomic<std::uint64_t> reloadGeneration_{0}; // incremented by each reload (off-thread, under reloadMutex_; read atomically by process)
|
||||
std::atomic<std::uint64_t> 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<LoadedInstrument> instrument;
|
||||
};
|
||||
std::vector<GraveyardEntry> 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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 <cmath>
|
||||
#include <cstdio>
|
||||
#include <cstring>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
|
||||
#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<std::uint8_t>& b, std::uint16_t v) {
|
||||
b.push_back(static_cast<std::uint8_t>(v & 0xFF));
|
||||
b.push_back(static_cast<std::uint8_t>((v >> 8) & 0xFF));
|
||||
}
|
||||
static void putU32sm(std::vector<std::uint8_t>& b, std::uint32_t v) {
|
||||
b.push_back(static_cast<std::uint8_t>(v & 0xFF));
|
||||
b.push_back(static_cast<std::uint8_t>((v >> 8) & 0xFF));
|
||||
b.push_back(static_cast<std::uint8_t>((v >> 16) & 0xFF));
|
||||
b.push_back(static_cast<std::uint8_t>((v >> 24) & 0xFF));
|
||||
}
|
||||
static void putTagsm(std::vector<std::uint8_t>& b, const char* t) {
|
||||
for (int i = 0; i < 4; ++i) b.push_back(static_cast<std::uint8_t>(t[i]));
|
||||
}
|
||||
static void putFloatsm(std::vector<std::uint8_t>& 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 <typename Fn>
|
||||
static std::vector<std::uint8_t> buildWav(std::uint16_t channels,
|
||||
std::uint32_t sampleRate,
|
||||
std::size_t frames,
|
||||
Fn value) {
|
||||
const std::uint32_t dataBytes =
|
||||
static_cast<std::uint32_t>(frames * channels * 4u);
|
||||
std::vector<std::uint8_t> 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<std::uint16_t>(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<std::uint8_t> wav;
|
||||
putTagsm(wav, "RIFF");
|
||||
putU32sm(wav, static_cast<std::uint32_t>(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<float>(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<AudioSample> interleaved =
|
||||
extractFloatFrames(wav, layout, 0, layout.frameCount());
|
||||
CHECK(interleaved.size() == kFrames * 2);
|
||||
const std::vector<AudioSample> mono = downmixToMono(interleaved, layout.channelCount);
|
||||
CHECK(mono.size() == kFrames);
|
||||
for (std::size_t f = 0; f < kFrames; ++f) {
|
||||
const float expected = static_cast<float>(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<float>(f) * 0.5f;
|
||||
});
|
||||
WavLayout layout = parseWavLayout(wav);
|
||||
CHECK(layout.valid);
|
||||
CHECK(layout.channelCount == 1);
|
||||
const std::vector<AudioSample> interleaved =
|
||||
extractFloatFrames(wav, layout, 0, layout.frameCount());
|
||||
CHECK(interleaved.size() == kFrames);
|
||||
const std::vector<AudioSample> 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;
|
||||
|
||||
Reference in New Issue
Block a user