mirror of
https://github.com/navidrome/navidrome.git
synced 2026-05-03 06:51:16 +00:00
* test(artwork): add e2e suite documenting album/disc resolution Adds core/artwork/e2e/ with a real-tempdir + scanner harness that exercises artwork resolution end-to-end. Covers album and disc kinds; pending (PIt) cases document two known bugs in reader_album.go for regression-guard flipping once they are fixed. * refactor(artwork): add libraryFS helper to resolve MusicFS for a library * test(artwork): tighten libraryFS test isolation and add scheme-error case * test(artwork): update libraryFS test description to match implementation * refactor(artwork): convert fromExternalFile to use fs.FS Add a temporary fromExternalFileAbs shim so existing absolute-path callers still compile; the shim is removed once all readers are migrated. * refactor(artwork): make fromExternalFileAbs a thin delegator Introduce a minimal osDirectFS adapter so the shim no longer duplicates the matching loop. Both will be removed in Task 9. * refactor(artwork): convert fromTag to taglib.OpenStream over fs.FS Add a temporary fromTagAbs shim so existing absolute-path callers still compile; removed in Task 9. Reuses the osDirectFS adapter from Task 2. * refactor(artwork): defer fs.File close until after taglib reads finish Mirror the lifetime pattern used by adapters/gotaglib/gotaglib.go: keep the underlying fs.File open until taglib.File is closed, and pass WithFilename so format detection doesn't rely on content sniffing. * docs(artwork): note ffmpeg's path-based API limitation * refactor(artwork): migrate album reader to MusicFS - Add libFS (storage.MusicFS) field to albumArtworkReader; resolved once at construction time via libraryFS() - Switch fromCoverArtPriority from abs-path shims to FS-based fromTag/fromExternalFile; only fromFFmpegTag retains absolute path - Build imgFiles as library-relative forward-slash paths in loadAlbumFoldersPaths using path.Join(f.Path, f.Name, img) - Guard embedAbs so that an empty EmbedArtPath never produces a non-empty absolute path (prevents accidental ffmpeg invocation) - Register testfile:// storage scheme in artwork test suite to provide an os.DirFS-backed MusicFS without requiring the taglib extractor - Update test assertions from filepath.FromSlash(abs) to bare forward-slash relative strings * fix(artwork): use path package in compareImageFiles for forward-slash relative paths * refactor(artwork): migrate disc reader to MusicFS Replace os.Open absolute-path access with libFS.Open on library-relative forward-slash paths. Rename discFolders→discFoldersRel, split firstTrackPath into firstTrackRelPath (for fromTag) and firstTrackAbsPath (for fromFFmpegTag), and switch path.Dir/Base/Ext for forward-slash safety. * refactor(artwork): build discFoldersRel directly and guard empty first track * refactor(artwork): migrate mediafile reader to MusicFS * refactor(artwork): migrate artist album-art lookup to MusicFS * refactor(artwork): remove temporary path-based shims All readers now use the FS-based fromTag and fromExternalFile directly, so the absolute-path adapters and the osDirectFS helper that backed them can go away. * test(artwork): rewrite e2e suite to use storagetest.FakeFS Switches from real-tempdir + local storage to FakeFS via the storage registry. Adds a proper multi-disc scenario using the disc tag, which previously required curated MP3 fixtures we did not have. * test(artwork): use maps.Copy in trackFile tag merge Lint cleanup: replace the manual map-copy loop flagged by mapsloop. * test(artwork): reuse tests.MockFFmpeg in e2e harness Replace the hand-rolled noopFFmpeg stub with tests.NewMockFFmpeg, which already satisfies the full ffmpeg.FFmpeg interface and won't drift when new methods are added. Also tie imageBytes to imageFile so they cannot silently disagree on the on-disk encoding. * test(artwork): add e2e scenarios from artwork documentation Covers the behaviors documented at https://www.navidrome.org/docs/usage/library/artwork/: - Album: folder.*/front.* fallbacks and priority order with cover.*. - Disc: cd*.* match, cover.* inside disc folder, DiscArtPriority="" skip path, the documented multi-disc layout, and the discsubtitle keyword. - MediaFile: disc-level fallback for multi-disc tracks and album-level fallback for single-disc tracks (doc section "MediaFiles" items 2-3). - Artist: album/artist.* lookup via libFS (passes). The artist-folder branch is XIt-marked because fromArtistFolder still calls os.DirFS directly on an absolute path and can't read from a FakeFS-backed library — migrating that to storage.MusicFS is a follow-up. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): scope artist folder traversal to library root Route fromArtistFolder reads through storage.MusicFS and bound the parent-directory walk at the library root. This keeps artwork resolution scoped to the configured library and unblocks FakeFS-backed e2e scenarios that depend on the artist folder. Also consolidate the libraryFS + core.AbsolutePath pairing (used by three readers) into a single libraryFSAndRoot helper. * test(artwork): add ASCII file-tree diagrams to e2e scenarios Each It/PIt block now shows the on-disk layout it exercises, with arrows indicating which file wins (or should win, for the known-bug PIt cases). Makes scenarios readable at a glance without having to parse the MapFS map. * test(artwork): add e2e tests for playlist and radio artwork resolution Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): enhance e2e tests with real MP3 fixtures for embedded artwork Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): add support for animated WebP encoder detection and fallback handling Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): cover additional edge cases in e2e suite Add high-value scenarios uncovered by the existing specs: - Album: three-way basename tie (unsuffixed wins), unknown pattern in CoverArtPriority is skipped, embedded-first with no embedded art falls through. - Disc: discsubtitle with no matching image falls through. - Artist: ArtistArtPriority can reach images via album/<pattern>. - Playlist: generates a 2x2 tiled cover from album art when the playlist has no uploaded/sidecar/external image. New helper realPNG() produces real taglib/image-decodable bytes so the tiled-cover test can exercise the generator's decode + compose path. * test(artwork): refactor image upload logic in e2e tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): simplify animated WebP encoder check by removing context parameter Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): normalize rel path for fs.Glob on Windows filepath.Rel returns backslash-separated paths on Windows, but fs.Glob and path.Join require forward slashes. Convert with filepath.ToSlash after computing the relative path and use path.Dir for the parent walk so the artist-folder lookup works cross-platform. * fix(ffmpeg): retry animated WebP probe on transient failure The probe previously used the caller's request context inside sync.Once, so a single cancelled first request would permanently disable animated WebP for the rest of the process. Switch to a mutex + probed flag, use a fresh background context with its own timeout, and only cache the result when the probe actually succeeds. * test(ffmpeg): reset ffOnce so ConvertAnimatedImage test is order-independent The ConvertAnimatedImage stand-in test sets ffmpegPath directly but does not reset ffOnce. If ffmpegCmd() has not been called earlier in the test process, the next call inside hasAnimatedWebPEncoder runs ffOnce.Do and re-resolves the real ffmpeg binary, overwriting the stand-in and breaking the test. Reset ffOnce and conf.Server.FFmpegPath alongside the other globals to pin resolution to the stand-in. * test(artwork): unblock Windows CI — forward-slash fs paths and suite-level DB lifetime The internal artwork test planted a Windows absolute path (backslashes) into Folder.Path and then fed it through libFS.Open, which fs.ValidPath rejects. Rooting the testfile library at the temp dir directly and using filepath.ToSlash keeps the path model library-relative and forward-slash, matching production. The e2e suite opened a per-spec DB in a per-spec TempDir, but the go-sqlite3 singleton kept the file open across specs. Ginkgo's per-spec TempDir cleanup then tried to unlink a file still held by that handle — fine on POSIX, fails on Windows. Moving the DB to a suite-level tempdir and closing it in AfterSuite avoids the race. * test(artwork): keep Windows drive letters intact in testfile library URLs url.Parse on `testfile://C:/path` reads `C` as the host and the path loses the drive letter, so Windows libFS lookups go to `/path` and fail. testFileLibPath now prepends a `/` when the OS path has no leading slash, and the testfile constructor strips that extra slash back off before handing the path to os.Stat / os.DirFS. * refactor(artwork): consolidate libFS + root into libraryView helper Collapses the per-reader libFS/libPath/rootFolder/firstTrackAbsPath fields into a single libraryView{FS, absRoot} with an Abs(rel) method. Also folds the two library lookups (ds.Library.Get + core.AbsolutePath) into one, and uses mf.Path directly instead of stripping libRoot off an absolute path. * refactor(ffmpeg): replace hasAnimatedWebPEncoder with encoderProbe for state management Signed-off-by: Deluan <deluan@navidrome.org> * fix: escape artist folder names in artwork glob Escape glob metacharacters in the library-relative artist folder path before composing the fs.Glob pattern for artist image lookup. This preserves literal folder names such as Artist [Live] while keeping the configured filename pattern behavior unchanged, and adds a regression test for bracketed artist folders. Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): correct test path assertions after MusicFS migration Source functions (fromTag, fromExternalFile) now return forward-slash fs.FS-relative paths, so test assertions should compare against plain forward-slash strings, not filepath.FromSlash(). The artistArtPriority test needs filepath.FromSlash() on the suffix because findImageInFolder returns OS-native absolute paths via filepath.Join. * fix(artwork): normalize path separators in artistArtPriority assertion The two table entries exercise different code paths: entry 1 goes through fromArtistFolder (returns OS-native paths via filepath.Join), while entry 2 goes through fromExternalFile (returns forward-slash fs.FS paths). Using filepath.FromSlash on the expected value only works for entry 1. Normalize the actual path to forward slashes with filepath.ToSlash so a single HaveSuffix assertion works for both code paths on all platforms. --------- Signed-off-by: Deluan <deluan@navidrome.org>
268 lines
7.6 KiB
Go
268 lines
7.6 KiB
Go
package artwork
|
|
|
|
import (
|
|
"context"
|
|
"crypto/md5"
|
|
"fmt"
|
|
"io"
|
|
"path"
|
|
"path/filepath"
|
|
"strconv"
|
|
"strings"
|
|
"time"
|
|
|
|
"github.com/Masterminds/squirrel"
|
|
"github.com/navidrome/navidrome/conf"
|
|
"github.com/navidrome/navidrome/core/ffmpeg"
|
|
"github.com/navidrome/navidrome/log"
|
|
"github.com/navidrome/navidrome/model"
|
|
)
|
|
|
|
type discArtworkReader struct {
|
|
cacheKey
|
|
a *artwork
|
|
album model.Album
|
|
discNumber int
|
|
imgFiles []string // library-relative, forward-slash, no leading slash
|
|
discFoldersRel map[string]bool // library-relative folder paths
|
|
isMultiFolder bool
|
|
firstTrackRel string // library-relative; for fromTag / ffmpeg via lib.Abs
|
|
lib libraryView
|
|
updatedAt *time.Time
|
|
}
|
|
|
|
func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID) (*discArtworkReader, error) {
|
|
albumID, discNumber, err := model.ParseDiscArtworkID(artID.ID)
|
|
if err != nil {
|
|
return nil, fmt.Errorf("invalid disc artwork id '%s': %w", artID.ID, err)
|
|
}
|
|
|
|
al, err := a.ds.Album(ctx).Get(albumID)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
_, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, *al)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
// Query mediafiles for this album + disc to find folder associations and first track
|
|
mfs, err := a.ds.MediaFile(ctx).GetAll(model.QueryOptions{
|
|
Sort: "track_number",
|
|
Order: "ASC",
|
|
Filters: squirrel.Eq{"album_id": albumID, "disc_number": discNumber},
|
|
})
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
lib, err := loadLibraryView(ctx, a.ds, al.LibraryID)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
// Build disc folder set and find first track. mf.Path is already library-relative.
|
|
var firstTrackRel string
|
|
allFolderIDs := make(map[string]bool)
|
|
for _, mf := range mfs {
|
|
allFolderIDs[mf.FolderID] = true
|
|
if firstTrackRel == "" {
|
|
firstTrackRel = filepath.ToSlash(mf.Path)
|
|
}
|
|
}
|
|
|
|
// Resolve folder IDs to library-relative paths
|
|
discFoldersRel := make(map[string]bool)
|
|
if len(allFolderIDs) > 0 {
|
|
folderIDs := make([]string, 0, len(allFolderIDs))
|
|
for id := range allFolderIDs {
|
|
folderIDs = append(folderIDs, id)
|
|
}
|
|
folders, err := a.ds.Folder(ctx).GetAll(model.QueryOptions{
|
|
Filters: squirrel.Eq{"folder.id": folderIDs},
|
|
})
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
for _, f := range folders {
|
|
rel := strings.TrimPrefix(path.Join(f.Path, f.Name), "/")
|
|
discFoldersRel[rel] = true
|
|
}
|
|
}
|
|
|
|
isMultiFolder := len(al.FolderIDs) > 1
|
|
|
|
r := &discArtworkReader{
|
|
a: a,
|
|
album: *al,
|
|
discNumber: discNumber,
|
|
imgFiles: imgFiles,
|
|
discFoldersRel: discFoldersRel,
|
|
isMultiFolder: isMultiFolder,
|
|
firstTrackRel: firstTrackRel,
|
|
lib: lib,
|
|
updatedAt: imagesUpdatedAt,
|
|
}
|
|
r.cacheKey.artID = artID
|
|
if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) {
|
|
r.cacheKey.lastUpdate = *r.updatedAt
|
|
} else {
|
|
r.cacheKey.lastUpdate = al.UpdatedAt
|
|
}
|
|
return r, nil
|
|
}
|
|
|
|
func (d *discArtworkReader) Key() string {
|
|
hash := md5.Sum([]byte(conf.Server.DiscArtPriority))
|
|
return fmt.Sprintf(
|
|
"%s.%x",
|
|
d.cacheKey.Key(),
|
|
hash,
|
|
)
|
|
}
|
|
|
|
func (d *discArtworkReader) LastUpdated() time.Time {
|
|
return d.lastUpdate
|
|
}
|
|
|
|
func (d *discArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
|
|
var ff = d.fromDiscArtPriority(ctx, d.a.ffmpeg, conf.Server.DiscArtPriority)
|
|
// Fallback to album cover art
|
|
albumArtID := model.NewArtworkID(model.KindAlbumArtwork, d.album.ID, &d.album.UpdatedAt)
|
|
ff = append(ff, fromAlbum(ctx, d.a, albumArtID))
|
|
return selectImageReader(ctx, d.cacheKey.artID, ff...)
|
|
}
|
|
|
|
func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc {
|
|
var ff []sourceFunc
|
|
for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") {
|
|
pattern = strings.TrimSpace(pattern)
|
|
switch {
|
|
case pattern == "embedded":
|
|
ff = append(ff,
|
|
fromTag(ctx, d.lib.FS, d.firstTrackRel),
|
|
fromFFmpegTag(ctx, ffmpeg, d.lib.Abs(d.firstTrackRel)),
|
|
)
|
|
case pattern == "external":
|
|
// Not supported for disc art, silently ignore
|
|
case pattern == "discsubtitle":
|
|
if subtitle := strings.TrimSpace(d.album.Discs[d.discNumber]); subtitle != "" {
|
|
ff = append(ff, d.fromDiscSubtitle(ctx, subtitle))
|
|
}
|
|
case len(d.imgFiles) > 0:
|
|
ff = append(ff, d.fromExternalFile(ctx, pattern))
|
|
}
|
|
}
|
|
return ff
|
|
}
|
|
|
|
// fromDiscSubtitle returns a sourceFunc that matches image files whose stem
|
|
// (filename without extension) equals the disc subtitle (case-insensitive).
|
|
func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle string) sourceFunc {
|
|
return func() (io.ReadCloser, string, error) {
|
|
for _, file := range d.imgFiles {
|
|
name := path.Base(file)
|
|
stem := strings.TrimSuffix(name, path.Ext(name))
|
|
if !strings.EqualFold(stem, subtitle) {
|
|
continue
|
|
}
|
|
f, err := d.lib.FS.Open(file)
|
|
if err != nil {
|
|
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
|
continue
|
|
}
|
|
return f, file, nil
|
|
}
|
|
return nil, "", fmt.Errorf("disc %d: no image file matching subtitle %q", d.discNumber, subtitle)
|
|
}
|
|
}
|
|
|
|
// globMetaChars holds the substitution metacharacters understood by
|
|
// filepath.Match. The '\' escape character is intentionally excluded:
|
|
// disc art patterns come from user config and never include escaped
|
|
// metachars in practice, and treating '\' as a metachar would misalign
|
|
// the literal-prefix extraction in extractDiscNumber.
|
|
const globMetaChars = "*?["
|
|
|
|
// extractDiscNumber parses the disc number from a filename matched by a
|
|
// filepath.Match-style glob pattern.
|
|
//
|
|
// Both pattern and filename must already be lowercased by the caller, which
|
|
// is also expected to have verified that filepath.Match(pattern, filename)
|
|
// is true before calling this function.
|
|
func extractDiscNumber(pattern, filename string) (int, bool) {
|
|
metaIdx := strings.IndexAny(pattern, globMetaChars)
|
|
if metaIdx < 0 {
|
|
return 0, false
|
|
}
|
|
prefix := pattern[:metaIdx]
|
|
if !strings.HasPrefix(filename, prefix) {
|
|
return 0, false
|
|
}
|
|
|
|
start := len(prefix)
|
|
end := start
|
|
for end < len(filename) && filename[end] >= '0' && filename[end] <= '9' {
|
|
end++
|
|
}
|
|
if end == start {
|
|
return 0, false
|
|
}
|
|
num, err := strconv.Atoi(filename[start:end])
|
|
if err != nil {
|
|
return 0, false
|
|
}
|
|
return num, true
|
|
}
|
|
|
|
// fromExternalFile returns a sourceFunc that matches image files against a glob
|
|
// pattern. A numbered filename whose number equals the target disc wins over
|
|
// any unnumbered candidate; callers must pass a lowercase pattern.
|
|
func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc {
|
|
isLiteral := !strings.ContainsAny(pattern, globMetaChars)
|
|
return func() (io.ReadCloser, string, error) {
|
|
var fallbacks []string
|
|
for _, file := range d.imgFiles {
|
|
name := strings.ToLower(path.Base(file))
|
|
match, err := filepath.Match(pattern, name)
|
|
if err != nil {
|
|
log.Warn(ctx, "Error matching disc art file to pattern", "pattern", pattern, "file", file)
|
|
continue
|
|
}
|
|
if !match {
|
|
continue
|
|
}
|
|
|
|
if !isLiteral {
|
|
if num, hasNum := extractDiscNumber(pattern, name); hasNum {
|
|
if num != d.discNumber {
|
|
continue
|
|
}
|
|
f, err := d.lib.FS.Open(file)
|
|
if err != nil {
|
|
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
|
continue
|
|
}
|
|
return f, file, nil
|
|
}
|
|
}
|
|
|
|
if d.isMultiFolder && !d.discFoldersRel[path.Dir(file)] {
|
|
continue
|
|
}
|
|
fallbacks = append(fallbacks, file)
|
|
}
|
|
|
|
for _, file := range fallbacks {
|
|
f, err := d.lib.FS.Open(file)
|
|
if err != nil {
|
|
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
|
continue
|
|
}
|
|
return f, file, nil
|
|
}
|
|
return nil, "", fmt.Errorf("disc %d: pattern '%s' not matched by files", d.discNumber, pattern)
|
|
}
|
|
}
|