From 318893c700e4dc485d6e97fd01a7704d7c36b6ab Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Jul 2026 10:20:04 -0400 Subject: [PATCH] fix(artwork): hydrate the tracks reached through a playlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadTracks and the playlist-track cursor were the only entity-page paths that never hydrated artwork state, so a song reached through a playlist behaved differently from the same song in the songs list: Subsonic emitted a hashless coverArt id, which imghttp downgrades to no-cache, and advertised art even for known-absent albums; Jellyfin emitted AlbumPrimaryImageTag as the bare album id — a tag that never changes when the cover does — and no blurhash at all. The media-file hydration moves next to the other hydration helpers so both paths share one implementation rather than growing a third. --- persistence/artwork_hydration.go | 101 +++++++++++++++++++++++ persistence/artwork_hydration_test.go | 32 +++++++ persistence/mediafile_repository.go | 51 +----------- persistence/playlist_repository.go | 4 +- persistence/playlist_track_repository.go | 5 +- 5 files changed, 142 insertions(+), 51 deletions(-) diff --git a/persistence/artwork_hydration.go b/persistence/artwork_hydration.go index 4ea92152a..6ec49d812 100644 --- a/persistence/artwork_hydration.go +++ b/persistence/artwork_hydration.go @@ -6,6 +6,7 @@ import ( "slices" . "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" @@ -74,3 +75,103 @@ func applyItemImage(infos map[string]model.ItemArtworkInfo, id string, img *mode img.BlurHash = info.BlurHash } } + +// hydrateMediaFileArtwork mirrors MediaFile.CoverArtID: an embedded-eligible file with resolved own +// art uses it, else it falls back to the album's. Two batched item_artwork lookups, never a join. +func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.MediaFiles) { + if len(mfs) == 0 { + return + } + albumIDs := make([]string, len(mfs)) + var eligibleIDs []string + for i := range mfs { + albumIDs[i] = mfs[i].AlbumID + if mfs[i].HasCoverArt && conf.Server.EnableMediaFileCoverArt { + eligibleIDs = append(eligibleIDs, mfs[i].ID) + } + } + albumInfos := hydrateItemImages(ctx, db, model.KindAlbumArtwork, albumIDs) + mfInfos := hydrateItemImages(ctx, db, model.KindMediaFileArtwork, eligibleIDs) + for i := range mfs { + mf := &mfs[i] + applyItemImage(albumInfos, mf.AlbumID, &mf.AlbumImage) + eligible := mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt + ownInfo, ownResolved := mfInfos[mf.ID] + if eligible && ownResolved && !ownInfo.Absent() { + mf.ImageHash = ownInfo.Hash // own resolved art wins + mf.BlurHash = ownInfo.BlurHash + continue + } + // Fallback (see MediaFile.CoverArtID): inherit a found album hash for optimistic caching, + // but only for a single-disc track. A multi-disc track emits a dc- id served from + // disc-specific art of unknown identity, so stamping the album hash would advertise a + // wrong content-version; leave it bare (the served response still carries a correct ETag). + if album, ok := albumInfos[mf.AlbumID]; ok && !album.Absent() { + if mf.DiscNumber == 0 { + mf.ImageHash = album.Hash + mf.BlurHash = album.BlurHash + } + continue + } + // Nothing found. Mark absent only when serving would definitively yield a placeholder: + // a single-disc track whose album is known-absent and whose own art won't resolve. A + // multi-disc track resolves disc art provisionally (never known-absent), and an + // eligible-but-unresolved track can still extract its own embedded art — both stay + // requestable. + if mf.DiscNumber > 0 { + continue + } + ownWontResolve := !eligible || (ownResolved && ownInfo.Absent()) + if album, ok := albumInfos[mf.AlbumID]; ok && album.Absent() && ownWontResolve { + mf.ImageAbsent = true + } + } +} + +// hydrateCursor buffers a streamed page into batches and hydrates each before yielding, so a +// cursor carries the same artwork state as a fetched page without a per-row query. +func hydrateCursor[T any](cursor iter.Seq2[T, error], hydrate func([]T)) iter.Seq2[T, error] { + return func(yield func(T, error) bool) { + buf := make([]T, 0, artworkBatchSize) + flush := func() bool { + hydrate(buf) + for i := range buf { + if !yield(buf[i], nil) { + return false + } + } + buf = buf[:0] + return true + } + for row, err := range cursor { + if err != nil { + var zero T + yield(zero, err) + return + } + buf = append(buf, row) + if len(buf) == artworkBatchSize && !flush() { + return + } + } + if len(buf) > 0 { + flush() + } + } +} + +// hydratePlaylistTrackArtwork hydrates the MediaFile embedded in each playlist track, so a track +// reached through a playlist carries the same artwork state as one reached through the songs list. +func hydratePlaylistTrackArtwork(ctx context.Context, db dbx.Builder, tracks model.PlaylistTracks) { + if len(tracks) == 0 { + return + } + mfs := make(model.MediaFiles, len(tracks)) + for i := range tracks { + mfs[i] = tracks[i].MediaFile + } + hydrateMediaFileArtwork(ctx, db, mfs) + for i := range tracks { + tracks[i].MediaFile = mfs[i] + } +} diff --git a/persistence/artwork_hydration_test.go b/persistence/artwork_hydration_test.go index 8f9f09afe..f670e15a5 100644 --- a/persistence/artwork_hydration_test.go +++ b/persistence/artwork_hydration_test.go @@ -178,6 +178,38 @@ var _ = Describe("Artwork hydration", func() { Expect(err).ToNot(HaveOccurred()) Expect(got.ImageHash).To(Equal("plget8888888888")) }) + + // A track reached through a playlist must carry the same artwork state as one reached + // through the songs list, or its cover id has no hash to serve immutably or blur. + It("hydrates the tracks reached through a playlist", func() { + Expect(aw.PutImage(&model.Artwork{Hash: "pltrackhash1234", Mime: "image/jpeg", BlurHash: "LPLBLURhash"})).To(Succeed()) + putInfo("al", songDayInALife.AlbumID, "pltrackhash1234") + + pls, err := repo.GetWithTracks(plsBest.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + tracks := pls.Tracks + Expect(tracks).ToNot(BeEmpty()) + byID := map[string]model.PlaylistTrack{} + for _, t := range tracks { + byID[t.MediaFile.ID] = t + } + Expect(byID).To(HaveKey(songDayInALife.ID)) + Expect(byID[songDayInALife.ID].AlbumImage.ImageHash).To(Equal("pltrackhash1234")) + Expect(byID[songDayInALife.ID].BlurHash).To(Equal("LPLBLURhash")) + + cursor, err := repo.Tracks(plsBest.ID, true).GetCursor() + Expect(err).ToNot(HaveOccurred()) + var streamed *model.PlaylistTrack + for t, err := range cursor { + Expect(err).ToNot(HaveOccurred()) + if t.MediaFile.ID == songDayInALife.ID { + streamed = &t + } + } + Expect(streamed).ToNot(BeNil()) + Expect(streamed.AlbumImage.ImageHash).To(Equal("pltrackhash1234"), + "the streamed cursor Jellyfin uses must hydrate too") + }) }) Describe("radios", func() { diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 010bc6006..62b77a368 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -223,56 +223,9 @@ func (r *mediaFileRepository) GetAll(options ...model.QueryOptions) (model.Media return mfs, nil } -// hydrateArtwork mirrors MediaFile.CoverArtID: an embedded-eligible file with resolved own art uses -// it, else it falls back to the album's. Two batched item_artwork lookups per page, never a join. +// hydrateArtwork hydrates a fetched page in place. func (r *mediaFileRepository) hydrateArtwork(mfs model.MediaFiles) { - if len(mfs) == 0 { - return - } - albumIDs := make([]string, len(mfs)) - var eligibleIDs []string - for i := range mfs { - albumIDs[i] = mfs[i].AlbumID - if mfs[i].HasCoverArt && conf.Server.EnableMediaFileCoverArt { - eligibleIDs = append(eligibleIDs, mfs[i].ID) - } - } - albumInfos := hydrateItemImages(r.ctx, r.db, model.KindAlbumArtwork, albumIDs) - mfInfos := hydrateItemImages(r.ctx, r.db, model.KindMediaFileArtwork, eligibleIDs) - for i := range mfs { - mf := &mfs[i] - applyItemImage(albumInfos, mf.AlbumID, &mf.AlbumImage) - eligible := mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt - ownInfo, ownResolved := mfInfos[mf.ID] - if eligible && ownResolved && !ownInfo.Absent() { - mf.ImageHash = ownInfo.Hash // own resolved art wins - mf.BlurHash = ownInfo.BlurHash - continue - } - // Fallback (see MediaFile.CoverArtID): inherit a found album hash for optimistic caching, - // but only for a single-disc track. A multi-disc track emits a dc- id served from - // disc-specific art of unknown identity, so stamping the album hash would advertise a - // wrong content-version; leave it bare (the served response still carries a correct ETag). - if album, ok := albumInfos[mf.AlbumID]; ok && !album.Absent() { - if mf.DiscNumber == 0 { - mf.ImageHash = album.Hash - mf.BlurHash = album.BlurHash - } - continue - } - // Nothing found. Mark absent only when serving would definitively yield a placeholder: - // a single-disc track whose album is known-absent and whose own art won't resolve. A - // multi-disc track resolves disc art provisionally (never known-absent), and an - // eligible-but-unresolved track can still extract its own embedded art — both stay - // requestable. - if mf.DiscNumber > 0 { - continue - } - ownWontResolve := !eligible || (ownResolved && ownInfo.Absent()) - if album, ok := albumInfos[mf.AlbumID]; ok && album.Absent() && ownWontResolve { - mf.ImageAbsent = true - } - } + hydrateMediaFileArtwork(r.ctx, r.db, mfs) } // GetRandom uses two passes so the random sort runs over a narrow rowid index instead of the diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 0aa848dec..b6083ebd9 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -370,7 +370,9 @@ func (r *playlistRepository) loadTracks(query SelectBuilder, id string) (model.P if err != nil { return nil, err } - return tracks.toModels(), err + res := tracks.toModels() + hydratePlaylistTrackArtwork(r.ctx, r.db, res) + return res, err } func (r *playlistRepository) Count(options ...rest.QueryOptions) (int64, error) { diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index e51ff8ea6..1a64efa20 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -130,8 +130,11 @@ func (r *playlistTrackRepository) GetCursor(options ...model.QueryOptions) (mode if err != nil { return nil, err } - return model.PlaylistTrackCursor(wrapCursor(cursor, func(t dbPlaylistTrack) *model.PlaylistTrack { + tracks := wrapCursor(cursor, func(t dbPlaylistTrack) *model.PlaylistTrack { return t.PlaylistTrack + }) + return model.PlaylistTrackCursor(hydrateCursor(tracks, func(batch []model.PlaylistTrack) { + hydratePlaylistTrackArtwork(r.ctx, r.db, batch) })), nil }