CMRT 29: Threshold pref, cache-hit grouping fix, reanalyze fingerprint reset, lookup timestamp fix, redundant quality score fixes - #39
Open
Swarnadip-Kar wants to merge 7 commits into
Conversation
Coverage Report for CI Build 28433425207Coverage decreased (-4.2%) to 30.301%Details
Uncovered Changes
Coverage Regressions5 previously-covered lines in 5 files lost coverage.
Coverage Stats
💛 - Coveralls |
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
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.
Five small, independent fixes bundled into one PR
A — Match threshold preference
FingerprintMatcher's 90% match cutoff was a compile-time constant. It's now
read from a new Preferences > Fingerprint Analysis spin box
(kCmrtMatchThresholdConfigKey, default 90%, stored as a 0.0-1.0 fraction).
FingerprintMatcher::compare() takes the threshold as a parameter; it has no
UserSettings dependency of its own. CmrtGroupingService re-reads the pref on
every processTrack() call so a change takes effect without restarting Mixxx.
B — Cache hits never triggered grouping
processJob()'s cache-hit branch wrote the MBID to the library table but
skipped CmrtGroupingService::processTrack() entirely -- only fresh network
lookups triggered grouping. Exact-duplicate files whose second copy resolved
via acoustid_cache would get tagged but never grouped. Fixed by calling
processTrack() in both branches.
C — Reanalyze silently skipped fingerprint data
AnalyzerChromaprint::initialize() skips any track with a still-valid
fingerprint_metadata row -- by design, so normal analysis doesn't redo
fingerprinting. But Reanalyze (all three variants) never cleared that row, so
reanalyzing a track whose audio had actually changed left the stale
fingerprint, .chroma file, and CMRT group membership untouched. Extracted the
existing fingerprint-clear loop into clearFingerprintDataForSelection() and
call it from all three Reanalyze slots (and reused it in slotClearFingerprint
and slotClearAllMetadata, removing the prior duplication).
D — acoustid_lookup_at never written
Column has existed in the schema since revision 41; TrackDAO::updateAcoustIdResult()
never included it in the UPDATE. Now stamped with the current Unix timestamp
inside the DAO, matching how TrackFingerprintDao already handles
computed_at/queued_at -- no new parameter, no call-site changes.
E — Add TrackFingerprintDao::getMemberQualityScore()
cmrt_members.quality_score is written once per track, when it joins a group
as a member or canonical (addCmrtMember(), called from createNewGroup(),
assignToExistingGroup(), and replaceCanonical()).
Add getMemberQualityScore(trackId), a structural twin of
updateMemberOffset(): same guard clause, same WHERE track_id=:track_id,
SELECT instead of UPDATE. Returns the same -1.0 "not yet scored" sentinel
CmrtMember::qualityScore and addCmrtMember() already use.
F — replaceCanonical() re-scoring a track it was already given the score for
handleMatchedCandidate() scores the new track once to decide whether it should
replace the existing canonical. When it does, replaceCanonical() was re-reading
TrackQualityInfo and re-running QualityScorer on that same track to fill in its
CmrtMember row -- same input, same output, computed twice for no reason.
replaceCanonical() now takes newCanonicalQualityScore as a parameter and the
caller passes through the value it already has, matching the pattern
assignToExistingGroup() already used.
G — handleMatchedCandidate() re-scoring the canonical on every match into its group
handleMatchedCandidate() re-derived the existing
canonical's score from TrackQualityInfo on every new track that matched into
that group, instead of reusing the score already stored in
cmrt_members.quality_score from when that track became canonical. Added
TrackFingerprintDao::getMemberQualityScore() (DAO commit) and switched
handleMatchedCandidate() to use it, falling back to the old recompute path
only if the canonical somehow has no stored score (call-site commit).
Depends on: cmrt-canonical-reelection (#37)