fix(artwork): fallback mediafile cover art to disc artwork before album (#5216)
Some checks failed
Pipeline: Test, Lint, Build / Get version info (push) Failing after 3s
Pipeline: Test, Lint, Build / Lint Go code (push) Failing after 9s
Pipeline: Test, Lint, Build / Test Go code (push) Failing after 2s
Pipeline: Test, Lint, Build / Lint i18n files (push) Failing after 3s
Pipeline: Test, Lint, Build / Build-4 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-5 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-6 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-7 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-8 (push) Has been skipped
Pipeline: Test, Lint, Build / Test JS code (push) Failing after 3s
Pipeline: Test, Lint, Build / Check Docker configuration (push) Successful in 2s
Pipeline: Test, Lint, Build / Build (push) Has been skipped
Pipeline: Test, Lint, Build / Build-1 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-2 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-3 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-9 (push) Has been skipped
Pipeline: Test, Lint, Build / Build-10 (push) Has been skipped
Pipeline: Test, Lint, Build / Build Windows installers (push) Has been skipped
Pipeline: Test, Lint, Build / Package/Release (push) Has been skipped
Pipeline: Test, Lint, Build / Upload Linux PKG (push) Has been skipped
Pipeline: Test, Lint, Build / Push to GHCR (push) Has been skipped
Pipeline: Test, Lint, Build / Push to Docker Hub (push) Has been skipped
Pipeline: Test, Lint, Build / Cleanup digest artifacts (push) Has been skipped

* 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 <deluan@navidrome.org>

* 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 <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão 2026-03-16 18:08:39 -04:00 committed by GitHub
parent e7c6e78dd0
commit 2f5b2b5135
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 70 additions and 11 deletions

View file

@ -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() {

View file

@ -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(),

View file

@ -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...)
}