From 9e7d50b38ecdffb05f1140890d31424dec932777 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 11 Oct 2026 15:55:25 -0400 Subject: [PATCH] fix: FLAC, Ogg and Opus tracks carry their MusicBrainz recording id from the tag: one reader in library gives the scanner, the backfills and the importer the same ids, and the next scan re-reads the library (#5637) Co-Authored-By: Claude Opus 5.5 --- internal/importer/inspect.go | 22 +-------- internal/library/mbidbackfill.go | 5 +- internal/library/mbids.go | 83 +++++++++++++++++--------------- internal/library/mbids_test.go | 62 ++++++++++++++++++++---- internal/library/scanner.go | 13 +++-- 5 files changed, 109 insertions(+), 76 deletions(-) diff --git a/internal/importer/inspect.go b/internal/importer/inspect.go index a2a5f863..50a9a050 100644 --- a/internal/importer/inspect.go +++ b/internal/importer/inspect.go @@ -10,7 +10,6 @@ import ( "strings" "github.com/dhowden/tag" - "github.com/dhowden/tag/mbz" "git.fabledsword.com/bvandeusen/minstrel/internal/acoustid" "git.fabledsword.com/bvandeusen/minstrel/internal/library" @@ -136,15 +135,8 @@ func readFile(ctx context.Context, root, path string) File { f.Title, f.Artist, f.AlbumArtist, f.Album = m.Title(), m.Artist(), m.AlbumArtist(), m.Album() f.Track, _ = m.Track() f.Disc, _ = m.Disc() - info := mbz.Extract(m) - f.GroupMBID = clean(info.Get(mbz.ReleaseGroup)) - f.ReleaseMBID = clean(info.Get(mbz.Album)) - f.RecordingMBID = clean(info.Get(mbz.Recording)) - if f.RecordingMBID == "" && m.Format() == tag.VORBIS { - // Picard's Vorbis key for the recording is MUSICBRAINZ_TRACKID, which - // mbz files under its release-track name. - f.RecordingMBID = clean(info.Get(mbz.Track)) - } + ids := library.ReadTagMBIDs(m) + f.GroupMBID, f.ReleaseMBID, f.RecordingMBID = ids.ReleaseGroup, ids.Release, ids.Recording return f } @@ -155,16 +147,6 @@ func rel(root, path string) string { return filepath.Base(path) } -// clean takes the first of NUL-separated values: ID3v2.4 multi-values. -func clean(s string) string { - for _, part := range strings.Split(s, "\x00") { - if t := strings.TrimSpace(part); t != "" { - return t - } - } - return "" -} - // inspect reads every audio file under root and asks the printer about each. // fingerprinted reports whether the printer answered at all. func inspect(ctx context.Context, root string, p Printer) (files []File, fingerprinted bool, err error) { diff --git a/internal/library/mbidbackfill.go b/internal/library/mbidbackfill.go index 5d79ff96..e804f167 100644 --- a/internal/library/mbidbackfill.go +++ b/internal/library/mbidbackfill.go @@ -165,7 +165,8 @@ func readMBIDsForFile(path string, logger *slog.Logger) (albumMBID, artistMBID s logger.Warn("mbid backfill: tag read failed", "path", path, "err", err) return "", "" } - return extractMBIDs(meta) + ids := ReadTagMBIDs(meta) + return ids.Release, ids.Artist } // BackfillTrackMBIDsResult tallies the track recording-MBID backfill. @@ -258,5 +259,5 @@ func readRecordingMBIDForFile(path string, logger *slog.Logger) string { logger.Warn("track mbid backfill: tag read failed", "path", path, "err", err) return "" } - return extractRecordingMBID(meta) + return ReadTagMBIDs(meta).Recording } diff --git a/internal/library/mbids.go b/internal/library/mbids.go index f6ef768e..1a9c61bb 100644 --- a/internal/library/mbids.go +++ b/internal/library/mbids.go @@ -7,54 +7,57 @@ import ( "github.com/dhowden/tag/mbz" ) -// extractMBIDs reads MusicBrainz album and artist IDs from a tag.Metadata. +// TagMBIDs is what a file's MusicBrainz Picard tags name. Empty means the +// tag is absent. +type TagMBIDs struct { + // Release is musicbrainz_albumid: one edition, what albums.mbid stores. + Release string + Artist string + // ReleaseGroup is musicbrainz_releasegroupid. Wants name albums by + // release group, so re-acquisition and request completion key on it + // (#5241). + ReleaseGroup string + // Recording is what tracks.mbid stores and the ListenBrainz Labs + // similar-recordings API keys on. + Recording string +} + +// ReadTagMBIDs reads a file's MusicBrainz IDs, for the scanner, the MBID +// backfills and the importer alike. // // Delegates to dhowden/tag/mbz, which knows the per-format Raw() conventions: -// - Vorbis (FLAC/OGG) keys are lowercased by the parser. +// - Vorbis (FLAC/OGG/Opus) keys are lowercased by the parser. // - ID3v2 (MP3) TXXX frames are stored under "TXXX"/"TXXX_N" with // a *tag.Comm value whose Description holds the Picard tag name. // - MP4 (M4A) freeform iTunes atoms are stored under their bare sub-name. // -// Then sanitizes: dhowden's readTextWithDescrFrame (used for ID3v2 TXXX) -// does NOT strip the trailing/embedded NUL bytes that ID3v2.4 uses as -// frame terminator and multi-value separator — so the Text often comes -// out as "uuid\x00" or "uuid1\x00uuid2". Postgres rejects NULs in text -// columns (SQLSTATE 22021). We split on NUL and take the first non-empty -// segment — Minstrel uses the primary release artist for MBCAA lookup, -// so first-wins matches the original semantics. -func extractMBIDs(m tag.Metadata) (albumMBID, artistMBID string) { +// mbz maps keys by name alone, and Picard names the recording differently +// in Vorbis: MUSICBRAINZ_TRACKID there is the recording (the release track +// is MUSICBRAINZ_RELEASETRACKID), so mbz files it under mbz.Track, its name +// for the release track. Read mbz.Recording alone and every FLAC has no +// recording id (#5637). +func ReadTagMBIDs(m tag.Metadata) TagMBIDs { info := mbz.Extract(m) - return cleanMBID(info.Get(mbz.Album)), cleanMBID(info.Get(mbz.Artist)) -} - -// extractRecordingMBID reads the MusicBrainz *recording* ID — Picard's -// musicbrainz_recordingid, surfaced by dhowden/tag as mbz.Recording -// (tag display name "MusicBrainz Track Id"). This is the id the -// ListenBrainz Labs similar-recordings API keys on and what tracks.mbid -// stores. -// -// Deliberately NOT mbz.Track ("MusicBrainz Release Track Id"): that is -// per-release-track, whereas similarity is per-recording. Kept separate -// from extractMBIDs so its existing (album, artist) signature and unit -// tests stay untouched. -func extractRecordingMBID(m tag.Metadata) string { - return cleanMBID(mbz.Extract(m).Get(mbz.Recording)) -} - -// extractReleaseGroupMBID reads the MusicBrainz *release-group* ID — -// Picard's musicbrainz_releasegroupid, surfaced by dhowden/tag as -// mbz.ReleaseGroup. mbz.Album (what albums.mbid stores) is the release: one -// edition. Wants name albums by release group, so re-acquisition and -// request completion key on this one (#5241). Separate from extractMBIDs for -// the same reason extractRecordingMBID is. -func extractReleaseGroupMBID(m tag.Metadata) string { - return cleanMBID(mbz.Extract(m).Get(mbz.ReleaseGroup)) -} - -func cleanMBID(s string) string { - if s == "" { - return "" + ids := TagMBIDs{ + Release: cleanMBID(info.Get(mbz.Album)), + Artist: cleanMBID(info.Get(mbz.Artist)), + ReleaseGroup: cleanMBID(info.Get(mbz.ReleaseGroup)), + Recording: cleanMBID(info.Get(mbz.Recording)), } + if ids.Recording == "" && m.Format() == tag.VORBIS { + ids.Recording = cleanMBID(info.Get(mbz.Track)) + } + return ids +} + +// cleanMBID takes the first non-empty NUL-separated value. dhowden's +// readTextWithDescrFrame (used for ID3v2 TXXX) does NOT strip the +// trailing/embedded NUL bytes that ID3v2.4 uses as frame terminator and +// multi-value separator — so the Text often comes out as "uuid\x00" or +// "uuid1\x00uuid2". Postgres rejects NULs in text columns (SQLSTATE 22021). +// Minstrel uses the primary release artist for MBCAA lookup, so first-wins +// matches the original semantics. +func cleanMBID(s string) string { for _, part := range strings.Split(s, "\x00") { if t := strings.TrimSpace(part); t != "" { return t diff --git a/internal/library/mbids_test.go b/internal/library/mbids_test.go index db5225dc..8dafd64f 100644 --- a/internal/library/mbids_test.go +++ b/internal/library/mbids_test.go @@ -40,7 +40,8 @@ func TestExtractMBIDs_Vorbis(t *testing.T) { "musicbrainz_artistid": "def", }, } - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "abc" || ar != "def" { t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar) } @@ -62,7 +63,8 @@ func TestExtractMBIDs_ID3v2(t *testing.T) { }, }, } - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "abc-123" || ar != "def-456" { t.Errorf("got (%q, %q), want (\"abc-123\", \"def-456\")", a, ar) } @@ -78,7 +80,8 @@ func TestExtractMBIDs_MP4(t *testing.T) { "MusicBrainz Artist Id": "def", }, } - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "abc" || ar != "def" { t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar) } @@ -86,7 +89,8 @@ func TestExtractMBIDs_MP4(t *testing.T) { func TestExtractMBIDs_MissingTagsReturnEmpty(t *testing.T) { m := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{}} - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "" || ar != "" { t.Errorf("got (%q, %q), want empty", a, ar) } @@ -104,7 +108,8 @@ func TestExtractMBIDs_VorbisUppercaseKeysMiss(t *testing.T) { "MUSICBRAINZ_ARTISTID": "wrong-case", }, } - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "" || ar != "" { t.Errorf("uppercase Vorbis keys must not match dhowden output; got (%q, %q)", a, ar) } @@ -127,7 +132,8 @@ func TestExtractMBIDs_ID3v2StripsTrailingNUL(t *testing.T) { }, }, } - a, ar := extractMBIDs(m) + ids := ReadTagMBIDs(m) + a, ar := ids.Release, ids.Artist if a != "abc-123" || ar != "def-456" { t.Errorf("got (%q, %q), want trailing NUL stripped", a, ar) } @@ -146,7 +152,7 @@ func TestExtractMBIDs_ID3v2MultiValueTakesFirst(t *testing.T) { }, }, } - _, ar := extractMBIDs(m) + ar := ReadTagMBIDs(m).Artist if ar != "primary-mbid" { t.Errorf("got %q, want %q", ar, "primary-mbid") } @@ -186,17 +192,53 @@ func TestExtractReleaseGroupMBID(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - if got := extractReleaseGroupMBID(c.meta); got != "group-1" { + if got := ReadTagMBIDs(c.meta).ReleaseGroup; got != "group-1" { t.Errorf("release group = %q, want group-1", got) } - if album, _ := extractMBIDs(c.meta); album != "release-1" { + if album := ReadTagMBIDs(c.meta).Release; album != "release-1" { t.Errorf("album (release) = %q, want release-1", album) } }) } none := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"musicbrainz_albumid": "release-1"}} - if got := extractReleaseGroupMBID(none); got != "" { + if got := ReadTagMBIDs(none).ReleaseGroup; got != "" { t.Errorf("untagged release group = %q, want empty", got) } } + +// #5637: each format's recording id lands in Recording, and the release-track +// id never does. Picard calls the recording MUSICBRAINZ_TRACKID in Vorbis +// comments, the name mbz gives the release track. +func TestReadTagMBIDs_Recording(t *testing.T) { + cases := []struct { + name string + meta stubMeta + }{ + {"vorbis", stubMeta{format: tag.VORBIS, raw: map[string]interface{}{ + "musicbrainz_trackid": "rec-1", + "musicbrainz_releasetrackid": "reltrack-1", + }}}, + {"id3v2 ufid", stubMeta{format: tag.ID3v2_4, raw: map[string]interface{}{ + "UFID": &tag.UFID{Provider: "http://musicbrainz.org", Identifier: []byte("rec-1")}, + "TXXX_0": &tag.Comm{Description: "MusicBrainz Release Track Id", Text: "reltrack-1\x00"}, + }}}, + {"mp4", stubMeta{format: tag.MP4, raw: map[string]interface{}{ + "MusicBrainz Track Id": "rec-1", + "MusicBrainz Release Track Id": "reltrack-1", + }}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := ReadTagMBIDs(c.meta).Recording; got != "rec-1" { + t.Errorf("recording = %q, want rec-1", got) + } + }) + } + + // A Vorbis file carrying only the release-track id has no recording id. + only := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"musicbrainz_releasetrackid": "reltrack-1"}} + if got := ReadTagMBIDs(only).Recording; got != "" { + t.Errorf("release track only: recording = %q, want empty", got) + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 53b25e9e..b3670dde 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -64,7 +64,12 @@ var audioExtensions = map[string]bool{ // and MP4 (#2500). dhowden/tag kept only the last, and 3,658 of the // operator's 4,092 FLACs declare more than one, so most of their genres // were never stored. -const tagReadVersion int16 = 4 +// 5: the recording id is read from FLAC, Ogg Vorbis and Opus (#5637). Picard +// calls it MUSICBRAINZ_TRACKID there and dhowden/tag files that under the +// release-track name, so every Vorbis-tagged track came in without its +// tag id and waited on an AcoustID lookup. A re-read stores the tag id, +// over a looked-up one (D4). +const tagReadVersion int16 = 5 type Stats struct { Scanned int `json:"scanned"` @@ -272,9 +277,9 @@ func (s *Scanner) scanFile( if err != nil { return pgtype.UUID{}, false, fmt.Errorf("tag read: %w", err) } - albumMBID, artistMBID := extractMBIDs(meta) - recordingMBID := extractRecordingMBID(meta) - releaseGroupMBID := extractReleaseGroupMBID(meta) + ids := ReadTagMBIDs(meta) + albumMBID, artistMBID := ids.Release, ids.Artist + recordingMBID, releaseGroupMBID := ids.Recording, ids.ReleaseGroup artistName := meta.Artist() if artistName == "" {