mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-11 03:47:18 +02:00
fix(artwork): hydrate the tracks reached through a playlist
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.
This commit is contained in:
parent
d319af9807
commit
318893c700
5 changed files with 142 additions and 51 deletions
|
|
@ -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]
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue