diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index fbc5fd2c2..91028521d 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -318,16 +318,21 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int return nil, err } // Single-disc albums run the chain too: a disc can carry art distinct from the album cover. - selectImage := func() (io.ReadCloser, string, error) { - funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority) - return selectImageReader(ctx, artID, funcs...) + selectImage := func() (io.ReadCloser, error) { + res, err := dr.selectImage(ctx, s.ffmpeg, conf.Server.DiscArtPriority, &chainState{}) + if err != nil { + return nil, err + } + if res.reader == nil { + return nil, fmt.Errorf("could not get `%s` cover art for %s: %w", artID.Kind, artID, ErrUnavailable) + } + return res.reader, nil } albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID} // Disc art has no state row, hence no content hash: keying on id, album mtime and // DiscArtPriority lets a warm cache answer without running the chain or touching the disk. key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority) - img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square, - func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err }) + img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square, selectImage) if err != nil { if errors.Is(err, context.Canceled) { return nil, err diff --git a/core/artwork/disc.go b/core/artwork/disc.go index 81ce617ce..a7d1dcb72 100644 --- a/core/artwork/disc.go +++ b/core/artwork/disc.go @@ -154,14 +154,48 @@ func (d *discArtworkReader) discCandidates(ctx context.Context, ffmpeg ffmpeg.FF return cc } -// fromDiscArtPriority flattens the candidates for the serving path, which needs the sources in -// order and has no walk to report. -func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc { - var ff []sourceFunc +// selectImage walks the DiscArtPriority entries and returns the first that yields an image. +// chain records the walk; the serving path passes an untraced one and pays nothing for it. +func (d *discArtworkReader) selectImage(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string, + chain *chainState) (resolution, error) { for _, c := range d.discCandidates(ctx, ffmpeg, priority) { - ff = append(ff, c.sources...) + if err := ctx.Err(); err != nil { + return resolution{}, err + } + if c.skip != "" { + chain.record(c.pattern, OutcomeSkipped, c.skip) + continue + } + res, ok := d.openCandidate(ctx, c) + if res, ok = chain.try(c.pattern, res, ok); ok { + return res, nil + } } - return ff + 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 diff --git a/core/artwork/disc_test.go b/core/artwork/disc_test.go index 8264ee27b..f3e382582 100644 --- a/core/artwork/disc_test.go +++ b/core/artwork/disc_test.go @@ -179,16 +179,16 @@ var _ = Describe("Disc Artwork Reader", func() { lib: libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir}, } - ff := reader.fromDiscArtPriority(ctx, nil, "disc*.*, cover.*") - Expect(ff).To(HaveLen(2)) - r, path, err := ff[0]() + 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() - ff = reader.fromDiscArtPriority(ctx, nil, "cover.*, disc*.*") - Expect(ff).To(HaveLen(2)) - r, path, err = ff[0]() + 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() @@ -428,64 +428,89 @@ var _ = Describe("Disc Artwork Reader", func() { }) Describe("discArtworkReader", func() { - Describe("fromDiscArtPriority", func() { - var ( - reader *discArtworkReader - tmpDir string - ) + var ( + reader *discArtworkReader + tmpDir string + ) - BeforeEach(func() { - tmpDir = GinkgoT().TempDir() - reader = &discArtworkReader{ - discNumber: 2, - isMultiFolder: true, - discFoldersRel: map[string]bool{"music/album/cd2": true}, - imgFiles: []string{ - "music/album/cd1/disc.jpg", - "music/album/cd2/disc.jpg", - "music/album/cd2/disc2.jpg", - }, - firstTrackRel: "music/album/cd2/track1.flac", - lib: libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir}, - } + BeforeEach(func() { + tmpDir = GinkgoT().TempDir() + reader = &discArtworkReader{ + discNumber: 2, + isMultiFolder: true, + discFoldersRel: map[string]bool{"music/album/cd2": true}, + imgFiles: []string{ + "music/album/cd1/disc.jpg", + "music/album/cd2/disc.jpg", + "music/album/cd2/disc2.jpg", + }, + firstTrackRel: "music/album/cd2/track1.flac", + lib: libraryView{FS: osDirFS{os.DirFS(tmpDir)}, absRoot: tmpDir}, + } + }) + + Describe("selectImage", func() { + It("abandons the walk when the context is cancelled", func() { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + res, err := reader.selectImage(ctx, nil, "disc*.*, cover.*", &chainState{}) + + Expect(err).To(MatchError(context.Canceled)) + Expect(res.reader).To(BeNil()) }) + }) + Describe("discCandidates", func() { It("returns source funcs for glob patterns", func() { - ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*") - Expect(ff).To(HaveLen(1)) + cc := reader.discCandidates(context.Background(), nil, "disc*.*") + Expect(cc).To(HaveLen(1)) + Expect(cc[0].sources).To(HaveLen(1)) }) It("returns source funcs for embedded pattern", func() { - ff := reader.fromDiscArtPriority(context.Background(), nil, "embedded") - Expect(ff).To(HaveLen(2)) // fromTag + fromFFmpegTag + 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() { - ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*, cd*.*, embedded") - Expect(ff).To(HaveLen(4)) // disc*.* + cd*.* + fromTag + fromFFmpegTag + 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)) }) - It("ignores 'external' pattern silently", func() { - ff := reader.fromDiscArtPriority(context.Background(), nil, "external") - Expect(ff).To(HaveLen(0)) + It("skips an empty entry rather than building a glob that matches nothing", func() { + cc := reader.discCandidates(context.Background(), nil, "disc*.*,") + Expect(cc).To(HaveLen(1)) }) - It("returns no source funcs when imgFiles is empty and pattern is not embedded", func() { - reader.imgFiles = nil - ff := reader.fromDiscArtPriority(context.Background(), nil, "disc*.*") - Expect(ff).To(HaveLen(0)) - }) + // The skip reasons below are what `artwork explain` prints, so an entry that maps to no + // source must say why instead of vanishing from the walk. + DescribeTable("keeps an entry that maps to no source, with its reason", + func(setup func(), priority, reason string) { + setup() + cc := reader.discCandidates(context.Background(), nil, priority) + Expect(cc).To(HaveLen(1)) + Expect(cc[0].sources).To(BeEmpty()) + Expect(cc[0].skip).To(Equal(reason)) + }, + Entry("external is unsupported", func() {}, "external", + "external sources are not supported for disc artwork"), + Entry("no images in the album folder", func() { reader.imgFiles = nil }, "disc*.*", + "no images in album folder"), + Entry("the disc has no subtitle", + func() { reader.album = model.Album{Discs: model.Discs{2: ""}} }, "discsubtitle", + "disc has no subtitle"), + ) It("returns source func for discsubtitle pattern", func() { reader.album = model.Album{Discs: model.Discs{2: "Bonus Tracks"}} - ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle") - Expect(ff).To(HaveLen(1)) - }) - - It("returns no source func for discsubtitle when disc has no subtitle", func() { - reader.album = model.Album{Discs: model.Discs{2: ""}} - ff := reader.fromDiscArtPriority(context.Background(), nil, "discsubtitle") - Expect(ff).To(HaveLen(0)) + cc := reader.discCandidates(context.Background(), nil, "discsubtitle") + Expect(cc).To(HaveLen(1)) + Expect(cc[0].sources).To(HaveLen(1)) }) }) }) diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 4dccd0c66..da07a75e5 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -9,7 +9,6 @@ import ( "io/fs" "net/url" "os" - "path/filepath" "strings" "github.com/Masterminds/squirrel" @@ -463,34 +462,7 @@ func (r *resolver) resolveDisc(ctx context.Context, id string) (resolution, erro return resolution{}, err } chain := chainState{trace: traceFrom(ctx)} - for _, c := range dr.discCandidates(ctx, r.ffmpeg, conf.Server.DiscArtPriority) { - if c.skip != "" { - chain.record(c.pattern, OutcomeSkipped, c.skip) - continue - } - res, ok := openDiscCandidate(c, dr.lib) - if res, ok = chain.try(c.pattern, res, ok); ok { - return res, nil - } - } - return chain.exhausted(), nil -} - -func openDiscCandidate(c discCandidate, lib libraryView) (resolution, bool) { - source := "folder" - if c.pattern == "embedded" { - source = "embedded" - } - for _, sf := range c.sources { - if rd, path, _ := sf(); rd != nil { - // The disc sources disagree on this: only ffmpeg hands back an absolute path. - if !filepath.IsAbs(path) { - path = lib.Abs(path) - } - return resolution{reader: rd, source: source, sourcePath: path}, true - } - } - return resolution{}, false + return dr.selectImage(ctx, r.ffmpeg, conf.Server.DiscArtPriority, &chain) } // resolveExternalStep runs a single external sourceFunc through the named gate. extErr excludes diff --git a/core/artwork/sources.go b/core/artwork/sources.go index 885ca03cf..78b7dd68d 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -27,23 +27,6 @@ import ( // to open it is not evidence the entity has no artwork, so callers must not settle on absent. var errSourceUnreadable = errors.New("artwork source unreadable") -func selectImageReader(ctx context.Context, artID model.ArtworkID, extractFuncs ...sourceFunc) (io.ReadCloser, string, error) { - for _, f := range extractFuncs { - if ctx.Err() != nil { - return nil, "", ctx.Err() - } - start := time.Now() - r, path, err := f() - if r != nil { - msg := fmt.Sprintf("Artwork: Found %s artwork", artID.Kind) - log.Debug(ctx, msg, "artID", artID, "path", path, "source", f, "elapsed", time.Since(start)) - return r, path, nil - } - log.Trace(ctx, "Artwork: Failed trying to extract artwork", "artID", artID, "source", f, "elapsed", time.Since(start), err) - } - return nil, "", fmt.Errorf("could not get `%s` cover art for %s: %w", artID.Kind, artID, ErrUnavailable) -} - type sourceFunc func() (r io.ReadCloser, path string, err error) func (f sourceFunc) String() string {