diff --git a/src/core/instrument/engine/period_detect.cpp b/src/core/instrument/engine/period_detect.cpp index 5d9f563..35622e0 100644 --- a/src/core/instrument/engine/period_detect.cpp +++ b/src/core/instrument/engine/period_detect.cpp @@ -125,7 +125,7 @@ PeriodEstimate detectPeriod(const std::vector& pcm, int sampleRate, if (sampleRate <= 0 || spanCount == 0) return {}; if (spanFrom > pcm.size() || spanCount > pcm.size() - spanFrom) return {}; const double rate = static_cast(sampleRate); - std::size_t lagHi = static_cast(rate / kPeriodDetectMinHz); + std::size_t lagHi = longestLagFrames(sampleRate); const std::size_t lagLo = static_cast(rate / kPeriodDetectMaxHz); if (lagLo < 2) return {}; // a rate so low the whole search band collapses @@ -145,10 +145,14 @@ PeriodEstimate detectPeriod(const std::vector& pcm, int sampleRate, std::vector periods; std::vector 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 small = decimate(pcm, from, block); const std::size_t smallHi = lagHi / kDecimate; @@ -177,16 +181,17 @@ PeriodEstimate detectPeriod(const std::vector& pcm, int sampleRate, if (periods.empty()) return {}; - // ONE surviving probe: the span could not host a second probe position, so there is no - // second estimate for the majority rule below to rule on — it would be deciding on an - // empty comparison. The accept rests on pickPeriod's absolute threshold, which is a real - // test and not an absence of one: the block genuinely repeats at this lag across its whole - // analysis window. Refusing instead would deny every short one-shot a period, and a period - // that turns out wrong costs a mis-centred correlation search at the splice, not an - // unrefined one (pitch_shift.cpp's splice searches +/- maxLag around whichever jump it is - // handed). Do not "unify" this back into the majority test — at size 1 that test accepts - // unconditionally, which is the same behaviour with none of the reasoning. - if (periods.size() == 1) { + // ONE piece of evidence in the whole span — either it hosted a single probe position, or + // every other probe was silent. Nothing can rule against this estimate, so the accept rests + // on pickPeriod's absolute threshold, which is a real test and not an absence of one: the + // block genuinely repeats at this lag across its whole analysis window. Refusing instead + // would deny every short one-shot a period, and a period that turns out wrong costs a + // mis-centred correlation search at the splice, not an unrefined one (pitch_shift.cpp's + // splice searches +/- maxLag around whichever jump it is handed). Do not "unify" this back + // into the majority test — at one piece of evidence that test accepts unconditionally, which + // is the same behaviour with none of the reasoning. Nor key it on how many probes SURVIVED: + // 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& pcm, int sampleRate, confSum += confidences[i]; ++agree; } - // A STRICT MAJORITY of the valid probes must agree, not merely two of them: a source whose - // first half is one period and second half another gives two probes each way, and taking - // either as "the" period would misalign every splice in the other half. Refusing is the - // right answer there — the fixed-window fallback is what a source with no ONE period gets. - // Reached only with two or more probes; the lone-probe case returned above. - if (agree * 2 <= periods.size()) return {}; + // A STRICT MAJORITY of the probes that carried signal must agree, not merely two of them: a + // source whose first half is one period and second half another gives two probes each way, + // and taking either as "the" period would misalign every splice in the other half. Refusing + // is the right answer there — the fixed-window fallback is what a source with no ONE period + // gets. The denominator is `evidence` and not `periods.size()` because once probes overlap + // 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(agree); @@ -235,8 +243,7 @@ AnalysisSpan periodAnalysisSpan(std::size_t frameCount, std::int64_t loopStart, const std::size_t length = static_cast(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 = - 2 * static_cast(static_cast(sampleRate) / kPeriodDetectMinHz); + const std::size_t minimum = 2 * longestLagFrames(sampleRate); if (length < minimum) return whole; return AnalysisSpan{static_cast(loopStart), length}; } diff --git a/src/core/instrument/engine/period_detect.h b/src/core/instrument/engine/period_detect.h index 1d74bf2..c2605f7 100644 --- a/src/core/instrument/engine/period_detect.h +++ b/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 - // the tests and the measurement harness. It is deliberately NOT a second accept gate: every + // is `valid()` alone and the loader takes `.frames` without reading this — its only reader is + // 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(static_cast(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 -// on a span too short to host a second probe position — is accepted on the absolute threshold -// alone, because there is no second estimate for a majority rule to rule on and refusing would -// deny every short one-shot a period. +// A STRICT MAJORITY of the probes that CARRIED SIGNAL must agree. Silence is excluded from that +// denominator and a failure to find a period is not: a silent block is no evidence either way, +// whereas a block that carries signal and repeats at no lag is evidence against a single 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& pcm, int sampleRate, std::size_t from, std::size_t count); @@ -79,12 +96,20 @@ PeriodEstimate detectPeriod(const std::vector& 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 -// is at least `2 * (sampleRate / kPeriodDetectMinHz)` frames — the span below which detectPeriod -// starts shortening its own search band — so choosing the narrower span never costs search-band -// width and so can never lose a low fundamental that the whole source would have found. +// disagree its way to none over the whole source. `[loopStart, loopEnd)` is used only when it is +// at least one full probe block — `2 * longestLagFrames(sampleRate)`, the span below which +// detectPeriod starts shortening its own search band — so choosing the narrower span never costs +// 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 { diff --git a/tests/test_period_detect.cpp b/tests/test_period_detect.cpp index 57a610a..fc05199 100644 --- a/tests/test_period_detect.cpp +++ b/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 -// the carve-out: which sources land in it, and that they are accepted rather than refused. -// 30 Hz is first-class material here, and a short low-frequency source is exactly where the -// blunt "require two probes" fix would have silently stopped detecting. +// A lone piece of evidence is the one case the strict-majority rule cannot rule on, so pin both +// halves of the carve-out: which sources land in it, and that they are accepted rather than +// refused. 30 Hz is first-class material here, and a short low-frequency source is exactly where +// 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(rate / kPeriodDetectMinHz); - const std::size_t block = 2 * lagHi; - // Derive the frame count from the public constants rather than hardcoding one, so this test - // keeps naming the lone-probe case if the geometry ever moves. One probe fits while the - // 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 std::size_t block = 2 * longestLagFrames(rate); + // Exactly one probe BLOCK leaves zero room to place a second probe anywhere, whatever + // separation the placement uses — so this names the lone-probe case independently of the + // formula, where a length derived from that formula would only re-assert it. const double p = static_cast(rate) / 30.0; // 1470 frames - const PeriodEstimate est = detectPeriod(sineOfPeriod(frames, p), rate); - std::printf(" lone probe, %zu frames @ 30 Hz -> %s (%.3f, want %.3f)\n", frames, - est.valid() ? "detected" : "NONE", est.frames, p); - CHECK(est.valid()); - if (est.valid()) CHECK(std::fabs(est.frames - p) < 1.0); + const PeriodEstimate lone = detectPeriod(sineOfPeriod(block, p), rate); + std::printf(" lone probe, %zu frames @ 30 Hz -> %s (%.3f, want %.3f)\n", block, + lone.valid() ? "detected" : "NONE", lone.frames, p); + CHECK(lone.valid()); + 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. - const PeriodEstimate two = detectPeriod(sineOfPeriod(frames + 1, p), rate); - CHECK(1 + (frames + 1 - block) / lagHi == 2); - CHECK(two.valid()); - if (two.valid()) CHECK(std::fabs(two.frames - p) < 1.0); + // Growing the span past the lone case must not turn the answer off, and must not change it: + // a probe-count boundary is not allowed to be a discontinuity in what a stationary source + // reports. Asserted over the answer rather than over the count, so it survives the placement + // rule moving. + 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(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(n) / static_cast(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(mult * static_cast(lagHi)); + std::vector 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(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(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(rate) / 30.0; + + for (std::size_t lead : {lagHi, 2 * lagHi, 4 * lagHi}) { + std::vector src(n, 0.0f); + for (std::size_t i = lead; i < n; ++i) { + src[i] = static_cast( + std::sin(2.0 * kPi * static_cast(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 burst(n); + std::uint32_t rng = 4242u; + for (auto& x : burst) { + rng = rng * 1664525u + 1013904223u; + x = static_cast((static_cast(rng >> 8) / 8388608.0) - 1.0); + } + for (std::size_t i = 0; i < 2 * lagHi; ++i) { + burst[i] = static_cast(std::sin(2.0 * kPi * static_cast(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(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{0, 0, false}, @@ -397,6 +497,9 @@ int main() { testBelowTheBandReportsNoneAndAboveItReportsAWholeMultiple(); testAShortSourceShortensTheSearchRatherThanRefusing(); testASourceTooShortForASecondProbeIsStillDetectedOnItsOneProbe(); + testAStationarySourceDetectsAtEveryLengthAcrossTheProbeCountSteps(); + testAPeriodChangeIsRefusedWhereTheProbesOverlapToo(); + testSilenceIsNotDissentButAnAbsentPeriodIs(); testTheTwoLowFrequenciesTheShifterWasBuiltForAreDetected(); testTheLoopStandsInForTheSourceOnlyWhenItCostsNoSearchBand(); testAPhraseWhoseLoopIsPitchedDifferentlyFromItsHeadDetectsOverTheLoop(); diff --git a/tests/test_sample_map.cpp b/tests/test_sample_map.cpp index e1f221b..52c03c2 100644 --- a/tests/test_sample_map.cpp +++ b/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(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() {