From 2f5b2b51359db24573d965fe4b114a45c72d3b6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Mon, 16 Mar 2026 18:08:39 -0400 Subject: [PATCH] fix(artwork): fallback mediafile cover art to disc artwork before album (#5216) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(artwork): fallback mediafile cover art to disc artwork before album Changed the mediafile cover art fallback chain to go through disc artwork before album artwork (mediafile → disc → album). Previously, mediafiles without embedded art fell back directly to album cover, bypassing any disc-specific artwork. Renamed AlbumCoverArtID() to DiscCoverArtID() to encapsulate the disc-vs-album decision in a single method, used by both CoverArtID() and the mediafile artwork reader. Signed-off-by: Deluan * fix(artwork): fix cache invalidation for mediafile and album cover art Include imagesUpdatedAt from album folders in the mediafile artwork reader's cache key, so that when a cover image file changes on disk (without audio metadata changes) the mediafile cache properly invalidates. Also include CoverArtPriority unconditionally in the album artwork reader's cache key hash, so that changing the priority order with external services disabled correctly invalidates the album cache. * fix(artwork): skip disc artwork resolution for single-disc albums Single-disc albums with DiscNumber=1 were unnecessarily routed through discArtworkReader, which does extra DB queries only to fall through to album art anyway. Now only multi-disc albums use the disc fallback path. * refactor(artwork): restore AlbumCoverArtID as a separate method Extract AlbumCoverArtID back out of DiscCoverArtID so the single-disc fallback path in reader_mediafile can reference it by name instead of inlining the artwork ID construction. --------- Signed-off-by: Deluan --- core/artwork/artwork_internal_test.go | 28 +++++++++++++++++++++++++-- core/artwork/reader_album.go | 5 +++-- core/artwork/reader_mediafile.go | 20 +++++++++++++++---- model/mediafile.go | 11 ++++++++++- model/mediafile_test.go | 17 ++++++++++++++-- 5 files changed, 70 insertions(+), 11 deletions(-) diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index 5ca32f40..4b235989 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -28,7 +28,7 @@ var _ = Describe("Artwork", func() { var ffmpeg *tests.MockFFmpeg var folderRepo *fakeFolderRepo ctx := log.NewContext(context.TODO()) - var alOnlyEmbed, alEmbedNotFound, alOnlyExternal, alExternalNotFound, alMultipleCovers model.Album + var alOnlyEmbed, alEmbedNotFound, alOnlyExternal, alExternalNotFound, alMultipleCovers, alSingleDisc model.Album var arMultipleCovers model.Artist var mfWithEmbed, mfAnotherWithEmbed, mfWithoutEmbed, mfCorruptedCover model.MediaFile @@ -44,8 +44,9 @@ var _ = Describe("Artwork", func() { } alOnlyEmbed = model.Album{ID: "222", Name: "Only embed", EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", FolderIDs: []string{"f1"}} alEmbedNotFound = model.Album{ID: "333", Name: "Embed not found", EmbedArtPath: "tests/fixtures/NON_EXISTENT.mp3", FolderIDs: []string{"f1"}} - alOnlyExternal = model.Album{ID: "444", Name: "Only external", FolderIDs: []string{"f1"}} + alOnlyExternal = model.Album{ID: "444", Name: "Only external", FolderIDs: []string{"f1"}, Discs: model.Discs{1: "", 2: ""}} alExternalNotFound = model.Album{ID: "555", Name: "External not found", FolderIDs: []string{"f2"}} + alSingleDisc = model.Album{ID: "888", Name: "Single disc", FolderIDs: []string{"f1"}, Discs: model.Discs{1: ""}} arMultipleCovers = model.Artist{ID: "777", Name: "All options"} alMultipleCovers = model.Album{ ID: "666", @@ -193,6 +194,7 @@ var _ = Describe("Artwork", func() { ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ alOnlyEmbed, alOnlyExternal, + alSingleDisc, }) ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ mfWithEmbed, @@ -236,6 +238,28 @@ var _ = Describe("Artwork", func() { Expect(err).ToNot(HaveOccurred()) Expect(path).To(Equal("al-444_0")) }) + It("falls back to disc cover art when media file has a disc number on a multi-disc album", func() { + mfWithDisc := model.MediaFile{ID: "46", Path: "tests/fixtures/test.ogg", AlbumID: "444", DiscNumber: 2} + Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfWithDisc)).To(Succeed()) + + aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfWithDisc.ID)) + Expect(err).ToNot(HaveOccurred()) + _, path, err := aw.Reader(ctx) + Expect(err).ToNot(HaveOccurred()) + // Should fall back to disc art, which itself falls back to album art + Expect(path).To(Equal("dc-444:2_0")) + }) + It("falls back to album cover art for single-disc albums even with a disc number", func() { + mfOnSingleDisc := model.MediaFile{ID: "47", Path: "tests/fixtures/test.ogg", AlbumID: "888", DiscNumber: 1} + Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfOnSingleDisc)).To(Succeed()) + + aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfOnSingleDisc.ID)) + Expect(err).ToNot(HaveOccurred()) + _, path, err := aw.Reader(ctx) + Expect(err).ToNot(HaveOccurred()) + // Single-disc album should skip disc art and go straight to album art + Expect(path).To(Equal("al-888_0")) + }) }) }) Describe("playlistArtworkReader", func() { diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 98a2105e..6de1d31d 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -59,10 +59,11 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar } func (a *albumArtworkReader) Key() string { - var hash [16]byte + hashInput := conf.Server.CoverArtPriority if conf.Server.EnableExternalServices { - hash = md5.Sum([]byte(conf.Server.Agents + conf.Server.CoverArtPriority)) + hashInput += conf.Server.Agents } + hash := md5.Sum([]byte(hashInput)) return fmt.Sprintf( "%s.%x.%t", a.cacheKey.Key(), diff --git a/core/artwork/reader_mediafile.go b/core/artwork/reader_mediafile.go index c72d9543..cf25c8f5 100644 --- a/core/artwork/reader_mediafile.go +++ b/core/artwork/reader_mediafile.go @@ -26,16 +26,22 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode if err != nil { return nil, err } + _, _, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, *al) + if err != nil { + return nil, err + } a := &mediafileArtworkReader{ a: artwork, mediafile: *mf, album: *al, } a.cacheKey.artID = artID - if al.UpdatedAt.After(mf.UpdatedAt) { + a.cacheKey.lastUpdate = mf.UpdatedAt + if al.UpdatedAt.After(a.cacheKey.lastUpdate) { a.cacheKey.lastUpdate = al.UpdatedAt - } else { - a.cacheKey.lastUpdate = mf.UpdatedAt + } + if imagesUpdatedAt != nil && imagesUpdatedAt.After(a.cacheKey.lastUpdate) { + a.cacheKey.lastUpdate = *imagesUpdatedAt } return a, nil } @@ -60,6 +66,12 @@ func (a *mediafileArtworkReader) Reader(ctx context.Context) (io.ReadCloser, str fromFFmpegTag(ctx, a.a.ffmpeg, path), } } - ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.AlbumCoverArtID())) + // For multi-disc albums, fall back to disc artwork first; for single-disc albums, + // skip disc resolution (it would just fall through to album art anyway). + if len(a.album.Discs) > 1 { + ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.DiscCoverArtID())) + } else { + ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.AlbumCoverArtID())) + } return selectImageReader(ctx, a.artID, ff...) } diff --git a/model/mediafile.go b/model/mediafile.go index 20532bfb..ec83b76f 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -119,7 +119,16 @@ func (mf MediaFile) CoverArtID() ArtworkID { if mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt { return artworkIDFromMediaFile(mf) } - // if it does not have a coverArt, fallback to the album cover + // Otherwise fallback to disc (if available) or album cover + return mf.DiscCoverArtID() +} + +// DiscCoverArtID returns the disc artwork ID when the media file has a disc number, +// otherwise it returns the album artwork ID. +func (mf MediaFile) DiscCoverArtID() ArtworkID { + if mf.DiscNumber > 0 { + return NewArtworkID(KindDiscArtwork, DiscArtworkID(mf.AlbumID, mf.DiscNumber), nil) + } return mf.AlbumCoverArtID() } diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 207d3c15..038ac93d 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -504,13 +504,26 @@ var _ = Describe("MediaFile", func() { Expect(id.Kind).To(Equal(KindMediaFileArtwork)) Expect(id.ID).To(Equal(mf.ID)) }) - It("returns its album id if HasCoverArt is false", func() { + It("returns disc art id if HasCoverArt is false and DiscNumber > 0", func() { + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: false, DiscNumber: 2} + id := mf.CoverArtID() + Expect(id.Kind).To(Equal(KindDiscArtwork)) + Expect(id.ID).To(Equal("1:2")) + }) + It("returns its album id if HasCoverArt is false and DiscNumber is 0", func() { mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: false} id := mf.CoverArtID() Expect(id.Kind).To(Equal(KindAlbumArtwork)) Expect(id.ID).To(Equal(mf.AlbumID)) }) - It("returns its album id if EnableMediaFileCoverArt is disabled", func() { + It("returns disc art id if EnableMediaFileCoverArt is disabled and DiscNumber > 0", func() { + conf.Server.EnableMediaFileCoverArt = false + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true, DiscNumber: 3} + id := mf.CoverArtID() + Expect(id.Kind).To(Equal(KindDiscArtwork)) + Expect(id.ID).To(Equal("1:3")) + }) + It("returns its album id if EnableMediaFileCoverArt is disabled and DiscNumber is 0", func() { conf.Server.EnableMediaFileCoverArt = false mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true} id := mf.CoverArtID()