diff --git a/cmd/artwork.go b/cmd/artwork.go index fd3df3465..e2abd5784 100644 --- a/cmd/artwork.go +++ b/cmd/artwork.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" @@ -273,7 +274,7 @@ func runReprocess(ctx context.Context) { defer db.Init(ctx)() ds, ctx := getAdminContext(ctx) - if err := reprocessArtwork(ctx, ds, kinds, repositorySources(reprocessSources), + if err := reprocessArtwork(ctx, ds, kinds, repositorySources(reprocessSources), imageAgentCount(ds), reprocessDryRun, reprocessConfirm(reprocessYes, os.Stdin), os.Stdout); err != nil { log.Fatal(ctx, err) } @@ -322,13 +323,19 @@ func reprocessConfirm(yes bool, in io.Reader) confirmFunc { return promptConfirm(in) } -// externalEstimate is an upper bound: a higher-priority local candidate may win before the walk -// ever reaches an agent. +// externalEstimate is a floor, not a ceiling: plugin-provided agents are unregistered in a CLI that +// never starts the plugin manager, so the calls they would add are invisible here. func externalEstimate(n int64) string { if n == 0 { return "none" } - return fmt.Sprintf("up to %d", n) + return fmt.Sprintf("at least %d", n) +} + +// imageAgentCount counts only the built-in image agents, for the same reason. +func imageAgentCount(ds model.DataStore) artwork.ImageAgentCount { + ag := agents.GetAgents(ds, getPluginManager()) + return artwork.ImageAgentCount{Artist: len(ag.ArtistImageAgents()), Album: len(ag.AlbumImageAgents())} } func promptConfirm(in io.Reader) confirmFunc { @@ -379,7 +386,7 @@ func validateSources(q model.ArtworkQueueRepository, sources []string) error { // reprocessArtwork previews from CountBySource — rows matched — then reports what EnqueueBySource // actually inserted; the two differ because an already-queued row is left untouched. func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kind, sources []string, - dryRun bool, confirm confirmFunc, out io.Writer) error { + imageAgents artwork.ImageAgentCount, dryRun bool, confirm confirmFunc, out io.Writer) error { q := ds.ArtworkQueue(ctx) if err := validateSources(q, sources); err != nil { return err @@ -394,9 +401,7 @@ func reprocessArtwork(ctx context.Context, ds model.DataStore, kinds []model.Kin } matched[i] = n total += n - if artwork.MayFetchExternal(k) { - external += n - } + external += n * artwork.ExternalLookupsPerItem(k, imageAgents) } printReprocessPreview(out, kinds, matched, total, external, sources) diff --git a/cmd/artwork_test.go b/cmd/artwork_test.go index 85d6af9fe..b0ca17a82 100644 --- a/cmd/artwork_test.go +++ b/cmd/artwork_test.go @@ -269,10 +269,10 @@ var _ = Describe("promptConfirm", func() { BeforeEach(func() { out.Reset() }) - It("states the external cost and accepts an explicit yes", func() { + It("states the external cost as a floor and accepts an explicit yes", func() { Expect(promptConfirm(strings.NewReader("y\n"))(&out, 42, 7)).To(BeTrue()) Expect(out.String()).To(ContainSubstring("re-resolve 42 items")) - Expect(out.String()).To(ContainSubstring("7 external lookups")) + Expect(out.String()).To(ContainSubstring("at least 7 external lookups")) }) It("defaults to no on anything else", func() { @@ -309,6 +309,7 @@ var _ = Describe("reprocessArtwork", func() { var art *tests.MockArtworkRepo var queue *tests.MockArtworkQueueRepo var out strings.Builder + var imageAgents artwork.ImageAgentCount ctx := context.Background() kinds := []model.Kind{model.KindArtistArtwork, model.KindAlbumArtwork} accept := func(io.Writer, int64, int64) bool { return true } @@ -324,6 +325,7 @@ var _ = Describe("reprocessArtwork", func() { conf.Server.CoverArtPriority = "cover.*, external" conf.Server.ArtistArtPriority = "artist.*, external" conf.Server.EnableM3UExternalAlbumArt = false + imageAgents = artwork.ImageAgentCount{Artist: 1, Album: 1} ds = &tests.MockDataStore{} art = ds.Artwork(ctx).(*tests.MockArtworkRepo) queue = ds.ArtworkQueue(ctx).(*tests.MockArtworkQueueRepo) @@ -335,7 +337,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("previews the per-kind breakdown and queues nothing on a dry run", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, true, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("external:deezer")) Expect(out.String()).To(ContainSubstring("artist")) @@ -346,14 +348,14 @@ var _ = Describe("reprocessArtwork", func() { }) It("queues nothing when the operator declines", func() { - Expect(reprocessArtwork(ctx, ds, kinds, nil, false, decline, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, decline, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Aborted")) Expect(queue.Count()).To(BeZero()) }) It("queues the matching items at recheck priority, leaving their artwork state alone", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(2))) queued, err := queue.Get(model.KindAlbumArtwork, "al-1", model.ImageTypePrimary) @@ -368,7 +370,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("targets the absent state", func() { - Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{""}, imageAgents, false, accept, &out)).To(Succeed()) Expect(queue.Count()).To(Equal(int64(1))) _, err := queue.Get(model.KindArtistArtwork, "ar-2", model.ImageTypePrimary) @@ -379,7 +381,7 @@ var _ = Describe("reprocessArtwork", func() { Expect(queue.Enqueue(model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar-1", ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityBump})).To(Succeed()) - Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, false, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, kinds, []string{"external:deezer"}, imageAgents, false, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Queued 1 of 2 matched items")) Expect(out.String()).To(ContainSubstring("Already queued, left unchanged: 1")) @@ -390,7 +392,7 @@ var _ = Describe("reprocessArtwork", func() { }) It("stops at a selection that matches nothing instead of prompting", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, false, + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, false, func(io.Writer, int64, int64) bool { Fail("must not prompt when there is nothing to queue") return true @@ -401,23 +403,41 @@ var _ = Describe("reprocessArtwork", func() { }) It("reports an empty selection as a dry run when one was asked for", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, true, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindRadioArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Nothing matches")) Expect(out.String()).To(ContainSubstring("Dry run")) }) It("shows the external estimate on a dry run, which never reaches the prompt", func() { - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, true, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) - Expect(out.String()).To(ContainSubstring("External lookups: up to 2")) + Expect(out.String()).To(ContainSubstring("External lookups: at least 2")) + }) + + It("bills every agent per item, not one lookup per item", func() { + imageAgents = artwork.ImageAgentCount{Artist: 2, Album: 3} + var external int64 + capture := func(_ io.Writer, _, e int64) bool { external = e; return false } + + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, false, capture, &out)).To(Succeed()) + + Expect(external).To(Equal(int64(2*2+2*3)), "2 artists at 2 agents plus 2 albums at 3 agents") + Expect(out.String()).To(ContainSubstring("External lookups: at least 10")) + }) + + It("states the estimate as a floor, since plugin agents are invisible to the CLI", func() { + Expect(reprocessArtwork(ctx, ds, kinds, nil, imageAgents, true, accept, &out)).To(Succeed()) + + Expect(out.String()).To(ContainSubstring("External lookups: at least")) + Expect(out.String()).ToNot(ContainSubstring("up to")) }) It("says so when the selection needs no external lookup", func() { conf.Server.CoverArtPriority = "cover.*" put(model.KindPlaylistArtwork, "pl-1", "playlist") - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, true, accept, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, true, accept, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("External lookups: none")) }) @@ -429,20 +449,22 @@ var _ = Describe("reprocessArtwork", func() { var external int64 capture := func(_ io.Writer, _, e int64) bool { external = e; return false } - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, false, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(external).To(Equal(int64(1))) - Expect(out.String()).To(ContainSubstring("External lookups: up to 1")) + Expect(out.String()).To(ContainSubstring("External lookups: at least 1")) }) - It("counts playlists as external cost when their grid tiles walk an external album chain", func() { + It("bills a playlist for every album its grid samples, at every agent", func() { + imageAgents = artwork.ImageAgentCount{Album: 3} put(model.KindPlaylistArtwork, "pl-1", "playlist") var external int64 capture := func(_ io.Writer, _, e int64) bool { external = e; return false } - Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, false, capture, &out)).To(Succeed()) + Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindPlaylistArtwork}, nil, imageAgents, false, capture, &out)).To(Succeed()) - Expect(external).To(Equal(int64(1)), "sampling album art for the grid reaches the agents") + Expect(external).To(Equal(int64(artwork.PlaylistGridSamples*3)), + "one playlist samples 4 albums, each walking all 3 album agents") }) It("counts only the kinds that call an external agent as external cost", func() { @@ -451,14 +473,14 @@ var _ = Describe("reprocessArtwork", func() { capture := func(_ io.Writer, t, e int64) bool { total, external = t, e; return false } Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindAlbumArtwork, model.KindRadioArtwork}, - nil, false, capture, &out)).To(Succeed()) + nil, imageAgents, false, capture, &out)).To(Succeed()) Expect(total).To(Equal(int64(3))) Expect(external).To(Equal(int64(2)), "radio artwork never reaches an external agent") }) It("rejects an unknown source and names the ones in use", func() { - err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, true, accept, &out) + err := reprocessArtwork(ctx, ds, kinds, []string{"externa:deezer"}, imageAgents, true, accept, &out) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("externa:deezer")) @@ -470,7 +492,7 @@ var _ = Describe("reprocessArtwork", func() { It("accepts a source another kind uses, letting the empty selection report itself", func() { Expect(reprocessArtwork(ctx, ds, []model.Kind{model.KindArtistArtwork}, []string{"folder"}, - false, decline, &out)).To(Succeed()) + imageAgents, false, decline, &out)).To(Succeed()) Expect(out.String()).To(ContainSubstring("Nothing matches"), "a well-formed filter must not be reported as a typo because of the kinds selected") diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 456287d69..5c5a0331e 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -129,6 +129,33 @@ func MayFetchExternal(kind model.Kind) bool { } } +// ImageAgentCount is how many enabled agents provide artist and album images. +type ImageAgentCount struct{ Artist, Album int } + +// ExternalLookupsPerItem reports what resolving one item of this kind can cost: every image agent is +// tried, and a zero count still bills one, so agents the caller cannot see never read as free. +func ExternalLookupsPerItem(kind model.Kind, agents ImageAgentCount) int64 { + if !MayFetchExternal(kind) { + return 0 + } + switch kind { + case model.KindArtistArtwork: + return int64(max(agents.Artist, 1)) + case model.KindAlbumArtwork: + return int64(max(agents.Album, 1)) + case model.KindPlaylistArtwork: + var n int64 + if conf.Server.EnableM3UExternalAlbumArt { + n++ + } + if chainFetchesExternal(conf.Server.CoverArtPriority) { + n += PlaylistGridSamples * int64(max(agents.Album, 1)) + } + return n + } + return 0 +} + func chainFetchesExternal(priority string) bool { for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") { if strings.TrimSpace(pattern) == externalCandidate { @@ -286,6 +313,9 @@ func (r *resolver) resolveArtist(ctx context.Context, artistID string) (resoluti return chain.exhausted(), nil } +// PlaylistGridSamples is how many albums resolvePlaylist samples to build the generated grid. +const PlaylistGridSamples = 4 + // resolvePlaylist tries the uploaded image, the sidecar and ExternalImageURL, then a generated grid. func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (resolution, error) { pl, err := r.ds.Playlist(ctx).Get(playlistID) @@ -331,7 +361,8 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso } } - albumIDs, err := r.ds.Playlist(ctx).Tracks(pl.ID, false).GetAlbumIDs(model.QueryOptions{Max: 4, Sort: "random()"}) + albumIDs, err := r.ds.Playlist(ctx).Tracks(pl.ID, false). + GetAlbumIDs(model.QueryOptions{Max: PlaylistGridSamples, Sort: "random()"}) if err != nil { return resolution{}, err } @@ -357,7 +388,7 @@ func (r *resolver) resolvePlaylist(ctx context.Context, playlistID string) (reso if decErr == nil { tiles = append(tiles, tile) } - if len(tiles) == 4 { + if len(tiles) == PlaylistGridSamples { break } } diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index 634f7e604..0f61e57b4 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -707,3 +707,53 @@ var _ = Describe("MayFetchExternal", func() { Expect(MayFetchExternal(model.KindMediaFileArtwork)).To(BeFalse()) }) }) + +var _ = Describe("ExternalLookupsPerItem", func() { + count := ImageAgentCount{Artist: 3, Album: 2} + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.CoverArtPriority = "cover.*, external" + conf.Server.ArtistArtPriority = "artist.*, external" + conf.Server.EnableM3UExternalAlbumArt = false + }) + + It("bills one call per agent, since the walk only stops early on a hit", func() { + Expect(ExternalLookupsPerItem(model.KindArtistArtwork, count)).To(Equal(int64(3))) + Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, count)).To(Equal(int64(2))) + }) + + It("bills a playlist for every album its grid samples", func() { + Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)). + To(Equal(int64(PlaylistGridSamples) * 2)) + }) + + It("adds the m3u image fetch on top of the grid", func() { + conf.Server.EnableM3UExternalAlbumArt = true + Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)). + To(Equal(int64(PlaylistGridSamples)*2 + 1)) + }) + + It("bills only the m3u fetch when the album chain stays local", func() { + conf.Server.CoverArtPriority = "cover.*" + conf.Server.EnableM3UExternalAlbumArt = true + Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).To(Equal(int64(1))) + }) + + It("still bills a call when no agent is visible, which plugins never are offline", func() { + none := ImageAgentCount{} + Expect(ExternalLookupsPerItem(model.KindArtistArtwork, none)).To(Equal(int64(1))) + Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, none)).To(Equal(int64(1))) + Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, none)). + To(Equal(int64(PlaylistGridSamples))) + }) + + It("is zero whenever the kind reaches no agent at all", func() { + conf.Server.CoverArtPriority = "cover.*" + conf.Server.ArtistArtPriority = "artist.*" + Expect(ExternalLookupsPerItem(model.KindArtistArtwork, count)).To(BeZero()) + Expect(ExternalLookupsPerItem(model.KindAlbumArtwork, count)).To(BeZero()) + Expect(ExternalLookupsPerItem(model.KindPlaylistArtwork, count)).To(BeZero()) + Expect(ExternalLookupsPerItem(model.KindRadioArtwork, count)).To(BeZero()) + }) +})