diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index e765e1b1b..5e61684cc 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -357,6 +357,98 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // Regression introduced in v0.62.0 (#5451 + #5457): the parent-folder + // fallback can pick up images from the ARTIST folder, serving the artist + // thumbnail as album art for any album without its own image files. + When("an album has no images and the artist folder has folder.jpg", func() { + // Artist/ + // ├── folder.jpg ← artist thumbnail, must NOT become album art + // ├── Album A/ + // │ └── 01 - Track.mp3 (no images) + // └── Album B/ + // ├── 01 - Track.mp3 + // └── cover.jpg + It("does not use the artist image as album art", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/folder.jpg": imageFile("artist-thumbnail"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + "Artist/Album B/cover.jpg": imageFile("album-b"), + }) + scan() + + alA := albumByName("Album A") + _, err := readArtworkOrErr(alA.CoverArtID()) + Expect(err).To(HaveOccurred(), + "Album A has no images of its own, so it must fall through to the placeholder "+ + "instead of inheriting the artist folder's folder.jpg") + + alB := albumByName("Album B") + Expect(readArtwork(alB.CoverArtID())).To(Equal(imageBytes("album-b"))) + }) + }) + + When("a single-disc album is spread across sibling folders under the artist folder", func() { + // Artist/ + // ├── folder.jpg ← artist thumbnail, must NOT become album art + // ├── Album A/ + // │ └── 01 - Track.mp3 (album: "Album A") + // ├── Album A bonus/ + // │ └── 02 - Track.mp3 (album: "Album A" — same album, second folder) + // └── Album B/ + // ├── 01 - Track.mp3 + // └── cover.jpg + It("does not use the artist image as album art for the spread album", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/folder.jpg": imageFile("artist-thumbnail"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + "Artist/Album B/cover.jpg": imageFile("album-b"), + }) + scan() + + alA := albumByName("Album A") + Expect(alA.FolderIDs).To(HaveLen(2), + "sanity check: scanner should treat the two sibling folders as one spread album") + _, err := readArtworkOrErr(alA.CoverArtID()) + Expect(err).To(HaveOccurred(), + "the spread album has no images of its own, so it must fall through to the "+ + "placeholder instead of inheriting the artist folder's folder.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), + // │ must NOT shadow the album's own front.jpg + // ├── Album A/ + // │ ├── 01 - Track.mp3 (album: "Album A") + // │ └── front.jpg ← should win + // ├── Album A bonus/ + // │ └── 02 - Track.mp3 (album: "Album A") + // └── Album B/ + // └── 01 - Track.mp3 + It("prefers the album's own art over the artist image", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/cover.jpg": imageFile("artist-image"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album A/front.jpg": imageFile("album-a-front"), + "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + }) + scan() + + alA := albumByName("Album A") + Expect(alA.FolderIDs).To(HaveLen(2), + "sanity check: scanner should treat the two sibling folders as one spread album") + Expect(readArtwork(alA.CoverArtID())).To(Equal(imageBytes("album-a-front"))) + }) + }) + When("embedded is first in CoverArtPriority but the track has no embedded art", func() { // Artist/ // └── Album/ diff --git a/core/artwork/e2e/suite_test.go b/core/artwork/e2e/suite_test.go index 733e2e98c..06cc05b6f 100644 --- a/core/artwork/e2e/suite_test.go +++ b/core/artwork/e2e/suite_test.go @@ -2,6 +2,7 @@ package artworke2e_test import ( "context" + "fmt" "path/filepath" "testing" @@ -104,3 +105,16 @@ func firstAlbum() model.Album { Expect(albums).To(HaveLen(1), "expected exactly one album, got %d", len(albums)) return albums[0] } + +func albumByName(name string) model.Album { + GinkgoHelper() + albums, err := ds.Album(ctx).GetAll(model.QueryOptions{}) + Expect(err).ToNot(HaveOccurred()) + for _, al := range albums { + if al.Name == name { + return al + } + } + Fail(fmt.Sprintf("album %q not found among %d albums", name, len(albums))) + return model.Album{} +} diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 73ba9b5ee..8ad07773b 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -113,28 +113,12 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return nil, nil, nil, err } - folderIDSet := make(map[string]bool, len(folderIDs)) - for _, id := range folderIDs { - folderIDSet[id] = true + parent, err := albumRootParent(ctx, ds, folders, folderIDs) + if err != nil { + return nil, nil, nil, err } - - // Check if all folders share a common parent that is not already included. - // This finds cover art in the album root folder (e.g., "Artist/Album/cover.jpg" - // when tracks are in disc subfolders like "Artist/Album/CD1/" and "Artist/Album/CD2/"). - // For single-folder albums, the parent is only included when the folder has no - // images of its own (indicating a disc subfolder needing parent artwork). - if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" { - if len(folders) >= 2 || !anyFolderHasImages(folders) { - parentFolder, err := ds.Folder(ctx).Get(commonParentID) - if errors.Is(err, model.ErrNotFound) { - log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) - } else if err != nil { - return nil, nil, nil, err - } - if parentFolder != nil && parentFolder.ParentID != "" { - folders = append(folders, *parentFolder) - } - } + if parent != nil { + folders = append(folders, *parent) } var paths []string @@ -159,6 +143,50 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return paths, imgFiles, &updatedAt, nil } +// albumRootParent returns the common parent of the album's folders when it +// qualifies as the album's root folder (e.g. "Artist/Album" above disc +// subfolders), or nil when there is no such parent. This finds cover art in +// the album root folder when tracks live in disc subfolders, like +// "Artist/Album/cover.jpg" with tracks in "Artist/Album/CD1/" and +// "Artist/Album/CD2/". The parent must look like an album root, not an +// artist-level folder — it qualifies only when it holds no audio belonging to +// other albums — so artist images are never served as album art. +func albumRootParent(ctx context.Context, ds model.DataStore, folders []model.Folder, folderIDs []string) (*model.Folder, error) { + folderIDSet := make(map[string]bool, len(folderIDs)) + for _, id := range folderIDs { + folderIDSet[id] = true + } + commonParentID := commonParentFolder(folders, folderIDSet) + if commonParentID == "" { + return nil, nil + } + // Single-folder albums only use the parent when the folder has no images + // of its own (indicating a disc subfolder needing parent artwork). + if len(folders) < 2 && anyFolderHasImages(folders) { + return nil, nil + } + parent, err := ds.Folder(ctx).Get(commonParentID) + if errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) + return nil, nil + } + if err != nil { + return nil, err + } + if parent.ParentID == "" { + // The library root can never be an album root + return nil, nil + } + hasOtherAudio, err := ds.Folder(ctx).HasAudioOutsideFolders(*parent, folderIDs) + if err != nil { + return nil, err + } + if hasOtherAudio { + return nil, nil + } + return parent, nil +} + func anyFolderHasImages(folders []model.Folder) bool { for _, f := range folders { if len(f.ImageFiles) > 0 { diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index 1cf039bee..fe4a1a545 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -339,6 +339,61 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("does not include parent images when other albums' audio lives under the parent", func() { + // Simulates: Artist/folder.jpg with Artist/Album (no images) and + // another album's tracks elsewhere under the artist folder + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "artistFolder", + Path: ".", + Name: "Artist", + ParentID: "libraryRoot", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"folder.jpg"}, + } + repo.hasOtherAudio = true + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(BeEmpty()) + }) + + It("propagates errors from the album-root check", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + repo.otherAudioErr = errors.New("db connection failed") + + _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).To(MatchError("db connection failed")) + }) + It("propagates non-ErrNotFound errors from parent folder lookup", func() { repo.result = []model.Folder{ { diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 50ca3a2ce..6d6d58fc5 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -702,12 +702,20 @@ type fakeFolderRepo struct { getErr error getCallCount int err error + // hasOtherAudio is returned by HasAudioOutsideFolders (the album-root + // check). False means the parent qualifies as an album root. + hasOtherAudio bool + otherAudioErr error } func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { return f.result, f.err } +func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) { + return f.hasOtherAudio, f.otherAudioErr +} + func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { f.getCallCount++ if f.getErr != nil { diff --git a/model/folder.go b/model/folder.go index 7a769735e..39cb6db84 100644 --- a/model/folder.go +++ b/model/folder.go @@ -86,6 +86,10 @@ type FolderRepository interface { GetAll(...QueryOptions) ([]Folder, error) CountAll(...QueryOptions) (int64, error) GetFolderUpdateInfo(lib Library, targetPaths ...string) (map[string]FolderUpdateInfo, error) + // HasAudioOutsideFolders reports whether any folder in parent's subtree + // (including parent itself) contains audio files and is not one of the + // given folder IDs. + HasAudioOutsideFolders(parent Folder, excludeFolderIDs []string) (bool, error) Put(*Folder) error MarkMissing(missing bool, ids ...string) error GetTouchedWithPlaylists() (FolderCursor, error) diff --git a/persistence/folder_repository.go b/persistence/folder_repository.go index f7bb6a4fe..624ef21b8 100644 --- a/persistence/folder_repository.go +++ b/persistence/folder_repository.go @@ -7,6 +7,7 @@ import ( "iter" "maps" "os" + "path" "path/filepath" "slices" "strings" @@ -188,6 +189,33 @@ func (r folderRepository) queryFolderUpdateInfo(where And) (map[string]model.Fol return m, nil } +// HasAudioOutsideFolders reports whether any folder in parent's subtree +// (including parent itself) contains audio files and is not one of the given +// folder IDs. LIKE wildcards in the parent path are escaped, so it is always +// matched as a literal prefix. +func (r folderRepository) HasAudioOutsideFolders(parent model.Folder, excludeFolderIDs []string) (bool, error) { + if parent.NumAudioFiles > 0 { + return true, nil + } + parentPath := strings.TrimPrefix(path.Join(parent.Path, parent.Name), "/") + return r.exists(And{ + Eq{"library_id": parent.LibraryID, "missing": false}, + Gt{"num_audio_files": 0}, + NotEq{"id": excludeFolderIDs}, + Or{ + // Direct children have path = parentPath; deeper descendants match the prefix + Eq{"path": parentPath}, + Expr(`path LIKE ? ESCAPE '\'`, escapeLikePrefix(parentPath)+"/%"), + }, + }) +} + +// escapeLikePrefix escapes SQL LIKE wildcards so a string can be used as a +// literal prefix in a LIKE pattern (with ESCAPE '\'). +func escapeLikePrefix(s string) string { + return strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`).Replace(s) +} + func (r folderRepository) Put(f *model.Folder) error { dbf := dbFolder{Folder: f} _, err := r.put(dbf.ID, &dbf) diff --git a/persistence/folder_repository_test.go b/persistence/folder_repository_test.go index ebc08fd04..413a6b38f 100644 --- a/persistence/folder_repository_test.go +++ b/persistence/folder_repository_test.go @@ -217,6 +217,67 @@ var _ = Describe("FolderRepository", func() { }) }) + Describe("HasAudioOutsideFolders", func() { + var albumRoot, disc1, disc2 *model.Folder + + // TestHasAudio/Album/ + // ├── CD1/ (audio, belongs to the album) + // └── CD2/ (audio, belongs to the album) + BeforeEach(func() { + albumRoot = model.NewFolder(testLib, "TestHasAudio/Album") + disc1 = model.NewFolder(testLib, "TestHasAudio/Album/CD1") + disc1.NumAudioFiles = 5 + disc2 = model.NewFolder(testLib, "TestHasAudio/Album/CD2") + disc2.NumAudioFiles = 5 + for _, f := range []*model.Folder{albumRoot, disc1, disc2} { + Expect(repo.Put(f)).To(Succeed()) + } + }) + + It("returns false when all audio under the parent belongs to the given folders", func() { + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("returns true when another folder under the parent has audio", func() { + bonus := model.NewFolder(testLib, "TestHasAudio/Album/Bonus") + bonus.NumAudioFiles = 1 + Expect(repo.Put(bonus)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue()) + }) + + It("returns true when the parent itself contains audio files", func() { + albumRoot.NumAudioFiles = 2 + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue()) + }) + + It("ignores audio outside the parent's subtree", func() { + other := model.NewFolder(testLib, "TestHasAudio/Other Album") + other.NumAudioFiles = 10 + Expect(repo.Put(other)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("ignores missing folders", func() { + gone := model.NewFolder(testLib, "TestHasAudio/Album/Gone") + gone.NumAudioFiles = 3 + gone.Missing = true + Expect(repo.Put(gone)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("does not treat LIKE wildcards in the parent path as patterns", func() { + // "TestHas_udio" would LIKE-match "TestHasAudio" if "_" were not escaped + wildcardRoot := model.NewFolder(testLib, "TestHas_udio/Album") + Expect(repo.Put(wildcardRoot)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*wildcardRoot, []string{"none"})).To(BeFalse()) + }) + }) + Describe("wrapFolderCursor", func() { It("does not panic when the cursor yields a dbFolder with nil Folder", func() { // Simulate what queryWithStableResults does on the rows.Err() path: