Merge Γ-W1-T7 detect findings: the agreement denominator counts probes that carried signal, so an overlapping straddle can no longer turn a two-and-two split into an accept

src/core/instrument/engine/period_detect.cpp

@@ -125,7 +125,7 @@ PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate,
     if (sampleRate <= 0 || spanCount == 0) return {};
     if (spanFrom > pcm.size() || spanCount > pcm.size() - spanFrom) return {};
     const double rate = static_cast<double>(sampleRate);
-    std::size_t lagHi = static_cast<std::size_t>(rate / kPeriodDetectMinHz);
+    std::size_t lagHi = longestLagFrames(sampleRate);
     const std::size_t lagLo = static_cast<std::size_t>(rate / kPeriodDetectMaxHz);
     if (lagLo < 2) return {};  // a rate so low the whole search band collapses
 
@@ -145,10 +145,14 @@ PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate,
 
     std::vector<double> periods;
     std::vector<double> confidences;
+    // Probes that carried signal — the agreement denominator. A silent block is no evidence
+    // either way and is excluded; every other outcome, a period found or not, is evidence.
+    std::size_t evidence = 0;
     for (std::size_t p = 0; p < probes; ++p) {
+        // (probes - 1) * stride <= room by construction, so the last block always fits.
         const std::size_t from = spanFrom + p * stride;
-        if (from + block > spanFrom + spanCount) break;
         if (blockRms(pcm, from, block) < kSilenceRms) continue;
+        ++evidence;
 
         const std::vector<double> small = decimate(pcm, from, block);
         const std::size_t smallHi = lagHi / kDecimate;
@@ -177,16 +181,17 @@ PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate,
 
     if (periods.empty()) return {};
 
-    // ONE surviving probe: the span could not host a second probe position, so there is no
+    // ONE piece of evidence in the whole span — either it hosted a single probe position, or
-    // second estimate for the majority rule below to rule on — it would be deciding on an
+    // every other probe was silent. Nothing can rule against this estimate, so the accept rests
-    // empty comparison. The accept rests on pickPeriod's absolute threshold, which is a real
+    // on pickPeriod's absolute threshold, which is a real test and not an absence of one: the
-    // test and not an absence of one: the block genuinely repeats at this lag across its whole
+    // block genuinely repeats at this lag across its whole analysis window. Refusing instead
-    // analysis window. Refusing instead would deny every short one-shot a period, and a period
+    // would deny every short one-shot a period, and a period that turns out wrong costs a
-    // that turns out wrong costs a mis-centred correlation search at the splice, not an
+    // mis-centred correlation search at the splice, not an unrefined one (pitch_shift.cpp's
-    // unrefined one (pitch_shift.cpp's splice searches +/- maxLag around whichever jump it is
+    // splice searches +/- maxLag around whichever jump it is handed). Do not "unify" this back
-    // handed). Do not "unify" this back into the majority test — at size 1 that test accepts
+    // into the majority test — at one piece of evidence that test accepts unconditionally, which
-    // unconditionally, which is the same behaviour with none of the reasoning.
+    // is the same behaviour with none of the reasoning. Nor key it on how many probes SURVIVED:
-    if (periods.size() == 1) {
+    // one survivor out of four that all carried signal is not this case at all.
+    if (evidence == 1) {
         PeriodEstimate lone;
         lone.frames = periods[0];
         lone.confidence = confidences[0];
@@ -208,12 +213,15 @@ PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate,
         confSum += confidences[i];
         ++agree;
     }
-    // A STRICT MAJORITY of the valid probes must agree, not merely two of them: a source whose
+    // A STRICT MAJORITY of the probes that carried signal must agree, not merely two of them: a
-    // first half is one period and second half another gives two probes each way, and taking
+    // source whose first half is one period and second half another gives two probes each way,
-    // either as "the" period would misalign every splice in the other half. Refusing is the
+    // and taking either as "the" period would misalign every splice in the other half. Refusing
-    // right answer there — the fixed-window fallback is what a source with no ONE period gets.
+    // is the right answer there — the fixed-window fallback is what a source with no ONE period
-    // Reached only with two or more probes; the lone-probe case returned above.
+    // gets. The denominator is `evidence` and not `periods.size()` because once probes overlap
-    if (agree * 2 <= periods.size()) return {};
+    // a straddling block finds no period at all rather than a third one, and counting only the
+    // survivors turned that two-and-two split into a two-of-three accept.
+    // Reached only with two or more pieces of evidence; the lone case returned above.
+    if (agree * 2 <= evidence) return {};
 
     PeriodEstimate est;
     est.frames = sum / static_cast<double>(agree);
@@ -235,8 +243,7 @@ AnalysisSpan periodAnalysisSpan(std::size_t frameCount, std::int64_t loopStart,
     const std::size_t length = static_cast<std::size_t>(loopEnd - loopStart);
     // One full probe block. Below it detectPeriod shortens lagHi to fit, which raises the
     // lowest findable fundamental — the one thing the narrower span may never cost.
-    const std::size_t minimum =
+    const std::size_t minimum = 2 * longestLagFrames(sampleRate);
-        2 * static_cast<std::size_t>(static_cast<double>(sampleRate) / kPeriodDetectMinHz);
     if (length < minimum) return whole;
     return AnalysisSpan{static_cast<std::size_t>(loopStart), length};
 }

src/core/instrument/engine/period_detect.h

@@ -21,8 +21,8 @@ using audio::AudioSample;
 struct PeriodEstimate {
     double frames = 0.0;      // 0 = no single period (inharmonic, polyphonic, percussive, noise)
     // 1 - the accepted dissimilarity, [0,1]; 0 when frames == 0. Diagnostic: the accept decision
-    // is `valid()` alone and the loader takes `.frames` without reading this — its consumers are
+    // is `valid()` alone and the loader takes `.frames` without reading this — its only reader is
-    // the tests and the measurement harness. It is deliberately NOT a second accept gate: every
+    // tests/test_period_detect.cpp. It is deliberately NOT a second accept gate: every
     // accepted probe already cleared kPeriodDetectThreshold, so confidence > 0.88 holds by
     // construction and any gate below that is a no-op while any gate above it is a tuned number
     // with nothing to derive it from.
@@ -44,6 +44,14 @@ inline constexpr double kPeriodDetectMaxHz = 2000.0;
 // global minimum — the difference between "quiet but real" and "the least bad of nothing".
 inline constexpr double kPeriodDetectThreshold = 0.12;
 
+// The longest lag searched, in frames — THE one derivation of it. A probe block is twice this,
+// and `periodAnalysisSpan`'s minimum is one block; both read this rather than re-deriving the
+// same expression, so "choosing the loop never narrows the search band" is a fact and not a
+// coincidence between two literals.
+inline std::size_t longestLagFrames(int sampleRate) {
+    return static_cast<std::size_t>(static_cast<double>(sampleRate) / kPeriodDetectMinHz);
+}
+
 // How many blocks across the sample are estimated independently, and how far apart two of them
 // may land and still be called the same period. Agreement is what separates a genuinely
 // periodic source from one whose opening happens to look periodic.
@@ -53,8 +61,6 @@ inline constexpr double kPeriodDetectThreshold = 0.12;
 // whole cycle of the lowest frequency in the band, so neither can be a trivially shifted copy
 // of the other at any period searched. Requiring DISJOINT blocks instead left every source
 // under ~4x the longest lag with a single probe and so with no agreement to check at all.
-// One probe survives as an irreducible case below `block + longest lag` frames and is accepted
-// on the absolute threshold alone — see detectPeriod's contract.
 inline constexpr int kPeriodDetectProbes = 4;
 inline constexpr double kPeriodDetectAgreeTolerance = 0.02;  // 2% of the median
 
@@ -66,10 +72,21 @@ inline constexpr double kPeriodDetectAgreeTolerance = 0.02;  // 2% of the median
 // Returns an invalid estimate (frames == 0) for silence, noise, and anything whose probes
 // disagree — the caller's documented fallback is the fixed-window splice geometry.
 //
-// Two probes or more must reach a STRICT MAJORITY agreement. A lone probe — which only happens
+// A STRICT MAJORITY of the probes that CARRIED SIGNAL must agree. Silence is excluded from that
-// on a span too short to host a second probe position — is accepted on the absolute threshold
+// denominator and a failure to find a period is not: a silent block is no evidence either way,
-// alone, because there is no second estimate for a majority rule to rule on and refusing would
+// whereas a block that carries signal and repeats at no lag is evidence against a single period.
-// deny every short one-shot a period.
+// A capture with a silent head or tail therefore still detects, while a mostly-noise source with
+// one pitched burst is refused rather than accepted on that burst alone. A LONE piece of
+// evidence — the whole span too short for a second probe position, or every other probe silent —
+// is accepted on the absolute threshold alone, because there is nothing to rule against it and
+// refusing would deny every short one-shot a period.
+//
+// The answer is NOT monotone in span length, and cannot be made so: no rule that refuses a
+// two-and-two split at four probes can also accept a lone probe unconditionally, and the probe
+// count steps at 3x, 4x, 5x and 6x the longest lag before saturating. What IS pinned, by a
+// length sweep in the tests, is that a STATIONARY source detects at every length — a source
+// whose period varies by more than kPeriodDetectAgreeTolerance is the only class that moves
+// with the count, and refusing it is this contract's own answer.
 PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate,
                             std::size_t from, std::size_t count);
 
@@ -79,12 +96,20 @@ PeriodEstimate detectPeriod(const std::vector<AudioSample>& pcm, int sampleRate)
 // The frames detection should analyse for a capture that carries a sustain loop, and the reason
 // the answer is not simply "all of them": under Gate the loop region is asymptotically ALL the
 // splicer plays, so a phrase whose head is pitched differently from its sustain would otherwise
-// disagree its way to none over the whole source. `[loopStart, loopEnd)` is used only when it
+// disagree its way to none over the whole source. `[loopStart, loopEnd)` is used only when it is
-// is at least `2 * (sampleRate / kPeriodDetectMinHz)` frames — the span below which detectPeriod
+// at least one full probe block — `2 * longestLagFrames(sampleRate)`, the span below which
-// starts shortening its own search band — so choosing the narrower span never costs search-band
+// detectPeriod starts shortening its own search band — so choosing the narrower span never costs
-// width and so can never lose a low fundamental that the whole source would have found.
+// search-band WIDTH. It can still change the ANSWER: the agreement rule rules on content, so a
+// source periodic over most of its length whose loop region is noisy detects whole and refuses
+// over the loop. That is the intent — the loop is what a Gate voice plays.
 // Anything else (no loop, an out-of-range span, a short one) yields the whole source.
 //
+// It takes NO play mode, deliberately, even though loop_span's resolveLoop does and refuses the
+// loop outright under Trigger. A loop edit is structurally reload-bound — it moves the PCM span
+// this cache was derived from — whereas play mode's exclusion from live delivery is a listed,
+// reversible decision (deck_groups' isLiveDeckParam). Keying a load-time cache on it would work
+// today and silently serve a stale period the day that decision is revisited.
+//
 // The read path's loop-validity authority is loop_span's resolveLoop; the bounds check here is
 // on a cache input, not a second validity rule, and it refuses rather than repairs the same way.
 struct AnalysisSpan {

tests/test_period_detect.cpp

@@ -10,7 +10,8 @@
 //   3. graceful degradation — noise, silence, and a source whose period changes mid-sample all
 //      return NONE. That is the contract the shifter's fixed-window fallback rests on: an
 //      estimate that is merely wrong would misalign every splice, which is worse than none.
-//   4. the band edges and the short-sample path, including the lone-probe accept.
+//   4. the band edges and the short-sample path, including the lone-probe accept, the length
+//      sweep across every probe-count step, and what counts as evidence against a period.
 //   5. the analysis span: a sustain loop stands in for the whole source, but never at the cost
 //      of search-band width.
 //   6. what the load pays, and that it does not grow with the sample length.
@@ -221,32 +222,118 @@ static void testAShortSourceShortensTheSearchRatherThanRefusing() {
     CHECK(!detectPeriod(sineOfPeriod(40, 20.0), 44100).valid());
 }
 
-// A lone probe is the one case the strict-majority rule cannot rule on, so pin BOTH halves of
+// A lone piece of evidence is the one case the strict-majority rule cannot rule on, so pin both
-// the carve-out: which sources land in it, and that they are accepted rather than refused.
+// halves of the carve-out: which sources land in it, and that they are accepted rather than
-// 30 Hz is first-class material here, and a short low-frequency source is exactly where the
+// refused. 30 Hz is first-class material here, and a short low-frequency source is exactly where
-// blunt "require two probes" fix would have silently stopped detecting.
+// the blunt "require two probes" fix would have silently stopped detecting.
 static void testASourceTooShortForASecondProbeIsStillDetectedOnItsOneProbe() {
     const int rate = 44100;
-    const std::size_t lagHi = static_cast<std::size_t>(rate / kPeriodDetectMinHz);
+    const std::size_t block = 2 * longestLagFrames(rate);
-    const std::size_t block = 2 * lagHi;
+    // Exactly one probe BLOCK leaves zero room to place a second probe anywhere, whatever
-    // Derive the frame count from the public constants rather than hardcoding one, so this test
+    // separation the placement uses — so this names the lone-probe case independently of the
-    // keeps naming the lone-probe case if the geometry ever moves. One probe fits while the
+    // formula, where a length derived from that formula would only re-assert it.
-    // span leaves less than lagHi of room after the first block.
-    const std::size_t frames = block + lagHi - 1;   // 8819 at 44.1k -> exactly one probe
-    CHECK(1 + (frames - block) / lagHi == 1);
-
     const double p = static_cast<double>(rate) / 30.0;  // 1470 frames
-    const PeriodEstimate est = detectPeriod(sineOfPeriod(frames, p), rate);
+    const PeriodEstimate lone = detectPeriod(sineOfPeriod(block, p), rate);
-    std::printf("  lone probe, %zu frames @ 30 Hz -> %s (%.3f, want %.3f)\n", frames,
+    std::printf("  lone probe, %zu frames @ 30 Hz -> %s (%.3f, want %.3f)\n", block,
-                est.valid() ? "detected" : "NONE", est.frames, p);
+                lone.valid() ? "detected" : "NONE", lone.frames, p);
-    CHECK(est.valid());
+    CHECK(lone.valid());
-    if (est.valid()) CHECK(std::fabs(est.frames - p) < 1.0);
+    if (lone.valid()) CHECK(std::fabs(lone.frames - p) < 1.0);
 
-    // One frame more buys a second probe position; the answer must not change character.
+    // Growing the span past the lone case must not turn the answer off, and must not change it:
-    const PeriodEstimate two = detectPeriod(sineOfPeriod(frames + 1, p), rate);
+    // a probe-count boundary is not allowed to be a discontinuity in what a stationary source
-    CHECK(1 + (frames + 1 - block) / lagHi == 2);
+    // reports. Asserted over the answer rather than over the count, so it survives the placement
-    CHECK(two.valid());
+    // rule moving.
-    if (two.valid()) CHECK(std::fabs(two.frames - p) < 1.0);
+    for (std::size_t extra : {std::size_t{1}, block / 4, block / 2, block, 2 * block}) {
+        const PeriodEstimate more = detectPeriod(sineOfPeriod(block + extra, p), rate);
+        CHECK(more.valid());
+        if (more.valid()) CHECK(std::fabs(more.frames - p) < 1.0);
+    }
+}
+
+// The acceptance property behind the position-placement change: a STATIONARY source must not
+// lose detection at any length. The probe count steps at 3x, 4x, 5x and 6x the longest lag and
+// the agreement bar steps with it, so a length sweep is the only thing that pins the whole
+// band — a single 160 000-frame case sits above every step and cannot see them.
+static void testAStationarySourceDetectsAtEveryLengthAcrossTheProbeCountSteps() {
+    const int rate = 44100;
+    const std::size_t lagHi = longestLagFrames(rate);
+    int refused = 0;
+    for (double hz : {30.0, 29.0, 55.0}) {
+        const double p = static_cast<double>(rate) / hz;
+        for (std::size_t n = 2 * lagHi; n <= 12 * lagHi; n += lagHi / 4) {
+            const PeriodEstimate est = detectPeriod(sineOfPeriod(n, p), rate);
+            if (!est.valid() || std::fabs(est.frames - p) > 1.0) {
+                ++refused;
+                std::printf("  %.0f Hz at %zu frames (%.2fx lagHi): %s (%.3f)\n", hz, n,
+                            static_cast<double>(n) / static_cast<double>(lagHi),
+                            est.valid() ? "wrong" : "NONE", est.frames);
+            }
+        }
+    }
+    CHECK(refused == 0);
+}
+
+// The regime the position-placement change actually moved: probes overlap only while the stride
+// is under one block, i.e. below 8x the longest lag. A straddling block finds NO period rather
+// than a third one, so counting only the probes that survived turned a genuine two-and-two split
+// into a two-of-three accept — a source with two periods reported as having the first one, which
+// is the outcome the module calls worse than none.
+static void testAPeriodChangeIsRefusedWhereTheProbesOverlapToo() {
+    const int rate = 44100;
+    const std::size_t lagHi = longestLagFrames(rate);
+    for (double mult : {3.0, 4.0, 5.0, 6.0, 7.0}) {
+        const std::size_t n = static_cast<std::size_t>(mult * static_cast<double>(lagHi));
+        std::vector<AudioSample> src(n);
+        double phase = 0.0;
+        for (std::size_t i = 0; i < n; ++i) {
+            phase += 2.0 * kPi / (i < n / 2 ? 300.0 : 700.0);
+            src[i] = static_cast<AudioSample>(std::sin(phase));
+        }
+        const PeriodEstimate est = detectPeriod(src, rate);
+        std::printf("  period change over %.0fx lagHi (%zu frames, %.2f s) -> %s (%.3f)\n", mult,
+                    n, static_cast<double>(n) / rate, est.valid() ? "DETECTED" : "none",
+                    est.frames);
+        CHECK(!est.valid());
+    }
+}
+
+// Silence and an absent period are NOT the same evidence, and the agreement denominator has to
+// tell them apart: a capture with a silent head still detects, while a source that is mostly
+// aperiodic with one pitched burst must not be accepted on that burst alone.
+static void testSilenceIsNotDissentButAnAbsentPeriodIs() {
+    const int rate = 44100;
+    const std::size_t lagHi = longestLagFrames(rate);
+    const std::size_t n = 10 * lagHi;
+    const double p = static_cast<double>(rate) / 30.0;
+
+    for (std::size_t lead : {lagHi, 2 * lagHi, 4 * lagHi}) {
+        std::vector<AudioSample> src(n, 0.0f);
+        for (std::size_t i = lead; i < n; ++i) {
+            src[i] = static_cast<AudioSample>(
+                std::sin(2.0 * kPi * static_cast<double>(i - lead) / p));
+        }
+        const PeriodEstimate est = detectPeriod(src, rate);
+        std::printf("  %zu frames of leading silence -> %s (%.3f)\n", lead,
+                    est.valid() ? "detected" : "NONE", est.frames);
+        CHECK(est.valid());
+        if (est.valid()) CHECK(std::fabs(est.frames - p) < 1.0);
+    }
+
+    // Noise everywhere but the opening: three probes carry signal and find no period, one finds
+    // one. Counting only the survivor accepted this on a single unopposed estimate.
+    std::vector<AudioSample> burst(n);
+    std::uint32_t rng = 4242u;
+    for (auto& x : burst) {
+        rng = rng * 1664525u + 1013904223u;
+        x = static_cast<AudioSample>((static_cast<double>(rng >> 8) / 8388608.0) - 1.0);
+    }
+    for (std::size_t i = 0; i < 2 * lagHi; ++i) {
+        burst[i] = static_cast<AudioSample>(std::sin(2.0 * kPi * static_cast<double>(i) / p));
+    }
+    const PeriodEstimate est = detectPeriod(burst, rate);
+    std::printf("  noise with one pitched burst -> %s (%.3f)\n", est.valid() ? "DETECTED" : "none",
+                est.frames);
+    CHECK(!est.valid());
 }
 
 static void testTheTwoLowFrequenciesTheShifterWasBuiltForAreDetected() {
@@ -270,7 +357,20 @@ static void testTheLoopStandsInForTheSourceOnlyWhenItCostsNoSearchBand() {
     const std::size_t frames = 120000;
     // One full probe block — the span below which detectPeriod starts shortening its own
     // longest lag, which is the only thing the narrower span may never cost.
-    const std::size_t minimum = 2 * static_cast<std::size_t>(rate / kPeriodDetectMinHz);
+    const std::size_t minimum = 2 * longestLagFrames(rate);
+
+    // An unusable rate and an empty capture take the same refuse-rather-than-repair path the
+    // loop-bounds branches do; nothing downstream may see a span it cannot analyse.
+    const AnalysisSpan noRate = periodAnalysisSpan(frames, 0, 90000, true, 0);
+    CHECK(noRate.from == 0 && noRate.count == frames);
+    const AnalysisSpan negRate = periodAnalysisSpan(frames, 0, 90000, true, -44100);
+    CHECK(negRate.from == 0 && negRate.count == frames);
+    const AnalysisSpan empty = periodAnalysisSpan(0, 0, 0, true, rate);
+    CHECK(empty.from == 0 && empty.count == 0);
+    // An empty capture with a loop still reaching past it refuses to the (empty) whole source
+    // rather than handing detection a span past the end of the PCM.
+    const AnalysisSpan emptyLooped = periodAnalysisSpan(0, 0, 44100, true, rate);
+    CHECK(emptyLooped.from == 0 && emptyLooped.count == 0);
 
     // No loop, an inverted span, and a span reaching past the PCM all yield the whole source.
     for (const auto& [lo, hi, has] : {std::tuple<std::int64_t, std::int64_t, bool>{0, 0, false},
@@ -397,6 +497,9 @@ int main() {
     testBelowTheBandReportsNoneAndAboveItReportsAWholeMultiple();
     testAShortSourceShortensTheSearchRatherThanRefusing();
     testASourceTooShortForASecondProbeIsStillDetectedOnItsOneProbe();
+    testAStationarySourceDetectsAtEveryLengthAcrossTheProbeCountSteps();
+    testAPeriodChangeIsRefusedWhereTheProbesOverlapToo();
+    testSilenceIsNotDissentButAnAbsentPeriodIs();
     testTheTwoLowFrequenciesTheShifterWasBuiltForAreDetected();
     testTheLoopStandsInForTheSourceOnlyWhenItCostsNoSearchBand();
     testAPhraseWhoseLoopIsPitchedDifferentlyFromItsHeadDetectsOverTheLoop();

tests/test_sample_map.cpp

@@ -957,12 +957,14 @@ static void testBuildSampleDataDetectsOverTheSustainLoopNotTheWholeSource() {
     CHECK(std::fabs(sd.sourcePeriodFrames - loopPeriod) < 2.0);
 
     // A loop too short to host the full search band falls back to the whole source rather than
-    // to none — here that whole source has no one period, so the answer is the bare one above.
+    // to none — here that whole source has no one period, so the answer is none. Asserted
+    // against the literal, not against `bare`: the two agreeing would also hold if both
+    // regressed together, which is no evidence that the fallback ran.
     InstrumentParams shortLoop;
     shortLoop.loopOverride = SampleLoop{true, 118000, static_cast<std::int64_t>(frames)};
     const SampleData shortSd = buildSampleData(resolveCapture(ref("b/a.wav", 60), shortLoop),
                                                DecodedPcm{pcm, rate, {}});
-    CHECK(shortSd.sourcePeriodFrames == bare.sourcePeriodFrames);
+    CHECK(shortSd.sourcePeriodFrames == 0.0);
 }
 
 static void testBuildSampleDataCarriesTheVelocityCurve() {