From a04ef54eec99f63ba2ce2bcd50329a004ebe1787 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 26 Jul 2026 16:05:01 -0400 Subject: [PATCH] fix(tests): rework testNoteOffReleasesNewestSameNote to actually prove newest-first release; add single-frame-loop test; add two comments in sampler_core.cpp --- src/vst/sampler_core.cpp | 5 ++- tests/test_sampler_core.cpp | 90 +++++++++++++++++++++++++++++++------ 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/src/vst/sampler_core.cpp b/src/vst/sampler_core.cpp index 2325a3e..ae502bb 100644 --- a/src/vst/sampler_core.cpp +++ b/src/vst/sampler_core.cpp @@ -199,6 +199,9 @@ AudioSample Voice::renderFrame() { if (loopUsable && i1 >= loop.end) { i1 = loop.start; // seamless wrap for the interpolation partner. } + // i0 is always in [0, frameCount) after the early-out above; the guard is purely + // defensive. i1 (the interpolation partner) can exceed frameCount when no loop + // wraps it — only that partner actually needs the clamp. const double s0 = (i0 >= 0 && i0 < frameCount) ? static_cast(pcm[i0]) : 0.0; const double s1 = (i1 >= 0 && i1 < frameCount) ? static_cast(pcm[i1]) : 0.0; const double interp = s0 + (s1 - s0) * frac; @@ -288,7 +291,7 @@ void VoiceEngine::noteOff(int note) { void VoiceEngine::render(std::vector& out, std::size_t frameCount) { const std::size_t base = out.size(); - out.resize(base + frameCount, 0.0f); + out.resize(base + frameCount, 0.0f); // S4: caller must pre-reserve — no allocation allowed under the VST3 process callback. for (Voice& voice : voices_) { if (!voice.active()) continue; for (std::size_t f = 0; f < frameCount; ++f) { diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index e885c1e..4a3a5a7 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -309,26 +309,62 @@ static void testPolyphonicAllocation() { } static void testNoteOffReleasesNewestSameNote() { - Keymap km = Keymap::singleSampleChromatic(dcSample(1000, 60)); - // Long release so we can observe which voice entered release. + // Prove that noteOff releases the NEWEST (highest startOrder) instance of a + // re-triggered note, leaving the older voice in sustain. + // + // Two voices at distinct velocities so their output is distinguishable: + // "first" (older) -> velocity 64 -> gain ~0.504 (G_old) + // "second" (newer) -> velocity 127 -> gain 1.0 (G_new) + // + // With a DC-1 sample and sustain=1, while both are held: + // render sum == G_old + G_new. + // + // After noteOff (must release newest), the newer voice enters a short release. + // Render past releaseFrames: newer voice finishes; only the older voice remains. + // Sum then equals G_old, and activeVoiceCount drops to 1. If the WRONG voice + // were released, the older would finish and the remaining sum would equal G_new + // (1.0 vs ~0.504) — the velocities make the error distinguishable. + const int velOld = 64; + const int velNew = 127; + const double gainOld = velOld / 127.0; // ~0.504 + const double gainNew = velNew / 127.0; // 1.0 + + Keymap km = Keymap::singleSampleChromatic(dcSample(100000, 60)); AdsrParams a = flatAdsr(); - a.releaseFrames = 1000; + a.releaseFrames = 10; // short but non-zero so voice stays active through release VoiceEngine eng(8, km, a); - std::size_t first = eng.noteOn(60, 100); - std::size_t second = eng.noteOn(60, 100); // same note re-triggered + std::size_t first = eng.noteOn(60, velOld); // older voice, lower gain + std::size_t second = eng.noteOn(60, velNew); // newer voice, higher gain CHECK(first != second); CHECK(eng.activeVoiceCount() == 2); - eng.noteOff(60); // releases the NEWEST (second) - std::vector out; - eng.render(out, 1); - // Both still active (long release), but only the newest is releasing. - CHECK(eng.activeVoiceCount() == 2); - // A second note-off releases the older one too. + // While both are held, combined output equals gainOld + gainNew. + { + std::vector out; + eng.render(out, 1); + CHECK(approx(out[0], gainOld + gainNew, 1e-4)); + } + + // Release once — must target the NEWEST voice (second). eng.noteOff(60); - eng.render(out, 1); - CHECK(eng.activeVoiceCount() == 2); // both releasing, not yet finished + + // Render past the release (releaseFrames == 10): newer voice goes Finished. + std::vector out; + eng.render(out, 20); + + // Newer voice must be done; only the older voice remains. + CHECK(eng.activeVoiceCount() == 1); + // Tail frames must equal gainOld (~0.504), NOT gainNew (1.0). + // If the older voice were released instead, the tail would be ~1.0 here. + for (std::size_t i = 15; i < out.size(); ++i) { + CHECK(approx(out[i], gainOld, 1e-4)); + } + + // A second note-off releases the remaining older voice. + eng.noteOff(60); + eng.render(out, 20); + CHECK(eng.activeVoiceCount() == 0); } static void testOutOfZoneNoteConsumesNoVoice() { @@ -444,6 +480,33 @@ static void testZeroLengthLoopGoesSilent() { for (std::size_t i = 60; i < out.size(); ++i) CHECK(approx(out[i], 0.0, 1e-6)); } +static void testSingleFrameLoop() { + // A loop of exactly one frame [start, start+1) — the narrowest valid loop. + // The path is correct-by-luck (loopLen = 1.0 divides evenly into any integer + // readPos advance at unity ratio), but Tier-2 tight loops make it load-bearing. + SampleData s; + s.frames.resize(10); + for (int i = 0; i < 10; ++i) s.frames[i] = static_cast(i) * 0.1f; + s.rootNote = 60; + s.loop.hasLoop = true; + s.loop.start = 5; + s.loop.end = 6; // single-frame loop: [5, 6) + + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, flatAdsr()); + eng.noteOn(60, 127); // unity ratio, full velocity + + std::vector out; + eng.render(out, 50); // well past the sample end + + // Voice must still be active — the single-frame loop keeps it alive. + CHECK(eng.activeVoiceCount() == 1); + // Every frame from the loop-start onward must be the value of frame 5 (0.5). + for (std::size_t i = 10; i < out.size(); ++i) { + CHECK(approx(out[i], 0.5, 1e-4)); + } +} + static void testAbsentLoopGoesSilent() { // No loop at all: held note runs off the end and goes idle (same as zero-length). SampleData s = dcSample(50, 60); @@ -515,6 +578,7 @@ int main() { testStealsOldestWhenNoneReleasing(); testLoopSustainSeamless(); testZeroLengthLoopGoesSilent(); + testSingleFrameLoop(); testAbsentLoopGoesSilent(); testVelocityToVolume(); testPolyphonyMixesAdditively();