Skip to content

CMRT 27: Pipeline: Wire MBID -> SimHash -> overlap -> quality -> group assignment - #36

Open
Swarnadip-Kar wants to merge 2 commits into
cmrt-matcher-corefrom
cmrt-pipeline-grouping
Open

CMRT 27: Pipeline: Wire MBID -> SimHash -> overlap -> quality -> group assignment#36
Swarnadip-Kar wants to merge 2 commits into
cmrt-matcher-corefrom
cmrt-pipeline-grouping

Conversation

@Swarnadip-Kar

Copy link
Copy Markdown
Owner

Wires CMRT 24-26 together into something that actually runs: a new
CmrtGroupingService that AcoustIdWorker calls after every AcoustID lookup,
plus the two DAO methods it turned out to need that didn't exist yet.

CmrtGroupingService::processTrack(trackId, musicbrainzRecordingId)

  1. Load fingerprint_metadata for trackId; no-op if missing/invalid.
  2. Score quality via QualityScorer.
  3. Candidate groups: getGroupsForMbid() if an MBID was found, else
    getAllGroupCandidates() as a fallback.
  4. Per candidate: SimHash pre-filter, then (only if that passes) full
    FingerprintMatcher::compare() against the candidate's canonical .chroma.
  5. Best match across all candidates that cleared the threshold (not just
    the first one): compare quality scores. Higher score -> replaceCanonical();
    otherwise -> assignToExistingGroup() as a regular member.
  6. No match: createNewGroup() -- this track is the first seen for this
    mastering.

New DAO methods (TrackFingerprintDao)

  • getTrackQualityInfo(trackId) -> std::optional
  • updateCanonicalTrack(groupId, newCanonicalTrackId)
  • updateMemberOffset(trackId, offsetFromCanonical)

The latter two didn't exist before this PR -- addCmrtMember() only inserts,
and nothing previously updated canonical_track_id outside
clearFingerprintData()'s own inline SQL.

Wiring into AcoustIdWorker

m_pGroupingService is constructed right after m_pFingerprintDao in doRun()
(and reset right before it, in the reverse order) since it holds a
reference into that DAO, not ownership. processJob() calls
m_pGroupingService->processTrack() after the match-found and
no-match-found outcomes, gated behind kCmrtAutoGroupingEnabledConfigKey
(new preference key, defaults to true).

Notes for reviewers

  • getTrackQualityInfo() returns std::optional, not this DAO's usual
    std::unique_ptr -- flagged in case the inconsistency within
    TrackFingerprintDao itself is unwanted; happy to change it.
  • The cache-hit path in processJob() (Step 2) does not call
    processTrack() yet -- left as a TODO(XXX) with reasoning in the comment,
    not silently dropped.
  • replaceCanonical()'s "recalculate everyone else's offset" loop reads one
    .chroma file per remaining group member. Fine for expected group sizes;
    flagged as a possible future hotspot, not optimized preemptively.

Depends on: cmrt-matcher-core (#35)

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 27872458852

Coverage decreased (-0.1%) to 30.317%

Details

  • Coverage decreased (-0.1%) from the base build.
  • Patch coverage: 239 uncovered changes across 3 files (0 of 239 lines covered, 0.0%).
  • 2 coverage regressions across 1 file.

Uncovered Changes

File Changed Covered %
src/musicbrainz/cmrtgroupingservice.cpp 165 0 0.0%
src/library/dao/trackfingerprintdao.cpp 68 0 0.0%
src/musicbrainz/acoustidworker.cpp 6 0 0.0%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File Lines Losing Coverage Coverage
src/library/dao/trackfingerprintdao.cpp 2 0.81%

Coverage Stats

Coverage Status
Relevant Lines: 116694
Covered Lines: 35378
Line Coverage: 30.32%
Coverage Strength: 59035.14 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants