From 49b8cfc2611691ff2f23353d3013a5c3fa5194af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Fri, 11 Apr 2025 23:39:57 -0400 Subject: [PATCH] fix(artwork): always select the coverArt of the first disc in multi-disc albums (#3950) * feat(artwork): sort image files for consistent cover art selection Signed-off-by: Deluan * feat(artwork): improve album cover art selection by considering disc numbers Signed-off-by: Deluan --------- Signed-off-by: Deluan --- core/artwork/reader_album.go | 6 ++ core/artwork/reader_album_test.go | 76 +++++++++++++++++++++++++ model/mediafile.go | 31 +++++++++- model/mediafile_test.go | 95 +++++++++++++++++++++++++++++++ 4 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 core/artwork/reader_album_test.go diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index a320533ed..55d8b4352 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "path/filepath" + "slices" "strings" "time" @@ -113,5 +114,10 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo imgFiles = append(imgFiles, filepath.Join(path, img)) } } + + // Sort image files to ensure consistent selection of cover art + // This prioritizes files from lower-numbered disc folders by sorting the paths + slices.Sort(imgFiles) + return paths, imgFiles, &updatedAt, nil } diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go new file mode 100644 index 000000000..2665632b9 --- /dev/null +++ b/core/artwork/reader_album_test.go @@ -0,0 +1,76 @@ +package artwork + +import ( + "context" + "path/filepath" + "time" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Album Artwork Reader", func() { + Describe("loadAlbumFoldersPaths", func() { + var ( + ctx context.Context + ds *fakeDataStore + repo *fakeFolderRepo + album model.Album + now time.Time + expectedAt time.Time + ) + + BeforeEach(func() { + ctx = context.Background() + now = time.Now().Truncate(time.Second) + expectedAt = now.Add(5 * time.Minute) + + // Set up the test folders with image files + repo = &fakeFolderRepo{ + result: []model.Folder{ + { + Path: "Artist/Album/Disc1", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg", "back.jpg"}, + }, + { + Path: "Artist/Album/Disc2", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + Path: "Artist/Album/Disc10", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + }, + err: nil, + } + ds = &fakeDataStore{ + folderRepo: repo, + } + album = model.Album{ + ID: "album1", + Name: "Album", + FolderIDs: []string{"folder1", "folder2", "folder3"}, + } + }) + + It("returns sorted image files", func() { + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + + // Check that image files are sorted alphabetically + Expect(imgFiles).To(HaveLen(4)) + + // The files should be sorted by full path + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/Disc1/back.jpg"))) + Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/Disc1/cover.jpg"))) + Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Disc10/cover.jpg"))) + Expect(imgFiles[3]).To(Equal(filepath.FromSlash("Artist/Album/Disc2/cover.jpg"))) + }) + }) +}) diff --git a/model/mediafile.go b/model/mediafile.go index 896442436..354c5ea47 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -183,6 +183,8 @@ func (mfs MediaFiles) ToAlbum() Album { tags := make(TagList, 0, len(mfs[0].Tags)*len(mfs)) a.Missing = true + embedArtPath := "" + embedArtDisc := 0 for _, m := range mfs { // We assume these attributes are all the same for all songs in an album a.ID = m.AlbumID @@ -211,15 +213,15 @@ func (mfs MediaFiles) ToAlbum() Album { comments = append(comments, m.Comment) mbzAlbumIds = append(mbzAlbumIds, m.MbzAlbumID) mbzReleaseGroupIds = append(mbzReleaseGroupIds, m.MbzReleaseGroupID) - if m.HasCoverArt && a.EmbedArtPath == "" { - a.EmbedArtPath = m.Path - } if m.DiscNumber > 0 { a.Discs.Add(m.DiscNumber, m.DiscSubtitle) } tags = append(tags, m.Tags.FlattenAll()...) a.Participants.Merge(m.Participants) + // Find the MediaFile with cover art and the lowest disc number to use for album cover + embedArtPath, embedArtDisc = firstArtPath(embedArtPath, embedArtDisc, m) + if m.ExplicitStatus == "c" && a.ExplicitStatus != "e" { a.ExplicitStatus = "c" } else if m.ExplicitStatus == "e" { @@ -231,6 +233,7 @@ func (mfs MediaFiles) ToAlbum() Album { a.Missing = a.Missing && m.Missing } + a.EmbedArtPath = embedArtPath a.SetTags(tags) a.FolderIDs = slice.Unique(slice.Map(mfs, func(m MediaFile) string { return m.FolderID })) a.Date, _ = allOrNothing(dates) @@ -305,6 +308,28 @@ func fixAlbumArtist(a *Album) { } } +// firstArtPath determines which media file path should be used for album artwork +// based on disc number (preferring lower disc numbers) and path (for consistency) +func firstArtPath(currentPath string, currentDisc int, m MediaFile) (string, int) { + if !m.HasCoverArt { + return currentPath, currentDisc + } + + // If current has no disc number (currentDisc == 0) or new file has lower disc number + if currentDisc == 0 || (m.DiscNumber < currentDisc && m.DiscNumber > 0) { + return m.Path, m.DiscNumber + } + + // If disc numbers are equal, use path for ordering + if m.DiscNumber == currentDisc { + if m.Path < currentPath || currentPath == "" { + return m.Path, m.DiscNumber + } + } + + return currentPath, currentDisc +} + type MediaFileCursor iter.Seq2[MediaFile, error] type MediaFileRepository interface { diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 74f5e5264..7f583cf75 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -305,6 +305,101 @@ var _ = Describe("MediaFiles", func() { }) }) }) + Context("Album Art", func() { + When("we have media files with cover art from multiple discs", func() { + BeforeEach(func() { + mfs = MediaFiles{ + { + Path: "Artist/Album/Disc2/01.mp3", + HasCoverArt: true, + DiscNumber: 2, + }, + { + Path: "Artist/Album/Disc1/01.mp3", + HasCoverArt: true, + DiscNumber: 1, + }, + { + Path: "Artist/Album/Disc3/01.mp3", + HasCoverArt: true, + DiscNumber: 3, + }, + } + }) + It("selects the cover art from the lowest disc number", func() { + album := mfs.ToAlbum() + Expect(album.EmbedArtPath).To(Equal("Artist/Album/Disc1/01.mp3")) + }) + }) + + When("we have media files with cover art from the same disc number", func() { + BeforeEach(func() { + mfs = MediaFiles{ + { + Path: "Artist/Album/Disc1/02.mp3", + HasCoverArt: true, + DiscNumber: 1, + }, + { + Path: "Artist/Album/Disc1/01.mp3", + HasCoverArt: true, + DiscNumber: 1, + }, + } + }) + It("selects the cover art with the lowest path alphabetically", func() { + album := mfs.ToAlbum() + Expect(album.EmbedArtPath).To(Equal("Artist/Album/Disc1/01.mp3")) + }) + }) + + When("we have media files with some missing cover art", func() { + BeforeEach(func() { + mfs = MediaFiles{ + { + Path: "Artist/Album/Disc1/01.mp3", + HasCoverArt: false, + DiscNumber: 1, + }, + { + Path: "Artist/Album/Disc2/01.mp3", + HasCoverArt: true, + DiscNumber: 2, + }, + } + }) + It("selects the file with cover art even if from a higher disc number", func() { + album := mfs.ToAlbum() + Expect(album.EmbedArtPath).To(Equal("Artist/Album/Disc2/01.mp3")) + }) + }) + + When("we have media files with path names that don't correlate with disc numbers", func() { + BeforeEach(func() { + mfs = MediaFiles{ + { + Path: "Artist/Album/file-z.mp3", // Path would be sorted last alphabetically + HasCoverArt: true, + DiscNumber: 1, // But it has lowest disc number + }, + { + Path: "Artist/Album/file-a.mp3", // Path would be sorted first alphabetically + HasCoverArt: true, + DiscNumber: 2, // But it has higher disc number + }, + { + Path: "Artist/Album/file-m.mp3", + HasCoverArt: true, + DiscNumber: 3, + }, + } + }) + It("selects the cover art from the lowest disc number regardless of path", func() { + album := mfs.ToAlbum() + Expect(album.EmbedArtPath).To(Equal("Artist/Album/file-z.mp3")) + }) + }) + }) }) }) })