From eb283b1646511b567fde2eaa54740b189eb83848 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 24 Jul 2026 21:40:51 -0400 Subject: [PATCH] fix(artwork): run disc resolution for single-disc albums too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ed4178a6 gated serveDisc on len(album.Discs) > 1, claiming parity with the legacy reader. The legacy reader has no such gate: artwork.go dispatches every dc- id to newDiscArtworkReader, whose Reader() walks DiscArtPriority unconditionally. The gate also lost art. For a single-disc album whose only image is disc1.jpg, the disc request skipped the chain and fell through to album art, which does not match CoverArtPriority — so tracks tagged disc 1 (whose CoverArtID is a dc- id) served nothing at all, where before they served disc1.jpg. A single disc can legitimately have its own cover, distinct from the album's, and DiscArtPriority is what expresses that preference. Drop the gate and restore the single-disc e2e scenarios that covered it. --- core/artwork/e2e/acquire_serve_test.go | 75 +++++ core/artwork/e2e/album_test.go | 46 ++- core/artwork/e2e/artist_test.go | 59 +++- core/artwork/e2e/disc_test.go | 328 ++++++++++---------- core/artwork/e2e/mediafile_test.go | 23 ++ core/artwork/e2e/playlist_test.go | 49 +++ core/artwork/e2e/resolution_harness_test.go | 52 +++- core/artwork/serving.go | 19 +- core/artwork/serving_test.go | 7 +- 9 files changed, 463 insertions(+), 195 deletions(-) diff --git a/core/artwork/e2e/acquire_serve_test.go b/core/artwork/e2e/acquire_serve_test.go index fe5f7e430..a348c6e1c 100644 --- a/core/artwork/e2e/acquire_serve_test.go +++ b/core/artwork/e2e/acquire_serve_test.go @@ -5,6 +5,7 @@ import ( "errors" "io" "os" + "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -211,6 +212,73 @@ var _ = Describe("Acquisition → serve loop", func() { Expect(readAll(resolved)).To(Equal(provisionalBytes)) }) + It("stores dimensions, mime and a real blurhash alongside the acquired bytes", func() { + seedFolderAlbum("al1") + worker.Bump("al", "al1") + runWorkerUntil(ctx, worker, itemFound(model.KindAlbumArtwork, "al1")) + + ia, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + art, err := artRepo.GetImage(ia.Hash) + Expect(err).ToNot(HaveOccurred()) + Expect(art.Mime).To(Equal("image/jpeg")) + Expect(art.Width).To(BeNumerically(">", 0)) + Expect(art.Height).To(BeNumerically(">", 0)) + Expect(art.SizeBytes).To(BeNumerically("==", len(coverBytes))) + // Never a synthesized value: the blurhash is encoded from the real pixels. + Expect(art.BlurHash).ToNot(BeEmpty()) + }) + + It("deduplicates byte-identical art across entities onto one image row", func() { + folderRepo.result = []model.Folder{{Path: albumFolderPath, ImageFiles: []string{"cover.jpg"}}} + albumRepo.SetData(model.Albums{ + {ID: "al1", Name: "Album", FolderIDs: []string{"f1"}, LibraryID: 0}, + {ID: "al2", Name: "Same Cover", FolderIDs: []string{"f1"}, LibraryID: 0}, + }) + worker.Bump("al", "al1") + worker.Bump("al", "al2") + runWorkerUntil(ctx, worker, func() bool { + return itemFound(model.KindAlbumArtwork, "al1")() && itemFound(model.KindAlbumArtwork, "al2")() + }) + + ia1, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + ia2, err := artRepo.GetItemArtwork(model.KindAlbumArtwork, "al2", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia1.Hash).To(Equal(ia2.Hash), "identical bytes must share one content hash") + Expect(readAll(mustGet(svc.Get(ctx, model.MustParseArtworkID("al-al2"), 0, false)))).To(Equal(coverBytes)) + }) + + It("stops serving a file-backed image once its source file changes underneath", func() { + name := writeUpload(consts.EntityRadio, "radio-stale.jpg", coverFixture) + radioRepo.Data["ra1"] = &model.Radio{ID: "ra1", Name: "Station", UploadedImage: name} + worker.Bump("ra", "ra1") + runWorkerUntil(ctx, worker, itemFound(model.KindRadioArtwork, "ra1")) + + ia, err := artRepo.GetItemArtwork(model.KindRadioArtwork, "ra1", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + staleHash := ia.Hash + + // Replace the backing file with different bytes and a newer mtime. + path := model.UploadedImagePath(consts.EntityRadio, name) + Expect(os.WriteFile(path, readFixture(artistPngFixture), 0o600)).To(Succeed()) + newer := time.Now().Add(2 * time.Second) + Expect(os.Chtimes(path, newer, newer)).To(Succeed()) + + // The mtime no longer matches the state row, so the old hash's bytes are never served. + _, err = svc.Get(ctx, model.MustParseArtworkID("ra-ra1"), 0, false) + Expect(err).To(MatchError(artwork.ErrUnavailable)) + + // That read enqueued a re-resolution; draining it republishes the new bytes. + runWorkerUntil(ctx, worker, func() bool { + cur, gerr := artRepo.GetItemArtwork(model.KindRadioArtwork, "ra1", model.ImageTypePrimary) + return gerr == nil && cur.Hash != "" && cur.Hash != staleHash + }) + img, err := svc.Get(ctx, model.MustParseArtworkID("ra-ra1"), 0, false) + Expect(err).ToNot(HaveOccurred()) + Expect(readAll(img)).To(Equal(readFixture(artistPngFixture))) + }) + It("records an absent state for an entity with no art and reports it unavailable", func() { albumRepo.SetData(model.Albums{{ID: "alx", Name: "Artless", LibraryID: 0}}) worker.Bump("al", "alx") @@ -224,3 +292,10 @@ var _ = Describe("Acquisition → serve loop", func() { Expect(img.Placeholder).To(BeTrue()) }) }) + +// mustGet unwraps a Service.Get result for inline byte assertions. +func mustGet(img *artwork.Image, err error) *artwork.Image { + GinkgoHelper() + Expect(err).ToNot(HaveOccurred()) + return img +} diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index aa219b197..92fabf9fc 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -354,8 +354,10 @@ var _ = Describe("Album artwork resolution", func() { }) scan() - expectAbsent(albumByName("Album A")) + // Album B first: a drain settles every ready item, and folder art is only + // byte-servable while the album still has no state row. expectFolderCover(albumByName("Album B"), "Artist/Album B/cover.jpg") + expectAbsent(albumByName("Album A")) }) }) @@ -387,6 +389,48 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // albumRootParent refuses the library root as an album root (parent.ParentID == ""), so a + // stray image at the top of the library never becomes some album's cover. + When("a multi-disc album sits directly at the library root with a cover.jpg beside it", func() { + // (library root) + // ├── cover.jpg ← must NOT be adopted: the root is never an album root + // ├── CD1/ + // │ └── 01 - Track.mp3 + // └── CD2/ + // └── 01 - Track.mp3 + It("does not adopt the library-root image as album art", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "cover.jpg": smallPNG("library-root"), + "CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"album": "Rootless", "disc": "1"}), + "CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"album": "Rootless", "disc": "2"}), + }) + scan() + expectAbsent(firstAlbum()) + }) + }) + + // compareImageFiles prefers shallower paths on a basename tie, so an artist-folder cover.jpg + // would outrank the album's own if the parent folder were ever considered here. It is not: + // albumRootParent skips the parent for a single-folder album that has images of its own. + When("a single-folder album has its own cover.jpg and the artist folder has one too", func() { + // Artist/ + // ├── cover.jpg ← shallower, but must NOT win + // └── Album/ + // ├── 01 - Track.mp3 + // └── cover.jpg ← should win (the album has images of its own) + It("prefers the album's own cover over the shallower artist-folder cover", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/cover.jpg": smallPNG("artist-image"), + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), + "Artist/Album/cover.jpg": smallPNG("album-own"), + }) + scan() + expectFolderCover(firstAlbum(), "Artist/Album/cover.jpg") + }) + }) + When("a spread album has its own front.jpg but the artist folder has cover.jpg", func() { // Artist/ // ├── cover.jpg ← artist image; matches cover.* (first pattern), diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go index fd36e82eb..f912805bf 100644 --- a/core/artwork/e2e/artist_test.go +++ b/core/artwork/e2e/artist_test.go @@ -8,6 +8,7 @@ import ( "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -73,22 +74,6 @@ var _ = Describe("Artist artwork resolution", func() { }) }) - When("ArtistArtPriority uses album/ (not just album/artist.*)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── artist.jpg ← matched by album/artist.* - It("resolves the pattern against the artist's album image files", func() { - conf.Server.ArtistArtPriority = "album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/Album/artist.jpg": smallPNG("album-artist"), - }) - scan() - expectArtistFolder(soleArtist(), "Artist/Album/artist.jpg") - }) - }) - When("an artist has an uploaded image and a matching artist.* file", func() { // / // └── artwork/ @@ -119,6 +104,48 @@ var _ = Describe("Artist artwork resolution", func() { }) }) + When("ArtistArtPriority uses album/ (not just album/artist.*)", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 + // └── artist.jpg ← matched by album/artist.* + It("resolves the pattern against the artist's album image files", func() { + conf.Server.ArtistArtPriority = "album/artist.*, external" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), + "Artist/Album/artist.jpg": smallPNG("album-artist"), + }) + scan() + expectArtistFolder(soleArtist(), "Artist/Album/artist.jpg") + }) + }) + + // resolveArtist only samples albums where this artist is the SOLE album artist, so a + // collaboration or compilation never donates its images as the artist's own. + When("the artist's only album is credited to two album artists", func() { + // Artist/ + // └── Collab Album/ (album artists: "Artist" + a collaborator) + // ├── 01 - Track.mp3 + // └── artist.jpg ← must NOT become the artist image + It("ignores the album's images and settles absent", func() { + conf.Server.ArtistArtPriority = "album/artist.*" + // " / " is a default artists split separator, so this single tag yields two album artists. + setLayout(fstest.MapFS{ + "Artist/Collab Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist / Collaborator"}), + "Artist/Collab Album/artist.jpg": smallPNG("collab-artist"), + }) + scan() + Expect(firstAlbum().Participants[model.RoleAlbumArtist]).To(HaveLen(2), + "sanity check: the album must be credited to two album artists") + + ar := soleArtist() + ia := acquire(model.KindArtistArtwork, ar.ID) + Expect(ia.Hash).To(BeEmpty()) + Expect(serveErr(model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil))). + To(MatchError(artwork.ErrUnavailable)) + }) + }) + When("ArtistArtPriority starts with image-folder and ArtistImageFolder has a name-matching image", func() { // / // └── Artist.jpg ← matched by artist name (image-folder source) diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go index e0ec8a1e8..f869cadae 100644 --- a/core/artwork/e2e/disc_test.go +++ b/core/artwork/e2e/disc_test.go @@ -11,14 +11,79 @@ import ( ) // Disc art is a serve-time read through the library FS (no worker state row), so per-disc images -// are asserted byte-for-byte. Only multi-disc albums use the disc chain; a single-disc album serves -// its album art directly (a deliberate change from the legacy reader), so those cases are covered by -// the album suite. Album-root covers here are folder-backed and asserted on the album state row. +// are asserted byte-for-byte. Single-disc albums run the disc chain too — a disc can carry art +// distinct from the album cover. Album-root covers are folder-backed and asserted on the state row. var _ = Describe("Disc artwork resolution", func() { BeforeEach(func() { setupResolutionHarness() }) + When("the album is single-disc with a disc1.jpg in the only folder", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 + // └── disc1.jpg ← matched by disc*.* + It("returns the disc1.jpg image (matched as disc*.*)", func() { + conf.Server.DiscArtPriority = "disc*.*, cd*.*, cover.*, folder.*, front.*, embedded" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), + "Artist/Album/disc1.jpg": smallPNG("disc1-image"), + }) + scan() + expectDiscImage(firstAlbum(), 1, "disc1-image") + }) + }) + + When("the album has no per-disc image and no album cover", func() { + // Artist/ + // └── Album/ + // └── 01 - Track.mp3 (no disc or album art — nothing to serve) + It("reports the disc lookup as unavailable", func() { + conf.Server.DiscArtPriority = "disc*.*, cd*.*" + conf.Server.CoverArtPriority = "cover.*, folder.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), + }) + scan() + Expect(serveErr(discArtID(firstAlbum(), 1))).To(MatchError(artwork.ErrUnavailable)) + }) + }) + + When("the album has no per-disc image but has an album cover", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 + // └── cover.jpg ← album-level fallback (no disc art present) + It("falls back to the album cover", func() { + conf.Server.DiscArtPriority = "disc*.*, cd*.*" + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), + "Artist/Album/cover.jpg": smallPNG("album-cover"), + }) + scan() + expectDiscImage(firstAlbum(), 1, "album-cover") + }) + }) + + When("multiple disc images exist in the same folder (disc1 vs disc10)", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 + // ├── disc1.jpg ← matches request for disc 1 + // └── disc10.jpg + It("matches the requested disc number, not a higher-numbered one", func() { + conf.Server.DiscArtPriority = "disc*.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), + "Artist/Album/disc1.jpg": smallPNG("disc-one"), + "Artist/Album/disc10.jpg": smallPNG("disc-ten"), + }) + scan() + expectDiscImage(firstAlbum(), 1, "disc-one") + }) + }) + When("a multi-disc album has per-disc covers", func() { // Artist/ // └── Album/ @@ -41,95 +106,6 @@ var _ = Describe("Disc artwork resolution", func() { }) }) - When("multiple disc images exist in the same folder (disc1 vs disc10)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 (disc 1) - // ├── 02 - Track.mp3 (disc 10) - // ├── disc1.jpg ← matches request for disc 1 - // └── disc10.jpg - It("matches the requested disc number, not a higher-numbered one", func() { - conf.Server.DiscArtPriority = "disc*.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/02 - Track.mp3": trackFile(2, "T10", map[string]any{"disc": "10"}), - "Artist/Album/disc1.jpg": smallPNG("disc-one"), - "Artist/Album/disc10.jpg": smallPNG("disc-ten"), - }) - scan() - expectDiscImage(firstAlbum(), 1, "disc-one") - }) - }) - - When("a multi-disc album has no per-disc image but has an album cover", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ └── 01 - Track.mp3 - // ├── CD2/ - // │ └── 01 - Track.mp3 - // └── cover.jpg ← album-level fallback (no disc art present) - It("falls back to the album cover", func() { - conf.Server.DiscArtPriority = "disc*.*, cd*.*" - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/cover.jpg": smallPNG("album-cover"), - }) - scan() - expectDiscImage(firstAlbum(), 1, "album-cover") - }) - }) - - When("a multi-disc album has no per-disc image and no album cover", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ └── 01 - Track.mp3 - // └── CD2/ - // └── 01 - Track.mp3 (no images anywhere — nothing to serve) - It("reports the disc lookup as unavailable", func() { - conf.Server.DiscArtPriority = "disc*.*, cd*.*" - conf.Server.CoverArtPriority = "cover.*, folder.*" - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - }) - scan() - Expect(serveErr(discArtID(firstAlbum(), 1))).To(MatchError(artwork.ErrUnavailable)) - }) - }) - - When("DiscArtPriority is the empty string", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── disc1.jpg (ignored — DiscArtPriority is empty) - // ├── CD2/ - // │ ├── 01 - Track.mp3 - // │ └── cd2.png (ignored — DiscArtPriority is empty) - // └── cover.jpg ← used for every disc (album-level fallback) - It("skips every disc-level source and returns the album cover", func() { - conf.Server.DiscArtPriority = "" - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/disc1.jpg": smallPNG("disc-1"), - "Artist/Album/CD2/cd2.png": smallPNG("cd-2"), - "Artist/Album/cover.jpg": smallPNG("album-cover"), - }) - scan() - - al := firstAlbum() - for _, n := range []int{1, 2} { - expectDiscImage(al, n, "album-cover") - } - }) - }) - // Doc scenarios from: // https://www.navidrome.org/docs/usage/library/artwork/#disc-cover-art // Default DiscArtPriority is "disc*.*, cd*.*, cover.*, folder.*, front.*, discsubtitle, embedded". @@ -177,22 +153,55 @@ var _ = Describe("Disc artwork resolution", func() { }) }) + When("DiscArtPriority is the empty string", func() { + // Artist/ + // └── Album/ + // ├── CD1/ + // │ ├── 01 - Track.mp3 + // │ └── disc1.jpg (ignored — DiscArtPriority is empty) + // ├── CD2/ + // │ ├── 01 - Track.mp3 + // │ └── cd2.png (ignored — DiscArtPriority is empty) + // └── cover.jpg ← used for every disc (album-level fallback) + It("skips every disc-level source and returns the album cover", func() { + conf.Server.DiscArtPriority = "" + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), + "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), + "Artist/Album/CD1/disc1.jpg": smallPNG("disc-1"), + "Artist/Album/CD2/cd2.png": smallPNG("cd-2"), + "Artist/Album/cover.jpg": smallPNG("album-cover"), + }) + scan() + + al := firstAlbum() + for _, n := range []int{1, 2} { + expectDiscImage(al, n, "album-cover") + } + }) + }) + When("the documented multi-disc layout is used (disc1.jpg + cd2.png + album-root cover.jpg)", func() { // Artist/ // └── Album/ // ├── disc1/ // │ ├── disc1.jpg ← matched by disc*.* for disc 1 - // │ └── 01 - Track.mp3 + // │ ├── 01 - Track.mp3 + // │ └── 02 - Track.mp3 // ├── disc2/ // │ ├── cd2.png ← matched by cd*.* for disc 2 - // │ └── 01 - Track.mp3 - // └── cover.jpg ← album-level cover - It("matches the per-disc image for each disc and the album-root cover for the album", func() { + // │ ├── 01 - Track.mp3 + // │ └── 02 - Track.mp3 + // └── cover.jpg (album-level fallback, unused here) + It("matches the per-disc image for each disc", func() { conf.Server.DiscArtPriority = defaultDiscPriority conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ "Artist/Album/disc1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), + "Artist/Album/disc1/02 - Track.mp3": trackFile(2, "T2", map[string]any{"disc": "1"}), "Artist/Album/disc2/01 - Track.mp3": trackFile(1, "T3", map[string]any{"disc": "2"}), + "Artist/Album/disc2/02 - Track.mp3": trackFile(2, "T4", map[string]any{"disc": "2"}), "Artist/Album/disc1/disc1.jpg": smallPNG("disc-1"), "Artist/Album/disc2/cd2.png": smallPNG("cd-2"), "Artist/Album/cover.jpg": smallPNG("album-root"), @@ -202,47 +211,72 @@ var _ = Describe("Disc artwork resolution", func() { al := firstAlbum() expectDiscImage(al, 1, "disc-1") expectDiscImage(al, 2, "cd-2") - expectAlbumFolderCover(al, "Artist/Album/cover.jpg") }) }) When("discsubtitle keyword matches an image whose stem equals the disc's subtitle", func() { // Artist/ // └── Album/ - // ├── CD1/ - // │ └── 01 - Track.mp3 (discsubtitle="Bonus Tracks") - // ├── CD2/ - // │ └── 01 - Track.mp3 - // └── Bonus Tracks.jpg ← matched by "discsubtitle" keyword for disc 1 + // ├── 01 - Track.mp3 (discsubtitle="Bonus Tracks") + // └── Bonus Tracks.jpg ← matched by "discsubtitle" keyword It("selects the subtitle-named image", func() { conf.Server.DiscArtPriority = "discsubtitle" setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/Bonus Tracks.jpg": smallPNG("bonus-tracks"), + "Artist/Album/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), + "Artist/Album/Bonus Tracks.jpg": smallPNG("bonus-tracks"), }) scan() expectDiscImage(firstAlbum(), 1, "bonus-tracks") }) }) - When("discsubtitle is set but no image filename matches the subtitle", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 (discsubtitle="Bonus Tracks") - // │ └── cover.jpg ← wins (discsubtitle has no match, falls through) - // └── CD2/ - // └── 01 - Track.mp3 - It("falls through to the next priority entry", func() { - conf.Server.DiscArtPriority = "discsubtitle, cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/cover.jpg": smallPNG("disc1-cover"), - }) + // Reproduces https://github.com/navidrome/navidrome/issues/5456 + // Deeply nested layout matching the reporter's actual structure. + When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Genre/Artist/Album/ ← album root with cover.jpg + // ├── cover.jpg ← album-level cover + // ├── Disc 01 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── ... (12 discs) + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + discNames := []string{ + "Disc 01 (Birth of the Dead - The Studio Sides)", + "Disc 02 (Birth of the Dead - The Live Sides)", + "Disc 03 (The Grateful Dead)", + "Disc 04 (Anthem of the Sun)", + "Disc 05 (Aoxomoxoa)", + "Disc 06 (Live; Dead)", + "Disc 07 (Workingman's Dead)", + "Disc 08 (American Beauty)", + "Disc 09 (Grateful Dead)", + "Disc 10 (Europe '72)", + "Disc 11 (Europe '72)", + "Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))", + } + layout := fstest.MapFS{ + "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": smallPNG("album-root-cover"), + } + for i, name := range discNames { + discNum := i + 1 + prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) + layout[prefix+"folder.jpg"] = smallPNG(fmt.Sprintf("disc-%02d-folder", discNum)) + } + setLayout(layout) scan() - expectDiscImage(firstAlbum(), 1, "disc1-cover") + + al := firstAlbum() + expectAlbumFolderCover(al, "(2001) The Golden Road/cover.jpg") + for i := range discNames { + discNum := i + 1 + expectDiscImage(al, discNum, fmt.Sprintf("disc-%02d-folder", discNum)) + } }) }) @@ -282,47 +316,19 @@ var _ = Describe("Disc artwork resolution", func() { }) }) - // Reproduces https://github.com/navidrome/navidrome/issues/5456 - // Deeply nested layout matching the reporter's actual structure. - When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { - // Pop; Rock/Grateful Dead/(2001) The Golden Road/ ← album root with cover.jpg - // ├── cover.jpg ← album-level cover - // ├── Disc 01 (Subtitle)/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← disc 1 art - // ├── Disc 02 (Subtitle)/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // └── ... (6 discs) - It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - conf.Server.CoverArtPriority = defaultCoverPriority - discNames := []string{ - "Disc 01 (Birth of the Dead - The Studio Sides)", - "Disc 02 (Birth of the Dead - The Live Sides)", - "Disc 03 (The Grateful Dead)", - "Disc 04 (Anthem of the Sun)", - "Disc 05 (Aoxomoxoa)", - "Disc 06 (Live; Dead)", - } - layout := fstest.MapFS{ - "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": smallPNG("album-root-cover"), - } - for i, name := range discNames { - discNum := i + 1 - prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) - layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) - layout[prefix+"folder.jpg"] = smallPNG(fmt.Sprintf("disc-%02d-folder", discNum)) - } - setLayout(layout) + When("discsubtitle is set but no image filename matches the subtitle", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 (discsubtitle="Bonus Tracks") + // └── cover.jpg ← wins (discsubtitle has no match, falls through) + It("falls through to the next priority entry", func() { + conf.Server.DiscArtPriority = "discsubtitle, cover.*" + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), + "Artist/Album/cover.jpg": smallPNG("cover"), + }) scan() - - al := firstAlbum() - expectAlbumFolderCover(al, "(2001) The Golden Road/cover.jpg") - for i := range discNames { - discNum := i + 1 - expectDiscImage(al, discNum, fmt.Sprintf("disc-%02d-folder", discNum)) - } + expectDiscImage(firstAlbum(), 1, "cover") }) }) }) diff --git a/core/artwork/e2e/mediafile_test.go b/core/artwork/e2e/mediafile_test.go index 2e211f798..6667b8dc2 100644 --- a/core/artwork/e2e/mediafile_test.go +++ b/core/artwork/e2e/mediafile_test.go @@ -111,6 +111,29 @@ var _ = Describe("MediaFile artwork resolution", func() { Expect(storedBytes(ia)).To(Equal(embeddedArtBytes)) }) }) + + When("EnableMediaFileCoverArt is turned off after the track was scanned", func() { + // Artist/ + // └── Album/ + // ├── 01 - Track.mp3 ← has embedded picture (must NOT be served) + // └── cover.jpg ← wins (per-track art disabled at serve time) + It("serves the album cover instead of the track's embedded art", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"has_picture": "true"}), + "Artist/Album/cover.jpg": smallPNG("album-cover"), + }) + scan() + replaceWithRealMP3("Artist/Album/01 - Track.mp3") + + // The setting is not part of the artwork fingerprint, so a direct mf- request must + // honor it at serve time rather than serving previously-eligible embedded art. + conf.Server.EnableMediaFileCoverArt = false + mf := mediafileOn("Artist/Album/01 - Track.mp3") + trackArtID := model.NewArtworkID(model.KindMediaFileArtwork, mf.ID, nil) + Expect(serveBytes(trackArtID)).To(Equal(pngBytes("album-cover"))) + }) + }) }) func mediafileOn(relPath string) model.MediaFile { diff --git a/core/artwork/e2e/playlist_test.go b/core/artwork/e2e/playlist_test.go index 01e8429ad..2db8d4dda 100644 --- a/core/artwork/e2e/playlist_test.go +++ b/core/artwork/e2e/playlist_test.go @@ -1,6 +1,7 @@ package e2e import ( + "image/color" "os" "path/filepath" "testing/fstest" @@ -153,6 +154,54 @@ var _ = Describe("Playlist artwork resolution", func() { data := storedBytes(ia) // The tiled cover is a PNG-encoded image; exact bytes vary (random album order). Expect(data[:8]).To(Equal([]byte{0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a})) + // Two tiles are mirrored into the 2x2 grid as [A B B A], so opposite corners match. + q := gridQuadrants(data) + Expect(q[0]).To(Equal(q[3])) + Expect(q[1]).To(Equal(q[2])) + Expect(q[0]).ToNot(Equal(q[1])) + }) + }) + + When("a playlist has tracks from four albums, each with its own cover", func() { + // Library: + // Artist/ + // ├── AlbumA/{01 - Track.mp3, cover.png} ← tile 1 + // ├── AlbumB/{01 - Track.mp3, cover.png} ← tile 2 + // ├── AlbumC/{01 - Track.mp3, cover.png} ← tile 3 + // └── AlbumD/{01 - Track.mp3, cover.png} ← tile 4 + // Four distinct tiles fill the grid outright, with no mirroring. + It("fills all four grid quadrants with distinct album art", func() { + conf.Server.CoverArtPriority = "cover.*" + layout := fstest.MapFS{} + for _, name := range []string{"AlbumA", "AlbumB", "AlbumC", "AlbumD"} { + layout["Artist/"+name+"/01 - Track.mp3"] = trackFile(1, "T"+name, map[string]any{"album": name}) + layout["Artist/"+name+"/cover.png"] = smallPNG(name) + } + setLayout(layout) + scan() + + mfs, err := rds.MediaFile(rctx).GetAll(model.QueryOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(4)) + ids := make([]string, 0, len(mfs)) + for _, mf := range mfs { + ids = append(ids, mf.ID) + } + + pl := model.Playlist{ID: "pl-8", Name: "Four", OwnerID: "admin-1"} + pl.AddMediaFilesByID(ids) + Expect(rds.Playlist(rctx).Put(&pl)).To(Succeed()) + + ia := acquire(model.KindPlaylistArtwork, pl.ID) + Expect(ia.Source).To(Equal("generated")) + q := gridQuadrants(storedBytes(ia)) + Expect([]color.RGBA{q[0], q[1], q[2], q[3]}).To(HaveLen(4)) + Expect(q[0]).ToNot(Equal(q[1])) + Expect(q[0]).ToNot(Equal(q[2])) + Expect(q[0]).ToNot(Equal(q[3])) + Expect(q[1]).ToNot(Equal(q[2])) + Expect(q[1]).ToNot(Equal(q[3])) + Expect(q[2]).ToNot(Equal(q[3])) }) }) }) diff --git a/core/artwork/e2e/resolution_harness_test.go b/core/artwork/e2e/resolution_harness_test.go index fb116e464..6b90e7795 100644 --- a/core/artwork/e2e/resolution_harness_test.go +++ b/core/artwork/e2e/resolution_harness_test.go @@ -11,6 +11,7 @@ import ( "io" "os" "path/filepath" + "strings" "sync" "testing/fstest" "time" @@ -178,16 +179,43 @@ func serveErr(artID model.ArtworkID) error { return err } -// expectAlbumFolderCover asserts the worker selected a folder image at the given path suffix as the -// album cover. Folder art is file-backed (served via os.Open, which the in-memory FS can't satisfy), -// so the persisted state row — which captures the resolver's selection — is what we assert. +// libFileBytes returns the contents of the one library file whose path ends with suffix. +func libFileBytes(suffix string) []byte { + GinkgoHelper() + var match string + for name := range fakeFS.MapFS { + if strings.HasSuffix(name, suffix) { + Expect(match).To(BeEmpty(), "suffix %q is ambiguous: %q and %q", suffix, match, name) + match = name + } + } + Expect(match).ToNot(BeEmpty(), "no library file ends with %q", suffix) + return fakeFS.MapFS[match].Data +} + +// expectAlbumFolderCover asserts the album resolves to the library image at the given path suffix, +// byte-for-byte. The serve happens before acquisition on purpose: with no state row the request +// path resolves locally through the library FS, whereas a settled folder row is file-backed and +// read with os.Open, which the in-memory FS cannot satisfy. func expectAlbumFolderCover(al model.Album, suffix string) { GinkgoHelper() + requireNoStateRow(model.KindAlbumArtwork, al.ID) + Expect(serveBytes(al.CoverArtID())).To(Equal(libFileBytes(suffix))) ia := acquire(model.KindAlbumArtwork, al.ID) Expect(ia.Source).To(Equal("folder")) Expect(ia.SourcePath).To(HaveSuffix(suffix)) } +// requireNoStateRow guards the byte-level folder assertions: a drain resolves every ready queue +// row, so acquiring one entity can settle others. Once settled, folder art is file-backed and the +// in-memory FS cannot serve it — so these assertions must come before any acquire in a spec. +func requireNoStateRow(kind model.Kind, id string) { + GinkgoHelper() + _, err := rds.Artwork(rctx).GetItemArtwork(kind, id, model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound), + "assert %s %q before acquiring any other entity in this spec", kind, id) +} + // expectAlbumAbsent asserts the album settled absent (no source resolved) and serves unavailable. func expectAlbumAbsent(al model.Album) { GinkgoHelper() @@ -200,6 +228,8 @@ func expectAlbumAbsent(al model.Album) { // as the artist image; like album folder art it is file-backed, so it is asserted on the state row. func expectArtistFolder(ar model.Artist, suffix string) { GinkgoHelper() + requireNoStateRow(model.KindArtistArtwork, ar.ID) + Expect(serveBytes(model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil))).To(Equal(libFileBytes(suffix))) ia := acquire(model.KindArtistArtwork, ar.ID) Expect(ia.Source).To(Equal("folder")) Expect(ia.SourcePath).To(HaveSuffix(suffix)) @@ -226,6 +256,22 @@ func expectDiscImage(al model.Album, disc int, label string) { Expect(serveBytes(discArtID(al, disc))).To(Equal(pngBytes(label))) } +// gridQuadrants decodes a generated 2x2 playlist cover and samples the center of each quadrant, +// in rect() order: top-left, top-right, bottom-left, bottom-right. Each tile is a solid color, so +// the samples identify which album art landed where (and whether tiles were mirrored). +func gridQuadrants(data []byte) [4]color.RGBA { + GinkgoHelper() + img, _, err := image.Decode(bytes.NewReader(data)) + Expect(err).ToNot(HaveOccurred()) + b := img.Bounds() + qw, qh := b.Dx()/4, b.Dy()/4 + at := func(x, y int) color.RGBA { + c := color.RGBAModel.Convert(img.At(b.Min.X+x, b.Min.Y+y)) + return c.(color.RGBA) + } + return [4]color.RGBA{at(qw, qh), at(3*qw, qh), at(qw, 3*qh), at(3*qw, 3*qh)} +} + // storedBytes returns the bytes the worker placed in the content-addressed store for a // store-backed resolution (embedded/generated). Folder/upload sources are file-backed and are // not in the store; assert those on ia.SourcePath instead. diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 6e6bed4e0..a65565861 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -299,17 +299,14 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int if err != nil { return nil, err } - // Only multi-disc albums use disc-specific resolution (matching the legacy reader); a - // single-disc album serves album art directly, so a stray disc*/embedded image can't - // shadow higher-priority album art. - if len(dr.album.Discs) > 1 { - funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority) - if r, path, err := selectImageReader(ctx, artID, funcs...); err == nil && r != nil { - defer r.Close() - if data, rerr := readCapped(r); rerr == nil { - if hash, herr := HashImage(bytes.NewReader(data)); herr == nil { - return s.serveBytes(ctx, hash, data, unixMtime(mtimeViaFS(dr.lib.FS, path)), size, square) - } + // Single-disc albums run the chain too: a disc can carry its own art, distinct from the + // album cover, and DiscArtPriority is what expresses that preference. + funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority) + if r, path, err := selectImageReader(ctx, artID, funcs...); err == nil && r != nil { + defer r.Close() + if data, rerr := readCapped(r); rerr == nil { + if hash, herr := HashImage(bytes.NewReader(data)); herr == nil { + return s.serveBytes(ctx, hash, data, unixMtime(mtimeViaFS(dr.lib.FS, path)), size, square) } } } diff --git a/core/artwork/serving_test.go b/core/artwork/serving_test.go index 58f9585ba..acba61875 100644 --- a/core/artwork/serving_test.go +++ b/core/artwork/serving_test.go @@ -307,7 +307,9 @@ var _ = Describe("Service", func() { Expect(readAll(img)).To(Equal(coverBytes)) }) - It("delegates a single-disc track straight to the album, skipping disc resolution", func() { + It("routes a single-disc track through disc resolution too", func() { + // A single disc can carry its own art: DiscArtPriority still applies, so the disc + // image is served even when the album has different found art. folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}} albumRepo.SetData(model.Albums{{ID: "alsd", Name: "Album", FolderIDs: []string{"f1"}, Discs: model.Discs{1: ""}}}) seedFoundStore("al", "alsd", []byte("album-art-distinct")) @@ -315,8 +317,7 @@ var _ = Describe("Service", func() { img, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf6"), 0, false) Expect(err).ToNot(HaveOccurred()) - // Single-disc album: album art wins; the folder disc image must not shadow it. - Expect(readAll(img)).To(Equal([]byte("album-art-distinct"))) + Expect(readAll(img)).To(Equal(coverBytes)) }) })