remote/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-12 20:37:12 +02:00
Code Issues Projects Releases Packages Wiki Activity

fix(artwork): reload animated covers in the UI once converted

Browse source 
A resized animated GIF is first served as a static stand-in while the conversion
runs in the background. The converted image is cached under the same URL, since the
cover hash identifies the original upload, so the UI kept painting the stand-in
from its blob cache until a full page reload.

When a background conversion is stored, the artwork service now broadcasts a
refreshResource event for the item (the service receives the event broker via
wire). In the UI, useArtworkRefresh returns when the last event named a record,
ignoring wildcards so scans do not refetch every cover, and useImageUrl takes that
as a version: a changed version re-fetches the cached URL, keeps showing the old
blob until the new one arrives, and keeps it if the re-fetch fails.
This commit is contained in:
Deluan 2026-09-14 21:31:14 -04:00
commit 2cc2ab1160
12 changed files with 226 additions and 32 deletions
Download patch file Download diff file Expand all files Collapse all files

11
cmd/wire_gen.go
View file

@ -89,13 +89,13 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router {
fileCache := artwork.GetImageCache()
imageStore := artwork.GetImageStore()
fFmpeg := ffmpeg.New()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg)
broker := events.GetBroker()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker)
transcodingCache := stream.GetTranscodingCache()
mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache)
share := core.NewShare(dataStore)
archiver := core.NewArchiver(mediaStreamer, dataStore, share)
players := core.NewPlayers(dataStore)
broker := events.GetBroker()
metricsMetrics := metrics.GetPrometheusInstance(dataStore)
manager := plugins.GetManager(dataStore, broker, metricsMetrics)
agentsAgents := agents.GetAgents(dataStore, manager)
@ -119,12 +119,12 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router {
fileCache := artwork.GetImageCache()
imageStore := artwork.GetImageStore()
fFmpeg := ffmpeg.New()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg)
broker := events.GetBroker()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker)
transcodingCache := stream.GetTranscodingCache()
mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache)
transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg)
players := core.NewPlayers(dataStore)
broker := events.GetBroker()
metricsMetrics := metrics.GetPrometheusInstance(dataStore)
manager := plugins.GetManager(dataStore, broker, metricsMetrics)
playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager)
@ -145,7 +145,8 @@ func CreatePublicRouter() *public.Router {
fileCache := artwork.GetImageCache()
imageStore := artwork.GetImageStore()
fFmpeg := ffmpeg.New()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg)
broker := events.GetBroker()
artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker)
transcodingCache := stream.GetTranscodingCache()
mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache)
share := core.NewShare(dataStore)

28
core/artwork/artwork.go
View file

