From 334022c0f18b06ae181f6d9949494a8c4e394798 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 03:01:15 -0400 Subject: [PATCH] Gamma-W1-T7: gate PSOLA's two untested wires, fix the cadence test's contradictory rationale, add a vacuity guard --- src/core/instrument/engine/period_detect.h | 5 ++++- src/core/instrument/engine/time_stretch.h | 6 +++++- tests/test_pitch_shift.cpp | 24 +++++++++++++++------- tests/test_sample_map.cpp | 18 ++++++++++++++++ tests/test_sampler_core.cpp | 20 ++++++++++++++++++ 5 files changed, 64 insertions(+), 9 deletions(-) diff --git a/src/core/instrument/engine/period_detect.h b/src/core/instrument/engine/period_detect.h index 515c0ff..f07112b 100644 --- a/src/core/instrument/engine/period_detect.h +++ b/src/core/instrument/engine/period_detect.h @@ -19,7 +19,10 @@ using audio::AudioSample; // authored and never persisted — this is a cache, not state. struct PeriodEstimate { double frames = 0.0; // 0 = no single period (inharmonic, polyphonic, percussive, noise) - double confidence = 0.0; // 1 - the accepted dissimilarity, [0,1]; 0 when frames == 0 + // 1 - the accepted dissimilarity, [0,1]; 0 when frames == 0. Diagnostic only today: the + // accept decision is `valid()` alone, and the loader takes `.frames` without reading this — + // do not assume it is load-bearing without checking who reads it. + double confidence = 0.0; bool valid() const { return frames > 0.0; } }; diff --git a/src/core/instrument/engine/time_stretch.h b/src/core/instrument/engine/time_stretch.h index d259fd8..1552feb 100644 --- a/src/core/instrument/engine/time_stretch.h +++ b/src/core/instrument/engine/time_stretch.h @@ -50,7 +50,11 @@ namespace reasampler::instrument::engine { // The fix is not a wider window: it is a nominal jump that is a whole number of the source's // own periods, so an aligned landing point exists by construction (pitch_shift.h's // periodAlignedJump, fed by period_detect at load). The same measurements then read 0.00% and -// 0.00%, and 29 Hz renders at +0.0 cents. What survives: a period longer than the reachable +// 0.00%, and 29 Hz renders at +0.0 cents — all from `preserve_low_frequency_tests` (Release, +// hand-run; it is not in the gated ctest set), the same harness/config as the 3.6%/15.5%/-133 +// cents readings above. The gated suite's own number for this is the floor-relative excess in +// pitch_shift_tests' testThirtyHertzSplicesAlignOnceTheSourcePeriodIsKnown, a different +// quantity from the raw percentages here. What survives: a period longer than the reachable // jump (~1.25 windows, so below ~16 Hz at 50 ms) still cannot align, and a source with no // single period falls back to this fixed-window geometry by design. inline constexpr double kStretchRateMin = 0.5; diff --git a/tests/test_pitch_shift.cpp b/tests/test_pitch_shift.cpp index 393b1ff..99706c7 100644 --- a/tests/test_pitch_shift.cpp +++ b/tests/test_pitch_shift.cpp @@ -780,10 +780,11 @@ static void testStretchCadenceCornerArtifactEnergyAtRate2ShiftQuarter() { const std::size_t outFrames = 60000; const std::size_t from = 20000, len = 32768; - // Below the safe floor (P > 315 frames): the cadence inequality predicts real damage, - // measured at 7-21% (see above). The threshold (5%) sits above the alignable control's - // near-zero floor and under the observed range, so it discriminates a genuine cadence hit - // from a clean render; the ceiling (30%) is a generous margin above the highest measured + // Below the safe floor (P > 315 frames): per the header's CORRECTED note, this raw reading + // is mostly the metric's own mainlobe-leakage floor, not cadence damage (the real excess is + // ~0.2-0.5%, asserted in testCadenceCornerIsUnmovedByAPitchSynchronousSplice). The bounds + // below are a RAW-READING STABILITY TRIPWIRE, not a discriminator — they catch a large swing + // in the raw measurement; the ceiling (30%) is a generous margin above the highest measured // value, there to catch a much worse regression rather than to chase today's exact number. for (double period : {500.0, 600.0, 700.0}) { const double f0 = 1.0 / period; @@ -918,7 +919,7 @@ static void testThirtyHertzSplicesAlignOnceTheSourcePeriodIsKnown() { {"34 Hz +2 st, rate 1.0", 34.0, 1.0, 2.0}, // control: alignable without a period {"34 Hz rate 2.0", 34.0, 2.0, 0.0}, }; - double controlWorst = 0.0, subjectWorst = 0.0; + double controlWorst = 0.0, subjectWorst = 0.0, subjectOffWorst = 0.0; for (const Row& r : rows) { const double period = 44100.0 / r.freq; std::vector src(srcLen); @@ -938,8 +939,12 @@ static void testThirtyHertzSplicesAlignOnceTheSourcePeriodIsKnown() { std::printf(" [30 Hz] %-24s (want %6.1f fr, metric floor %.2f%%) excess energy: " "fixed window %6.2f%% -> pitch-synchronous %6.2f%%\n", r.label, want, floor, pctOff, pctOn); - if (r.freq == 34.0) controlWorst = std::max(controlWorst, pctOn); - else subjectWorst = std::max(subjectWorst, pctOn); + if (r.freq == 34.0) { + controlWorst = std::max(controlWorst, pctOn); + } else { + subjectWorst = std::max(subjectWorst, pctOn); + subjectOffWorst = std::max(subjectOffWorst, pctOff); + } } // 30 Hz stops being a special case: with the period known its excess over the metric's own // floor is no worse than the alignable neighbour's, measured identically. Against the @@ -949,6 +954,11 @@ static void testThirtyHertzSplicesAlignOnceTheSourcePeriodIsKnown() { subjectWorst, controlWorst); CHECK(subjectWorst < 0.10); CHECK(subjectWorst <= controlWorst + 0.05); // 0.05 absorbs the floor subtraction's sign noise + // Vacuity guard, matching testTwentyNineHertzAtRateTwoKeepsItsPitch's sibling check: the + // FIXED-WINDOW (no period set) arm is asserted too, so a setSourcePeriod that silently did + // nothing would render both arms identically and fail here rather than passing on the + // absolute bound above by luck. + CHECK(subjectOffWorst > subjectWorst + 1.0); } // The sharpest single symptom of the geometry: at 29 Hz the nearest multiple misses the diff --git a/tests/test_sample_map.cpp b/tests/test_sample_map.cpp index f96e4c9..a32aa32 100644 --- a/tests/test_sample_map.cpp +++ b/tests/test_sample_map.cpp @@ -909,6 +909,23 @@ static void testBuildSampleDataEmptyPcmIsUnplayable() { CHECK(sd.frames.empty()); } +// Major-1 remediation: buildSampleData (sample_map.cpp:333-334) is the ONE call site wiring +// load-time detection to SampleData; detectPeriod's own unit coverage (test_period_detect.cpp) +// never exercises this call, so a deleted wire passed the gated suite unnoticed. Goes through +// the real build, not a direct detectPeriod call. +static void testBuildSampleDataDetectsThirtyHertzSourcePeriod() { + const int rate = 44100; + const std::size_t frames = 30000; + std::vector pcm(frames); + for (std::size_t i = 0; i < frames; ++i) { + pcm[i] = static_cast( + std::sin(2.0 * 3.14159265358979323846 * 30.0 * static_cast(i) / rate)); + } + const SampleData sd = buildSampleData(resolveCapture(ref("b/a.wav", 60), InstrumentParams{}), + DecodedPcm{pcm, rate, {}}); + CHECK(std::fabs(sd.sourcePeriodFrames - 1470.0) < 2.0); // 44100 / 30 Hz +} + static void testBuildSampleDataCarriesTheVelocityCurve() { InstrumentParams p; p.velocityCurve = VelocityCurve::linear(); @@ -976,6 +993,7 @@ int main() { testBuildSampleDataDropsMismatchedSecondChannel(); testBuildSampleDataEmptyPcmIsUnplayable(); testBuildSampleDataCarriesTheVelocityCurve(); + testBuildSampleDataDetectsThirtyHertzSourcePeriod(); if (g_fail == 0) std::printf("sample_map: all tests passed\n"); return g_fail != 0; diff --git a/tests/test_sampler_core.cpp b/tests/test_sampler_core.cpp index 9ead4e7..3ab30ef 100644 --- a/tests/test_sampler_core.cpp +++ b/tests/test_sampler_core.cpp @@ -2991,6 +2991,25 @@ static void testPreserveUnityRateIsBitIdenticalToTheShippedRead() { } } +// --- The load->voice wire is load-bearing: Voice::start (voice.cpp:248-249) is the only place +// a detected period reaches the shifter. Assert the two SampleDatas actually render +// differently rather than only that setSourcePeriod is callable — dropping either of the +// wire's two lines left this whole gated suite green before this test existed. +static void testSourcePeriodChangesTheRenderedStream() { + const std::int64_t w = 2205; // the product window at 44.1k + const std::size_t n = 6000; + SampleData off = stretchProbeSample(20000, false); + SampleData on = off; + // 30 Hz @ 44.1k: periodAlignedJump takes ONE whole period (1470), which cannot fit twice + // inside the shifter's reachable jump bound — a different splice geometry from the fixed + // 2205-frame window, so a wire failure here cannot pass by coincidence. + on.sourcePeriodFrames = 1470.0; + std::vector lOff(n), lOn(n), rUnused; + renderVoice(off, /*note=*/67, /*rate=*/1.0, w, false, lOff, rUnused); // +7 st: real splices + renderVoice(on, 67, 1.0, w, false, lOn, rUnused); + CHECK(hashStream(lOff) != hashStream(lOn)); +} + // --- Rate changes DURATION only; the transposition alone sets pitch. --- static void testPreserveStretchChangesDurationNotPitch() { // Gate, no loop: the voice's life is exactly how long the source lasts, so the frame at @@ -3419,6 +3438,7 @@ int main() { // The Preserve read path's stretch generalization. testPreserveUnityRateIsBitIdenticalToTheShippedRead(); + testSourcePeriodChangesTheRenderedStream(); testPreserveStretchChangesDurationNotPitch(); testPreserveStretchSpeaksOnFrameZeroAtEveryRate(); testPreserveStretchLoopsTheSourceSpan();