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();