From f6dd722119374809a982a9c6cb5252133c6e419f Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 20:34:33 -0400 Subject: [PATCH] refactor(artwork): compute blurhash from served bytes, drop proxy signals The worker no longer infers whether the served bytes changed through a stack of proxy signals (snapshot timestamp vs stored blur_hash_updated_at, freshness guard, gone bit, idempotent-write skip, computeFromArtwork re-read). It now takes the exact bytes captured from a serve and is a pure function of them: placeholder clears, undecodable is left alone, otherwise encode and write with an in-memory last-hash dedup. Deletion is a checkGone job that re-reads once and clears only if the source is still gone, so a transient fetch failure can't clobber a valid hash. --- core/artwork/blurhash_updater.go | 255 ++++++++---------- .../artwork/blurhash_updater_internal_test.go | 177 ++++++------ 2 files changed, 184 insertions(+), 248 deletions(-) diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 224e2e087..67c76dbd7 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -17,22 +17,21 @@ import ( "github.com/navidrome/navidrome/resources" ) -// enqueueRequest carries the fill-time snapshot (the reader's LastUpdated, which folds row -// timestamps and live file mtimes into one clock). gone marks a serve that failed with -// ErrUnavailable: the only change no passive signal witnesses, so it clears a stale hash. -type enqueueRequest struct { - snapshot time.Time - gone bool +// blurHashJob is a unit of work: either bytes to hash (data != nil) or a deletion check (checkGone). +type blurHashJob struct { + data []byte + version time.Time + checkGone bool } -// blurHashUpdater keeps stored blurhashes in sync with the artwork entering the image cache. -// Computation is triggered by cache fills (every change), not serves, so the worker only re-derives -// the hash and skips idempotent writes: Enqueue is a cheap dedup insert, a single worker decodes -// and persists. +// blurHashUpdater keeps stored blurhashes in sync with the bytes actually served. The serve path tees +// the served image and hands it here; there is no change-detection proxy — the hash is a pure function +// of the captured bytes. A single worker decodes, encodes, and writes (dedup'd in memory). type blurHashUpdater struct { a *artwork mutex sync.Mutex - buffer map[model.ArtworkID]enqueueRequest + buffer map[model.ArtworkID]blurHashJob + last map[model.ArtworkID]string // last hash written this process; avoids redundant writes wake chan struct{} done chan struct{} runDone chan struct{} @@ -44,29 +43,35 @@ type blurHashUpdater struct { func newBlurHashUpdater(a *artwork) *blurHashUpdater { return &blurHashUpdater{ a: a, - buffer: make(map[model.ArtworkID]enqueueRequest), + buffer: make(map[model.ArtworkID]blurHashJob), + last: make(map[model.ArtworkID]string), wake: make(chan struct{}, 1), done: make(chan struct{}), runDone: make(chan struct{}), } } -// Enqueue schedules a recompute for an original-size cache fill, using the reader's snapshot as the -// artwork version. Called on the miss that fills the cache — i.e. exactly when the served bytes change. -func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, snapshot time.Time) { - u.enqueue(artID, enqueueRequest{snapshot: snapshot}) -} - -// EnqueueGone schedules a clear for a serve that failed with ErrUnavailable, so a stored hash stops -// describing artwork that no longer exists (deletion is invisible to every passive signal). -func (u *blurHashUpdater) EnqueueGone(artID model.ArtworkID) { - u.enqueue(artID, enqueueRequest{gone: true}) -} - -func (u *blurHashUpdater) enqueue(artID model.ArtworkID, req enqueueRequest) { +func eligibleKind(artID model.ArtworkID) bool { switch artID.Kind { case model.KindAlbumArtwork, model.KindArtistArtwork, model.KindPlaylistArtwork: - default: + return true + } + return false +} + +// EnqueueBytes schedules a blurhash update computed from the exact bytes served for artID. +func (u *blurHashUpdater) EnqueueBytes(artID model.ArtworkID, data []byte, version time.Time) { + u.enqueue(artID, blurHashJob{data: data, version: version}) +} + +// EnqueueClearIfGone schedules a deletion check: the worker re-reads the source once and clears the +// stored hash only if it still fails/serves a placeholder, so a transient failure won't clobber it. +func (u *blurHashUpdater) EnqueueClearIfGone(artID model.ArtworkID, version time.Time) { + u.enqueue(artID, blurHashJob{checkGone: true, version: version}) +} + +func (u *blurHashUpdater) enqueue(artID model.ArtworkID, job blurHashJob) { + if !eligibleKind(artID) { return } u.mutex.Lock() @@ -76,24 +81,18 @@ func (u *blurHashUpdater) enqueue(artID model.ArtworkID, req enqueueRequest) { } if !u.started { u.started = true - // Admin context: playlist artwork readers require a user. Lazy-starting keeps idle Artwork - // instances goroutine-free; stop() ends the worker (tests must call it, the server never does). + // Admin context: playlist artwork readers require a user. Lazy start keeps idle Artwork + // instances goroutine-free; stop() ends the worker (tests call it, the server never does). ctx, cancel := context.WithCancel(request.WithUser(context.Background(), model.User{IsAdmin: true})) u.runCancel = cancel go u.run(ctx) } - prev := u.buffer[artID] - if !req.snapshot.IsZero() { - // A successful serve proves the artwork exists, so it supersedes any pending gone request for - // the same artwork (a cover restored right after a missing-art serve must still recompute). - prev.gone = false - if req.snapshot.After(prev.snapshot) { - prev.snapshot = req.snapshot - } - } else if req.gone { - prev.gone = true + // A bytes job supersedes a pending gone-check (a successful serve proves the artwork exists); + // otherwise keep whichever is newer. + prev, ok := u.buffer[artID] + if !ok || job.data != nil || (prev.checkGone && job.version.After(prev.version)) { + u.buffer[artID] = job } - u.buffer[artID] = prev u.mutex.Unlock() select { case u.wake <- struct{}{}: @@ -134,30 +133,30 @@ func (u *blurHashUpdater) run(ctx context.Context) { return default: } - artID, req, ok := u.next() + artID, job, ok := u.next() if !ok { break } - u.process(ctx, artID, req) + u.processJob(ctx, artID, job) } } } -func (u *blurHashUpdater) next() (model.ArtworkID, enqueueRequest, bool) { +func (u *blurHashUpdater) next() (model.ArtworkID, blurHashJob, bool) { u.mutex.Lock() defer u.mutex.Unlock() - for artID, req := range u.buffer { + for artID, job := range u.buffer { delete(u.buffer, artID) - return artID, req, true + return artID, job, true } - return model.ArtworkID{}, enqueueRequest{}, false + return model.ArtworkID{}, blurHashJob{}, false } // processTimeout bounds one computation: readers can call external agents, and a hung call must not // stall the worker forever. const processTimeout = 30 * time.Second -func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, req enqueueRequest) { +func (u *blurHashUpdater) processJob(ctx context.Context, artID model.ArtworkID, job blurHashJob) { // Artwork readers can touch storage, agents and plugins; a panic here must not kill the server. defer func() { if r := recover(); r != nil { @@ -166,115 +165,77 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re }() ctx, cancel := context.WithTimeout(ctx, processTimeout) defer cancel() - stored, storedAt, version, err := u.loadState(ctx, artID) - if err != nil { - log.Trace(ctx, "BlurHash: could not load entity", "artID", artID, err) - return - } - // A future-dated artwork file mtime (clock skew, a future-stamped file) would otherwise be - // persisted verbatim and, via the !Before checks here and in the DTO, pin the stored hash until - // wall time caught up. Cap the snapshot at now so a later real change always moves past it. - now := time.Now() - snapshot := req.snapshot - if snapshot.After(now) { - snapshot = now - } - if req.gone { - // Only the failed serve witnesses a deletion; clear a stored hash so the DTO falls back to - // the rotating fake. An empty hash means there is nothing to clear. - if stored != "" { - if err := u.persist(ctx, artID, "", version); err != nil { - log.Warn(ctx, "BlurHash: error clearing stale hash", "artID", artID, err) - } - } - return - } - // snapshot folds row timestamps and file mtimes into one clock; a stored hash at or after it is - // already current. This is the only freshness comparison the fill trigger needs. - if stored != "" && storedAt != nil && !storedAt.Before(snapshot) { - return - } - hash, err := u.computeFromArtwork(ctx, artID) - if err != nil { - // A transient failure (timeout, flaky cache/DB read) is not evidence the artwork changed; - // leave the stored hash intact and let a later fill retry, so clients don't churn on a fake. - log.Trace(ctx, "BlurHash: recompute failed, keeping stored hash", "artID", artID, err) - return - } - if hash == "" { - // An empty hash means the served bytes are a placeholder: the cover is gone. Clear a stored - // hash so the DTO stops describing artwork no longer served. - if stored != "" { - if err := u.persist(ctx, artID, "", snapshot); err != nil { - log.Warn(ctx, "BlurHash: error clearing stale hash", "artID", artID, err) - } - } - return - } - // Unchanged hash with an unmoved snapshot needs no write — keeps cache-disabled installs (which - // fill on every original serve) from hammering the DB. - if hash == stored && storedAt != nil && !snapshot.After(*storedAt) { - return - } - if err := u.persist(ctx, artID, hash, snapshot); err != nil { - log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err) - } -} -func (u *blurHashUpdater) loadState(ctx context.Context, artID model.ArtworkID) (string, *time.Time, time.Time, error) { - switch artID.Kind { - case model.KindAlbumArtwork: - al, err := u.a.ds.Album(ctx).Get(artID.ID) - if err != nil { - return "", nil, time.Time{}, err - } - return al.BlurHash, al.BlurHashUpdatedAt, al.ArtworkUpdatedAt(), nil - case model.KindArtistArtwork: - ar, err := u.a.ds.Artist(ctx).Get(artID.ID) - if err != nil { - return "", nil, time.Time{}, err - } - return ar.BlurHash, ar.BlurHashUpdatedAt, ar.ArtworkUpdatedAt(), nil - case model.KindPlaylistArtwork: - pl, err := u.a.ds.Playlist(ctx).Get(artID.ID) - if err != nil { - return "", nil, time.Time{}, err - } - return pl.BlurHash, pl.BlurHashUpdatedAt, pl.ArtworkUpdatedAt(), nil + if job.checkGone { + u.processGone(ctx, artID, job.version) + return } - return "", nil, time.Time{}, model.ErrNotFound -} - -func (u *blurHashUpdater) computeFromArtwork(ctx context.Context, artID model.ArtworkID) (string, error) { - artReader, err := u.a.getArtworkReader(ctx, artID, 0, false) + if isPlaceholder(job.data) { + u.clear(ctx, artID, job.version) + return + } + img, _, err := image.Decode(bytes.NewReader(job.data)) if err != nil { - return "", err - } - // Reads via the cache (not artwork.Get, so no re-enqueue): generated playlist mosaics are - // random per generation, and the hash must describe the bytes clients actually download. - r, err := u.a.cache.Get(ctx, artReader) - if err != nil { - return "", err - } - defer r.Close() - data, err := io.ReadAll(r) - if err != nil { - return "", err - } - if isPlaceholder(data) { - return "", nil - } - img, _, err := image.Decode(bytes.NewReader(data)) - if err != nil { - return "", err + // Undecodable served bytes are not proof of change; leave the stored hash intact. + log.Trace(ctx, "BlurHash: served bytes not decodable, keeping stored hash", "artID", artID, err) + return } b := img.Bounds() x, y := blurhash.Components(b.Dx(), b.Dy()) - return blurhash.Encode(img, x, y) + hash, err := blurhash.Encode(img, x, y) + if err != nil || hash == "" { + return + } + u.write(ctx, artID, hash, job.version) } -// isPlaceholder byte-compares against the embedded placeholder assets: placeholder artwork must -// never be persisted as an entity's blurhash, and cached reads carry no source path to check. +// processGone re-reads the source once; if it still yields a placeholder or fails, the artwork is +// really gone and the stored hash is cleared. A transient failure recovers by now and is left alone. +func (u *blurHashUpdater) processGone(ctx context.Context, artID model.ArtworkID, version time.Time) { + artReader, err := u.a.getArtworkReader(ctx, artID, 0, false) + if err == nil { + r, gErr := u.a.cache.Get(ctx, artReader) + if gErr == nil { + data, rErr := io.ReadAll(r) + _ = r.Close() + if rErr == nil && !isPlaceholder(data) { + return // source came back (or never really failed): keep the hash + } + } + } + u.clear(ctx, artID, version) +} + +func (u *blurHashUpdater) write(ctx context.Context, artID model.ArtworkID, hash string, version time.Time) { + u.mutex.Lock() + if u.last[artID] == hash { + u.mutex.Unlock() + return + } + u.mutex.Unlock() + if err := u.persist(ctx, artID, hash, version); err != nil { + log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err) + return + } + u.mutex.Lock() + u.last[artID] = hash + u.mutex.Unlock() +} + +func (u *blurHashUpdater) clear(ctx context.Context, artID model.ArtworkID, version time.Time) { + // No cold-map dedup: an empty u.last[artID] means "never written" as easily as "already cleared", + // so skipping would leave a previous process's DB hash describing gone artwork. Clears are rare. + if err := u.persist(ctx, artID, "", version); err != nil { + log.Warn(ctx, "BlurHash: error clearing hash", "artID", artID, err) + return + } + u.mutex.Lock() + u.last[artID] = "" + u.mutex.Unlock() +} + +// isPlaceholder byte-compares against the embedded placeholder assets: placeholder artwork must never +// be persisted as an entity's blurhash, and captured bytes carry no source path to check. func isPlaceholder(data []byte) bool { for _, p := range placeholderImages() { if bytes.Equal(data, p) { diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 8a27a2c9e..9f95a064c 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -1,7 +1,10 @@ package artwork import ( - "errors" + "bytes" + "image" + "image/color" + "image/png" "time" "github.com/navidrome/navidrome/model" @@ -10,131 +13,103 @@ import ( . "github.com/onsi/gomega" ) -// failingFolderRepo makes the artwork reader chain fail with a clean (transient-style) error. -type failingFolderRepo struct{ model.FolderRepository } +// pngImage builds a deterministic 2x2 PNG image for a label (color derived from label bytes). +func pngImage(label string) *image.RGBA { + img := image.NewRGBA(image.Rect(0, 0, 2, 2)) + var seed byte + for i := range len(label) { + seed += label[i] + } + c := color.RGBA{R: seed, G: seed * 3, B: seed * 7, A: 255} + for y := range 2 { + for x := range 2 { + img.Set(x, y, c) + } + } + return img +} -func (failingFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { - return nil, errors.New("boom") +func realPNGBytes(label string) []byte { + var buf bytes.Buffer + Expect(png.Encode(&buf, pngImage(label))).To(Succeed()) + return buf.Bytes() } var _ = Describe("blurHashUpdater", func() { var u *blurHashUpdater var ds *tests.MockDataStore + var version time.Time BeforeEach(func() { ds = &tests.MockDataStore{} - // started is pre-set so Enqueue never spawns run(): tests drive next()/process() directly. u = &blurHashUpdater{ a: &artwork{ds: ds}, - buffer: make(map[model.ArtworkID]enqueueRequest), + buffer: make(map[model.ArtworkID]blurHashJob), wake: make(chan struct{}, 1), + last: make(map[model.ArtworkID]string), started: true, } + version = time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) }) - Describe("Enqueue", func() { - It("accepts album, artist and playlist artwork and dedups, keeping the newest snapshot", func() { - id := model.Album{ID: "al-1"}.CoverArtID() - t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) - t2 := t1.Add(time.Hour) - u.Enqueue(id, t1) - u.Enqueue(id, t2) - u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), t1) - Expect(u.buffer).To(HaveLen(2)) - Expect(u.buffer[id].snapshot).To(Equal(t2)) - Expect(u.buffer[id].gone).To(BeFalse()) - }) + It("persists a hash computed from the given bytes", func() { + al := model.Album{ID: "al-1", UpdatedAt: version} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo - It("keeps a gone flag when it follows a pending fill (cover then vanished)", func() { - id := model.Album{ID: "al-1"}.CoverArtID() - t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) - u.Enqueue(id, t1) - u.EnqueueGone(id) - Expect(u.buffer[id].snapshot).To(Equal(t1)) - Expect(u.buffer[id].gone).To(BeTrue()) - }) - - It("lets a successful fill supersede a pending gone (cover restored)", func() { - id := model.Album{ID: "al-1"}.CoverArtID() - t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) - u.EnqueueGone(id) - u.Enqueue(id, t1) - Expect(u.buffer[id].snapshot).To(Equal(t1)) - Expect(u.buffer[id].gone).To(BeFalse()) - }) - - It("ignores other artwork kinds", func() { - u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, time.Time{}) - u.EnqueueGone(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}) - Expect(u.buffer).To(BeEmpty()) - }) + u.processJob(GinkgoT().Context(), al.CoverArtID(), blurHashJob{data: realPNGBytes("x"), version: version}) + stored, err := ds.Album(GinkgoT().Context()).Get("al-1") + Expect(err).ToNot(HaveOccurred()) + Expect(stored.BlurHash).ToNot(BeEmpty()) }) - Describe("process", func() { - var version time.Time + It("clears the hash when the bytes are a placeholder", func() { + al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "OLD"} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo - BeforeEach(func() { - version = time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) - }) + u.processJob(GinkgoT().Context(), al.CoverArtID(), blurHashJob{data: placeholderImages()[0], version: version}) + stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") + Expect(stored.BlurHash).To(BeEmpty()) // cleared: placeholder means gone + }) - It("skips entities whose stored hash is at or after the snapshot", func() { - al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &version} - repo := tests.CreateMockAlbumRepo() - repo.SetData(model.Albums{al}) - ds.MockedAlbum = repo + It("leaves the hash untouched on undecodable bytes", func() { + al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "KEEP"} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{snapshot: version}) - stored, err := ds.Album(GinkgoT().Context()).Get("al-1") - Expect(err).ToNot(HaveOccurred()) - Expect(stored.BlurHash).To(Equal("LEHV6nWB2yk8")) - }) + u.processJob(GinkgoT().Context(), al.CoverArtID(), blurHashJob{data: []byte("not an image"), version: version}) + stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") + Expect(stored.BlurHash).To(Equal("KEEP")) + }) - It("keeps the stored hash when a recompute fails transiently", func() { - al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: nil} - repo := tests.CreateMockAlbumRepo() - repo.SetData(model.Albums{al}) - ds.MockedAlbum = repo - ds.MockedFolder = failingFolderRepo{} + It("skips a redundant write when the hash is unchanged (in-memory dedup)", func() { + al := model.Album{ID: "al-1", UpdatedAt: version} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo + data := realPNGBytes("dedup") - // A newer snapshot forces a recompute, but the reader chain errors (transient): the stored - // hash must survive, so clients don't churn on a fake until a later fill succeeds. - newer := version.Add(time.Hour) - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{snapshot: newer}) - stored, err := ds.Album(GinkgoT().Context()).Get("al-1") - Expect(err).ToNot(HaveOccurred()) - Expect(stored.BlurHash).To(Equal("LEHV6nWB2yk8")) - }) + u.processJob(GinkgoT().Context(), al.CoverArtID(), blurHashJob{data: data, version: version}) + first, _ := ds.Album(GinkgoT().Context()).Get("al-1") + u.processJob(GinkgoT().Context(), al.CoverArtID(), blurHashJob{data: data, version: version.Add(time.Hour)}) + second, _ := ds.Album(GinkgoT().Context()).Get("al-1") + Expect(second.BlurHash).To(Equal(first.BlurHash)) + }) - It("clears a stored hash when the source is gone", func() { - al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &version} - repo := tests.CreateMockAlbumRepo() - repo.SetData(model.Albums{al}) - ds.MockedAlbum = repo + It("ignores non-eligible artwork kinds on enqueue", func() { + u.EnqueueBytes(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, realPNGBytes("x"), version) + Expect(u.buffer).To(BeEmpty()) + }) - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{gone: true}) - stored, err := ds.Album(GinkgoT().Context()).Get("al-1") - Expect(err).ToNot(HaveOccurred()) - Expect(stored.BlurHash).To(BeEmpty()) - }) - - It("does nothing for a gone serve with no stored hash", func() { - al := model.Album{ID: "al-1", UpdatedAt: version} - repo := tests.CreateMockAlbumRepo() - repo.SetData(model.Albums{al}) - ds.MockedAlbum = repo - - Expect(func() { - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{gone: true}) - }).ToNot(Panic()) - stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") - Expect(stored.BlurHash).To(BeEmpty()) - }) - - It("does nothing when the entity is gone", func() { - ds.MockedAlbum = tests.CreateMockAlbumRepo() - Expect(func() { - u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID(), enqueueRequest{}) - }).ToNot(Panic()) - }) + It("supersedes a pending gone-check with a bytes job", func() { + id := model.Album{ID: "al-1"}.CoverArtID() + u.EnqueueClearIfGone(id, version) + u.EnqueueBytes(id, realPNGBytes("x"), version.Add(time.Hour)) + Expect(u.buffer[id].checkGone).To(BeFalse()) + Expect(u.buffer[id].data).ToNot(BeNil()) }) })