From 80ecc2f7e79c2c5e4443f421d8265dbaf8f55241 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 20 Oct 2026 11:49:44 -0400 Subject: [PATCH] fix: imported FLAC, Ogg and Opus files keep their track and disc numbers (#6171) The importer wrote track=N/M for every format, which ffmpeg stores in a Vorbis comment as TRACKNUMBER=3/20; dhowden/tag reads a Vorbis TRACKNUMBER with a bare Atoi, so it came back 0. All 245 tracks imported since 2026-10-10 (Humanz, Plastic Beach, the Gorillaz super deluxe and twelve more albums) have no track or disc number, and their album pages have no order. - The importer writes a Vorbis track and disc number alone, with TRACKTOTAL and DISCTOTAL, as Picard does. ID3 and MP4 keep N/M. - ReadTrackAndDisc takes the number before a slash when the Vorbis reader returns 0, for the scanner and for the import's own check of a download. - tagReadVersion 6 re-reads the files already held (tag reads only). Co-Authored-By: Claude Opus 5.5 --- internal/importer/inspect.go | 3 +-- internal/importer/place.go | 18 +++++++++++++- internal/importer/verify_test.go | 12 +++++++++- internal/library/mbids_test.go | 19 +++++++++++++++ internal/library/scanner.go | 9 ++++--- internal/library/tracknumber.go | 40 ++++++++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 internal/library/tracknumber.go diff --git a/internal/importer/inspect.go b/internal/importer/inspect.go index 50a9a050..e13359a2 100644 --- a/internal/importer/inspect.go +++ b/internal/importer/inspect.go @@ -133,8 +133,7 @@ func readFile(ctx context.Context, root, path string) File { return f } f.Title, f.Artist, f.AlbumArtist, f.Album = m.Title(), m.Artist(), m.AlbumArtist(), m.Album() - f.Track, _ = m.Track() - f.Disc, _ = m.Disc() + f.Track, f.Disc = library.ReadTrackAndDisc(m) ids := library.ReadTagMBIDs(m) f.GroupMBID, f.ReleaseMBID, f.RecordingMBID = ids.ReleaseGroup, ids.Release, ids.Recording return f diff --git a/internal/importer/place.go b/internal/importer/place.go index e2e3d5c5..03d6a2a9 100644 --- a/internal/importer/place.go +++ b/internal/importer/place.go @@ -38,16 +38,32 @@ type Tags struct { // ID3 takes Picard's TXXX descriptions, which ffmpeg writes for any key it // has no frame for. MP4's freeform atoms ffmpeg cannot write, so an M4A // gets the plain names only. +// +// A Vorbis track or disc number is the number alone, its count in +// TRACKTOTAL and DISCTOTAL, as Picard writes them: dhowden/tag reads +// TRACKNUMBER "3/20" as 0, and every album imported before #6171 lost its +// order that way. ID3 and MP4 take "3/20", which their readers parse. func (t Tags) args(ext string) []string { + vorbis := ext == ".flac" || ext == ".ogg" || ext == ".opus" + track, disc := ofN(t.Track, t.Tracks), ofN(t.Disc, t.Discs) + if vorbis { + track, disc = ofN(t.Track, 0), ofN(t.Disc, 0) + } kv := [][2]string{ {"title", t.Title}, {"artist", t.Artist}, {"album", t.Album}, {"album_artist", t.AlbumArtist}, - {"track", ofN(t.Track, t.Tracks)}, {"disc", ofN(t.Disc, t.Discs)}, + {"track", track}, {"disc", disc}, } if t.Year > 0 { kv = append(kv, [2]string{"date", strconv.Itoa(t.Year)}) } switch ext { case ".flac", ".ogg", ".opus": + if t.Track > 0 && t.Tracks > 0 { + kv = append(kv, [2]string{"TRACKTOTAL", strconv.Itoa(t.Tracks)}) + } + if t.Disc > 0 && t.Discs > 0 { + kv = append(kv, [2]string{"DISCTOTAL", strconv.Itoa(t.Discs)}) + } kv = append(kv, [2]string{"MUSICBRAINZ_ALBUMID", t.ReleaseMBID}, [2]string{"MUSICBRAINZ_RELEASEGROUPID", t.GroupMBID}, diff --git a/internal/importer/verify_test.go b/internal/importer/verify_test.go index 82565a79..21909098 100644 --- a/internal/importer/verify_test.go +++ b/internal/importer/verify_test.go @@ -212,11 +212,21 @@ func TestTagsArgs(t *testing.T) { tg := Tags{Title: "Get Lucky", Artist: "Daft Punk", Track: 8, Tracks: 13, Disc: 1, Discs: 1, Year: 2013, RecordingMBID: "rec-08", GroupMBID: "rg-ram"} flac := strings.Join(tg.args(".flac"), " ") - for _, s := range []string{"title=Get Lucky", "track=8/13", "date=2013", "MUSICBRAINZ_TRACKID=rec-08", "MUSICBRAINZ_RELEASEGROUPID=rg-ram"} { + for _, s := range []string{ + "title=Get Lucky", "track=8 ", "TRACKTOTAL=13", "disc=1 ", "DISCTOTAL=1", "date=2013", + "MUSICBRAINZ_TRACKID=rec-08", "MUSICBRAINZ_RELEASEGROUPID=rg-ram", + } { if !strings.Contains(flac, s) { t.Errorf("flac args lack %q: %s", s, flac) } } + // dhowden/tag reads a Vorbis TRACKNUMBER of "8/13" as 0 (#6171). + if strings.Contains(flac, "/13") { + t.Errorf("flac track number carries its count: %s", flac) + } + if mp3 := strings.Join(tg.args(".mp3"), " "); !strings.Contains(mp3, "track=8/13") { + t.Errorf("mp3 track should be 8/13, which ID3 readers parse: %s", mp3) + } if strings.Contains(flac, "MUSICBRAINZ_ALBUMID") { t.Errorf("an empty release id was written: %s", flac) } diff --git a/internal/library/mbids_test.go b/internal/library/mbids_test.go index 8dafd64f..95acc2cf 100644 --- a/internal/library/mbids_test.go +++ b/internal/library/mbids_test.go @@ -242,3 +242,22 @@ func TestReadTagMBIDs_Recording(t *testing.T) { t.Errorf("release track only: recording = %q, want empty", got) } } + +// A Vorbis TRACKNUMBER of "3/20", as Minstrel's importer wrote it, is track 3 +// (#6171): dhowden/tag reads it as 0. +func TestReadTrackAndDisc(t *testing.T) { + for _, tc := range []struct { + name string + m stubMeta + track, disc int + }{ + {"vorbis with counts", stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"tracknumber": "3/20", "discnumber": "2/2"}}, 3, 2}, + {"vorbis bare", stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"tracknumber": " 7 "}}, 7, 0}, + {"vorbis junk", stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"tracknumber": "A1", "discnumber": "-1"}}, 0, 0}, + {"id3 is the library's own reading", stubMeta{format: tag.ID3v2_4, raw: map[string]interface{}{"tracknumber": "3/20"}}, 0, 0}, + } { + if track, disc := ReadTrackAndDisc(tc.m); track != tc.track || disc != tc.disc { + t.Errorf("%s: got track %d disc %d, want %d and %d", tc.name, track, disc, tc.track, tc.disc) + } + } +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 7b1fd91a..e61f2abb 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -70,7 +70,11 @@ var audioExtensions = map[string]bool{ // 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 +// 6: a Vorbis TRACKNUMBER or DISCNUMBER written as "3/20" is read (#6171). +// dhowden/tag reads it as 0, and Minstrel's own importer wrote it that +// way, so all 245 FLAC tracks it imported came in with no track or disc +// number and their albums lost their order. +const tagReadVersion int16 = 6 type Stats struct { Scanned int `json:"scanned"` @@ -316,8 +320,7 @@ func (s *Scanner) scanFile( return pgtype.UUID{}, false, fmt.Errorf("album: %w", err) } - trackNum, _ := meta.Track() - discNum, _ := meta.Disc() + trackNum, discNum := ReadTrackAndDisc(meta) // An unchanged file being re-read only to refresh tag-derived columns // doesn't need another ffprobe: the stored duration is still accurate, and diff --git a/internal/library/tracknumber.go b/internal/library/tracknumber.go new file mode 100644 index 00000000..3b99240a --- /dev/null +++ b/internal/library/tracknumber.go @@ -0,0 +1,40 @@ +package library + +import ( + "strconv" + "strings" + + "github.com/dhowden/tag" +) + +// ReadTrackAndDisc reads a file's track and disc numbers. dhowden/tag reads a +// Vorbis TRACKNUMBER or DISCNUMBER only as a bare number, but taggers also +// write "3/20" there (Minstrel's own importer did until #6171), and that +// reads as 0. For Vorbis the number before the slash is taken instead. +func ReadTrackAndDisc(m tag.Metadata) (track, disc int) { + track, _ = m.Track() + disc, _ = m.Disc() + if m.Format() != tag.VORBIS { + return track, disc + } + raw := m.Raw() + if track == 0 { + track = leadingNumber(raw["tracknumber"]) + } + if disc == 0 { + disc = leadingNumber(raw["discnumber"]) + } + return track, disc +} + +// leadingNumber is the number in a tag value of "3" or "3/20"; 0 for +// anything else. +func leadingNumber(v any) int { + s, _ := v.(string) + before, _, _ := strings.Cut(s, "/") + n, err := strconv.Atoi(strings.TrimSpace(before)) + if err != nil || n < 0 { + return 0 + } + return n +}