From 056ccd003ec28b6847fd12c136059306174fc826 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 06:37:36 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(voice):=20declick=20every=20takeover=20?= =?UTF-8?q?of=20a=20sounding=20voice=20=E2=80=94=20mono=20retrig/fallback?= =?UTF-8?q?=20and=20poly=20at-cap=20steal;=20same-block=20double-steal=20k?= =?UTF-8?q?eeps=20its=20seed;=20over-cap=20repro=20proves=20the=20engine?= =?UTF-8?q?=20steals=20exactly=20one=20voice=20per=20note-on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/vst/reasampler_processor.h | 7 +- src/vst/sampler_core.cpp | 67 +++++++++- src/vst/sampler_core.h | 46 ++++++- tests/test_sampler_core.cpp | 231 +++++++++++++++++++++++++++++++++ 4 files changed, 341 insertions(+), 10 deletions(-) diff --git a/src/vst/reasampler_processor.h b/src/vst/reasampler_processor.h index 14f5f6b..4366a49 100644 --- a/src/vst/reasampler_processor.h +++ b/src/vst/reasampler_processor.h @@ -52,6 +52,11 @@ struct LoadedInstrument { PreviewCard preview; // Phase S: the isolated preview voice — never part of the pool std::uint64_t installedAt = 0; // reload generation at which this was installed + // The takeover declick (GA fix) is opted IN here — the PRODUCT default: any restart of a + // sounding voice (mono Retrigger takeover/fallback, cross-sample legato restart, POLY + // at-cap steal) fades the cut tone over a few ms instead of clicking. The pure core + // defaults it off (regression baseline) — same layering as the kDefaultPitchEngine + // product default. LoadedInstrument(Keymap km, std::size_t maxVoices, std::uint64_t gen, std::size_t preserveVoiceCap = 0, std::int64_t preserveWindowFrames = 0, @@ -59,7 +64,7 @@ struct LoadedInstrument { MonoTrigger monoTrigger = MonoTrigger::Retrigger) : keymap(std::move(km)), engine(maxVoices, keymap, preserveVoiceCap, preserveWindowFrames, - voiceMode, monoTrigger), + voiceMode, monoTrigger, /*takeoverDeclick=*/true), preview(keymap, preserveWindowFrames), installedAt(gen) {} diff --git a/src/vst/sampler_core.cpp b/src/vst/sampler_core.cpp index c48b546..a2cd775 100644 --- a/src/vst/sampler_core.cpp +++ b/src/vst/sampler_core.cpp @@ -261,7 +261,27 @@ void Voice::presizePreserveShifters(std::int64_t windowFrames) { void Voice::start(int note, int velocity, const SampleData& sample, int rootNote, double keyTrack, const vst::VelocityCurve& velocityCurve, - bool unityVarispeedBypass) { + bool unityVarispeedBypass, bool declickTakeover) { + // Takeover declick (Phase S GA fix): BEFORE any state reset, seed the compensation + // from the last rendered output IFF this start is a takeover/steal of a SOUNDING voice + // and the caller opted in. The seed is exactly the value the hard cut removes, so the + // first new frame carries the old level and the step becomes a fast fade (see + // kDeclickDecay). A fresh start (idle voice) always clears the declick state — no + // phantom ramp. lastOut{L,R}_ are deliberately NOT zeroed here: a SECOND same-block + // takeover (two steals of this voice with no frame rendered between) must re-seed from + // the same pre-cut output level — zeroing would drop the pending ramp and bring the + // click back on that edge. The next rendered frame overwrites lastOut anyway. + if (declickTakeover && active_) { + declickL_ = lastOutL_; + declickR_ = lastOutR_; + declickActive_ = (declickL_ > kDeclickFloor || declickL_ < -kDeclickFloor || + declickR_ > kDeclickFloor || declickR_ < -kDeclickFloor); + } else { + declickL_ = 0.0; + declickR_ = 0.0; + declickActive_ = false; + } + active_ = true; releasing_ = false; amplitudeDone_ = false; @@ -506,8 +526,30 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { ratio_ = baseRatio_ * envFactor; } + // Takeover declick (Phase S GA fix): sum the decaying compensation seeded by a + // takeover/steal start() so the restart's hard cut has no step. Engine-agnostic — applied + // after either pitch-engine branch, on the shared epilogue. Inactive (the common case) + // costs one branch. + if (declickActive_) { + outL += declickL_; + if (stereo) outRlocal += declickR_; + declickL_ *= kDeclickDecay; + declickR_ *= kDeclickDecay; + if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && + declickR_ < kDeclickFloor && declickR_ > -kDeclickFloor) { + declickActive_ = false; + } + } + if (stereo) outR = static_cast(outRlocal); + // Track the value this voice actually contributed THIS frame (post-gain, incl. any running + // declick) — a future takeover restart seeds its declick from exactly this. In a mono + // render the R track mirrors L (dual-mono semantics, matching the stereo mirror of a mono + // sample), so a later stereo takeover still has a sane R seed. + lastOutL_ = outL; + lastOutR_ = stereo ? outRlocal : outL; + readPos_ += ratio_; if (amplitudeDone_) { @@ -533,7 +575,8 @@ void Voice::renderFrameStereo(AudioSample& l, AudioSample& r) { VoiceEngine::VoiceEngine(std::size_t maxVoices, const Keymap& keymap, std::size_t preserveVoiceCap, std::int64_t preserveWindowFrames, - VoiceMode voiceMode, MonoTrigger monoTrigger) + VoiceMode voiceMode, MonoTrigger monoTrigger, + bool takeoverDeclick) // MONO always uses voices_[0] only (last-note priority, single voice); size to 1 so // the "only voices_[0] is ever driven" invariant is structurally enforced — no latent // RT-discipline risk if a future mono path touched voices_[1..]. maxVoices == 0 clamps @@ -542,7 +585,8 @@ VoiceEngine::VoiceEngine(std::size_t maxVoices, const Keymap& keymap, : (maxVoices == 0 ? 1 : maxVoices)), keymap_(keymap), preserveVoiceCap_(preserveVoiceCap), - voiceMode_(voiceMode), monoTrigger_(monoTrigger) { + voiceMode_(voiceMode), monoTrigger_(monoTrigger), + takeoverDeclick_(takeoverDeclick) { // Pre-size every voice's Preserve shifters HERE (construction is off the audio thread), so // note-on never allocates. A 0 window leaves them pass-through (no ring). This is the one // allocation point for the shifter rings across the engine's lifetime. @@ -636,7 +680,10 @@ std::size_t VoiceEngine::monoNoteOn(int note, int velocity) { return 0; } // RETRIGGER takeover / first note of a phrase / cross-sample legato: (re)start the voice. - v.start(note, velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve); + // The declick opt-in rides every mono restart: start() self-gates it on the voice being + // ACTIVE, so a first-note fresh start never ramps — only a hard cut of a sounding tone. + v.start(note, velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve, + /*unityVarispeedBypass=*/false, /*declickTakeover=*/takeoverDeclick_); v.setStartOrder(nextStartOrder_++); return 0; } @@ -669,8 +716,10 @@ void VoiceEngine::monoNoteOff(int note) { return; } // Retrigger (or cross-sample) fallback: re-strike the fallen-back-to note at its own - // original velocity. - v.start(fb.note, fb.velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve); + // original velocity. Peer restart site of monoNoteOn's takeover — same declick opt-in + // (the fallback also hard-cuts the sounding tone). + v.start(fb.note, fb.velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve, + /*unityVarispeedBypass=*/false, /*declickTakeover=*/takeoverDeclick_); v.setStartOrder(nextStartOrder_++); } @@ -697,8 +746,12 @@ std::size_t VoiceEngine::noteOn(int note, int velocity) { // The voice's Preserve shifters were pre-sized at engine construction (off-thread), so // start() only reset()s + warm()s them — no allocation on this audio-thread path. + // The takeover declick rides the STEAL restart too (GA fix): start() self-gates on the + // voice being active, so a free-voice start never ramps — only an at-cap steal, which is + // the same hard cut of a sounding tone as the mono retrig takeover. const std::size_t v = allocateVoice(); - voices_[v].start(note, velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve); + voices_[v].start(note, velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve, + /*unityVarispeedBypass=*/false, /*declickTakeover=*/takeoverDeclick_); voices_[v].setStartOrder(nextStartOrder_++); return v; } diff --git a/src/vst/sampler_core.h b/src/vst/sampler_core.h index 2207a84..722f03f 100644 --- a/src/vst/sampler_core.h +++ b/src/vst/sampler_core.h @@ -381,6 +381,22 @@ private: std::int64_t pos_ = 0; }; +// Takeover declick (Phase S GA fix — audible click when a sounding voice is restarted). A +// takeover restart HARD-CUTS the sounding tone: the read head and envelope restart in one +// frame, a step discontinuity that clicks. This is the same physics on EVERY restart-of-a- +// sounding-voice path — the MONO Retrigger takeover/fallback, the mono cross-sample legato +// restart, AND the POLY at-cap voice steal. When the caller opts in (start()'s +// declickTakeover; the engine passes it on all of those restart paths when constructed with +// takeoverDeclick), start() seeds a compensation from the voice's last rendered output; +// each frame it is summed into the output and decays by kDeclickDecay, so the step becomes a +// fast fade-out of the old tone under the new note's attack. The decay is a per-FRAME DSP +// micro-ramp (~2-4 ms to the -80 dB floor across 44.1-96 kHz), not a stored wall-clock +// quantity — no rate resolution needed. OFF by default so the bare core stays byte-identical +// to the pre-fix engine (the regression baseline); the processor shell opts in, mirroring the +// kDefaultPitchEngine layering. +inline constexpr double kDeclickDecay = 0.95; // per-frame decay of the compensation +inline constexpr double kDeclickFloor = 1e-4; // below this the ramp is done (~ -80 dB) + // --------------------------------------------------------------------------- // A single voice: one active note playing one repitched, enveloped sample. Reads // the sample by fractional frame position with linear interpolation, advancing by @@ -411,10 +427,14 @@ public: // PREVIEW card opts in (it fires at the root, so this is its zero-added-latency path); the // MIDI VoiceEngine does NOT (default false) — a chromatic line must not step ~25 ms faster // at the root note than one semitone away (the FA1-review timing-step finding). + // `declickTakeover` (Phase S GA fix): when TRUE and this voice is currently ACTIVE (a + // takeover/steal restart, not a fresh start), seed the takeover declick from the last + // rendered output — see kDeclickDecay above. A fresh start never declicks. void start(int note, int velocity, const SampleData& sample, int rootNote, double keyTrack = 1.0, const vst::VelocityCurve& velocityCurve = vst::VelocityCurve::flat(), - bool unityVarispeedBypass = false); + bool unityVarispeedBypass = false, + bool declickTakeover = false); // MONO LEGATO takeover (Phase S): re-pitch this ACTIVE voice to `note` without touching the // amplitude envelope, the read position, or the shifter state — pitch moves, no re-attack. @@ -518,6 +538,18 @@ private: PitchShifter shiftL_; PitchShifter shiftR_; + // Takeover declick state (see kDeclickDecay above). lastOut{L,R}_ track the voice's + // most recent rendered output (post-gain, incl. any running declick) so a takeover/steal + // restart can seed declick{L,R}_ with the exact value the hard cut removed. lastOut is + // NOT zeroed by start() — a second same-block takeover (no frame rendered between) must + // re-seed from the same pre-cut level, not from a phantom 0. declickActive_ gates the + // per-frame add + decay; the declick trio is cleared on a fresh (non-takeover) start. + bool declickActive_ = false; + double declickL_ = 0.0; + double declickR_ = 0.0; + double lastOutL_ = 0.0; + double lastOutR_ = 0.0; + std::uint64_t startOrder_ = 0; }; @@ -557,10 +589,19 @@ public: // takeover without a re-attack). Both default to today's behavior (Poly / Retrigger). The // engine's config is immutable — a mode/count change rebuilds the engine off-thread through // the processor's drain-slot reload, so ringing tails survive the swap. + // + // `takeoverDeclick` (Phase S GA fix): when TRUE, every RESTART of a SOUNDING voice — + // the MONO Retrigger takeover, the retrigger fallback on note-off, the cross-sample + // legato restart, and the POLY at-cap voice STEAL — seeds the per-voice declick ramp + // (see kDeclickDecay) so the hard cut of the old tone does not click. start() self-gates + // on the voice being active, so a fresh start (free voice) never ramps. Default FALSE + // keeps the bare core byte-identical to the pre-fix engine (regression baseline); the + // processor shell opts in — the same layering as the kDefaultPitchEngine product default. VoiceEngine(std::size_t maxVoices, const Keymap& keymap, std::size_t preserveVoiceCap = 0, std::int64_t preserveWindowFrames = 0, VoiceMode voiceMode = VoiceMode::Poly, - MonoTrigger monoTrigger = MonoTrigger::Retrigger); + MonoTrigger monoTrigger = MonoTrigger::Retrigger, + bool takeoverDeclick = false); // MIDI note-on. Resolves the note+velocity to a zone; if none matches (out of // zone) it is a defined no-op (no voice consumed). Otherwise allocates a free @@ -654,6 +695,7 @@ private: std::uint64_t nextStartOrder_ = 1; // monotonic; 0 reserved for "never started" VoiceMode voiceMode_ = VoiceMode::Poly; MonoTrigger monoTrigger_ = MonoTrigger::Retrigger; + bool takeoverDeclick_ = false; // GA fix: declick every restart/steal of a sounding voice std::array heldStack_{}; // mono held notes, press order; top = heldCount_-1 std::size_t heldCount_ = 0; }; diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index 2ea67c7..bcb9fd3 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -1896,6 +1896,231 @@ static void testVoiceCountBoundsPolyphony() { CHECK(e0.maxVoices() == 1); // documented degenerate: clamped to 1 } +// GA declick (bug 2): a MONO Retrigger TAKEOVER hard-cuts the sounding tone (read head + +// envelope restart in one frame) — pre-fix the output stepped from the old level to the new +// attack's ~0 in one sample, the audible click. With the engine's takeoverDeclick opt-in +// the boundary frame carries the old level and every later frame moves by a bounded small +// delta while the compensation decays under the new attack. +static void testMonoRetrigTakeoverDeclicksRestart() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 100; // real attack: the new tone starts near 0 + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Mono, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + eng.noteOn(60, 127); + std::vector pre; + eng.render(pre, 200); // past the attack: sustained at 1.0 + CHECK(approx(pre.back(), 1.0, 1e-6)); + + eng.noteOn(64, 127); // Retrigger takeover: hard restart + std::vector post; + eng.render(post, 400); + // No step at the boundary: the first post-takeover frame still carries the old level + // (pre-fix it was the new attack's ~0 — a full-scale step). + CHECK(approx(post[0], 1.0, 0.06)); + // Bounded slope everywhere across the takeover: max per-frame delta is the declick decay + // step (~0.05) + the attack slope (0.01), never a click-sized jump. + double prev = static_cast(pre.back()); + double maxDelta = 0.0; + for (AudioSample v : post) { + const double d = std::fabs(static_cast(v) - prev); + if (d > maxDelta) maxDelta = d; + prev = static_cast(v); + } + CHECK(maxDelta < 0.07); + // The compensation dies out: the tail is the new note's sustain alone. + CHECK(approx(post.back(), 1.0, 1e-3)); +} + +// Peer restart site (peer-symmetry): the Retrigger FALLBACK on note-off — the most-recent +// still-held note re-strikes the voice — is the same hard cut and gets the same declick. +static void testMonoRetrigFallbackDeclicksRestart() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 100; + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Mono, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + eng.noteOn(60, 127); + std::vector a; + eng.render(a, 400); // 60 sustains at 1.0 + eng.noteOn(64, 127); // takeover (declicked, settles back to 1.0) + std::vector b; + eng.render(b, 400); + CHECK(approx(b.back(), 1.0, 1e-3)); + + eng.noteOff(64); // FALLBACK re-strikes held 60 — hard restart + std::vector post; + eng.render(post, 400); + CHECK(approx(post[0], 1.0, 0.06)); // boundary carries the old level, no step + double prev = static_cast(b.back()); + double maxDelta = 0.0; + for (AudioSample v : post) { + const double d = std::fabs(static_cast(v) - prev); + if (d > maxDelta) maxDelta = d; + prev = static_cast(v); + } + CHECK(maxDelta < 0.07); + CHECK(approx(post.back(), 1.0, 1e-3)); +} + +// The declick is TAKEOVER-only: a fresh mono start (idle voice — first note of a phrase, or +// a re-press after a full gate-off) must NOT ramp from a stale last output; the attack starts +// at ~0 exactly as before. +static void testMonoDeclickOnlyOnTakeover() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 100; + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Mono, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + // First note of the phrase: no phantom compensation, attack from ~0. + eng.noteOn(60, 127); + CHECK(probeFrame(eng) < 0.02); + std::vector a; + eng.render(a, 400); // sustain 1.0 (lastOut is now nonzero) + // Full gate-off (release 0 -> instant idle): the next start is FRESH, not a takeover. + eng.noteOff(60); + std::vector gap; + eng.render(gap, 4); + CHECK(approx(gap.back(), 0.0, 1e-9)); + eng.noteOn(62, 127); + CHECK(probeFrame(eng) < 0.02); // no declick from the stale last output +} + +// Peer restart site (peer-symmetry): a POLY at-cap STEAL is the same hard cut as the mono +// retrig takeover — read head + envelope restart on a SOUNDING voice — and gets the same +// declick ramp. Pool of 1 makes the steal deterministic: the second note-on must steal the +// only (sounding) voice, and with the opt-in the boundary carries the old level instead of +// stepping to the new attack's ~0. +static void testPolyStealDeclicksRestart() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 100; + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Poly, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + eng.noteOn(60, 127); + std::vector pre; + eng.render(pre, 200); // past the attack: sustained at 1.0 + CHECK(approx(pre.back(), 1.0, 1e-6)); + + CHECK(eng.noteOn(64, 127) != VoiceEngine::kNoVoice); // at cap: steals the sounding voice + std::vector post; + eng.render(post, 400); + CHECK(approx(post[0], 1.0, 0.06)); // boundary carries the old level, no step + double prev = static_cast(pre.back()); + double maxDelta = 0.0; + for (AudioSample v : post) { + const double d = std::fabs(static_cast(v) - prev); + if (d > maxDelta) maxDelta = d; + prev = static_cast(v); + } + CHECK(maxDelta < 0.07); + CHECK(approx(post.back(), 1.0, 1e-3)); // compensation dies out; new note sustains +} + +// SAME-BLOCK double takeover: two steals of the same voice with NO frame rendered between +// (a two-note chord arriving at cap in one block). The second start() must re-seed the ramp +// from the same pre-cut output level — if start() zeroed lastOut, the pending ramp would be +// dropped and the click would return on exactly this edge. +static void testSameBlockDoubleTakeoverKeepsDeclickSeed() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 100; + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Poly, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + eng.noteOn(60, 127); + std::vector pre; + eng.render(pre, 200); // sustained at 1.0 + CHECK(approx(pre.back(), 1.0, 1e-6)); + + eng.noteOn(64, 127); // steal #1 (no render yet) + eng.noteOn(67, 127); // steal #2, same block + std::vector post; + eng.render(post, 400); + CHECK(approx(post[0], 1.0, 0.06)); // seed survived the double restart + double prev = static_cast(pre.back()); + double maxDelta = 0.0; + for (AudioSample v : post) { + const double d = std::fabs(static_cast(v) - prev); + if (d > maxDelta) maxDelta = d; + prev = static_cast(v); + } + CHECK(maxDelta < 0.07); + CHECK(approx(post.back(), 1.0, 1e-3)); +} + +// GA-VoiceSteal repro (DAW bug): voiceCount 3, a triad note-on'd at the SAME sample time +// (three note-ons in one block, no render between), then a 4th note. The steal must take +// EXACTLY ONE voice (the oldest, none releasing) and leave the other two RINGING — the DAW +// symptom was every tone cutting out. Configured like the live instrument: Preserve engine +// (product default), a real OLA window, sine PCM, default-ish AHDSR (3 ms attack, sustain 1, +// 60 ms release), rendered stereo between events like process() does. +static void testOverCapChordStealsExactlyOne() { + SampleData s = sineSample(96000, 2000.0, 60); // ~2 s at 48k + s.play.adsr.attackFrames = 144; // 3 ms @ 48k + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 2880; // 60 ms @ 48k + s.play.pitchEngine = PitchEngine::Preserve; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + // Mirrors the processor: kPreserveVoiceCap = 8, 50 ms OLA window at 48k = 2400 frames. + VoiceEngine eng(3, km, /*preserveVoiceCap=*/8, /*preserveWindowFrames=*/2400); + + // The chord: three note-ons at one sample time (same block, no render between). + CHECK(eng.noteOn(60, 100) != VoiceEngine::kNoVoice); + CHECK(eng.noteOn(64, 100) != VoiceEngine::kNoVoice); + CHECK(eng.noteOn(67, 100) != VoiceEngine::kNoVoice); + CHECK(eng.activeVoiceCount() == 3); + + // Ring for a while (stereo, like the negotiated bus) — all three still sounding and finite. + std::vector l(4800, 0.0f), r(4800, 0.0f); + eng.render(l.data(), r.data(), l.size()); + CHECK(eng.activeVoiceCount() == 3); + bool finite = true; + for (AudioSample v : l) { if (!std::isfinite(v)) { finite = false; break; } } + CHECK(finite); + + // The 4th note: must steal exactly ONE voice (the oldest = note 60) — never all. + CHECK(eng.noteOn(62, 100) != VoiceEngine::kNoVoice); + CHECK(eng.activeVoiceCount() == 3); + + // Note 60 was the stolen one: its note-off finds no voice (count unchanged after the + // release window). Notes 64 and 67 must still hold their voices — each note-off drops + // the count by one once the 60 ms release tail has run out. + std::fill(l.begin(), l.end(), 0.0f); std::fill(r.begin(), r.end(), 0.0f); + eng.noteOff(60); + eng.render(l.data(), r.data(), l.size()); // 4800 frames > 2880 release + CHECK(eng.activeVoiceCount() == 3); // 60 no longer owns a voice: no-op + + eng.noteOff(64); + std::fill(l.begin(), l.end(), 0.0f); std::fill(r.begin(), r.end(), 0.0f); + eng.render(l.data(), r.data(), l.size()); + CHECK(eng.activeVoiceCount() == 2); // 64 was still ringing — ONE voice released + + eng.noteOff(67); + std::fill(l.begin(), l.end(), 0.0f); std::fill(r.begin(), r.end(), 0.0f); + eng.render(l.data(), r.data(), l.size()); + CHECK(eng.activeVoiceCount() == 1); // 67 was still ringing too + + eng.noteOff(62); + std::fill(l.begin(), l.end(), 0.0f); std::fill(r.begin(), r.end(), 0.0f); + eng.render(l.data(), r.data(), l.size()); + CHECK(eng.activeVoiceCount() == 0); // the stolen-into 4th note releases last +} + // PREVIEW-CARD ISOLATION: the card never consumes a pool voice, a FULL pool never drops a // preview, and pool stealing never touches the ringing preview. The two sum independently. static void testPreviewCardIsolatedFromPool() { @@ -2034,6 +2259,12 @@ int main() { testMonoLegatoSameNoteRepressReattacks(); testMonoOutOfRangeNotesRejected(); testVoiceCountBoundsPolyphony(); + testOverCapChordStealsExactlyOne(); + testMonoRetrigTakeoverDeclicksRestart(); + testMonoRetrigFallbackDeclicksRestart(); + testMonoDeclickOnlyOnTakeover(); + testPolyStealDeclicksRestart(); + testSameBlockDoubleTakeoverKeepsDeclickSeed(); testPreviewCardIsolatedFromPool(); testPreviewCardReplaceStaleOffAndOutOfZone(); From 39ea6ce7db514853ee2bcba00f31b17a634a64b8 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 06:59:05 -0400 Subject: [PATCH 2/2] fix(sampler_core): gate takeover declick by (1-amp) to remove zero-attack +6 dB blip and long-attack notch; clamp seed to +/-1; add zero-attack test --- src/vst/sampler_core.cpp | 17 +++++++++++++---- tests/test_sampler_core.cpp | 30 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/vst/sampler_core.cpp b/src/vst/sampler_core.cpp index a2cd775..c0d4eed 100644 --- a/src/vst/sampler_core.cpp +++ b/src/vst/sampler_core.cpp @@ -272,8 +272,10 @@ void Voice::start(int note, int velocity, const SampleData& sample, int rootNote // the same pre-cut output level — zeroing would drop the pending ramp and bring the // click back on that edge. The next rendered frame overwrites lastOut anyway. if (declickTakeover && active_) { - declickL_ = lastOutL_; - declickR_ = lastOutR_; + // Clamp the seed to ±1.0: closes the theoretical same-frame-repeat accumulation edge + // (successive starts before any frame is rendered cannot grow the seed above full scale). + declickL_ = (lastOutL_ > 1.0) ? 1.0 : (lastOutL_ < -1.0) ? -1.0 : lastOutL_; + declickR_ = (lastOutR_ > 1.0) ? 1.0 : (lastOutR_ < -1.0) ? -1.0 : lastOutR_; declickActive_ = (declickL_ > kDeclickFloor || declickL_ < -kDeclickFloor || declickR_ > kDeclickFloor || declickR_ < -kDeclickFloor); } else { @@ -530,9 +532,16 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { // takeover/steal start() so the restart's hard cut has no step. Engine-agnostic — applied // after either pitch-engine branch, on the shared epilogue. Inactive (the common case) // costs one branch. + // + // Gate by (1 - amp): the compensation fills only the HOLE the new attack leaves. When amp + // is near 0 (slow attack, typical 3 ms) the gate is ~1 — full compensation, no change in + // feel. When amp is 1 (zero-attack, instant sustain) the gate is 0 — no compensation added, + // so there is no +6 dB blip. For long attacks the gate tapers the compensation proportionally, + // removing the notch that arose when both the old tail and the new level were present in full. if (declickActive_) { - outL += declickL_; - if (stereo) outRlocal += declickR_; + const double gate = 1.0 - amp; + outL += declickL_ * gate; + if (stereo) outRlocal += declickR_ * gate; declickL_ *= kDeclickDecay; declickR_ *= kDeclickDecay; if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index bcb9fd3..44ce9a3 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -2063,6 +2063,35 @@ static void testSameBlockDoubleTakeoverKeepsDeclickSeed() { CHECK(approx(post.back(), 1.0, 1e-3)); } +// Declick with ZERO-ATTACK takeover: the new voice reaches full level on frame 0 (amp == 1), +// so the gated compensation adds nothing (gate = 1 - 1 = 0). Output on the boundary frame is +// exactly the new voice's level, never exceeding full scale. Without the (1-amp) gate a zero- +// attack takeover from a sustained voice produced newOnset + oldLevel -> up to 2x (+6 dB). +static void testZeroAttackTakeoverNeverExceedsFullScale() { + SampleData s = dcSample(200000, 60); + s.play.adsr.attackFrames = 0; // zero-attack: amp == 1 on the very first frame + s.play.adsr.sustainLevel = 1.0; + s.play.adsr.releaseFrames = 0; + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Mono, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + eng.noteOn(60, 127); + std::vector pre; + eng.render(pre, 200); // sustained at 1.0 + CHECK(approx(pre.back(), 1.0, 1e-6)); + + eng.noteOn(64, 127); // zero-attack takeover: amp hits 1 on frame 0 + std::vector post; + eng.render(post, 400); + // Every output frame must stay within [-1, 1]: no +6 dB blip. + for (AudioSample v : post) { + CHECK(v <= 1.0f + 1e-4f && v >= -1.0f - 1e-4f); + } + // The zero-attack note settles at sustain 1.0 immediately. + CHECK(approx(post[0], 1.0, 1e-4)); +} + // GA-VoiceSteal repro (DAW bug): voiceCount 3, a triad note-on'd at the SAME sample time // (three note-ons in one block, no render between), then a 4th note. The steal must take // EXACTLY ONE voice (the oldest, none releasing) and leave the other two RINGING — the DAW @@ -2265,6 +2294,7 @@ int main() { testMonoDeclickOnlyOnTakeover(); testPolyStealDeclicksRestart(); testSameBlockDoubleTakeoverKeepsDeclickSeed(); + testZeroAttackTakeoverNeverExceedsFullScale(); testPreviewCardIsolatedFromPool(); testPreviewCardReplaceStaleOffAndOutOfZone();