@ -16,6 +16,7 @@ import (
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/resources"
"github.com/navidrome/navidrome/server/events"
"github.com/navidrome/navidrome/utils/cache"
)
@ -49,8 +50,8 @@ type Artwork interface {
GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error)
}
func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ffm ffmpeg.FFmpeg) Artwork {
return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm}
func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ffm ffmpeg.FFmpeg, broker events.Broker) Artwork {
return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm, broker: broker}
}
// entityExists reports whether the entity an artwork id points at is still there: state rows
@ -86,6 +87,7 @@ type service struct {
cache cache.FileCache
store *ImageStore
ffmpeg ffmpeg.FFmpeg
broker events.Broker
}
func (s *service) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error) {
@ -136,7 +138,7 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i
// serveSource is the one place bytes become an Image. hash is the pixel identity ("" for disc art)
// and doubles as the full-size validator, so an ETag is only needed when resized or hash is "".
func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate time.Time,
func (s *service) serveSource(ctx context.Context, artID model.ArtworkID, key, hash string, lastUpdate time.Time,
size int, square bool, open func() (io.ReadCloser, error),
) (*Image, error) {
if size == 0 && !square {
@ -158,7 +160,7 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate
deferAnimated: s.cache.Available(ctx)}
stream, err := s.cache.Get(ctx, item)
if deferred, ok := errors.AsType[*deferredAnimation](err); ok {
s.convertInBackground(item, deferred.data)
s.convertInBackground(artID, item, deferred.data)
return &Image{ReadCloser: io.NopCloser(deferred.standIn), Hash: hash, LastUpdated: lastUpdate, Transient: true}, nil
}
if err != nil {
@ -175,7 +177,7 @@ const animConvertTimeout = time.Minute
// convertInBackground caches item's conversion of data. When the slot is busy it does nothing,
// and a later request for the key tries again.
func (s *service) convertInBackground(item *resizedItem, data []byte) {
func (s *service) convertInBackground(artID model.ArtworkID, item *resizedItem, data []byte) {
select {
case animConvertSlot <- struct{}{}:
default:
@ -195,6 +197,10 @@ func (s *service) convertInBackground(item *resizedItem, data []byte) {
return
}
log.Debug(ctx, "Artwork: Converted animated image", "key", key, "bytes", len(out), "elapsed", time.Since(start))
// The image URL is unchanged, so UIs holding the stand-in only reload it when told.
if res, ok := artworkKindToResource[artID.Kind]; ok {
s.broker.SendBroadcastMessage(ctx, (&events.RefreshResource{}).With(res, artID.ID))
}
}()
}
@ -212,7 +218,7 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode
}
return nil, err
}
img, err := s.serveSource(ctx, ia.Hash, ia.Hash, ia.UpdatedAt, size, square,
img, err := s.serveSource(ctx, artID, ia.Hash, ia.Hash, ia.UpdatedAt, size, square,
func() (io.ReadCloser, error) { return openOriginal(ia, art.Mime, s.store) })
if err != nil {
if errors.Is(err, context.Canceled) {
@ -270,11 +276,11 @@ func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size i
s.enqueue(ctx, artID, model.ArtworkPriorityBump)
log.Debug(ctx, "Artwork: Provisional read-through, no state row yet", "artID", artID,
"source", res.source, "hit", res.reader != nil)
return s.serveResolution(ctx, res, size, square)
return s.serveResolution(ctx, artID, res, size, square)
}
// serveResolution turns a local resolution's bytes into a servable Image (byte-hash only, no decode).
func (s *service) serveResolution(ctx context.Context, res resolution, size int, square bool) (*Image, error) {
func (s *service) serveResolution(ctx context.Context, artID model.ArtworkID, res resolution, size int, square bool) (*Image, error) {
if res.reader == nil {
return nil, ErrUnavailable
}
@ -288,7 +294,7 @@ func (s *service) serveResolution(ctx context.Context, res resolution, size int,
return nil, ErrUnavailable
}
// Keyed by the byte-hash, so the entry lines up with the worker's eventual store entry.
return s.serveSource(ctx, hash, hash, unixMtime(res.refMtime), size, square,
return s.serveSource(ctx, artID, hash, hash, unixMtime(res.refMtime), size, square,
func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil })
}
@ -338,7 +344,7 @@ func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID
// Eligible but unextractable: fall back the way CoverArtID does, not to a placeholder.
return s.Get(ctx, mf.DiscCoverArtID(), size, square)
}
return s.serveResolution(ctx, res, size, square)
return s.serveResolution(ctx, artID, res, size, square)
}
// serveDisc reads disc art through with no state row and no enqueue, falling back to the album cover.
@ -356,7 +362,7 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int
// Disc art has no state row, hence no content hash: keying on id, album mtime and
// DiscArtPriority lets a warm cache answer without running the chain or touching the disk.
key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority)
img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square, selectImage)
img, err := s.serveSource(ctx, artID, key, "", dr.cacheTime(), size, square, selectImage)
if err != nil {
if errors.Is(err, context.Canceled) {
return nil, err

16
core/artwork/artwork_test.go
View file

@ -16,6 +16,7 @@ import (
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/resources"
"github.com/navidrome/navidrome/server/events"
"github.com/navidrome/navidrome/tests"
"github.com/navidrome/navidrome/utils/cache"
. "github.com/onsi/ginkgo/v2"
@ -109,7 +110,7 @@ var _ = Describe("Artwork", func() {
return arg.(artworkReader).Reader(ctx)
})
Eventually(func() bool { return imgCache.Available(ctx) }, 10*time.Second).Should(BeTrue())
svc = NewArtwork(ds, imgCache, store, ffm)
svc = NewArtwork(ds, imgCache, store, ffm, events.NoopBroker())
})
Describe("found state", func() {
@ -428,6 +429,7 @@ var _ = Describe("Artwork", func() {
var (
gifBytes []byte
fake *animFFmpeg
broker *fakeEventBroker
)
get := func(id string) *Image {
@ -453,7 +455,8 @@ var _ = Describe("Artwork", func() {
BeforeEach(func() {
gifBytes = createAnimatedGIF(3)
fake = &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg(""), out: []byte("animated-webp")}
svc = NewArtwork(ds, imgCache, store, fake)
broker = &fakeEventBroker{}
svc = NewArtwork(ds, imgCache, store, fake, broker)
seedFoundStore("al", "al1", gifBytes)
DeferCleanup(waitForConversions)
})
@ -470,6 +473,15 @@ var _ = Describe("Artwork", func() {
Expect(fake.calls.Load()).To(Equal(int32(1)))
})
It("tells the UI to reload the item once its conversion is cached", func() {
readAll(get("al-al1"))
waitForConversions()
sent := broker.getEvents()
Expect(sent).To(HaveLen(1))
Expect(sent[0].Data(sent[0])).To(MatchJSON(`{"album":["al1"]}`))
})
It("runs one conversion at a time and does not queue the others", func() {
fake.release = make(chan struct{})
release := sync.OnceFunc(func() { close(fake.release) })

2
core/artwork/e2e/acquire_serve_test.go
View file

@ -108,7 +108,7 @@ var _ = Describe("Acquisition → serve loop", func() {
})
Eventually(func() bool { return imgCache.Available(ctx) }, 10*time.Second).Should(BeTrue())
svc = artwork.NewArtwork(ds, imgCache, store, ffm)
svc = artwork.NewArtwork(ds, imgCache, store, ffm, events.NoopBroker())
worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache)
})

2
core/artwork/e2e/resolution_harness_test.go
View file

@ -119,7 +119,7 @@ func setupResolutionHarness() {
})
Eventually(func() bool { return imgCache.Available(rctx) }, 10*time.Second).Should(BeTrue())
rsvc = artwork.NewArtwork(rds, imgCache, rstore, ffm)
rsvc = artwork.NewArtwork(rds, imgCache, rstore, ffm, events.NoopBroker())
rworker = artwork.NewWorker(rds, rstore, agents.GetAgents(rds, nil), ffm, events.NoopBroker(), imgCache)
}

2
server/subsonic/e2e/subsonic_artwork_test.go
View file

@ -132,7 +132,7 @@ var _ = Describe("Artwork Serving", Ordered, func() {
store := artwork.NewImageStore(GinkgoT().TempDir())
imgCache := newDummyImageCache(ctx)
ffm := harness.NoopFFmpeg{}
artSvc = artwork.NewArtwork(ds, imgCache, store, ffm)
artSvc = artwork.NewArtwork(ds, imgCache, store, ffm, events.NoopBroker())
worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache)
artRouter = buildArtworkRouter(artSvc)

4
ui/src/common/Artwork.jsx
View file

@ -5,6 +5,7 @@ import { makeStyles } from '@material-ui/core/styles'
import config from '../config'
import subsonic from '../subsonic'
import { useImageUrl } from './useImageUrl'
import { useArtworkRefresh } from './useArtworkRefresh'
import { ThumbHashCanvas } from './ThumbHashCanvas'
// Drives both the CSS transition and the timer that retires the placeholder, so they cannot drift.
@ -47,7 +48,8 @@ export const Artwork = ({
}) => {
const classes = useStyles()
const url = record ? subsonic.getCoverArtUrl(record, size, square) : ''
const { imgUrl, fromCache } = useImageUrl(url)
const version = useArtworkRefresh(record?.id)
const { imgUrl, fromCache } = useImageUrl(url, version)
const [decoded, setDecoded] = useState(false)
const [faded, setFaded] = useState(false)

10
ui/src/common/Artwork.test.jsx
View file

@ -2,6 +2,7 @@ import { render, fireEvent, act } from '@testing-library/react'
import { describe, it, expect, vi, beforeEach } from 'vitest'
vi.mock('./useImageUrl', () => ({ useImageUrl: vi.fn() }))
vi.mock('./useArtworkRefresh', () => ({ useArtworkRefresh: vi.fn() }))
vi.mock('../subsonic', () => ({
default: {
// Mirrors the real hash suffix, so a refreshed record yields a different URL.
@ -13,6 +14,7 @@ vi.mock('../subsonic', () => ({
vi.mock('../config', () => ({ default: { uiCoverArtSize: 300 } }))
import { useImageUrl } from './useImageUrl'
import { useArtworkRefresh } from './useArtworkRefresh'
import { Artwork } from './Artwork'
const withArt = {
@ -28,6 +30,14 @@ describe('Artwork', () => {
HTMLCanvasElement.prototype.getContext = vi.fn(() => null)
})
it('loads the image under the version of the last refresh naming its record', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: true })
useArtworkRefresh.mockReturnValue(1234)
render(<Artwork record={withArt} />)
expect(useArtworkRefresh).toHaveBeenCalledWith('al-1')
expect(useImageUrl).toHaveBeenCalledWith('/rest/getCoverArt?id=al-1', 1234)
})
it('renders nothing without a record', () => {
useImageUrl.mockReturnValue({ imgUrl: null, loading: false })
const { container } = render(<Artwork record={null} />)

23
ui/src/common/useArtworkRefresh.js Normal file
View file

@ -0,0 +1,23 @@
import { useState } from 'react'
import { useSelector } from 'react-redux'
const namesId = (resources, id) =>
Object.values(resources || {}).some(
(ids) => Array.isArray(ids) && ids.includes(id),
)
// Artwork can change behind an unchanged URL (a background conversion replacing its stand-in), so
// this returns when the server last announced new artwork for id, for useImageUrl to version by.
export const useArtworkRefresh = (id) => {
const refresh = useSelector((state) => state.activity?.refresh)
const [refreshedAt, setRefreshedAt] = useState()
if (
id &&
refresh &&
namesId(refresh.resources, id) &&
refresh.lastReceived !== refreshedAt
) {
setRefreshedAt(refresh.lastReceived)
}
return refreshedAt
}

44
ui/src/common/useArtworkRefresh.test.js Normal file
View file

@ -0,0 +1,44 @@
import { renderHook } from '@testing-library/react-hooks'
import { vi, describe, it, expect, beforeEach } from 'vitest'
import { useSelector } from 'react-redux'
import { useArtworkRefresh } from './useArtworkRefresh'
vi.mock('react-redux', () => ({ useSelector: vi.fn() }))
const withRefresh = (refresh) =>
useSelector.mockImplementation((select) => select({ activity: { refresh } }))
describe('useArtworkRefresh', () => {
beforeEach(() => {
vi.clearAllMocks()
})
it('has no version before any refresh event', () => {
withRefresh(undefined)
const { result } = renderHook(() => useArtworkRefresh('ar-1'))
expect(result.current).toBeUndefined()
})
it('versions the artwork with the time of an event naming its id', () => {
withRefresh({ lastReceived: 100, resources: { artist: ['ar-1'] } })
const { result } = renderHook(() => useArtworkRefresh('ar-1'))
expect(result.current).toBe(100)
})
it('keeps its version when a later event names something else', () => {
withRefresh({ lastReceived: 100, resources: { artist: ['ar-1'] } })
const { result, rerender } = renderHook(() => useArtworkRefresh('ar-1'))
withRefresh({ lastReceived: 200, resources: { album: ['al-9'] } })
rerender()
expect(result.current).toBe(100)
})
// Honoring a wildcard would refetch every cover on screen after each scan.
it('ignores wildcard events', () => {
withRefresh({ lastReceived: 100, resources: { '*': '*', artist: ['*'] } })
const { result } = renderHook(() => useArtworkRefresh('ar-1'))
expect(result.current).toBeUndefined()
})
})

30
ui/src/common/useImageUrl.js
View file

@ -34,9 +34,10 @@ const evictIfNeeded = () => {
/**
* Loads an image via fetch() with AbortController so that in-flight requests
* are canceled on unmount (e.g., during pagination). Uses a module-level cache
* so remounting returns the cached blob URL instantly.
* so remounting returns the cached blob URL instantly. A new version re-fetches a cached url whose
* image changed behind it, painting the old blob until the new one arrives.
*/
export const useImageUrl = (url) => {
export const useImageUrl = (url, version) => {
const cached = url ? cache.get(url) : null
const [imgUrl, setImgUrl] = useState(cached?.blobUrl || null)
const [loading, setLoading] = useState(!!url && !cached)
@ -68,7 +69,7 @@ export const useImageUrl = (url) => {
// Re-check: another component's effect may have populated the cache
// between this component's render and effect execution.
const entry = cache.get(url)
if (entry) {
if (entry && entry.version === version) {
entry.refCount++
setImgUrl(entry.blobUrl)
setLoading(false)
@ -77,12 +78,15 @@ export const useImageUrl = (url) => {
entry.refCount--
}
}
const stale = entry?.blobUrl ? entry : null
const controller = new AbortController()
let queued = true
setImgUrl(null)
setLoading(true)
setError(false)
if (!stale) {
setImgUrl(null)
setLoading(true)
setError(false)
}
const doFetch = () => {
queued = false
@ -105,12 +109,15 @@ export const useImageUrl = (url) => {
// Handle concurrent fetches: if another component already cached
// this URL, use its entry and discard our blob.
const existing = cache.get(url)
if (existing && existing.blobUrl) {
if (existing && existing.blobUrl && existing.version === version) {
existing.refCount++
URL.revokeObjectURL(objectUrl)
setImgUrl(existing.blobUrl)
} else {
cache.set(url, { blobUrl: objectUrl, refCount: 1 })
if (existing?.blobUrl && existing.refCount <= 0) {
URL.revokeObjectURL(existing.blobUrl)
}
cache.set(url, { blobUrl: objectUrl, refCount: 1, version })
evictIfNeeded()
setImgUrl(objectUrl)
}
@ -122,8 +129,11 @@ export const useImageUrl = (url) => {
if (err.name === 'AbortError') {
return // Expected on unmount or URL change
}
if (stale) {
return // The old image is still valid; keep showing it
}
// Cache the error so repeated mounts don't re-fetch broken URLs
cache.set(url, { blobUrl: null, error: true, refCount: 0 })
cache.set(url, { blobUrl: null, error: true, refCount: 0, version })
setError(true)
setLoading(false)
})
@ -150,7 +160,7 @@ export const useImageUrl = (url) => {
entry.refCount--
}
}
}, [url])
}, [url, version])
return { imgUrl, loading, error, fromCache }
}

86
ui/src/common/useImageUrl.test.js
View file

@ -234,4 +234,90 @@ describe('useImageUrl', () => {
// A remembered failure has no blob, so callers must not treat it as instantly painted.
expect(result2.current.fromCache).toBe(false)
})
describe('version', () => {
const ok = (data) =>
Promise.resolve({
ok: true,
blob: () => Promise.resolve(new Blob([data])),
})
const renderVersioned = (version) =>
renderHook(
({ version }) => useImageUrl('http://example.com/img.jpg', version),
{ initialProps: { version } },
)
it('re-fetches a cached url when its version changes, showing the old image meanwhile', async () => {
let resolveNew
global.URL.createObjectURL = vi
.fn()
.mockReturnValueOnce('blob:old')
.mockReturnValueOnce('blob:new')
global.fetch = vi
.fn()
.mockImplementationOnce(() => ok('old'))
.mockImplementationOnce(
() => new Promise((resolve) => (resolveNew = resolve)),
)
const { result, rerender } = renderVersioned(undefined)
await act(async () => {
await flushPromises()
})
expect(result.current.imgUrl).toBe('blob:old')
rerender({ version: 1 })
await act(async () => {
await flushPromises()
})
expect(global.fetch).toHaveBeenCalledTimes(2)
expect(result.current.imgUrl).toBe('blob:old')
expect(result.current.loading).toBe(false)
await act(async () => {
resolveNew({ ok: true, blob: () => Promise.resolve(new Blob(['new'])) })
await flushPromises()
})
expect(result.current.imgUrl).toBe('blob:new')
})
it('keeps the old image when the re-fetch fails', async () => {
global.fetch = vi
.fn()
.mockImplementationOnce(() => ok('old'))
.mockImplementationOnce(() =>
Promise.resolve({ ok: false, status: 500 }),
)
const { result, rerender } = renderVersioned(undefined)
await act(async () => {
await flushPromises()
})
rerender({ version: 1 })
await act(async () => {
await flushPromises()
})
expect(global.fetch).toHaveBeenCalledTimes(2)
expect(result.current.imgUrl).toBe('blob:mock-url')
expect(result.current.error).toBe(false)
})
it('does not re-fetch on remount with the version it already has', async () => {
global.fetch = vi.fn(() => ok('data'))
const { unmount } = renderVersioned(1)
await act(async () => {
await flushPromises()
})
unmount()
const { result } = renderVersioned(1)
await act(async () => {
await flushPromises()
})
expect(global.fetch).toHaveBeenCalledTimes(1)
expect(result.current.fromCache).toBe(true)
})
})
})