diff --git a/model/mediafile.go b/model/mediafile.go index e910bdcb9..bfb9d9058 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -151,8 +151,10 @@ func (mf MediaFile) DiscCoverArtID() ArtworkID { return mf.AlbumCoverArtID() } +// AlbumCoverArtID uses AlbumImage, not the track's own ItemImage: an album id must carry the +// album's content hash even when the track resolved art of its own. func (mf MediaFile) AlbumCoverArtID() ArtworkID { - return artworkIDFromAlbum(Album{ID: mf.AlbumID, ItemImage: mf.ItemImage}) + return artworkIDFromAlbum(Album{ID: mf.AlbumID, ItemImage: mf.AlbumImage}) } func (mf MediaFile) StructuredLyrics() (LyricList, error) { diff --git a/persistence/artwork_hydration.go b/persistence/artwork_hydration.go index 6ec49d812..ae9564d44 100644 --- a/persistence/artwork_hydration.go +++ b/persistence/artwork_hydration.go @@ -102,12 +102,14 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi mf.BlurHash = ownInfo.BlurHash continue } + ownWontResolve := !eligible || (ownResolved && ownInfo.Absent()) // 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). + // but only when the album's bytes are what serving will actually return. A multi-disc track + // emits a dc- id served from disc art, and an eligible-but-unresolved track still extracts + // its own embedded image — stamping the album hash on either advertises a content-version + // (and a blurhash) belonging to a different image. if album, ok := albumInfos[mf.AlbumID]; ok && !album.Absent() { - if mf.DiscNumber == 0 { + if mf.DiscNumber == 0 && ownWontResolve { mf.ImageHash = album.Hash mf.BlurHash = album.BlurHash } @@ -121,7 +123,6 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi if mf.DiscNumber > 0 { continue } - ownWontResolve := !eligible || (ownResolved && ownInfo.Absent()) if album, ok := albumInfos[mf.AlbumID]; ok && album.Absent() && ownWontResolve { mf.ImageAbsent = true } diff --git a/persistence/artwork_hydration_test.go b/persistence/artwork_hydration_test.go index f670e15a5..d3fefb343 100644 --- a/persistence/artwork_hydration_test.go +++ b/persistence/artwork_hydration_test.go @@ -374,14 +374,18 @@ var _ = Describe("Artwork hydration", func() { Expect(byID["2002"].ImageHash).To(BeEmpty()) }) - It("uses the album hash for an eligible file whose own art is unresolved but album is found", func() { + // serveMediaFile extracts this track's own embedded art (provisionalEmbedded), so the + // album's hash would advertise a content-version for bytes nobody will serve. + It("leaves the hash bare for an eligible file whose own art is unresolved", func() { setCover("1004", true) DeferCleanup(func() { setCover("1004", false) }) putInfo("al", "103", "alh103found11111") byID := getByID() Expect(byID["1004"].ImageAbsent).To(BeFalse()) - Expect(byID["1004"].ImageHash).To(Equal("alh103found11111")) + Expect(byID["1004"].ImageHash).To(BeEmpty()) + Expect(byID["1004"].AlbumImage.ImageHash).To(Equal("alh103found11111"), + "the album's own hash still hydrates, for AlbumCoverArtID") }) It("uses album info for an eligible file when EnableMediaFileCoverArt is off", func() { diff --git a/server/subsonic/helpers_test.go b/server/subsonic/helpers_test.go index eeb1ae544..e437ada6c 100644 --- a/server/subsonic/helpers_test.go +++ b/server/subsonic/helpers_test.go @@ -349,8 +349,17 @@ var _ = Describe("helpers", func() { }) Describe("childFromMediaFile", func() { - It("suffixes coverArt with the content hash when resolved", func() { - mf := model.MediaFile{ID: "mf-1", AlbumID: "al-1", ItemImage: model.ItemImage{ImageHash: hash}} + // The album id carries the album's hash, which hydration puts in AlbumImage; the + // track's own ItemImage describes its own art and must not be stamped onto an al- id. + It("suffixes coverArt with the album's content hash when resolved", func() { + mf := model.MediaFile{ID: "mf-1", AlbumID: "al-1", AlbumImage: model.ItemImage{ImageHash: hash}} + Expect(childFromMediaFile(ctx, mf).CoverArt).To(Equal("al-al-1_" + hash)) + }) + + It("does not stamp a track's own art hash onto the album id", func() { + mf := model.MediaFile{ID: "mf-1", AlbumID: "al-1", + ItemImage: model.ItemImage{ImageHash: "0000ownarthash00"}, + AlbumImage: model.ItemImage{ImageHash: hash}} Expect(childFromMediaFile(ctx, mf).CoverArt).To(Equal("al-al-1_" + hash)) }) It("omits coverArt when known absent", func() {