diff --git a/src/core/instrument/engine/time_stretch.h b/src/core/instrument/engine/time_stretch.h index 545ec3d..d13ad5a 100644 --- a/src/core/instrument/engine/time_stretch.h +++ b/src/core/instrument/engine/time_stretch.h @@ -11,13 +11,22 @@ namespace reasampler::instrument::engine { // The playback rates the Preserve DSP is measured over, and therefore the only ones it -// accepts. Two independent reasons they are here and not wider: -// - the ceiling is what bounds a voice's per-output-frame feed loop (kMaxFeedPerFrame source -// frames), which is the RT-safety argument for feeding a variable count at all; -// - the splice search can only align a period it can see. The tap's delay drifts at -// |rate - shift| per frame, so a wide rate over a deep DOWN-shift splices faster than one -// period of the output tone and the correlation stops holding the pitch: measured at rate -// 4.0 with -24 st, the observed period came out 539 frames against 785 wanted. +// accepts. The ceiling also bounds a voice's per-output-frame feed loop (kMaxFeedPerFrame +// source frames) — the RT-safety argument for feeding a variable count at all. +// +// This range NARROWS the splice-cadence failure onto the source fundamental; it does not +// eliminate it. A splice recurs every `window / |rate - shift|` output frames (the tap's +// delay drifts across one window at that per-frame rate); the shifted tone's own period is +// `sourcePeriod / shift` output frames. Whenever the recurrence interval is shorter than +// that period, a splice lands inside a single perceived cycle and the correlation search +// has less than one period to align against. Measured at rate 4.0, shift 0.25 (-24 st): +// interval 2205/3.75 ~= 588 vs period ~4*P ~= 785 frames (P ~= 196) — matches the originally +// observed 539-vs-785 failure. This range's ceiling (2.0, not 4.0) raises the safe floor, it +// does not remove it: at rate 2.0, shift 0.25, interval = 2205/1.75 = 1260 still fails for +// any source period P > 315 frames (~140 Hz at 44.1k) — inside bass/low-vocal material, and +// -24 st is reachable from the Pitch knob alone. (The pre-stretch rate-1.0 engine's floor by +// the same inequality is P > 735, ~60 Hz — what this range raises the floor from, not what +// it removes.) inline constexpr double kStretchRateMin = 0.5; inline constexpr double kStretchRateMax = 2.0; inline constexpr int kMaxFeedPerFrame = 2; // ceil(kStretchRateMax) diff --git a/src/core/instrument/engine/voice.cpp b/src/core/instrument/engine/voice.cpp index f4aee7e..5badbc4 100644 --- a/src/core/instrument/engine/voice.cpp +++ b/src/core/instrument/engine/voice.cpp @@ -327,6 +327,8 @@ void Voice::retune(int note) { // Filter key-tracking follows the pitch: it is a function of the note, so a slide moves it // too. The velocity offset deliberately stays the first note's, matching velocityGain_. if (filterOn_) updateFilterCutoffBase(note); + // stretchRate_ (Preserve's duration control) is untouched here too — it is a note-on latch + // like velocityGain_, not a per-note property to re-resolve on a legato slide. } void Voice::release() { diff --git a/src/core/instrument/engine/voice.h b/src/core/instrument/engine/voice.h index 00e1b15..1349f9d 100644 --- a/src/core/instrument/engine/voice.h +++ b/src/core/instrument/engine/voice.h @@ -192,9 +192,9 @@ private: // This frame's amplitude in [0,1] from the active envelope. Spline: the drawn contour read // at the normalized position (one cached-segment compare per frame). Gate: AHDSR ticks once // per output frame (envelope time is wall-clock, independent of read rate). Trigger: the AHD - // is evaluated at the source offset (readPos - startFrame) so its stages anchor to source - // frames regardless of pitch engine. Sets amplitudeDone_ on finish so advanceFrame frees - // the voice. + // is evaluated at the source offset (readPos - startFrame) — see the `ratio_ = stretchRate_` + // note below for what that means for Preserve's stage-time/rate coupling. Sets + // amplitudeDone_ on finish so advanceFrame frees the voice. double tickAmplitude() { double amp; // playMode_ is Trigger whenever a spline is genuinely reachable (resolvePlay forces it — @@ -454,7 +454,10 @@ private: // 2^((note-root + pitchEnvSemis)/12) — the pitch envelope adds to the shift amount, // never to the read rate. The feed runs one window ahead of readPos_ (the rings were // primed with that window at start()), under the same sustain-loop wrap rule, - // reading integer source frames (nothing to interpolate). + // reading integer source frames into the ring — no RATE-DEPENDENT interpolation + // (unlike Varispeed's readPos_ below). The shifter's own read tap still carries a + // splice's sub-sample `frac` (pitch_shift.cpp), so it interpolates on every read, + // splice or no; that constant fractional delay is not a rate coupling. const bool stereoOut = stereo && haveR && shiftR_.configured(); // The last real source frame is playEnd_-1 for Trigger or frameCount-1 for Gate. // Once the feed reaches that bound the source is exhausted — feeding the held last @@ -528,6 +531,16 @@ private: // Preserve advances the read head at the STRETCH rate — the one duration control. // Everything downstream of it (the loop wrap, the Trigger span, the spline phase) // therefore stays a source-frame fact and scales by construction. + // + // Consequence (§2.4 of instrument-control-surface.md is explicit that staged + // envelopes' stage times are wall-clock and do NOT scale with rate): Trigger's amp + // AHD and filter AHD are both evaluated at sourceOffset() = readPos_ - startFrame_ + // (tickAmplitude/tickFilterCutoff above), which now advances at stretchRate_ instead + // of always 1.0 — so those two envelopes will scale with a future non-unity Rate. + // This is NEW here: Preserve's ratio_ was pinned at 1.0 before this track, so those + // stage times were exact wall-clock. It is latent (nothing publishes a non-unity + // rate yet) and owned by the track that adds the Rate control, not this one — Gate's + // AHDSR (env_.tick(), per-output-frame) and every spline contour are unaffected. ratio_ = stretchRate_; } else { // VARISPEED: pitch and duration coupled. The read rate carries the repitch; the diff --git a/tests/test_pitch_shift.cpp b/tests/test_pitch_shift.cpp index ffd1f17..be8d0bf 100644 --- a/tests/test_pitch_shift.cpp +++ b/tests/test_pitch_shift.cpp @@ -737,6 +737,38 @@ static void testStretchAndShiftComposeSafely() { } } +// The [0.5, 2.0] rate bound (time_stretch.h) narrows the splice-cadence failure onto the +// source fundamental rather than eliminating it. At rate 2.0, shift 0.25 (-24 st) — both +// inside the shipped range — the header's own derivation puts the safe-source floor at a +// period of 315 frames (~140 Hz @ 44.1k): testStretchAndShiftComposeSafely's probe period of +// 196.37 frames (~225 Hz) sits ABOVE that floor, so it passes because of the probe, not +// because of headroom. This probe sits BELOW the floor on purpose, asserting the corner +// rather than assuming it. A failure here is the inequality's PREDICTED outcome, not a +// defect this test exists to chase — report it, don't retune the tolerance to hide it. +static void testStretchCadenceBelowSafeFloorAtRate2ShiftQuarter() { + const std::int64_t w = 2205; + const double rate = 2.0; + const double shift = std::pow(2.0, -24.0 / 12.0); // 0.25 + for (double period : {500.0, 600.0, 700.0}) { + const double f0 = 1.0 / period; + const std::size_t srcLen = 400000; + std::vector src(srcLen); + for (std::size_t i = 0; i < srcLen; ++i) { + src[i] = static_cast(std::sin(2.0 * kPi * f0 * static_cast(i))); + } + const std::size_t outFrames = 60000; + const std::vector out = runStretch(src, w, rate, shift, outFrames, nullptr); + for (double v : out) CHECK(std::isfinite(v)); + const double p = periodIn(out, 20000, 50000); + const double want = period / shift; + const bool ok = approx(p, want, want * 0.12); + std::printf(" [floor probe] period %.0f (rate 2.0, -24 st): observed %.2f want %.2f " + "-> %s\n", period, p, want, ok ? "held" : "FAILED (predicted by the " + "inequality in time_stretch.h)"); + CHECK(ok); + } +} + // The two new entry points on a shifter that was never configured (a Varispeed voice's) — // neither may touch the empty ring. static void testStretchEntryPointsOnPassThrough() { @@ -758,6 +790,7 @@ int main() { testStereoLinkedLagSharedSchedule(); testStretchMovesDurationNotPitch(); testStretchAndShiftComposeSafely(); + testStretchCadenceBelowSafeFloorAtRate2ShiftQuarter(); testStretchEntryPointsOnPassThrough(); if (g_fail == 0) { diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index 8b6c059..d593670 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -20,11 +20,11 @@ #include "../src/core/instrument/engine/voice_engine.h" #include +#include #include #include #include #include -#include #include using namespace reasampler; @@ -2947,11 +2947,13 @@ static void renderVoice(const SampleData& s, int note, double rate, std::int64_t } // --- The null case, asserted against a baseline the SHIPPED engine produced. --- -// The four constants below were captured by running this same function against the -// pre-stretch build (phase-g, before the rate seam existed) and printing the hashes; they are -// therefore a witness that the generalized read path reproduces the shipped Preserve output -// bit for bit at rate 1.0, not a self-consistency check. A change here is a change to what -// every already-saved project sounds like — re-derive the cause before re-baselining. +// The four constants below are a witness against `phase-g`'s tip, commit 0a7778b — the last +// commit before this track's rate seam — not a self-consistency check. To re-derive: check +// out 0a7778b, add this file's stretchProbeSample/hashStream/renderVoice/test body to it, and +// drop the trailing `, rate` argument from renderVoice's `v.start(...)` call (0a7778b's +// Voice::start has no 5th parameter) — then build, run, and print the hashes. A change here is +// a change to what every already-saved project sounds like — re-derive the cause before +// re-baselining. static void testPreserveUnityRateIsBitIdenticalToTheShippedRead() { const std::int64_t w = 2205; // the product window at 44.1k const std::size_t n = 6000; @@ -3084,11 +3086,16 @@ static void testPreserveStretchSpeaksOnFrameZeroAtEveryRate() { // The primed ring parks the tap ON the start frame, so output frame 0 is source // frame `startFrame` exactly — at every rate and every transposition. A stretcher // that buffered a window before speaking would fail here, which is the whole point. + // This bit-exact check is what actually carries "no first-frame smear"; the loop + // below is a coarser, complementary DROPOUT detector (see its own comment). CHECK(first == s.frames[500]); - // ...and it keeps speaking: no first-window dip while the schedule settles. The - // 256-frame measuring window spans most of a period even at the lowest note tested - // (-12 st stretches the probe's 196-frame period to 393), so a continuous tone - // peaks well above the floor in every one of them and only a real gap can sink it. + // ...and it keeps speaking: no first-window DROPOUT while the schedule settles. + // `lo > 0.5` over twenty 256-frame peak windows catches a gap of roughly a window, + // but a smeared or phase-scrambled first window can still peak above 0.5 and pass + // here — it cannot see that; the CHECK above is what does. The 256-frame measuring + // window spans most of a period even at the lowest note tested (-12 st stretches + // the probe's 196-frame period to 393), so a continuous tone peaks well above the + // floor in every one of them and only a real gap can sink it. double lo = 1e9; for (int i = 0; i < 20; ++i) { double peak = 0.0; @@ -3130,14 +3137,84 @@ static void testPreserveStretchLoopsTheSourceSpan() { CHECK(approx(sum / 200.0, 0.5, 0.05)); } } + + // The two assertions above hold even if stretchRate_ were ignored outright — the loop's + // constant content proves nothing about cadence. A one-time marker AFTER the primed window + // but BEFORE the loop start is the source-frame witness that the feed genuinely consumes + // source AT THE RATE: note-on primes the ring with the first `window` source frames up + // front (played back at 1 frame/output-frame, independent of rate — a marker inside that + // span was measured landing at a FIXED output frame at every rate, confirming it is not a + // rate witness). Past it, new content only enters the ring via the ongoing due()-scheduled + // feed, at `rate` source frames per output frame on average: the marker's single output + // appearance lands at `window + (markerFrame - window) / rate` output frames. Note == root + // (shift == 1.0), isolating the rate's effect from the pitch engine's own transposition. + // + // Excludes rate 2.0: at shift 1.0 that is drift = |rate-shift| = 1.0 exactly, and this + // geometry's own splice trigger (0.75x window output frames from note-on, measured) fires + // BEFORE the primed span even finishes playing back (< window frames) whenever drift >= + // ~0.75 — so no marker placed "past the prime" can be reached before a splice relocates + // the tap first. Confirmed by measurement, not assumed: a rate-2.0 attempt at this marker + // came back with the tap having moved on (no witness value in the output at all). The + // write-side consumption-at-the-rate claim at every rate, splice-immune because it never + // goes through the shifter, is what test_time_stretch.cpp's StretchCursor tests assert. + // + // A marker placed INSIDE the steady-state loop instead would NOT show rate-dependence + // either: once ring-resident, the read tap's own pace is governed by SHIFT alone ("shift + // the output" — ratio_ advances posA_ every output frame unconditionally), so it revisits + // every loopLength ring slots at 1 slot/output-frame regardless of how fast the writer + // filled them — confirmed by measurement (median recurrence gap 200 frames at rate 0.5, + // 1.0 AND 2.0 alike, for a 200-frame loop). Rate governs the feed/splice cadence, not the + // loop's own output period, once its content is already in the ring. + for (double rate : {0.5, 1.0}) { + SampleData s; + s.frames.assign(1000, 0.0f); + for (int i = 300; i < 900; ++i) s.frames[i] = 0.5f; + s.frames[650] = 1.0f; // past the 600-frame primed span, before the loop at 700 + s.rootNote = 60; + s.sampleRate = 48000; + s.loop.hasLoop = true; + s.loop.start = 700; + s.loop.end = 900; + s.play.adsr = flatAdsr(); + s.play.pitchEngine = PitchEngine::Preserve; + Voice v; + v.presizePreserveShifters(600); + v.start(60, 127, s, /*declickTakeover=*/false, rate); // root note: shift == 1.0 + const std::size_t total = 3000; + std::vector out(total); + for (std::size_t i = 0; i < total; ++i) out[i] = v.renderFrame(); + std::size_t hitAt = 0; + for (std::size_t i = 0; i < total; ++i) { + if (out[i] > 0.7f) { hitAt = i; break; } + } + CHECK(hitAt > 0); + const double want = 600.0 + (650.0 - 600.0) / rate; + if (!approx(static_cast(hitAt), want, want * 0.15 + 5.0)) { + std::printf(" rate %.2f: marker at frame %zu want %.2f\n", rate, hitAt, want); + } + CHECK(approx(static_cast(hitAt), want, want * 0.15 + 5.0)); + } } // --- The 32-voice measurement gate. Asserts correctness; PRINTS the cost, which is the -// number reported for the algorithm decision (meaningful only in a Release build). --- +// number reported for the algorithm decision (meaningful only in a Release build). +// +// Methodology: std::chrono::steady_clock (not std::clock() — a single wall-clock diff has no +// warm-up and no spread), kWarmupReps discarded, kTimedReps repetitions per rate, median + +// [min, max] reported. secs is wall-clock for 1.0 s of audio on ONE thread with no other work +// scheduled onto it, so 100*secs is % of REALTIME consumed — not "% of one core" (that would +// additionally claim core-pinned exclusivity this benchmark never establishes). +// +// A separate, one-off Release A/B (unity-now vs the pre-stretch build at commit 0a7778b, same +// methodology, standalone harness outside this tree) found the two statistically +// indistinguishable at ~79-82 ns/voice/frame; that is a point-in-time finding to re-derive if +// this path changes materially, not a hardcoded regression bound here. --- static void testPreserveStretchThirtyTwoVoicesHoldUp() { const std::int64_t w = 2205; // the product window at 44.1k const std::size_t blockFrames = 44100; // one second of audio const std::size_t voiceCount = 32; + const int kWarmupReps = 2; + const int kTimedReps = 7; SampleData s = stretchProbeSample(200000, true); s.loop.hasLoop = true; // held notes: all 32 sound for the whole run s.loop.start = 40000; @@ -3147,36 +3224,49 @@ static void testPreserveStretchThirtyTwoVoicesHoldUp() { // 1.0 is the reference: it is the cost the shipped Preserve read already carries, so the // two stretched rows are read as a delta against it rather than in isolation. for (double rate : {1.0, 0.5, 2.0}) { - std::vector voices(voiceCount); - for (std::size_t i = 0; i < voiceCount; ++i) { - voices[i].presizePreserveShifters(w); - voices[i].start(48 + static_cast(i), 100, s, /*declickTakeover=*/false, rate); - } - const std::clock_t t0 = std::clock(); - double guard = 0.0; - std::size_t sounding = 0; - for (std::size_t f = 0; f < blockFrames; ++f) { - AudioSample l = 0.0f, r = 0.0f; + std::vector nsPerVoiceFrame; + nsPerVoiceFrame.reserve(kTimedReps); + for (int rep = 0; rep < kWarmupReps + kTimedReps; ++rep) { + std::vector voices(voiceCount); for (std::size_t i = 0; i < voiceCount; ++i) { - AudioSample a = 0.0f, b = 0.0f; - voices[i].renderFrameStereo(a, b); - l += a; - r += b; + voices[i].presizePreserveShifters(w); + voices[i].start(48 + static_cast(i), 100, s, /*declickTakeover=*/false, + rate); + } + const auto t0 = std::chrono::steady_clock::now(); + double guard = 0.0; + std::size_t sounding = 0; + for (std::size_t f = 0; f < blockFrames; ++f) { + AudioSample l = 0.0f, r = 0.0f; + for (std::size_t i = 0; i < voiceCount; ++i) { + AudioSample a = 0.0f, b = 0.0f; + voices[i].renderFrameStereo(a, b); + l += a; + r += b; + } + guard += static_cast(l) + static_cast(r); + CHECK(std::isfinite(l) && std::isfinite(r)); + } + const double secs = + std::chrono::duration(std::chrono::steady_clock::now() - t0).count(); + for (std::size_t i = 0; i < voiceCount; ++i) { + if (voices[i].active()) ++sounding; + } + CHECK(sounding == voiceCount); // all 32 held the whole second (the loop kept them up) + CHECK(std::fabs(guard) > 0.0); // ...and genuinely produced audio + if (rep >= kWarmupReps) { + nsPerVoiceFrame.push_back(secs * 1e9 / (static_cast(blockFrames) * + static_cast(voiceCount))); } - guard += static_cast(l) + static_cast(r); - CHECK(std::isfinite(l) && std::isfinite(r)); } - const double secs = static_cast(std::clock() - t0) / CLOCKS_PER_SEC; - for (std::size_t i = 0; i < voiceCount; ++i) { - if (voices[i].active()) ++sounding; - } - CHECK(sounding == voiceCount); // all 32 held the whole second (the loop kept them up) - CHECK(std::fabs(guard) > 0.0); // ...and genuinely produced audio - std::printf(" [measure] 32 stereo Preserve voices @ rate %.2f: %.3f s wall for 1.0 s " - "audio (%.1f%% of one core, %.1f ns/voice/frame)\n", - rate, secs, 100.0 * secs, - secs * 1e9 / (static_cast(blockFrames) * - static_cast(voiceCount))); + std::sort(nsPerVoiceFrame.begin(), nsPerVoiceFrame.end()); + const double medianNs = nsPerVoiceFrame[nsPerVoiceFrame.size() / 2]; + const double secsAtMedian = + medianNs * static_cast(blockFrames) * static_cast(voiceCount) / 1e9; + std::printf(" [measure] 32 stereo Preserve voices @ rate %.2f: median %.1f ns/voice/" + "frame [%.1f .. %.1f] over %d reps (%.1f%% of realtime at the median)\n", + rate, medianNs, nsPerVoiceFrame.front(), nsPerVoiceFrame.back(), kTimedReps, + 100.0 * secsAtMedian); } }