diff --git a/core/artwork/disc.go b/core/artwork/disc.go index a7d1dcb72..acd8a3740 100644 --- a/core/artwork/disc.go +++ b/core/artwork/disc.go @@ -117,11 +117,14 @@ func newDiscArtworkReader(ctx context.Context, ds model.DataStore, artID model.A // all, so a chain walk can say why instead of leaving a configured entry unaccounted for. type discCandidate struct { pattern string - sources []sourceFunc + resolve func() (resolution, bool) skip string } func (d *discArtworkReader) discCandidates(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []discCandidate { + folder := func(sf sourceFunc) func() (resolution, bool) { + return func() (resolution, bool) { return resolveFolderSource(d.lib, sf) } + } var cc []discCandidate for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") { pattern = strings.TrimSpace(pattern) @@ -131,9 +134,8 @@ func (d *discArtworkReader) discCandidates(ctx context.Context, ffmpeg ffmpeg.FF c := discCandidate{pattern: pattern} switch { case pattern == "embedded": - c.sources = []sourceFunc{ - fromTag(ctx, d.lib.FS, d.firstTrackRel), - fromFFmpegTag(ctx, ffmpeg, d.lib.Abs(d.firstTrackRel)), + c.resolve = func() (resolution, bool) { + return resolveEmbedded(ctx, d.lib, ffmpeg, d.firstTrackRel) } case pattern == externalCandidate: c.skip = "external sources are not supported for disc artwork" @@ -142,12 +144,12 @@ func (d *discArtworkReader) discCandidates(ctx context.Context, ffmpeg ffmpeg.FF if subtitle == "" { c.skip = "disc has no subtitle" } else { - c.sources = []sourceFunc{d.fromDiscSubtitle(ctx, subtitle)} + c.resolve = folder(d.fromDiscSubtitle(ctx, subtitle)) } case len(d.imgFiles) == 0: c.skip = "no images in album folder" default: - c.sources = []sourceFunc{d.fromExternalFile(ctx, pattern)} + c.resolve = folder(d.fromExternalFile(ctx, pattern)) } cc = append(cc, c) } @@ -166,7 +168,11 @@ func (d *discArtworkReader) selectImage(ctx context.Context, ffmpeg ffmpeg.FFmpe chain.record(c.pattern, OutcomeSkipped, c.skip) continue } - res, ok := d.openCandidate(ctx, c) + start := time.Now() + res, ok := c.resolve() + log.Trace(ctx, "Artwork: Tried a disc artwork candidate", "albumID", d.album.ID, + "disc", d.discNumber, "pattern", c.pattern, "hit", ok, "path", res.sourcePath, + "elapsed", time.Since(start)) if res, ok = chain.try(c.pattern, res, ok); ok { return res, nil } @@ -174,30 +180,6 @@ func (d *discArtworkReader) selectImage(ctx context.Context, ffmpeg ffmpeg.FFmpe return chain.exhausted(), nil } -func (d *discArtworkReader) openCandidate(ctx context.Context, c discCandidate) (resolution, bool) { - source := "folder" - if c.pattern == "embedded" { - source = "embedded" - } - for _, sf := range c.sources { - start := time.Now() - rd, path, err := sf() - if rd == nil { - log.Trace(ctx, "Artwork: Failed trying to extract disc artwork", "albumID", d.album.ID, - "disc", d.discNumber, "pattern", c.pattern, "source", sf, "elapsed", time.Since(start), err) - continue - } - // The disc sources disagree on this: only ffmpeg hands back an absolute path. - if !filepath.IsAbs(path) { - path = d.lib.Abs(path) - } - log.Debug(ctx, "Artwork: Found disc artwork", "albumID", d.album.ID, "disc", d.discNumber, - "pattern", c.pattern, "path", path, "elapsed", time.Since(start)) - return resolution{reader: rd, source: source, sourcePath: path}, true - } - return resolution{}, false -} - // 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 { diff --git a/core/artwork/disc_test.go b/core/artwork/disc_test.go index f3e382582..8852741cf 100644 --- a/core/artwork/disc_test.go +++ b/core/artwork/disc_test.go @@ -6,6 +6,8 @@ import ( "path/filepath" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/slice" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -181,17 +183,17 @@ var _ = Describe("Disc Artwork Reader", func() { cc := reader.discCandidates(ctx, nil, "disc*.*, cover.*") Expect(cc).To(HaveLen(2)) - r, path, err := cc[0].sources[0]() - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal(f2)) - r.Close() + res, ok := cc[0].resolve() + Expect(ok).To(BeTrue()) + Expect(res.sourcePath).To(Equal(reader.lib.Abs(f2))) + res.reader.Close() cc = reader.discCandidates(ctx, nil, "cover.*, disc*.*") Expect(cc).To(HaveLen(2)) - r, path, err = cc[0].sources[0]() - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal(f1)) - r.Close() + res, ok = cc[0].resolve() + Expect(ok).To(BeTrue()) + Expect(res.sourcePath).To(Equal(reader.lib.Abs(f1))) + res.reader.Close() }) DescribeTable("numbered match wins over shared fallback within a pattern", @@ -459,27 +461,47 @@ var _ = Describe("Disc Artwork Reader", func() { Expect(err).To(MatchError(context.Canceled)) Expect(res.reader).To(BeNil()) }) + + // "the track has no embedded art" and "the track is there but unreadable" are the two + // answers a wrong-artwork report needs told apart; only the second is worth retrying. + It("reports a track it cannot parse as unreadable, not as a miss", func() { + trace := &ChainTrace{} + track := filepath.Join(tmpDir, filepath.FromSlash(reader.firstTrackRel)) + Expect(os.MkdirAll(filepath.Dir(track), 0755)).To(Succeed()) + Expect(os.WriteFile(track, []byte("not audio"), 0600)).To(Succeed()) + + res, err := reader.selectImage(context.Background(), tests.NewMockFFmpeg(""), "embedded", + &chainState{trace: trace}) + + Expect(err).ToNot(HaveOccurred()) + Expect(res.localError).To(BeTrue()) + Expect(trace.Steps()).To(Equal([]TraceStep{{Candidate: "embedded", Outcome: OutcomeUnreadable}})) + }) + + It("reports a disc with no tracks to read as a miss", func() { + trace := &ChainTrace{} + reader.firstTrackRel = "" + + res, err := reader.selectImage(context.Background(), tests.NewMockFFmpeg(""), "embedded", + &chainState{trace: trace}) + + Expect(err).ToNot(HaveOccurred()) + Expect(res.localError).To(BeFalse(), "there was nothing to read, so nothing failed to read") + Expect(trace.Steps()).To(Equal([]TraceStep{{Candidate: "embedded", Outcome: OutcomeMiss}})) + }) }) Describe("discCandidates", func() { - It("returns source funcs for glob patterns", func() { + It("returns a resolvable candidate for glob patterns", func() { cc := reader.discCandidates(context.Background(), nil, "disc*.*") Expect(cc).To(HaveLen(1)) - Expect(cc[0].sources).To(HaveLen(1)) + Expect(cc[0].resolve).ToNot(BeNil()) }) - It("returns source funcs for embedded pattern", func() { - cc := reader.discCandidates(context.Background(), nil, "embedded") - Expect(cc).To(HaveLen(1)) - Expect(cc[0].sources).To(HaveLen(2)) // fromTag + fromFFmpegTag - }) - - It("handles multiple comma-separated patterns", func() { + It("returns one candidate per entry, in order", func() { cc := reader.discCandidates(context.Background(), nil, "disc*.*, cd*.*, embedded") - Expect(cc).To(HaveLen(3)) - Expect(cc[0].sources).To(HaveLen(1)) - Expect(cc[1].sources).To(HaveLen(1)) - Expect(cc[2].sources).To(HaveLen(2)) + Expect(slice.Map(cc, func(c discCandidate) string { return c.pattern })). + To(Equal([]string{"disc*.*", "cd*.*", "embedded"})) }) It("skips an empty entry rather than building a glob that matches nothing", func() { @@ -494,7 +516,7 @@ var _ = Describe("Disc Artwork Reader", func() { setup() cc := reader.discCandidates(context.Background(), nil, priority) Expect(cc).To(HaveLen(1)) - Expect(cc[0].sources).To(BeEmpty()) + Expect(cc[0].resolve).To(BeNil()) Expect(cc[0].skip).To(Equal(reason)) }, Entry("external is unsupported", func() {}, "external", @@ -510,7 +532,7 @@ var _ = Describe("Disc Artwork Reader", func() { reader.album = model.Album{Discs: model.Discs{2: "Bonus Tracks"}} cc := reader.discCandidates(context.Background(), nil, "discsubtitle") Expect(cc).To(HaveLen(1)) - Expect(cc[0].sources).To(HaveLen(1)) + Expect(cc[0].resolve).ToNot(BeNil()) }) }) }) diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index da07a75e5..518b6e100 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -511,14 +511,20 @@ func resolveEmbedded(ctx context.Context, lib libraryView, ffm ffmpeg.FFmpeg, em return resolution{localError: unreadable}, false } -func resolveFolderFile(ctx context.Context, lib libraryView, imgFiles []string, pattern string) (resolution, bool) { - r, path, err := fromExternalFile(ctx, lib.FS, imgFiles, pattern)() +// resolveFolderSource turns a source that yields a library-relative image path into a folder +// resolution, keeping an existing-but-unopenable file distinct from an absent one. +func resolveFolderSource(lib libraryView, sf sourceFunc) (resolution, bool) { + r, path, err := sf() if r == nil { return resolution{localError: errors.Is(err, errSourceUnreadable)}, false } return resolution{reader: r, source: "folder", sourcePath: lib.Abs(path), refMtime: mtimeViaFS(lib.FS, path)}, true } +func resolveFolderFile(ctx context.Context, lib libraryView, imgFiles []string, pattern string) (resolution, bool) { + return resolveFolderSource(lib, fromExternalFile(ctx, lib.FS, imgFiles, pattern)) +} + func resolveArtistImageFolder(ar *model.Artist) (resolution, bool) { folder := conf.Server.ArtistImageFolder if folder == "" {