Add best-guess fallback for low-note detection#30
Conversation
When the CMNDF threshold search fails (no dip below 0.15), fall back to the global CMNDF minimum instead of returning silence. This ensures harmonically complex signals like distorted guitar still trigger the ring modulator with a rough pitch estimate. Also widen analysis window from sampleRate/70 to sampleRate/60 for better low-frequency coverage (~5ms additional latency).
user1303836
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal change that addresses the low-note detection gap identified post-merge.
What this PR does:
-
Window widening (
/70.0->/60.0): Extends the analysis window to cover fundamentals down to ~60 Hz (was ~70 Hz). Adds ~5ms latency at 44.1kHz — acceptable tradeoff for better low-register coverage. -
Best-guess CMNDF fallback: When the absolute threshold (0.15) finds no candidate, the global CMNDF minimum is used instead. This prevents silence on harmonically complex signals where strong overtones prevent the CMNDF from dipping below threshold.
Observations (non-blocking):
- The fallback may lock onto subharmonics for complex timbres, but this is explicitly acceptable for a ring modulator (musically related frequencies). The confidence value from a non-threshold hit will naturally be lower, so the PitchSmoother sensitivity gate provides a second line of defense.
- The silence test correctly verifies that
tauEstimateremains 0 when all CMNDF values equal 1.0 (flat signal), since the fallback loop only updates on strict<comparison. - The test tolerance for the harmonically complex E2 test is intentionally wide (40-200 Hz) — appropriate for a fallback path where subharmonic ambiguity is expected.
Test coverage is solid: fallback path exercised, silence regression guarded, and threshold-first preference verified for clean signals.
| if (cmndf[tau] < minVal) | ||
| { | ||
| minVal = cmndf[tau]; | ||
| tauEstimate = tau; |
There was a problem hiding this comment.
The fallback scans the entire CMNDF for the global minimum, which for harmonically complex signals may land on a subharmonic (e.g. octave below) rather than the fundamental. This is fine for the ring modulator use case since subharmonic detection still produces a musically related modulation frequency, and the confidence value from the CMNDF minimum will naturally be lower, letting the PitchSmoother's sensitivity gate attenuate the effect.
Worth noting: if accuracy requirements tighten in the future, a refinement would be to only accept the fallback when minVal is below some secondary threshold (e.g. 0.5), rejecting truly ambiguous frames. But that's not needed now.
| feedHarmonicComplex(yin, 44100.0, 82.4f, 44100, 0.15f); | ||
|
|
||
| auto result = yin.getResult(); | ||
| REQUIRE(result.frequency > 0.0f); |
There was a problem hiding this comment.
The 40-200 Hz tolerance band is very wide (over 2 octaves) for an 82.4 Hz fundamental. This is intentional given the fallback's subharmonic ambiguity, but it means this test mainly verifies "something was detected in a plausible range" rather than "the fundamental was correctly identified." That's a reasonable stance for a fallback path — just calling it out for clarity.
Summary
sampleRate/70tosampleRate/60for better low-frequency margin (~5ms additional latency).Test plan