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")