From 04d5a5565740f58fe49de73a4ac495e24ec1c242 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Jul 2026 14:28:14 -0400 Subject: [PATCH] perf(artwork): stop reading and hashing disc art on every request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resize-cache key was the content hash, which cannot be computed without reading the file, so a warm cache never prevented the I/O: every sized disc request read up to 20MB and hashed it before the lookup. The legacy reader keyed on the id and the album's mtime and touched the file only on a miss. Disc art has no state row and therefore no stored hash, so the key is that same identity — id, album mtime, DiscArtPriority — and the selection chain now runs only when the cache misses. Full-size requests stream the source instead of buffering and hashing it. This matters more now that single-disc albums keep running the disc chain, which puts every disc-tagged track without embedded art on this path. --- core/artwork/serving.go | 39 +++++++++++++++++++++++++++--------- core/artwork/serving_test.go | 22 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-) diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 5021b1c1c..c0e486a00 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -303,17 +303,38 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int } // Single-disc albums run the chain too: a disc can carry its own art, distinct from the // album cover, and DiscArtPriority is what expresses that preference. - funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority) - if r, path, err := selectImageReader(ctx, artID, funcs...); err == nil && r != nil { - defer r.Close() - if data, rerr := readCapped(r); rerr == nil { - if hash, herr := HashImage(bytes.NewReader(data)); herr == nil { - return s.serveBytes(ctx, hash, data, unixMtime(mtimeViaFS(dr.lib.FS, path)), size, square) - } - } + selectImage := func() (io.ReadCloser, string, error) { + funcs := dr.fromDiscArtPriority(ctx, s.ffmpeg, conf.Server.DiscArtPriority) + return selectImageReader(ctx, artID, funcs...) } albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID} - return s.Get(ctx, albumArtID, size, square) + if size == 0 && !square { + r, path, err := selectImage() + if err != nil || r == nil { + return s.Get(ctx, albumArtID, size, square) + } + return &Image{ReadCloser: r, LastUpdated: unixMtime(mtimeViaFS(dr.lib.FS, path))}, nil + } + + // Disc art has no state row, so there is no stored content hash to key the resize cache on. + // Keying on the id and the album's mtime — as the legacy reader did — lets a warm cache + // answer without touching the filesystem at all; the chain runs only on a miss. The key + // carries DiscArtPriority so changing it invalidates. resizedItem.hash is key material + // here, not a content hash, so the response carries no Image.Hash. + key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.album.UpdatedAt.UnixNano(), conf.Server.DiscArtPriority) + item := &resizedItem{ + hash: key, + size: size, + square: square, + lastUpdate: dr.album.UpdatedAt, + ffmpeg: s.ffmpeg, + open: func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err }, + } + stream, err := s.cache.Get(ctx, item) + if err != nil { + return s.Get(ctx, albumArtID, size, square) + } + return &Image{ReadCloser: stream, ETag: representationTag(key, size, square), LastUpdated: dr.album.UpdatedAt}, nil } // dangling enqueues a re-resolution at Scan priority and reports the artwork as diff --git a/core/artwork/serving_test.go b/core/artwork/serving_test.go index 37a2131a4..bb5a58f42 100644 --- a/core/artwork/serving_test.go +++ b/core/artwork/serving_test.go @@ -331,6 +331,28 @@ var _ = Describe("Service", func() { Expect(readAll(img)).To(Equal(coverBytes)) }) + // The resize cache is keyed on the id and the album's mtime, not on the image bytes, so a + // warm hit needs no filesystem access. Dropping the source between the two requests is + // how that shows: re-reading would fall back to the album instead. + It("serves a sized disc image from cache without re-reading the source", func() { + folderRepo.result = []model.Folder{{Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}}} + albumRepo.SetData(model.Albums{{ID: "aldc3", Name: "Album", FolderIDs: []string{"f1"}}}) + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID("aldc3", 1), nil) + + first, err := svc.Get(ctx, discID, 64, false) + Expect(err).ToNot(HaveOccurred()) + warmed := readAll(first) + Expect(warmed).ToNot(BeEmpty()) + + // With the source gone and the album holding no art of its own, a re-read would + // fall through to the album and fail. + folderRepo.result = nil + + second, err := svc.Get(ctx, discID, 64, false) + Expect(err).ToNot(HaveOccurred()) + Expect(readAll(second)).To(Equal(warmed), "a warm sized request must not touch the source") + }) + It("falls back to album art when no disc image matches", func() { folderRepo.result = nil albumRepo.SetData(model.Albums{{ID: "aldc2", Name: "Album"}})