Conversation
The autocorrelation lag grid is coarse (~0.8 BPM per lag at 125) and parabolic peak interpolation lands 0.1-0.5 BPM off the true tempo often enough that roughly a third of a real library came out fractional (120.85 for a 121 track). The error also poisons the grid: the tempogram phase is computed at the wrong period, so the grid drifts a full beat over a typical track and the downbeat lands wrong. The old post-phase rounding (within 0.15) was too narrow to catch the error band and kept the already-computed wrong-period phase anyway. snapVerifyBPM runs BEFORE any phase work so the grid is fit at the corrected tempo. It scores candidate tempos by snapCoherence: the mean resultant length of per-window unit phasors at the candidate period, clarity-weighted so no window can dominate. At the true period the phasors agree track-wide; 0.2 BPM of error rotates them through most of a turn and the resultant collapses. (windowedTempogramPhase's amplitude-cubed weights are useless for this: the band-norm EMA warmup spike gives the first window ~10^5 times the weight of any other, so its combine barely sees cross-window rotation.) Real coherence landscapes shaped the decision rules. Tracks with a phase discontinuity (a breakdown re-entering off the extrapolated grid) CANCEL exactly at the true tempo - a null at the integer with symmetric side lobes that reach well past lobe-noise levels - so candidate-vs-candidate comparison is noise on them. Only near-perfect track-wide coherence (>= 0.9) may overrule the integer prior and keep a refined fractional tempo. Otherwise the prior rules: a flanking integer at >= 0.5 coherence and triple its rival overrides proximity (the front-end can land a full BPM off a clean track), then the nearest integer within 0.35 wins outright, and only the ambiguous x.4-x.6 band gets a coherence vote, with half-integers needing double the best integer to take it. Swept against a real library: nearly every fractional BPM snaps to its integer, the rare genuinely fractional tempos keep their refined value on near-perfect coherence, and a few stale integer rows move to a decisively stronger integer the old pipeline had missed. cacheVersion bumps to 27 so v26 BPMs and grids re-analyze.
applyAnalysisToTrack was fill-only: it set BPM/duration/key on empty rows but never updated an existing value, so a cacheVersion bump that changes detection (like the v27 integer snap) re-analyzed the audio and regenerated the beat grid at the new tempo while the library row - what the web UI, track details, and CDJ menus display - kept the old BPM forever. The row would say 120.85 while the deck played a 121 grid. BPM now follows the analysis result unless the user has overridden it (DetectedBPM != 0 is the override snapshot the BPM-edit endpoint takes, and a clear restores it), keeping the displayed tempo consistent with the grid that actually plays. Duration/key/bitrate stay fill-only. For imported libraries this means a re-analysis after a version bump also updates the imported BPM - consistent with the existing behaviour that the imported ANLZ grid itself is re-analyzed away at the same moment. Covered by a unit test over the four paths: stale row follows, user override survives, empty row fills, identical result noops.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Roughly a third of a real library detected a BPM a fraction off a whole number (120.85 for a true 121): the autocorrelation lag grid is coarse (~0.8 BPM per lag at 125) and parabolic peak interpolation lands 0.1-0.5 BPM off, past the old 0.15 rounding threshold. The error also poisons the grid, since the tempogram phase is computed at the wrong period the grid drifts a full beat over a typical track and the downbeat lands wrong.
snapVerifyBPM now runs before any phase work, so the grid is fit at the corrected tempo. Candidates are scored by snapCoherence, the clarity-weighted mean resultant of per-window beat phasors at the candidate period: at the true tempo the phasors agree track-wide, while 0.2 BPM of error rotates them through most of a turn and the resultant collapses. The existing amplitude-cubed phase weights could not be reused for this because the band-norm EMA warmup gives the first window a weight several orders of magnitude above any other.
The decision rules came from real coherence landscapes, not theory. Tracks with a phase discontinuity (a breakdown re-entering off the extrapolated grid) cancel exactly at their true tempo, a null at the integer with strong side lobes, so candidate comparisons are noise on them. Only near-perfect coherence (>= 0.9) may keep a fractional tempo; otherwise the integer prior rules, with a decisive-evidence override for the case where the front-end lands a full BPM off a clean track.
Swept against the library: nearly every fractional BPM snaps to its integer, the rare genuinely fractional tempos keep their refined value on near-perfect coherence, and a few stale integer rows move to a decisively stronger integer the old pipeline had missed. No coin-flip changes among the other previously-integer tracks.
The second commit makes the library row follow re-analysis: applyAnalysisToTrack was fill-only, so the re-analyzed BPM (and the grid actually served to decks) would diverge forever from what the web UI, track details, and CDJ menus display. BPM now updates unless the user has overridden it (DetectedBPM snapshot); manual overrides survive and clear still restores detection. cacheVersion bumps to 27 so v26 libraries re-analyze once.
Verified headlessly: three synthetic-tempo unit tests (integer snaps exactly, genuine fractional survives, grid runs on the snapped period), a writeback unit test over all four row paths, and a full-library sweep. analysis/snapdiag_test.go is a new env-gated diagnostic that prints the coherence curve any track was decided on.
Hardware testing
Checklist
go build ./...,go vet ./..., andgo test ./...passgofmt -l .is cleanGPL-3.0-or-later)