fix(artwork): search parent folders for album cover art in multi-disc layouts (#5157)
* fix(artwork): search parent folders for album cover art in multi-disc layouts When albums have tracks in subdirectories (e.g., CD1/, CD2/), Navidrome only searched those subdirectories for cover images. This meant cover art placed in the album's root folder (e.g., "Artist/Album/cover.jpg") was not found. Now loadAlbumFoldersPaths also queries parent folders of the album's media folders, so cover art in the album root is discovered. * fix(artwork): simplify parent folder detection for album cover art lookup Signed-off-by: Deluan <deluan@navidrome.org> * fix(album): propagate non-ErrNotFound errors from parent folder lookup Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
@@ -4,6 +4,7 @@ import (
|
|||||||
"cmp"
|
"cmp"
|
||||||
"context"
|
"context"
|
||||||
"crypto/md5"
|
"crypto/md5"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
@@ -17,6 +18,7 @@ import (
|
|||||||
"github.com/navidrome/navidrome/core"
|
"github.com/navidrome/navidrome/core"
|
||||||
"github.com/navidrome/navidrome/core/external"
|
"github.com/navidrome/navidrome/core/external"
|
||||||
"github.com/navidrome/navidrome/core/ffmpeg"
|
"github.com/navidrome/navidrome/core/ffmpeg"
|
||||||
|
"github.com/navidrome/navidrome/log"
|
||||||
"github.com/navidrome/navidrome/model"
|
"github.com/navidrome/navidrome/model"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -103,6 +105,28 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, nil, nil, err
|
return nil, nil, nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
folderIDSet := make(map[string]bool, len(folderIDs))
|
||||||
|
for _, id := range folderIDs {
|
||||||
|
folderIDSet[id] = true
|
||||||
|
}
|
||||||
|
|
||||||
|
// For multi-disc albums (2+ folders), 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 "Artist/Album/CD1/" and "Artist/Album/CD2/").
|
||||||
|
// We skip single-folder albums to avoid pulling images from the artist folder.
|
||||||
|
if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" {
|
||||||
|
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 {
|
||||||
|
folders = append(folders, *parentFolder)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
var paths []string
|
var paths []string
|
||||||
var imgFiles []string
|
var imgFiles []string
|
||||||
var updatedAt time.Time
|
var updatedAt time.Time
|
||||||
@@ -125,6 +149,24 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
|
|||||||
return paths, imgFiles, &updatedAt, nil
|
return paths, imgFiles, &updatedAt, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// commonParentFolder returns the shared parent folder ID when all folders have the
|
||||||
|
// same parent and that parent is not already in folderIDSet. Returns "" otherwise.
|
||||||
|
func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) string {
|
||||||
|
if len(folders) < 2 {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
parentID := folders[0].ParentID
|
||||||
|
if parentID == "" || folderIDSet[parentID] {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
for _, f := range folders[1:] {
|
||||||
|
if f.ParentID != parentID {
|
||||||
|
return ""
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return parentID
|
||||||
|
}
|
||||||
|
|
||||||
// compareImageFiles compares two image file paths for sorting.
|
// compareImageFiles compares two image file paths for sorting.
|
||||||
// It extracts the base filename (without extension) and compares case-insensitively.
|
// It extracts the base filename (without extension) and compares case-insensitively.
|
||||||
// This ensures that "cover.jpg" sorts before "cover.1.jpg" since "cover" < "cover.1".
|
// This ensures that "cover.jpg" sorts before "cover.1.jpg" since "cover" < "cover.1".
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package artwork
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"errors"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -116,5 +117,181 @@ var _ = Describe("Album Artwork Reader", func() {
|
|||||||
Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg")))
|
Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg")))
|
||||||
Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Folder.jpg")))
|
Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Folder.jpg")))
|
||||||
})
|
})
|
||||||
|
|
||||||
|
It("includes images from parent folder for multi-disc albums", func() {
|
||||||
|
// Simulates: Artist/Album/cover.jpg with tracks in Artist/Album/CD1/ and Artist/Album/CD2/
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD1",
|
||||||
|
ParentID: "parentFolder",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
ID: "folder2",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD2",
|
||||||
|
ParentID: "parentFolder",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
repo.parentResult = &model.Folder{
|
||||||
|
ID: "parentFolder",
|
||||||
|
Path: "Artist",
|
||||||
|
Name: "Album",
|
||||||
|
ImagesUpdatedAt: expectedAt,
|
||||||
|
ImageFiles: []string{"cover.jpg", "back.jpg"},
|
||||||
|
}
|
||||||
|
|
||||||
|
_, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
Expect(*imagesUpdatedAt).To(Equal(expectedAt))
|
||||||
|
Expect(imgFiles).To(HaveLen(2))
|
||||||
|
Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/back.jpg")))
|
||||||
|
Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg")))
|
||||||
|
})
|
||||||
|
|
||||||
|
It("does not query parent when parent ID is already in album folders", func() {
|
||||||
|
// When the parent folder is already one of the album's folders, skip it
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist",
|
||||||
|
Name: "Album",
|
||||||
|
ParentID: "folder2",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{"cover.jpg"},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
ID: "folder2",
|
||||||
|
Path: "",
|
||||||
|
Name: "Artist",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
Expect(imgFiles).To(HaveLen(1))
|
||||||
|
Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg")))
|
||||||
|
// Get should not have been called (parent already in folder set)
|
||||||
|
Expect(repo.getCallCount).To(Equal(0))
|
||||||
|
})
|
||||||
|
|
||||||
|
It("does not query parent when folders have different parents", func() {
|
||||||
|
// When album folders span different parents, don't search any parent
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist1/Album",
|
||||||
|
Name: "part1",
|
||||||
|
ParentID: "parentA",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{"cover.jpg"},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
ID: "folder2",
|
||||||
|
Path: "Artist2/Album",
|
||||||
|
Name: "part2",
|
||||||
|
ParentID: "parentB",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
Expect(imgFiles).To(HaveLen(1))
|
||||||
|
Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist1/Album/part1/cover.jpg")))
|
||||||
|
// Get should not have been called (different parents)
|
||||||
|
Expect(repo.getCallCount).To(Equal(0))
|
||||||
|
})
|
||||||
|
|
||||||
|
It("does not query parent for single-folder albums", func() {
|
||||||
|
// A single-folder album's parent is typically the artist folder,
|
||||||
|
// which should not be searched for cover art
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist",
|
||||||
|
Name: "Album",
|
||||||
|
ParentID: "artistFolder",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{"cover.jpg"},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
Expect(imgFiles).To(HaveLen(1))
|
||||||
|
Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg")))
|
||||||
|
// Get should not have been called (single folder, no parent lookup)
|
||||||
|
Expect(repo.getCallCount).To(Equal(0))
|
||||||
|
})
|
||||||
|
|
||||||
|
It("propagates non-ErrNotFound errors from parent folder lookup", func() {
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD1",
|
||||||
|
ParentID: "parentFolder",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{"cover.jpg"},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
ID: "folder2",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD2",
|
||||||
|
ParentID: "parentFolder",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
repo.getErr = errors.New("db connection failed")
|
||||||
|
|
||||||
|
_, _, _, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).To(MatchError("db connection failed"))
|
||||||
|
Expect(repo.getCallCount).To(Equal(1))
|
||||||
|
})
|
||||||
|
|
||||||
|
It("continues gracefully when parent folder is not found", func() {
|
||||||
|
// Parent folder may have been deleted; should log a warning and continue
|
||||||
|
repo.result = []model.Folder{
|
||||||
|
{
|
||||||
|
ID: "folder1",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD1",
|
||||||
|
ParentID: "missingParent",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{"cover.jpg"},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
ID: "folder2",
|
||||||
|
Path: "Artist/Album",
|
||||||
|
Name: "CD2",
|
||||||
|
ParentID: "missingParent",
|
||||||
|
ImagesUpdatedAt: now,
|
||||||
|
ImageFiles: []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
// parentResult is nil, so Get will return ErrNotFound
|
||||||
|
|
||||||
|
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
|
||||||
|
|
||||||
|
Expect(err).ToNot(HaveOccurred())
|
||||||
|
Expect(imgFiles).To(HaveLen(1))
|
||||||
|
Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/CD1/cover.jpg")))
|
||||||
|
Expect(repo.getCallCount).To(Equal(1))
|
||||||
|
})
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -418,6 +418,9 @@ var _ = Describe("artistArtworkReader", func() {
|
|||||||
type fakeFolderRepo struct {
|
type fakeFolderRepo struct {
|
||||||
model.FolderRepository
|
model.FolderRepository
|
||||||
result []model.Folder
|
result []model.Folder
|
||||||
|
parentResult *model.Folder
|
||||||
|
getErr error
|
||||||
|
getCallCount int
|
||||||
err error
|
err error
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -425,6 +428,17 @@ func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) {
|
|||||||
return f.result, f.err
|
return f.result, f.err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) {
|
||||||
|
f.getCallCount++
|
||||||
|
if f.getErr != nil {
|
||||||
|
return nil, f.getErr
|
||||||
|
}
|
||||||
|
if f.parentResult != nil {
|
||||||
|
return f.parentResult, nil
|
||||||
|
}
|
||||||
|
return nil, model.ErrNotFound
|
||||||
|
}
|
||||||
|
|
||||||
type fakeDataStore struct {
|
type fakeDataStore struct {
|
||||||
model.DataStore
|
model.DataStore
|
||||||
folderRepo *fakeFolderRepo
|
folderRepo *fakeFolderRepo
|
||||||
|
|||||||
Reference in New Issue
Block a user