From 5d3289f0bc45e046cf2b19559f08b3c225f89847 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 08:49:07 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(vst):=20declick=20rev=202=20=E2=80=94?= =?UTF-8?q?=20difference-seeded=20ungated=20takeover=20ramp=20(rev-1=20(1-?= =?UTF-8?q?amp)=20gate=20zeroed=20it=20on=20Trigger/zero-attack);=20previe?= =?UTF-8?q?w=20card=20opt-in?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/vst/reasampler_processor.h | 12 ++-- src/vst/sampler_core.cpp | 122 +++++++++++++++++++++++---------- src/vst/sampler_core.h | 74 ++++++++++++++------ tests/test_sampler_core.cpp | 122 +++++++++++++++++++++++++++++++-- 4 files changed, 262 insertions(+), 68 deletions(-) diff --git a/src/vst/reasampler_processor.h b/src/vst/reasampler_processor.h index 09deb49..fe4dd98 100644 --- a/src/vst/reasampler_processor.h +++ b/src/vst/reasampler_processor.h @@ -52,11 +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. + // The takeover declick (GA fix, rev 2) 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, AND the preview card's replace-restart) smooths the cut + // via the difference-seeded ramp 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, @@ -65,7 +65,7 @@ struct LoadedInstrument { : keymap(std::move(km)), engine(maxVoices, keymap, preserveVoiceCap, preserveWindowFrames, voiceMode, monoTrigger, /*takeoverDeclick=*/true), - preview(keymap, preserveWindowFrames), + preview(keymap, preserveWindowFrames, /*takeoverDeclick=*/true), installedAt(gen) {} // True when nothing in this snapshot is sounding — engine voices AND the preview card. diff --git a/src/vst/sampler_core.cpp b/src/vst/sampler_core.cpp index 1132f39..9db8ac9 100644 --- a/src/vst/sampler_core.cpp +++ b/src/vst/sampler_core.cpp @@ -262,27 +262,33 @@ 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 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. + // Takeover declick (Phase S GA fix, rev 2): BEFORE any state reset, record the PRE-CUT + // REFERENCE — the last rendered output — and mark the compensation PENDING iff this + // start is a takeover/steal of a SOUNDING voice and the caller opted in. The ramp itself + // is seeded on the FIRST frame rendered after the restart, from the DIFFERENCE between + // this reference and the new voice's raw output that frame (seedDeclick), so the + // boundary frame reproduces the old level EXACTLY — whatever the new envelope does + // (Gate attack, zero attack, Trigger's no-fade-in instant-unity onset) and whatever + // value the new sample starts on. [Rev 1 seeded the OLD value here and gated the add by + // (1 − newAmp) in the epilogue: every restart whose new amplitude was instantly ~1 got + // ZERO compensation and kept the full click — exactly the DAW-reported mono-retrig case + // on Trigger / zero-attack zones.] A fresh start (idle voice) 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 record the + // same pre-cut reference, not a phantom 0. The next rendered frame overwrites lastOut. if (declickTakeover && active_) { - // 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); + // Clamp the reference to ±1.0 full scale: a bounded seed whatever the voice was doing. + declickRefL_ = (lastOutL_ > 1.0) ? 1.0 : (lastOutL_ < -1.0) ? -1.0 : lastOutL_; + declickRefR_ = (lastOutR_ > 1.0) ? 1.0 : (lastOutR_ < -1.0) ? -1.0 : lastOutR_; + declickPending_ = true; } else { - declickL_ = 0.0; - declickR_ = 0.0; - declickActive_ = false; + declickPending_ = false; } + // Any in-flight ramp is superseded: pending re-derives from the reference, which already + // includes the running declick's contribution via lastOut (it tracks post-declick output). + declickActive_ = false; + declickL_ = 0.0; + declickR_ = 0.0; active_ = true; releasing_ = false; @@ -417,6 +423,22 @@ double Voice::tickAmplitude() { return amp; } +void Voice::seedDeclick(double newOutL, double newOutR) { + // First frame after a takeover restart: the compensation is the ACTUAL discontinuity — + // (pre-cut reference − the new voice's raw output this frame). Added ungated in the + // epilogue, so this frame's output is newOut + (ref − newOut) == ref: zero step at the + // boundary by construction. Clamped to ±2.0 (both operands are bounded near full scale). + declickPending_ = false; + double dl = declickRefL_ - newOutL; + double dr = declickRefR_ - newOutR; + dl = (dl > 2.0) ? 2.0 : (dl < -2.0) ? -2.0 : dl; + dr = (dr > 2.0) ? 2.0 : (dr < -2.0) ? -2.0 : dr; + declickL_ = dl; + declickR_ = dr; + declickActive_ = (dl > kDeclickFloor || dl < -kDeclickFloor || + dr > kDeclickFloor || dr < -kDeclickFloor); +} + AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { // Shared read/advance for the mono and stereo paths. The read-head geometry (loop wrap, // bracketing indices, interpolation partner) is computed ONCE and applied identically to @@ -453,8 +475,29 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { // trigger envelope also finishes at the same frame count; either latches the voice idle. const bool triggerRanOff = playMode_ == PlayMode::Trigger && readPos_ >= static_cast(playEnd_); - // Ran off the sample end with no usable loop -> voice is done. + // Ran off the sample end with no usable loop -> voice is done. Peer path of the + // epilogue: an in-flight takeover declick RINGS OUT here instead of hard-cutting — + // dropping it would re-introduce a step on exactly the path the ramp exists for (a + // restart whose new play span ends within the ~4 ms ramp). The voice stays active only + // until the ramp floors; with no declick (the common case, and the entire opt-out + // baseline) this is byte-identical to the plain idle-out. if (triggerRanOff || readPos_ >= static_cast(frameCount)) { + if (declickPending_) seedDeclick(0.0, 0.0); // the new output here is silence + if (declickActive_) { + const double l = declickL_; + const double r = declickR_; + declickL_ *= kDeclickDecay; + declickR_ *= kDeclickDecay; + if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && + declickR_ < kDeclickFloor && declickR_ > -kDeclickFloor) { + declickActive_ = false; + active_ = false; + } + lastOutL_ = l; + lastOutR_ = stereo ? r : l; + if (stereo) outR = static_cast(r); + return static_cast(l); + } active_ = false; if (stereo) outR = 0.0f; return 0.0f; @@ -528,20 +571,20 @@ 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. - // - // 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. + // Takeover declick (Phase S GA fix, rev 2): on the FIRST frame after a takeover/steal + // restart, seed the compensation from the ACTUAL discontinuity — the pre-cut output + // minus this frame's raw new output — then add the decaying compensation UNGATED. The + // boundary frame therefore carries the old level exactly (out + (ref − out) == ref) and + // every later frame moves by the new signal's own slope plus a ≤5%-of-seed decay step. + // Engine- and play-mode-agnostic: rev 1's (1 − amp) gate zeroed the ramp whenever the + // new envelope opened instantly at ~1 (Trigger's no-fade-in onset, zero-attack Gate) — + // exactly where the DAW still clicked. Because the seed is a DIFFERENCE, matching old + // and new levels seed ~0 and add nothing — no +6 dB blip without any gate. Inactive + // (the common case) costs one branch. + if (declickPending_) seedDeclick(outL, stereo ? outRlocal : outL); if (declickActive_) { - const double gate = 1.0 - amp; - outL += declickL_ * gate; - if (stereo) outRlocal += declickR_ * gate; + outL += declickL_; + if (stereo) outRlocal += declickR_; declickL_ *= kDeclickDecay; declickR_ *= kDeclickDecay; if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && @@ -561,7 +604,10 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { readPos_ += ratio_; - if (amplitudeDone_) { + // A finished amplitude envelope frees the voice — unless a takeover declick still rings: + // the envelope contributes 0 from here on, so the remaining frames are the bare ramp + // fading out (bounded: the ramp floors within ~4 ms). Baseline (no declick) unchanged. + if (amplitudeDone_ && !declickActive_) { active_ = false; } return static_cast(outL); @@ -859,8 +905,9 @@ std::size_t VoiceEngine::activeVoiceCount() const { // PreviewCard (Phase S) — the isolated preview voice. See the header contract. // --------------------------------------------------------------------------- -PreviewCard::PreviewCard(const Keymap& keymap, std::int64_t preserveWindowFrames) - : keymap_(keymap) { +PreviewCard::PreviewCard(const Keymap& keymap, std::int64_t preserveWindowFrames, + bool takeoverDeclick) + : keymap_(keymap), takeoverDeclick_(takeoverDeclick) { // Construction is off the audio thread (the shell builds the card inside its // LoadedInstrument) — the one allocation point for the preview voice's shifter rings, // mirroring VoiceEngine's constructor. <= 1 leaves them pass-through. @@ -877,8 +924,11 @@ void PreviewCard::noteOn(int note, int velocity) { // bypass is OPTED IN here (and only here) — the preview fires at the effective root, so // this is its zero-added-latency path (the FA1 fix, re-scoped off the MIDI engine). No // Preserve cap and no stealing interplay: the card is structurally outside the pool. + // The takeover declick (GA fix rev 2) rides the REPLACE-restart: start() self-gates on + // the voice being active, so a fresh preview (idle voice) never ramps — only the hard + // cut of a still-ringing preview, the same physics as the mono retrig takeover. voice_.start(note, velocity, sample, zone.rootNote, zone.keyTrack, zone.velocityCurve, - /*unityVarispeedBypass=*/true); + /*unityVarispeedBypass=*/true, /*declickTakeover=*/takeoverDeclick_); } void PreviewCard::noteOff(int note) { diff --git a/src/vst/sampler_core.h b/src/vst/sampler_core.h index 722f03f..79dbea8 100644 --- a/src/vst/sampler_core.h +++ b/src/vst/sampler_core.h @@ -381,19 +381,28 @@ 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. +// Takeover declick (Phase S GA fix, rev 2 — 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, the POLY at-cap voice steal, AND the preview card's +// replace-restart. When the caller opts in (start()'s declickTakeover; the engine and the +// preview card pass it on all of those restart paths when constructed with takeoverDeclick), +// the restart smooths the ACTUAL output discontinuity: start() records the last rendered +// output as the pre-cut reference, and the FIRST frame rendered after the restart seeds a +// compensation equal to (reference − that frame's raw new output). The compensation is +// summed into the output UNGATED and decays by kDeclickDecay per frame, so the boundary +// frame reproduces the old level EXACTLY — zero step whatever the new envelope's first +// value (Gate attack, zero attack, or Trigger's no-fade-in instant-unity onset) and +// whatever value the new sample starts on — and the residue fades in ~2-4 ms to the -80 dB +// floor across 44.1-96 kHz (a per-FRAME DSP micro-ramp, not a stored wall-clock quantity). +// [Rev 1 decayed the OLD output gated by (1 − newAmp): any restart whose new amplitude was +// instantly ~1 — a Trigger zone with no fade-in, a zero-attack Gate — got ZERO compensation +// and kept the full click. The difference seed has no such hole and needs no gate: when old +// and new levels already match, the seed is ~0 and nothing is added, so the +6 dB sum the +// gate defended against is structurally impossible.] OFF by default so the bare core stays +// byte-identical to the pre-fix engine (the regression baseline); the processor shell opts +// in for both the engine and the preview card, 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) @@ -428,8 +437,10 @@ public: // 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. + // takeover/steal restart, not a fresh start), smooth the restart's output discontinuity — + // the pre-cut output is recorded here and the difference-seeded compensation is armed on + // the first frame rendered after the restart (see the takeover-declick block above + // kDeclickDecay). 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(), @@ -538,13 +549,24 @@ 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. + // Seeds the takeover compensation on the FIRST frame after a restart: the ramp is the + // ACTUAL discontinuity — (pre-cut reference − the new voice's raw output this frame) — + // applied ungated so the boundary frame reproduces the old level exactly. See the + // takeover-declick block above kDeclickDecay. + void seedDeclick(double newOutL, double newOutR); + + // Takeover declick state (see kDeclickDecay above). lastOut{L,R}_ track the voice's most + // recent rendered output (post-gain, incl. any running declick). A takeover/steal start() + // records them as declickRef{L,R}_ (the pre-cut reference) and sets declickPending_; the + // first frame rendered after the restart calls seedDeclick to derive declick{L,R}_ from + // (reference − that frame's raw new output), and declickActive_ then gates the per-frame + // ungated add + decay. lastOut is NOT zeroed by start() — a second same-block takeover + // (no frame rendered between) must record the same pre-cut reference, not a phantom 0. + // The whole declick state is cleared on a fresh (non-takeover) start. + bool declickPending_ = false; bool declickActive_ = false; + double declickRefL_ = 0.0; + double declickRefR_ = 0.0; double declickL_ = 0.0; double declickR_ = 0.0; double lastOutL_ = 0.0; @@ -724,7 +746,14 @@ public: // the shell's LoadedInstrument next to the Keymap they read). `preserveWindowFrames` // pre-sizes the voice's Preserve shifters off-thread (0/1 = pass-through), mirroring the // engine's constructor, so an off-root Preserve preview never allocates at note-on. - explicit PreviewCard(const Keymap& keymap, std::int64_t preserveWindowFrames = 0); + // `takeoverDeclick` (Phase S GA fix, rev 2) mirrors VoiceEngine's flag: a preview fired + // over a RINGING preview REPLACES the single voice — the same hard cut as a mono + // takeover — and with the opt-in that replace-restart runs the same difference-seeded + // declick ramp (start() self-gates on the voice being active, so a fresh preview never + // ramps). Default FALSE keeps the bare core byte-identical (regression baseline); the + // processor shell opts in. + explicit PreviewCard(const Keymap& keymap, std::int64_t preserveWindowFrames = 0, + bool takeoverDeclick = false); // Fire the preview note (RT-safe: no allocation). A new preview replaces the ringing one // (single voice — the card is one finger, not a pool). Out-of-zone is a defined no-play. @@ -749,6 +778,7 @@ public: private: Voice voice_; const Keymap& keymap_; + bool takeoverDeclick_ = false; // GA fix rev 2: declick the replace-restart of a ringing preview }; } // namespace reasampler diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index 070c681..d4b2085 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -2092,10 +2092,12 @@ 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). +// Declick with a ZERO-ATTACK takeover onto the SAME DC level: the difference seed is +// (old level − new first raw output) = (1.0 − 1.0) = 0, so NOTHING is added — output stays +// exactly full scale, never above it. (This is the case the retired rev-1 (1 − amp) gate +// existed for: an ADDITIVE old-level ramp under an instant-unity attack summed to +6 dB. +// The difference seed makes the blip structurally impossible without any gate — and without +// the gate's fatal hole that kept the click on every instant-unity restart.) static void testZeroAttackTakeoverNeverExceedsFullScale() { SampleData s = dcSample(200000, 60); s.play.adsr.attackFrames = 0; // zero-attack: amp == 1 on the very first frame @@ -2121,6 +2123,114 @@ static void testZeroAttackTakeoverNeverExceedsFullScale() { CHECK(approx(post[0], 1.0, 1e-4)); } +// Shared discontinuity probe for the GA2 click tests: max sample-to-sample delta from the +// last pre-restart frame across the whole post-restart span. A hard cut shows up as a +// click-sized step (~ the old instantaneous level); a properly declicked restart moves by +// the signal's own slope plus the ≤5%-of-seed decay step per frame. +static double maxDeltaAcross(double lastPre, const std::vector& post) { + double prev = lastPre; + 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); + } + return maxDelta; +} + +// GA2 — the click that SURVIVED the rev-1 declick (DAW report: "mono retrigger STILL +// CLICKS"): a mono Retrigger takeover of a TRIGGER zone. Trigger with no fade-in is at FULL +// amplitude on frame 0, so the rev-1 compensation — gated by (1 − amp) — was zeroed exactly +// here and the restart still hard-cut from the old instantaneous level (~1.0 at the sine +// peak) to the new onset's 0. The difference-seeded declick reproduces the old level on the +// boundary frame and bounds every later delta. A sine (not DC) so the test sees the real +// waveform-value jump the DC-sample rev-1 tests masked. +static void testMonoRetrigTriggerZoneDeclicksRestart() { + SampleData s = sineSample(48000, 100.0, 60); // period 480 frames; slope <= ~0.013/frame + s.play.playMode = PlayMode::Trigger; // default fades: NO fade-in -> amp 1 at frame 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, 120); // quarter period: ringing at ~ the sine peak + CHECK(pre.back() > 0.99f); // the cut level is large — a real click pre-fix + + eng.noteOn(60, 127); // hammer the same key: Retrigger takeover + std::vector post; + eng.render(post, 400); + // Boundary continuity: the first post-restart frame reproduces the old level (pre-fix it + // stepped to the new onset's sin(0) == 0 — a full-scale discontinuity). + CHECK(std::fabs(static_cast(post[0]) - static_cast(pre.back())) < 0.01); + // Bounded slope across the whole restart: decay step (<= 0.05 of the seed) + sine slope. + CHECK(maxDeltaAcross(static_cast(pre.back()), post) < 0.08); +} + +// GA2 peer: the same gate hole on a GATE zone with ZERO attack (amp == 1 on frame 0 — the +// default AdsrParams, and any user-dialed instant attack). Rev-1's (1 − amp) gate zeroed the +// compensation here too; the difference seed closes it identically. +static void testZeroAttackGateRetrigNoStep() { + SampleData s = sineSample(48000, 100.0, 60); + s.play.adsr.attackFrames = 0; // instant-unity attack + 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, 120); // ringing at ~ the sine peak + CHECK(pre.back() > 0.99f); + + eng.noteOn(60, 127); // zero-attack Retrigger takeover + std::vector post; + eng.render(post, 400); + CHECK(std::fabs(static_cast(post[0]) - static_cast(pre.back())) < 0.01); + CHECK(maxDeltaAcross(static_cast(pre.back()), post) < 0.08); +} + +// GA2 — the PREVIEW click: a preview fired over a RINGING preview replaces the card's single +// voice — the same hard cut as a mono takeover, previously entirely un-declicked (the card +// never passed the opt-in). With takeoverDeclick opted in at construction, the +// replace-restart runs the same difference-seeded ramp: boundary continuity + bounded slope. +static void testPreviewRetriggerDeclicksRestart() { + SampleData s = sineSample(48000, 100.0, 60); // default ADSR: instant unity (worst case) + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + PreviewCard card(km, 0, /*takeoverDeclick=*/true); + + card.noteOn(60, 127); + std::vector pre(120, 0.0f); + card.render(pre.data(), pre.size()); // ringing at ~ the sine peak + CHECK(pre.back() > 0.99f); + + card.noteOn(60, 127); // audition again: replaces the ringing preview + std::vector post(400, 0.0f); + card.render(post.data(), post.size()); + CHECK(std::fabs(static_cast(post[0]) - static_cast(pre.back())) < 0.01); + CHECK(maxDeltaAcross(static_cast(pre.back()), post) < 0.08); +} + +// The preview declick is OPT-IN: a default-constructed card keeps the pre-fix hard cut +// byte-identical — the replace-restart's first frame is the new voice's raw onset (sin(0) +// == 0 here), pinning the pure-core regression baseline the shell layers the opt-in above. +static void testPreviewDefaultOffKeepsHardCutBaseline() { + SampleData s = sineSample(48000, 100.0, 60); + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + PreviewCard card(km); // declick NOT opted in + + card.noteOn(60, 127); + std::vector pre(120, 0.0f); + card.render(pre.data(), pre.size()); + CHECK(pre.back() > 0.99f); + + card.noteOn(60, 127); + std::vector post(1, 0.0f); + card.render(post.data(), post.size()); + CHECK(approx(post[0], 0.0, 1e-4)); // the raw hard cut: new onset, no ramp +} + // 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 @@ -2325,6 +2435,10 @@ int main() { testPolyStealDeclicksRestart(); testSameBlockDoubleTakeoverKeepsDeclickSeed(); testZeroAttackTakeoverNeverExceedsFullScale(); + testMonoRetrigTriggerZoneDeclicksRestart(); + testZeroAttackGateRetrigNoStep(); + testPreviewRetriggerDeclicksRestart(); + testPreviewDefaultOffKeepsHardCutBaseline(); testPreviewCardIsolatedFromPool(); testPreviewCardReplaceStaleOffAndOutOfZone(); From 3e9f5b3b4806f209871e787c54163307cea7ec15 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Tue, 28 Jul 2026 09:10:04 -0400 Subject: [PATCH 2/2] fix(sampler_core): bounded-blend declick (no overshoot), soundingNote() gates cap + legato Replaces frozen-difference declick with bounded blend (out*(1-w)+ref*w, bounded by max(|ref|,|out|)). soundingNote() gates activePreserveVoices and mono-Legato retune away from ramp-only past-end voices. Magnitude-bound test asserts |v|<=1 across the ramp. --- src/vst/sampler_core.cpp | 85 +++++++++++++++++++++++-------------- src/vst/sampler_core.h | 29 +++++++++---- tests/test_sampler_core.cpp | 45 ++++++++++++++++++++ 3 files changed, 118 insertions(+), 41 deletions(-) diff --git a/src/vst/sampler_core.cpp b/src/vst/sampler_core.cpp index 9db8ac9..83cd1ac 100644 --- a/src/vst/sampler_core.cpp +++ b/src/vst/sampler_core.cpp @@ -424,19 +424,27 @@ double Voice::tickAmplitude() { } void Voice::seedDeclick(double newOutL, double newOutR) { - // First frame after a takeover restart: the compensation is the ACTUAL discontinuity — - // (pre-cut reference − the new voice's raw output this frame). Added ungated in the - // epilogue, so this frame's output is newOut + (ref − newOut) == ref: zero step at the - // boundary by construction. Clamped to ±2.0 (both operands are bounded near full scale). + // First frame after a takeover restart: ARM the bounded blend. The weight starts at 1.0 + // so this frame's output is `out*(1-1) + ref*1 == ref` — exact boundary identity whatever + // the new envelope's first value. Each subsequent frame adds `w*(ref − outCurrent)` then + // decays w, so output is provably bounded by max(|ref|, |outCurrent|) — mid-ramp overshoot + // is impossible even if outCurrent rises while the weight is still significant. + // [Rev 1 stored the frozen difference (ref − x₀); if outₙ rose while that residue was + // still large the sum could exceed full scale. The ±2.0 clamp there was the only guard + // and it silently broke the boundary identity when |x₀| > 1. The bounded blend removes + // both the overshoot hole and the need for a clamp on the stored value.] + // newOutL/R are used only to decide whether an active ramp exists (the seed is purely + // the weight 1.0; ref was clamped to ±1 at start()). The ±2 clamp on the difference is + // gone: the blend formula keeps every output within max(|ref|,|outₙ|) by construction. + (void)newOutL; (void)newOutR; // consumed only for the floor guard below declickPending_ = false; - double dl = declickRefL_ - newOutL; - double dr = declickRefR_ - newOutR; - dl = (dl > 2.0) ? 2.0 : (dl < -2.0) ? -2.0 : dl; - dr = (dr > 2.0) ? 2.0 : (dr < -2.0) ? -2.0 : dr; - declickL_ = dl; - declickR_ = dr; - declickActive_ = (dl > kDeclickFloor || dl < -kDeclickFloor || - dr > kDeclickFloor || dr < -kDeclickFloor); + declickL_ = 1.0; + declickR_ = 1.0; + // The reference is already clamped to ±1.0 at start() (lines in start(): the ±1 clamp + // on lastOutL_/R_ before storing into declickRefL_/R_). No secondary clamp needed here. + // Activate only when the ref itself is above the floor — if ref ≈ 0 there is nothing to blend. + declickActive_ = (declickRefL_ > kDeclickFloor || declickRefL_ < -kDeclickFloor || + declickRefR_ > kDeclickFloor || declickRefR_ < -kDeclickFloor); } AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { @@ -484,12 +492,13 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { if (triggerRanOff || readPos_ >= static_cast(frameCount)) { if (declickPending_) seedDeclick(0.0, 0.0); // the new output here is silence if (declickActive_) { - const double l = declickL_; - const double r = declickR_; + // Bounded blend at silence: outCurrent == 0, so the blend is w*(ref − 0) == w*ref. + // The weight decays by kDeclickDecay each frame, floor-checked on the weight itself. + const double l = declickL_ * declickRefL_; + const double r = declickL_ * declickRefR_; // same weight for both channels declickL_ *= kDeclickDecay; declickR_ *= kDeclickDecay; - if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && - declickR_ < kDeclickFloor && declickR_ > -kDeclickFloor) { + if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor) { declickActive_ = false; active_ = false; } @@ -571,24 +580,24 @@ AudioSample Voice::advanceFrame(bool stereo, AudioSample& outR) { ratio_ = baseRatio_ * envFactor; } - // Takeover declick (Phase S GA fix, rev 2): on the FIRST frame after a takeover/steal - // restart, seed the compensation from the ACTUAL discontinuity — the pre-cut output - // minus this frame's raw new output — then add the decaying compensation UNGATED. The - // boundary frame therefore carries the old level exactly (out + (ref − out) == ref) and - // every later frame moves by the new signal's own slope plus a ≤5%-of-seed decay step. - // Engine- and play-mode-agnostic: rev 1's (1 − amp) gate zeroed the ramp whenever the - // new envelope opened instantly at ~1 (Trigger's no-fade-in onset, zero-attack Gate) — - // exactly where the DAW still clicked. Because the seed is a DIFFERENCE, matching old - // and new levels seed ~0 and add nothing — no +6 dB blip without any gate. Inactive - // (the common case) costs one branch. + // Takeover declick (Phase S GA fix, rev 2, bounded-blend revision): on the FIRST frame + // after a takeover/steal restart, seed the blend weight at 1.0 so this frame's output is + // outₙ*(1−w) + ref*w = out*(1−1) + ref*1 = ref (exact boundary identity). + // Each subsequent frame the blend add is `w*(ref − outCurrent)` and then w decays by + // kDeclickDecay. The output is therefore bounded by max(|ref|, |outCurrent|) in every + // frame — mid-ramp overshoot from a rising outCurrent is structurally impossible. + // [Rev 1 added the frozen difference (ref − x₀) ungated; if outₙ rose while the residue + // was still large the sum could exceed ±1 by up to ~+3.8 dB on an extreme retrig.] + // Inactive (the common case) costs one branch; the blend itself costs one extra subtract. if (declickPending_) seedDeclick(outL, stereo ? outRlocal : outL); if (declickActive_) { - outL += declickL_; - if (stereo) outRlocal += declickR_; + const double addL = declickL_ * (declickRefL_ - outL); + const double addR = declickL_ * (declickRefR_ - (stereo ? outRlocal : outL)); + outL += addL; + if (stereo) outRlocal += addR; declickL_ *= kDeclickDecay; - declickR_ *= kDeclickDecay; - if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor && - declickR_ < kDeclickFloor && declickR_ > -kDeclickFloor) { + declickR_ *= kDeclickDecay; // kept in sync (mirrors L — both channels share one weight) + if (declickL_ < kDeclickFloor && declickL_ > -kDeclickFloor) { declickActive_ = false; } } @@ -655,9 +664,13 @@ VoiceEngine::VoiceEngine(std::size_t maxVoices, const Keymap& keymap, } std::size_t VoiceEngine::activePreserveVoices() const { + // Count only voices that are SOUNDING A NOTE (playable span still running), not voices + // that have finished their note but are still ringing out a declick tail. A ramp-only + // past-end voice must not consume a cap slot — that would cause a new Preserve note-on to + // be dropped (kNoVoice return at :797-800) during the narrow ~4 ms window the ramp lives. std::size_t n = 0; for (const Voice& v : voices_) { - if (v.active() && v.pitchEngine() == PitchEngine::Preserve) ++n; + if (v.soundingNote() && v.pitchEngine() == PitchEngine::Preserve) ++n; } return n; } @@ -729,7 +742,13 @@ std::size_t VoiceEngine::monoNoteOn(int note, int velocity) { // re-attacked. NOTE: a one-held-note same-note re-press (heldCount_ becomes 1 after the // removeHeld/re-push above — so heldCount_ < 2) re-attacks rather than retuning, which is // the correct fresh-phrase behavior for that edge case.) Same-sample requirement unchanged. - if (v.active() && heldCount_ >= 2 && monoTrigger_ == MonoTrigger::Legato && + // + // soundingNote() (not just active()): a voice whose note has run to its play-end but is + // still ringing a declick tail must NOT be retuned — that would move the pitch of a dying + // ramp rather than restarting the new note, producing a silent note on the common + // "hammer same key while a past-end ring-out is active" path. The tail should keep fading; + // the new note-on restarts the voice normally (monoNoteOn falls through to start() below). + if (v.soundingNote() && heldCount_ >= 2 && monoTrigger_ == MonoTrigger::Legato && v.playingSample() == &sample) { v.retune(note, zone.rootNote, zone.keyTrack); return 0; diff --git a/src/vst/sampler_core.h b/src/vst/sampler_core.h index 79dbea8..82874c7 100644 --- a/src/vst/sampler_core.h +++ b/src/vst/sampler_core.h @@ -464,8 +464,16 @@ public: // instantly (which release() cannot do). RT-safe: no allocation, no lock. void hardStop(); - // True while this voice is producing (or about to produce) sound. + // True while this voice is producing (or about to produce) sound (including any + // declick ring-out tail past the note's playable span). bool active() const { return active_; } + // True while this voice is sounding a PLAYABLE NOTE — active AND the amplitude + // envelope has not yet finished. A voice whose note has run to its end but is still + // ringing out a declick tail is active() but NOT soundingNote(). Use this to + // distinguish "note is alive" (active) from "note occupies a voice slot" (soundingNote) + // for the Preserve-cap count and the mono-Legato takeover predicate — both must ignore + // a ramp-only past-end voice or a new note-on can be dropped / silently muted. + bool soundingNote() const { return active_ && !amplitudeDone_; } // The note this voice was started on (for note-off routing). Meaningless if idle. int note() const { return note_; } // Monotonic age counter — higher = started earlier relative to others. The voice @@ -557,17 +565,22 @@ private: // Takeover declick state (see kDeclickDecay above). lastOut{L,R}_ track the voice's most // recent rendered output (post-gain, incl. any running declick). A takeover/steal start() - // records them as declickRef{L,R}_ (the pre-cut reference) and sets declickPending_; the - // first frame rendered after the restart calls seedDeclick to derive declick{L,R}_ from - // (reference − that frame's raw new output), and declickActive_ then gates the per-frame - // ungated add + decay. lastOut is NOT zeroed by start() — a second same-block takeover - // (no frame rendered between) must record the same pre-cut reference, not a phantom 0. + // records them as declickRef{L,R}_ (the clamped pre-cut reference) and sets declickPending_; + // the first frame rendered after the restart calls seedDeclick to arm the BOUNDED BLEND: + // outₙ = outₙ*(1−w) + ref*w where w = declickL_/R_ starts at 1.0 and decays by + // kDeclickDecay each frame. This is algebraically `outₙ + w*(ref − outₙ)`, so the + // boundary frame (w=1) is exactly `ref` and every subsequent output is bounded by + // max(|ref|, |outₙ|) — mid-ramp overshoot is impossible regardless of outₙ rising. + // [Rev 1 stored the frozen difference (ref − x₀); when outₙ rose while that residue + // was still large the sum could exceed full scale by up to ~+3.8 dB.] + // lastOut is NOT zeroed by start() — a second same-block takeover (no frame rendered + // between) must record the same pre-cut reference, not a phantom 0. // The whole declick state is cleared on a fresh (non-takeover) start. bool declickPending_ = false; bool declickActive_ = false; - double declickRefL_ = 0.0; + double declickRefL_ = 0.0; // clamped pre-cut reference (bounded blend target) double declickRefR_ = 0.0; - double declickL_ = 0.0; + double declickL_ = 0.0; // blend weight w; 1.0 on seed, decays by kDeclickDecay/frame double declickR_ = 0.0; double lastOutL_ = 0.0; double lastOutR_ = 0.0; diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index d4b2085..4911e59 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -2335,6 +2335,50 @@ static void testPreviewCardReplaceStaleOffAndOutOfZone() { CHECK(!card.active()); } +// GA2 — bounded-blend overshoot regression: mid-ramp output must stay within full scale. +// +// Construction of the worst case (§1 reviewer finding): retrig a sine at a point where the +// pre-cut level is ~1.0 (old ref ≈ 1). The new voice starts at sin(0) == 0, so the OLD +// frozen-seed declick adds (ref − x₀) ≈ 1.0 to the compensation. The new sine has a short +// period (8 frames) so outₙ reaches ~1.0 again within just 2 frames; at that moment the +// frozen seed is still ~0.9 → outₙ + seed ≈ 1.9, roughly +3.8 dB over full scale. +// +// The bounded blend keeps every frame within max(|ref|, |outCurrent|) ≤ 1.0 + tol — this +// test must FAIL against the rev-2 frozen-seed code and PASS with the bounded blend. +static void testDeclickBoundedBlendNoOvershoot() { + // A sine with 8-frame period so it peaks within the declick ramp window (~80 frames). + // 48000 frames, 6000 cycles -> period = 8 frames; quarter period = 2 frames = the peak. + const std::size_t kFrames = 48000; + const double kCycles = 6000.0; // period = 8 frames + SampleData s = sineSample(kFrames, kCycles, 60); + s.play.playMode = PlayMode::Trigger; // no fade-in -> amp 1 on frame 0 (worst case) + Keymap km = Keymap::singleSampleChromatic(std::move(s)); + VoiceEngine eng(1, km, 0, 0, VoiceMode::Mono, MonoTrigger::Retrigger, + /*takeoverDeclick=*/true); + + // Start a voice and render to a quarter period so the sine is near its positive peak. + // Period = 48000/6000 = 8 frames. Frame index 2 = sin(2π*6000*2/48000) = sin(π/2) = 1.0. + // We render 3 frames (indices 0,1,2 are visited: readPos 0→1→2→3) so pre[2] reads + // frame index 2 at the sine peak. + eng.noteOn(60, 127); + std::vector pre; + eng.render(pre, 3); // frame index 2 (read on third render): sin(pi/2) ≈ 1.0 + CHECK(pre.back() > 0.99f); // at peak: ref ≈ 1.0 when we cut + + // Retrigger: hard restart at sin(0) == 0, ref == ~1.0. The frozen-seed approach would + // add ~0.9 to a new output of ~1.0 two frames later → ~1.9. The bounded blend must not. + eng.noteOn(60, 127); + const double tol = 1e-3; + std::vector post; + eng.render(post, 200); // 200 frames covers the full ramp (~80 frames at kDeclickDecay=0.95) + for (std::size_t i = 0; i < post.size(); ++i) { + const double v = static_cast(post[i]); + CHECK(v <= 1.0 + tol && v >= -1.0 - tol); + } + // Boundary identity: first frame must reproduce the pre-cut level (±small tol). + CHECK(std::fabs(static_cast(post[0]) - static_cast(pre.back())) < 0.01); +} + int main() { testChromaticSingleRoot(); testZonedRangesBoundaries(); @@ -2439,6 +2483,7 @@ int main() { testZeroAttackGateRetrigNoStep(); testPreviewRetriggerDeclicksRestart(); testPreviewDefaultOffKeepsHardCutBaseline(); + testDeclickBoundedBlendNoOvershoot(); testPreviewCardIsolatedFromPool(); testPreviewCardReplaceStaleOffAndOutOfZone();