From 2499de2bac6290576f413cc91e941004d2f603d0 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 21:46:12 -0400 Subject: [PATCH] perf(artwork): key blurhash dedup by identity, plus review cleanups The seen map was keyed by the full ArtworkID, which embeds the client token's LastUpdate: every scan bump rotated the key, so the same-bytes dedup (and its cheap version re-persist path) never fired in production and stale entries accumulated forever. The key now zeroes LastUpdate, making the map bounded by entity count and restoring the tested dedup behavior. Cleanups from the same review: the base83 encoder is now exported from the blurhash package and the DTO's duplicate copy is deleted; the always-set blurHashes field lost its test-shaped nil guards; clearIfStored dropped its inert version parameter (cleared rows never have their timestamp read); the teeReader done flag is replaced by nilling the callback; worker-era comments (async, cache-miss recompute) were updated to the inline design; and the e2e specs assert directly instead of polling, since the hash is persisted before the read helpers return. --- core/artwork/artwork.go | 14 +-- core/artwork/blurhash/blurhash.go | 14 ++- core/artwork/blurhash_updater.go | 14 ++- .../artwork/blurhash_updater_internal_test.go | 16 ++- core/artwork/e2e/blurhash_test.go | 111 +++++++----------- core/artwork/tee_reader.go | 8 +- model/album.go | 4 +- model/artist.go | 2 +- server/jellyfin/dto/blurhash.go | 25 +--- server/jellyfin/dto/blurhash_test.go | 4 + 10 files changed, 91 insertions(+), 121 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index a22cc471d..6fa60c816 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -38,9 +38,7 @@ type Artwork interface { } func NewArtwork(ds model.DataStore, cache cache.FileCache, ffmpeg ffmpeg.FFmpeg, provider external.Provider) Artwork { - a := &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg, provider: provider} - a.blurHashes = newBlurHashUpdater(ds) - return a + return &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg, provider: provider, blurHashes: newBlurHashUpdater(ds)} } type artwork struct { @@ -63,11 +61,9 @@ func (a *artwork) GetOrPlaceholder(ctx context.Context, id string, size int, squ reader, lastUpdate, err = a.Get(ctx, artID, size, square) } if errors.Is(err, ErrUnavailable) { - if a.blurHashes != nil { - // The client is receiving the placeholder, so a stored hash describing the old cover must - // clear — hash-what-you-serve applies to the fallback too. - a.blurHashes.clearIfStored(ctx, artID, time.Now()) - } + // The client is receiving the placeholder, so a stored hash describing the old cover must + // clear — hash-what-you-serve applies to the fallback too. + a.blurHashes.clearIfStored(ctx, artID) if artID.Kind == model.KindArtistArtwork { reader, _ = resources.FS().Open(consts.PlaceholderArtistArt) } else { @@ -92,7 +88,7 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa return nil, time.Time{}, err } reader = r - if a.blurHashes != nil && size == 0 && !square && eligibleKind(artID) { + if size == 0 && !square && eligibleKind(artID) { // Tee the served bytes: the blurhash is computed from exactly what the client downloads, so it // changes precisely when the served cover changes. Placeholder bytes (playlist fallback) clear. // The tee wraps r directly, so Close reaches the underlying stream (no fd leak). diff --git a/core/artwork/blurhash/blurhash.go b/core/artwork/blurhash/blurhash.go index ba466f467..302ec83c0 100644 --- a/core/artwork/blurhash/blurhash.go +++ b/core/artwork/blurhash/blurhash.go @@ -85,7 +85,7 @@ func Encode(img image.Image, xComp, yComp int) (string, error) { } var sb strings.Builder - sb.WriteString(encode83((xComp-1)+(yComp-1)*9, 1)) + sb.WriteString(Encode83((xComp-1)+(yComp-1)*9, 1)) ac := factors[1:] maxVal := 1.0 @@ -96,15 +96,15 @@ func Encode(img image.Image, xComp, yComp int) (string, error) { } quantMax := int(math.Max(0, math.Min(82, math.Floor(actualMax*166-0.5)))) maxVal = float64(quantMax+1) / 166 - sb.WriteString(encode83(quantMax, 1)) + sb.WriteString(Encode83(quantMax, 1)) } else { - sb.WriteString(encode83(0, 1)) + sb.WriteString(Encode83(0, 1)) } dc := factors[0] - sb.WriteString(encode83(linearToSRGB(dc[0])<<16|linearToSRGB(dc[1])<<8|linearToSRGB(dc[2]), 4)) + sb.WriteString(Encode83(linearToSRGB(dc[0])<<16|linearToSRGB(dc[1])<<8|linearToSRGB(dc[2]), 4)) for _, f := range ac { - sb.WriteString(encode83(quantAC(f[0], maxVal)*19*19+quantAC(f[1], maxVal)*19+quantAC(f[2], maxVal), 2)) + sb.WriteString(Encode83(quantAC(f[0], maxVal)*19*19+quantAC(f[1], maxVal)*19+quantAC(f[2], maxVal), 2)) } return sb.String(), nil } @@ -165,7 +165,9 @@ func linearToSRGB(v float64) int { return int((1.055*math.Pow(v, 1/2.4)-0.055)*255 + 0.5) } -func encode83(value, length int) string { +// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length, using the +// blurhash spec's alphabet. +func Encode83(value, length int) string { b := make([]byte, length) for i := length - 1; i >= 0; i-- { b[i] = alphabet[value%83] diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 0e05206fb..b628605b8 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -55,10 +55,13 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat log.Error(ctx, "BlurHash: recovered from panic", "artID", artID, "panic", r) } }() + // ArtworkID embeds the client token's LastUpdate; without zeroing it the seen key would rotate on + // every scan bump, defeating the same-bytes dedup and stranding stale entries forever. + artID.LastUpdate = time.Time{} // The response is already written when the tee fires; a client abort must not lose the write. ctx = context.WithoutCancel(ctx) if isPlaceholder(data) { - u.clearIfStored(ctx, artID, version) + u.clearIfStored(ctx, artID) return } sum := checksum(data) @@ -91,10 +94,11 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat // clearIfStored clears the persisted hash after a placeholder was served (a cold map costs one row // read to skip never-hashed entities); a failed read clears nothing — unknown state is not deletion. -func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.ArtworkID, version time.Time) { +func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.ArtworkID) { if !eligibleKind(artID) { return } + artID.LastUpdate = time.Time{} ctx = context.WithoutCancel(ctx) u.mutex.Lock() prev, ok := u.seen[artID] @@ -108,15 +112,15 @@ func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.Artwork return } if stored == "" { - u.remember(artID, blurHashState{version: version}) + u.remember(artID, blurHashState{}) return } } - if err := u.persist(ctx, artID, "", version); err != nil { + if err := u.persist(ctx, artID, "", time.Now()); err != nil { log.Warn(ctx, "BlurHash: error clearing hash", "artID", artID, err) return } - u.remember(artID, blurHashState{version: version}) + u.remember(artID, blurHashState{}) } func (u *blurHashUpdater) persistAndRemember(ctx context.Context, artID model.ArtworkID, hash string, sum uint64, version time.Time) { diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 0fa42eb08..c537bb378 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -111,8 +111,22 @@ var _ = Describe("blurHashUpdater", func() { It("ignores non-eligible artwork kinds", func() { Expect(func() { - u.clearIfStored(GinkgoT().Context(), model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, version) + u.clearIfStored(GinkgoT().Context(), model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}) }).ToNot(Panic()) Expect(u.seen).To(BeEmpty()) }) + + It("dedups across artwork ids that differ only in their embedded timestamp", func() { + // Client coverArt tokens embed a LastUpdate; the seen key must ignore it, or every scan bump + // would defeat the dedup and re-decode identical bytes. + id := album(model.Album{ID: "al-1", UpdatedAt: version}) + data := realPNGBytes("dedup") + u.update(GinkgoT().Context(), id, data, version) + Expect(repo.UpdateBlurHash("al-1", "TAMPERED", version)).To(Succeed()) + bumped := id + bumped.LastUpdate = version.Add(time.Hour) + u.update(GinkgoT().Context(), bumped, data, version) + Expect(stored("al-1").BlurHash).To(Equal("TAMPERED")) + Expect(u.seen).To(HaveLen(1)) + }) }) diff --git a/core/artwork/e2e/blurhash_test.go b/core/artwork/e2e/blurhash_test.go index f34c09c15..f78b6fe6f 100644 --- a/core/artwork/e2e/blurhash_test.go +++ b/core/artwork/e2e/blurhash_test.go @@ -16,6 +16,15 @@ var _ = Describe("BlurHash", func() { setupHarness() }) + // The blurhash is computed inline when the served reader is closed, so by the time the read + // helpers return, the hash is already persisted — no polling needed. + storedAlbum := func(id string) model.Album { + GinkgoHelper() + updated, err := ds.Album(ctx).Get(id) + Expect(err).ToNot(HaveOccurred()) + return *updated + } + It("persists a real blurhash after album artwork is served", func() { setLayout(fstest.MapFS{ "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), @@ -25,18 +34,14 @@ var _ = Describe("BlurHash", func() { al := firstAlbum() Expect(al.BlurHash).To(BeEmpty()) - // Serving the artwork enqueues the async blurhash computation. readArtwork(al.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(len(updated.BlurHash)).To(BeNumerically(">", 6)) - g.Expect(updated.BlurHashUpdatedAt).ToNot(BeNil()) - // The snapshot must not be before the artwork version, or the DTO would treat it as - // stale (it may exceed it: image file mtimes are folded in). - g.Expect(updated.BlurHashUpdatedAt.Before(updated.ArtworkUpdatedAt())).To(BeFalse()) - }, "10s", "100ms").Should(Succeed()) + updated := storedAlbum(al.ID) + Expect(len(updated.BlurHash)).To(BeNumerically(">", 6)) + Expect(updated.BlurHashUpdatedAt).ToNot(BeNil()) + // The snapshot must not be before the artwork version, or the DTO would treat it as + // stale (it may exceed it: image file mtimes are folded in). + Expect(updated.BlurHashUpdatedAt.Before(updated.ArtworkUpdatedAt())).To(BeFalse()) }) It("does not persist a future-dated blurhash timestamp", func() { @@ -50,15 +55,12 @@ var _ = Describe("BlurHash", func() { al := firstAlbum() readArtwork(al.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - g.Expect(updated.BlurHashUpdatedAt).ToNot(BeNil()) - // A future file mtime must be capped at now, or the !Before checks would pin the hash - // (and the client's cover cache) until wall time caught up. - g.Expect(updated.BlurHashUpdatedAt.After(time.Now())).To(BeFalse()) - }, "10s", "100ms").Should(Succeed()) + updated := storedAlbum(al.ID) + Expect(updated.BlurHash).ToNot(BeEmpty()) + Expect(updated.BlurHashUpdatedAt).ToNot(BeNil()) + // A future file mtime must be capped at now, or the !Before checks would pin the hash + // (and the client's cover cache) until wall time caught up. + Expect(updated.BlurHashUpdatedAt.After(time.Now())).To(BeFalse()) }) It("recomputes when the cover is swapped in place", func() { @@ -69,13 +71,8 @@ var _ = Describe("BlurHash", func() { scan() al := firstAlbum() readArtwork(al.CoverArtID()) - var firstHash string - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - firstHash = updated.BlurHash - }, "10s", "100ms").Should(Succeed()) + firstHash := storedAlbum(al.ID).BlurHash + Expect(firstHash).ToNot(BeEmpty()) // Swap the cover bytes and rescan, then serve: the tee hashes the newly-served bytes, so the // stored hash moves to describe the new cover. @@ -86,12 +83,9 @@ var _ = Describe("BlurHash", func() { scan() readArtwork(al.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - g.Expect(updated.BlurHash).ToNot(Equal(firstHash)) - }, "10s", "100ms").Should(Succeed()) + updated := storedAlbum(al.ID) + Expect(updated.BlurHash).ToNot(BeEmpty()) + Expect(updated.BlurHash).ToNot(Equal(firstHash)) }) It("clears the stored blurhash when the cover disappears", func() { @@ -102,25 +96,17 @@ var _ = Describe("BlurHash", func() { scan() al := firstAlbum() readArtwork(al.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - }, "10s", "100ms").Should(Succeed()) + Expect(storedAlbum(al.ID).BlurHash).ToNot(BeEmpty()) // No rescan: the folder row still lists the cover, but the file is gone. The serve falls back - // to the placeholder (GetOrPlaceholder, the real Jellyfin/Subsonic path), and the worker's - // gone-recheck confirms the source is really gone and clears the stored hash. + // to the placeholder (GetOrPlaceholder, the real Jellyfin/Subsonic path), which clears the + // stored hash inline. setLayout(fstest.MapFS{ "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), }) Expect(readOrPlaceholder(al.CoverArtID())).To(Equal(placeholderBytes())) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).To(BeEmpty()) - }, "10s", "100ms").Should(Succeed()) + Expect(storedAlbum(al.ID).BlurHash).To(BeEmpty()) }) It("recomputes when cover bytes change under a preserved mtime (cache disabled)", func() { @@ -134,13 +120,8 @@ var _ = Describe("BlurHash", func() { scan() al := firstAlbum() readArtwork(al.CoverArtID()) - var firstHash string - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - firstHash = updated.BlurHash - }, "10s", "100ms").Should(Succeed()) + firstHash := storedAlbum(al.ID).BlurHash + Expect(firstHash).ToNot(BeEmpty()) // Replace the bytes but keep the SAME mtime and do NOT rescan: only the served bytes change. swapped := realPNG("swapped-bytes") @@ -151,11 +132,7 @@ var _ = Describe("BlurHash", func() { }) readArtwork(al.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(Equal(firstHash)) - }, "10s", "100ms").Should(Succeed()) + Expect(storedAlbum(al.ID).BlurHash).ToNot(Equal(firstHash)) }) It("clears a stored playlist hash when it falls back to the placeholder", func() { @@ -169,21 +146,17 @@ var _ = Describe("BlurHash", func() { pl := putPlaylist(model.Playlist{ID: "pl-blur", Name: "MyList", Path: m3uPath}) readArtwork(pl.CoverArtID()) - Eventually(func(g Gomega) { - updated, err := ds.Playlist(ctx).Get(pl.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).ToNot(BeEmpty()) - }, "10s", "100ms").Should(Succeed()) + stored, err := ds.Playlist(ctx).Get(pl.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(stored.BlurHash).ToNot(BeEmpty()) // Remove the sidecar: the serve now falls through to the placeholder, captured by the tee. Expect(os.Remove(sidecar)).To(Succeed()) Expect(readArtwork(pl.CoverArtID())).To(Equal(placeholderBytes())) - Eventually(func(g Gomega) { - updated, err := ds.Playlist(ctx).Get(pl.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).To(BeEmpty()) - }, "10s", "100ms").Should(Succeed()) + stored, err = ds.Playlist(ctx).Get(pl.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(stored.BlurHash).To(BeEmpty()) }) It("does not persist a blurhash when the served image cannot be decoded", func() { @@ -196,10 +169,6 @@ var _ = Describe("BlurHash", func() { readArtwork(al.CoverArtID()) - Consistently(func(g Gomega) { - updated, err := ds.Album(ctx).Get(al.ID) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(updated.BlurHash).To(BeEmpty()) - }, "600ms", "100ms").Should(Succeed()) + Expect(storedAlbum(al.ID).BlurHash).To(BeEmpty()) }) }) diff --git a/core/artwork/tee_reader.go b/core/artwork/tee_reader.go index e55592ab0..958f43b59 100644 --- a/core/artwork/tee_reader.go +++ b/core/artwork/tee_reader.go @@ -15,7 +15,6 @@ type teeReader struct { onComplete func(data []byte) eof bool over bool - done bool } func newTeeReader(src io.ReadCloser, maxBytes int, onComplete func(data []byte)) *teeReader { @@ -40,9 +39,10 @@ func (t *teeReader) Read(p []byte) (int, error) { func (t *teeReader) Close() error { err := t.src.Close() - if !t.done && t.eof && !t.over && t.onComplete != nil { - t.done = true - t.onComplete(t.buf.Bytes()) + if t.eof && !t.over && t.onComplete != nil { + cb := t.onComplete + t.onComplete = nil // fire at most once, even on double Close + cb(t.buf.Bytes()) } return err } diff --git a/model/album.go b/model/album.go index f4d2fdedb..130cd376f 100644 --- a/model/album.go +++ b/model/album.go @@ -68,7 +68,7 @@ type Album struct { CreatedAt time.Time `structs:"created_at" json:"createdAt"` // Oldest CreatedAt for all songs in this album UpdatedAt time.Time `structs:"updated_at" json:"updatedAt"` // Newest UpdatedAt for all songs in this album - // BlurHash of the album cover, computed asynchronously from the served artwork. Excluded from + // BlurHash of the album cover, computed from the served artwork bytes. Excluded from // full-row writes (structs:"-"): only UpdateBlurHash writes it, so scans can't erase it. BlurHash string `structs:"-" json:"blurHash,omitempty" hash:"ignore"` BlurHashUpdatedAt *time.Time `structs:"-" json:"-" hash:"ignore"` @@ -80,7 +80,7 @@ func (a Album) CoverArtID() ArtworkID { // ArtworkUpdatedAt is the album's artwork version. ExternalInfoUpdatedAt is deliberately excluded: // it bumps on every agent TTL refresh even when the image is unchanged, and actual image changes -// are caught by the image-cache-miss recompute instead. +// are caught by hashing the served bytes instead. func (a Album) ArtworkUpdatedAt() time.Time { t := a.UpdatedAt if a.ImportedAt.After(t) { diff --git a/model/artist.go b/model/artist.go index 12b1dc629..baaf1e4b1 100644 --- a/model/artist.go +++ b/model/artist.go @@ -68,7 +68,7 @@ func (a Artist) CoverArtID() ArtworkID { // ArtworkUpdatedAt is the artist's artwork version. ExternalInfoUpdatedAt is deliberately // excluded: it bumps on every agent TTL refresh even when the image is unchanged, and actual -// image changes are caught by the image-cache-miss recompute instead. +// image changes are caught by hashing the served bytes instead. func (a Artist) ArtworkUpdatedAt() time.Time { if a.UpdatedAt == nil { return time.Time{} diff --git a/server/jellyfin/dto/blurhash.go b/server/jellyfin/dto/blurhash.go index d0404d6c3..ddcbeb08c 100644 --- a/server/jellyfin/dto/blurhash.go +++ b/server/jellyfin/dto/blurhash.go @@ -4,29 +4,10 @@ import ( "fmt" "hash/fnv" "time" + + "github.com/navidrome/navidrome/core/artwork/blurhash" ) -// base83Alphabet is the blurhash spec's base83 encoding alphabet; order is part of the spec. -const base83Alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~" - -// base83 encodes value as a fixed-width, big-endian base83 string of the given length. -func base83(value, length int) string { - b := make([]byte, length) - for i := 1; i <= length; i++ { - digit := (value / pow83(length-i)) % 83 - b[i-1] = base83Alphabet[digit] - } - return string(b) -} - -func pow83(n int) int { - result := 1 - for range n { - result *= 83 - } - return result -} - // blurHash returns a valid 6-char blurhash for a solid color derived from seed. Finamp only needs a // well-formed, per-tag-stable value (it uses this as a download de-dup key and blur placeholder), so // a solid color unique to the tag satisfies both without decoding cover art. @@ -36,7 +17,7 @@ func blurHash(seed string) string { sum := h.Sum(nil) r, g, b := int(sum[0]), int(sum[1]), int(sum[2]) dc := (r << 16) | (g << 8) | b - return "00" + base83(dc, 4) + return "00" + blurhash.Encode83(dc, 4) } // primaryBlurHash returns the stored blurhash when it was computed from the entity's current diff --git a/server/jellyfin/dto/blurhash_test.go b/server/jellyfin/dto/blurhash_test.go index 5fc74f293..0d35ae83f 100644 --- a/server/jellyfin/dto/blurhash_test.go +++ b/server/jellyfin/dto/blurhash_test.go @@ -8,6 +8,10 @@ import ( . "github.com/onsi/gomega" ) +// base83Alphabet duplicates the spec alphabet on purpose: the test must catch the production copy +// drifting, not drift along with it. +const base83Alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~" + var _ = Describe("blurHash", func() { It("returns a 6-char valid blurhash starting with the 1x1 component prefix", func() { h := blurHash("x")