diff --git a/adapters/gotaglib/gotaglib.go b/adapters/gotaglib/gotaglib.go index 7ea98a442..ab0b18bfb 100644 --- a/adapters/gotaglib/gotaglib.go +++ b/adapters/gotaglib/gotaglib.go @@ -21,6 +21,7 @@ import ( "time" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/storage/local" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model/metadata" @@ -112,13 +113,16 @@ func (e extractor) extractMetadata(filePath string) (info *metadata.Info, err er parseTIPL(normalizedTags) delete(normalizedTags, "tmcl") // TMCL is already parsed by TagLib - // Determine if file has embedded picture - hasPicture := len(props.Images) > 0 + var pictureHash string + if len(props.Images) > 0 { + pictureHash = props.Images[artwork.BestImageIndex(props.Images)].Hash + } return &metadata.Info{ Tags: normalizedTags, AudioProperties: ap, - HasPicture: hasPicture, + HasPicture: len(props.Images) > 0, + PictureHash: pictureHash, }, nil } diff --git a/adapters/gotaglib/gotaglib_test.go b/adapters/gotaglib/gotaglib_test.go index 05924914d..4f1d7cc39 100644 --- a/adapters/gotaglib/gotaglib_test.go +++ b/adapters/gotaglib/gotaglib_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.senan.xyz/taglib" ) var _ = Describe("Extractor", func() { @@ -35,6 +36,10 @@ var _ = Describe("Extractor", func() { Expect(m.Tags).To(HaveKeyWithValue("albumartist", []string{"Album Artist"})) Expect(m.HasPicture).To(BeTrue()) + props, err := taglib.ReadProperties("tests/fixtures/test.mp3") + Expect(err).NotTo(HaveOccurred()) + Expect(m.PictureHash).To(MatchRegexp(`^[0-9a-f]{16}$`)) + Expect(m.PictureHash).To(Equal(props.Images[0].Hash)) Expect(m.AudioProperties.Duration.String()).To(Equal("1.02s")) Expect(m.AudioProperties.BitRate).To(Equal(192)) Expect(m.AudioProperties.Channels).To(Equal(2)) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 663d06d25..1c89f799c 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -256,37 +256,27 @@ func (s *service) serveResolution(ctx context.Context, res resolution, size int, } func (s *service) serveMediaFile(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) { - // The setting is not in the config fingerprint, so honor it at serve time: a direct mf- URL - // must fall back to disc/album instead of serving stale persisted embedded art. - if !conf.Server.EnableMediaFileCoverArt { - mf, err := s.ds.MediaFile(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - return s.Get(ctx, mf.DiscCoverArtID(), size, square) - } - ia, err := s.ds.Artwork(ctx).GetItemArtwork(model.KindMediaFileArtwork, artID.ID, model.ImageTypePrimary) - switch { - case err == nil && ia.Hash != "": - return s.serveHash(ctx, artID, ia, size, square) - case err == nil: - // absent row: fall through - case errors.Is(err, model.ErrNotFound): - // no row: fall through - default: - return nil, err - } - noRow := errors.Is(err, model.ErrNotFound) - mf, err := s.ds.MediaFile(ctx).Get(artID.ID) if err != nil { return nil, err } - if noRow && conf.Server.EnableMediaFileCoverArt && mf.HasCoverArt { - return s.provisionalEmbedded(ctx, artID, *mf, size, square) + // Decided at serve time so a stale mf- URL answers with what the track advertises now, not with + // persisted embedded art (EnableMediaFileCoverArt is not in the config fingerprint). + if !mf.HasOwnCoverArt() { + return s.Get(ctx, mf.DiscCoverArtID(), size, square) + } + ia, err := s.ds.Artwork(ctx).GetItemArtwork(model.KindMediaFileArtwork, artID.ID, model.ImageTypePrimary) + switch { + case errors.Is(err, model.ErrNotFound): + return s.provisionalEmbedded(ctx, artID, *mf, size, square) + case err != nil: + return nil, err + case ia.Hash == "": + // Settled absent: mirror CoverArtID's fallback rather than serving a placeholder. + return s.Get(ctx, mf.DiscCoverArtID(), size, square) + default: + return s.serveHash(ctx, artID, ia, size, square) } - // Mirror MediaFile.CoverArtID: a track defers to its disc art, which falls back to the album. - return s.Get(ctx, mf.DiscCoverArtID(), size, square) } // provisionalEmbedded serves a track's embedded art immediately, leaving the state row to the worker. diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 907b300de..552be0b0c 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -244,12 +244,24 @@ var _ = Describe("Artwork", func() { Describe("media file", func() { It("serves a track's own found art", func() { seedFoundStore("mf", "mf1", coverBytes) + mfRepo.SetData(model.MediaFiles{{ID: "mf1", AlbumID: "alba", HasCoverArt: true}}) img, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf1"), 0, false) Expect(err).ToNot(HaveOccurred()) Expect(readAll(img)).To(Equal(coverBytes)) }) + It("ignores a resolved mf row and delegates to the album when the track's picture is the album's cover", func() { + seedFoundStore("mf", "mf8", []byte("duplicate embedded track art")) + seedFoundStore("al", "alby", coverBytes) + mfRepo.SetData(model.MediaFiles{{ID: "mf8", AlbumID: "alby", HasCoverArt: true, + EmbedArtHash: "samepicxxxxxxxxx", AlbumEmbedArtHash: "samepicxxxxxxxxx"}}) + + img, err := svc.Get(ctx, model.MustParseArtworkID("mf-mf8"), 0, false) + Expect(err).ToNot(HaveOccurred()) + Expect(readAll(img)).To(Equal(coverBytes), "album art, not the persisted embedded art") + }) + It("ignores a resolved mf row and delegates to the album when per-track art is disabled", func() { conf.Server.EnableMediaFileCoverArt = false seedFoundStore("mf", "mf7", []byte("stale embedded track art")) diff --git a/core/artwork/sources.go b/core/artwork/sources.go index f2abf9da5..9eb5f0426 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -55,13 +55,6 @@ func fromExternalFile(ctx context.Context, libFS fs.FS, files []string, pattern } } -// These regexes are used to match the picture type in the file, in the order they are listed. -var picTypeRegexes = []*regexp.Regexp{ - regexp.MustCompile(`(?i).*cover.*front.*|.*front.*cover.*`), - regexp.MustCompile(`(?i).*front.*`), - regexp.MustCompile(`(?i).*cover.*`), -} - func fromTag(ctx context.Context, libFS fs.FS, relPath string) sourceFunc { return func() (io.ReadCloser, string, error) { if relPath == "" { @@ -93,7 +86,8 @@ func fromTag(ctx context.Context, libFS fs.FS, relPath string) sourceFunc { return nil, "", fmt.Errorf("no embedded image found in %s", relPath) } - imageIndex := findBestImageIndex(ctx, images, relPath) + imageIndex := BestImageIndex(images) + log.Trace(ctx, "Artwork: Using embedded image", "type", images[imageIndex].Type, "path", relPath) data, err := tf.Image(imageIndex) if err != nil || len(data) == 0 { return nil, "", fmt.Errorf("could not load embedded image from %s", relPath) @@ -102,16 +96,23 @@ func fromTag(ctx context.Context, libFS fs.FS, relPath string) sourceFunc { } } -func findBestImageIndex(ctx context.Context, images []taglib.ImageDesc, path string) int { +// Ranks embedded picture types, front covers first. +var picTypeRegexes = []*regexp.Regexp{ + regexp.MustCompile(`(?i).*cover.*front.*|.*front.*cover.*`), + regexp.MustCompile(`(?i).*front.*`), + regexp.MustCompile(`(?i).*cover.*`), +} + +// BestImageIndex returns the index of the embedded picture to serve as cover art, defaulting to the +// first. The scanner hashes the same pick, so a track's embed_art_hash matches what is served. +func BestImageIndex(images []taglib.ImageDesc) int { for _, regex := range picTypeRegexes { for i, img := range images { if regex.MatchString(img.Type) { - log.Trace(ctx, "Artwork: Found embedded image", "type", img.Type, "path", path) return i } } } - log.Trace(ctx, "Artwork: Could not find a front image. Getting the first one", "type", images[0].Type, "path", path) return 0 } diff --git a/core/artwork/sources_internal_test.go b/core/artwork/sources_internal_test.go index 4282575a5..9d3eb2359 100644 --- a/core/artwork/sources_internal_test.go +++ b/core/artwork/sources_internal_test.go @@ -10,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "go.senan.xyz/taglib" ) var _ = Describe("fromExternalFile", func() { @@ -90,3 +91,14 @@ type nonSeekableFile struct{ r *bytes.Reader } func (n *nonSeekableFile) Read(p []byte) (int, error) { return n.r.Read(p) } func (n *nonSeekableFile) Close() error { return nil } func (n *nonSeekableFile) Stat() (fs.FileInfo, error) { return nil, errors.New("not implemented") } + +var _ = Describe("BestImageIndex", func() { + It("prefers the front cover over an earlier image", func() { + images := []taglib.ImageDesc{{Type: "Back Cover"}, {Type: "Cover (front)"}} + Expect(BestImageIndex(images)).To(Equal(1)) + }) + It("falls back to the first image", func() { + images := []taglib.ImageDesc{{Type: "Artist"}, {Type: "Band"}} + Expect(BestImageIndex(images)).To(Equal(0)) + }) +}) diff --git a/core/storage/storagetest/fake_storage.go b/core/storage/storagetest/fake_storage.go index 1b0d1a6c1..8d8b49a4d 100644 --- a/core/storage/storagetest/fake_storage.go +++ b/core/storage/storagetest/fake_storage.go @@ -271,10 +271,12 @@ func (ffs *FakeFS) parseFile(filePath string) (*metadata.Info, error) { if err != nil { return nil, err } + pictureHash, _ := data["picture_hash"].(string) p := metadata.Info{ Tags: map[string][]string{}, AudioProperties: metadata.AudioProperties{}, - HasPicture: data["has_picture"] == "true", + HasPicture: data["has_picture"] == "true" || pictureHash != "", + PictureHash: pictureHash, } if d, ok := data["duration"].(float64); ok { p.AudioProperties.Duration = time.Duration(d) * time.Second diff --git a/db/migrations/20260907041357_add_embed_art_hash.sql b/db/migrations/20260907041357_add_embed_art_hash.sql new file mode 100644 index 000000000..68ee7b4e1 --- /dev/null +++ b/db/migrations/20260907041357_add_embed_art_hash.sql @@ -0,0 +1,9 @@ +-- +goose Up + +ALTER TABLE media_file ADD COLUMN embed_art_hash TEXT NOT NULL DEFAULT ''; +ALTER TABLE album ADD COLUMN embed_art_hash TEXT NOT NULL DEFAULT ''; + +-- +goose Down + +ALTER TABLE media_file DROP COLUMN embed_art_hash; +ALTER TABLE album DROP COLUMN embed_art_hash; diff --git a/go.mod b/go.mod index 4339b9c55..57b623afa 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/navidrome/navidrome go 1.27 // Fork to implement raw tags support -replace go.senan.xyz/taglib => github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df +replace go.senan.xyz/taglib => github.com/deluan/go-taglib v0.0.0-20260907044645-42bf076d332f require ( github.com/Masterminds/squirrel v1.5.4 diff --git a/go.sum b/go.sum index 71d9facfd..bfd5e1738 100644 --- a/go.sum +++ b/go.sum @@ -29,8 +29,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1 h1:5RVFMOWjMyRy8cARdy79nAmgYw3hK/4HUq48LQ6Wwqo= github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1/go.mod h1:ZXNYxsqcloTdSy/rNShjYzMhyjf0LaoftYK0p+A3h40= -github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df h1:LdLQVAWVc6hCzqnrfVIEXOhP+r0iSit+EvsXwZDyL70= -github.com/deluan/go-taglib v0.0.0-20260905051825-df1d035571df/go.mod h1:QGxQ4Z1IWyY9w56xNEFjYAaWE8uSxA/gneQ7RPcFJrY= +github.com/deluan/go-taglib v0.0.0-20260907044645-42bf076d332f h1:jepb57KAgNZ00WBo9+sEugxCejlJJCsJ+o+wzliXY1E= +github.com/deluan/go-taglib v0.0.0-20260907044645-42bf076d332f/go.mod h1:QGxQ4Z1IWyY9w56xNEFjYAaWE8uSxA/gneQ7RPcFJrY= github.com/deluan/rest v0.0.0-20211102003136-6260bc399cbf h1:tb246l2Zmpt/GpF9EcHCKTtwzrd0HGfEmoODFA/qnk4= github.com/deluan/rest v0.0.0-20211102003136-6260bc399cbf/go.mod h1:tSgDythFsl0QgS/PFWfIZqcJKnkADWneY80jaVRlqK8= github.com/deluan/sanitize v0.0.0-20241120162836-fdfd8fdfaa55 h1:wSCnggTs2f2ji6nFwQmfwgINcmSMj0xF0oHnoyRSPe4= diff --git a/model/album.go b/model/album.go index ee24bfa96..080c19b0b 100644 --- a/model/album.go +++ b/model/album.go @@ -21,6 +21,7 @@ type Album struct { LibraryName string `structs:"-" json:"libraryName" hash:"ignore"` Name string `structs:"name" json:"name"` EmbedArtPath string `structs:"embed_art_path" json:"-"` + EmbedArtHash string `structs:"embed_art_hash" json:"-"` // Picture hash of the EmbedArtPath track AlbumArtistID string `structs:"album_artist_id" json:"albumArtistId"` // Deprecated, use Participants // AlbumArtist is the display name used for the album artist. AlbumArtist string `structs:"album_artist" json:"albumArtist"` diff --git a/model/mediafile.go b/model/mediafile.go index 2669018f3..f3140a3bb 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -30,6 +30,8 @@ type MediaFile struct { // AlbumImage is the parent album's artwork state, hydrated alongside the track's own so a // song's Jellyfin album-art tag can be pixel-versioned without a second query. AlbumImage ItemImage `structs:"-" json:"-" hash:"ignore"` + // AlbumEmbedArtHash is the album's embedded-cover hash, hydrated so HasOwnCoverArt can compare. + AlbumEmbedArtHash string `structs:"-" json:"-" hash:"ignore"` ID string `structs:"id" json:"id" hash:"ignore"` PID string `structs:"pid" json:"-" hash:"ignore"` @@ -48,6 +50,7 @@ type MediaFile struct { AlbumArtist string `structs:"album_artist" json:"albumArtist"` AlbumID string `structs:"album_id" json:"albumId" hash:"ignore"` HasCoverArt bool `structs:"has_cover_art" json:"hasCoverArt"` + EmbedArtHash string `structs:"embed_art_hash" json:"-"` // Tag reader's fingerprint of the embedded picture TrackNumber int `structs:"track_number" json:"trackNumber"` DiscNumber int `structs:"disc_number" json:"discNumber"` DiscSubtitle string `structs:"disc_subtitle" json:"discSubtitle,omitempty"` @@ -132,12 +135,19 @@ func (mf MediaFile) ContentType() string { return mime.TypeByExtension("." + mf.Suffix) } +// HasOwnCoverArt reports whether the track serves its own embedded art. A picture identical to the +// album's embedded cover defers to the album id instead, so clients cache one image per album. +func (mf MediaFile) HasOwnCoverArt() bool { + if !mf.HasCoverArt || !conf.Server.EnableMediaFileCoverArt { + return false + } + return mf.EmbedArtHash == "" || mf.EmbedArtHash != mf.AlbumEmbedArtHash +} + func (mf MediaFile) CoverArtID() ArtworkID { - // If it has a cover art, return it (if feature is disabled, skip) - if mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt { + if mf.HasOwnCoverArt() { return artworkIDFromMediaFile(mf) } - // Otherwise fallback to disc (if available) or album cover return mf.DiscCoverArtID() } @@ -337,9 +347,9 @@ func (mfs MediaFiles) ToAlbum() Album { tags := make(TagList, 0, len(mfs[0].Tags)*len(mfs)) a.Missing = true - embedArtPath := "" - embedArtDisc := 0 - for _, m := range mfs { + var embedArt *MediaFile + for i := range mfs { + m := &mfs[i] // We assume these attributes are all the same for all songs in an album a.ID = m.AlbumID a.LibraryID = m.LibraryID @@ -375,8 +385,7 @@ func (mfs MediaFiles) ToAlbum() Album { tags = append(tags, m.Tags.FlattenAll()...) a.Participants.Merge(m.Participants) - // Find the MediaFile with cover art and the lowest disc number to use for album cover - embedArtPath, embedArtDisc = firstArtPath(embedArtPath, embedArtDisc, m) + embedArt = firstArtTrack(embedArt, m) if m.ExplicitStatus == "c" && a.ExplicitStatus != "e" { a.ExplicitStatus = "c" @@ -389,7 +398,10 @@ func (mfs MediaFiles) ToAlbum() Album { a.Missing = a.Missing && m.Missing } - a.EmbedArtPath = embedArtPath + if embedArt != nil { + a.EmbedArtPath = embedArt.Path + a.EmbedArtHash = embedArt.EmbedArtHash + } a.SetTags(tags) a.FolderIDs = slice.Unique(slice.Map(mfs, func(m MediaFile) string { return m.FolderID })) a.Date, _ = allOrNothing(dates) @@ -495,26 +507,19 @@ func fixAlbumArtist(a *Album) { } } -// firstArtPath determines which media file path should be used for album artwork -// based on disc number (preferring lower disc numbers) and path (for consistency) -func firstArtPath(currentPath string, currentDisc int, m MediaFile) (string, int) { +// firstArtTrack picks the media file whose embedded picture serves as the album cover, preferring +// lower disc numbers and then path order for consistency. +func firstArtTrack(current, m *MediaFile) *MediaFile { if !m.HasCoverArt { - return currentPath, currentDisc + return current } - - // If current has no disc number (currentDisc == 0) or new file has lower disc number - if currentDisc == 0 || (m.DiscNumber < currentDisc && m.DiscNumber > 0) { - return m.Path, m.DiscNumber + if current == nil || current.DiscNumber == 0 || (m.DiscNumber < current.DiscNumber && m.DiscNumber > 0) { + return m } - - // If disc numbers are equal, use path for ordering - if m.DiscNumber == currentDisc { - if m.Path < currentPath || currentPath == "" { - return m.Path, m.DiscNumber - } + if m.DiscNumber == current.DiscNumber && m.Path < current.Path { + return m } - - return currentPath, currentDisc + return current } // ToM3U8 exports the playlist to the Extended M3U8 format, as specified in diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 9ca3489bb..a6cab18ed 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -31,7 +31,7 @@ var _ = Describe("MediaFiles", func() { OrderAlbumName: "OrderAlbumName", OrderArtistName: "OrderArtistName", OrderAlbumArtistName: "OrderAlbumArtistName", MbzAlbumArtistID: "MbzAlbumArtistID", MbzAlbumType: "MbzAlbumType", MbzAlbumComment: "MbzAlbumComment", MbzReleaseGroupID: "MbzReleaseGroupID", - Compilation: true, CatalogNum: "CatalogNum", HasCoverArt: true, Path: "music2/file2.mp3", FolderID: "Folder2", + Compilation: true, CatalogNum: "CatalogNum", HasCoverArt: true, EmbedArtHash: "picturehash2", Path: "music2/file2.mp3", FolderID: "Folder2", }, } }) @@ -53,6 +53,7 @@ var _ = Describe("MediaFiles", func() { Expect(album.CatalogNum).To(Equal("CatalogNum")) Expect(album.Compilation).To(BeTrue()) Expect(album.EmbedArtPath).To(Equal("music2/file2.mp3")) + Expect(album.EmbedArtHash).To(Equal("picturehash2")) Expect(album.FolderIDs).To(ConsistOf("Folder1", "Folder2")) }) }) @@ -587,6 +588,28 @@ var _ = Describe("MediaFile", func() { Expect(id.Kind).To(Equal(KindAlbumArtwork)) Expect(id.ID).To(Equal(mf.AlbumID)) }) + It("returns its album id if its embedded picture is the album's cover", func() { + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true, EmbedArtHash: "samepic", AlbumEmbedArtHash: "samepic"} + Expect(mf.HasOwnCoverArt()).To(BeFalse()) + id := mf.CoverArtID() + Expect(id.Kind).To(Equal(KindAlbumArtwork)) + Expect(id.ID).To(Equal(mf.AlbumID)) + }) + It("returns disc art id if its embedded picture is the album's cover and DiscNumber > 0", func() { + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true, DiscNumber: 2, EmbedArtHash: "samepic", AlbumEmbedArtHash: "samepic"} + id := mf.CoverArtID() + Expect(id.Kind).To(Equal(KindDiscArtwork)) + Expect(id.ID).To(Equal("1:2")) + }) + It("returns its own id if its embedded picture differs from the album's cover", func() { + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true, EmbedArtHash: "ownpic", AlbumEmbedArtHash: "samepic"} + Expect(mf.HasOwnCoverArt()).To(BeTrue()) + Expect(mf.CoverArtID().Kind).To(Equal(KindMediaFileArtwork)) + }) + It("returns its own id if its picture hash is unknown", func() { + mf := MediaFile{ID: "111", AlbumID: "1", HasCoverArt: true, AlbumEmbedArtHash: "samepic"} + Expect(mf.CoverArtID().Kind).To(Equal(KindMediaFileArtwork)) + }) }) Describe("AudioCodec", func() { diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go index b3ce4ef02..ccad3be41 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -65,6 +65,7 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { // General properties mf.HasCoverArt = md.HasPicture() + mf.EmbedArtHash = md.PictureHash() mf.Duration = md.Length() mf.BitRate = md.AudioProperties().BitRate mf.SampleRate = md.AudioProperties().SampleRate diff --git a/model/metadata/metadata.go b/model/metadata/metadata.go index 0efbe94ec..d87630eb3 100644 --- a/model/metadata/metadata.go +++ b/model/metadata/metadata.go @@ -23,6 +23,7 @@ type Info struct { Tags model.RawTags AudioProperties AudioProperties HasPicture bool + PictureHash string } type FileInfo interface { @@ -69,20 +70,22 @@ func NewPair(key, value string) string { func New(filePath string, info Info) Metadata { return Metadata{ - filePath: filePath, - fileInfo: info.FileInfo, - tags: clean(filePath, info.Tags), - audioProps: info.AudioProperties, - hasPicture: info.HasPicture, + filePath: filePath, + fileInfo: info.FileInfo, + tags: clean(filePath, info.Tags), + audioProps: info.AudioProperties, + hasPicture: info.HasPicture, + pictureHash: info.PictureHash, } } type Metadata struct { - filePath string - fileInfo FileInfo - tags model.Tags - audioProps AudioProperties - hasPicture bool + filePath string + fileInfo FileInfo + tags model.Tags + audioProps AudioProperties + hasPicture bool + pictureHash string } func (md Metadata) FilePath() string { return md.filePath } @@ -95,6 +98,7 @@ func (md Metadata) Suffix() string { func (md Metadata) AudioProperties() AudioProperties { return md.audioProps } func (md Metadata) Length() float32 { return float32(md.audioProps.Duration.Milliseconds()) / 1000 } func (md Metadata) HasPicture() bool { return md.hasPicture } +func (md Metadata) PictureHash() string { return md.pictureHash } func (md Metadata) All() model.Tags { return md.tags } func (md Metadata) Strings(key model.TagName) []string { return md.tags[key] } func (md Metadata) String(key model.TagName) string { return md.first(key) } diff --git a/model/metadata/metadata_test.go b/model/metadata/metadata_test.go index 09a2dfde0..b985d9810 100644 --- a/model/metadata/metadata_test.go +++ b/model/metadata/metadata_test.go @@ -36,8 +36,9 @@ var _ = Describe("Metadata", func() { Duration: time.Minute * 3, BitRate: 320, }, - HasPicture: true, - FileInfo: testFileInfo{fileInfo}, + HasPicture: true, + PictureHash: "0123456789abcdef", + FileInfo: testFileInfo{fileInfo}, } }) @@ -60,6 +61,7 @@ var _ = Describe("Metadata", func() { Expect(md.AudioProperties()).To(Equal(props.AudioProperties)) Expect(md.Length()).To(Equal(float32(3 * 60))) Expect(md.HasPicture()).To(Equal(props.HasPicture)) + Expect(md.PictureHash()).To(Equal(props.PictureHash)) Expect(md.Strings(model.TagTrackArtist)).To(Equal([]string{"First Artist", "Second Artist"})) Expect(md.String(model.TagTrackArtist)).To(Equal("First Artist")) Expect(md.Int(model.TagCatalogNumber)).To(Equal(int64(1234))) diff --git a/persistence/artwork_hydration.go b/persistence/artwork_hydration.go index fa9920f27..2739220ed 100644 --- a/persistence/artwork_hydration.go +++ b/persistence/artwork_hydration.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils/slice" "github.com/pocketbase/dbx" ) @@ -95,11 +96,12 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi if len(mfs) == 0 { return } + hydrateAlbumEmbedArtHashes(ctx, db, mfs) albumIDs := make([]string, len(mfs)) var eligibleIDs []string for i := range mfs { albumIDs[i] = mfs[i].AlbumID - if mfs[i].HasCoverArt && conf.Server.EnableMediaFileCoverArt { + if mfs[i].HasOwnCoverArt() { eligibleIDs = append(eligibleIDs, mfs[i].ID) } } @@ -108,7 +110,7 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi for i := range mfs { mf := &mfs[i] applyItemImage(albumInfos, mf.AlbumID, &mf.AlbumImage) - eligible := mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt + eligible := mf.HasOwnCoverArt() ownInfo, ownResolved := mfInfos[mf.ID] if eligible && ownResolved && !ownInfo.Absent() { mf.ItemImage = ownInfo.Image() @@ -134,6 +136,39 @@ func hydrateMediaFileArtwork(ctx context.Context, db dbx.Builder, mfs model.Medi } } +// hydrateAlbumEmbedArtHashes fills AlbumEmbedArtHash for tracks with a hashed embedded picture, the +// only input HasOwnCoverArt needs beyond the row itself. +func hydrateAlbumEmbedArtHashes(ctx context.Context, db dbx.Builder, mfs model.MediaFiles) { + if !conf.Server.EnableMediaFileCoverArt { + return + } + var albumIDs []string + for i := range mfs { + if mfs[i].EmbedArtHash != "" { + albumIDs = append(albumIDs, mfs[i].AlbumID) + } + } + if len(albumIDs) == 0 { + return + } + repo := sqlRepository{ctx: ctx, db: db, tableName: "album"} + hashes := map[string]string{} + for chunk := range slices.Chunk(slice.Unique(albumIDs), artworkChunkSize) { + var rows []struct{ ID, EmbedArtHash string } + sel := Select("id", "embed_art_hash").From("album").Where(Eq{"id": chunk}) + if err := repo.queryAll(sel, &rows); err != nil { + log.Error(ctx, "Failed to hydrate album embedded art hashes onto page", err) + return + } + for _, row := range rows { + hashes[row.ID] = row.EmbedArtHash + } + } + for i := range mfs { + mfs[i].AlbumEmbedArtHash = hashes[mfs[i].AlbumID] + } +} + // hydrateCursor hydrates a streamed cursor in batches, avoiding 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) { diff --git a/persistence/artwork_hydration_test.go b/persistence/artwork_hydration_test.go index bb9cda04d..f510ed0b0 100644 --- a/persistence/artwork_hydration_test.go +++ b/persistence/artwork_hydration_test.go @@ -231,6 +231,19 @@ var _ = Describe("Artwork hydration", func() { Expect(err).ToNot(HaveOccurred()) } + setEmbedHash := func(table, id, hash string) { + _, err := GetDBXBuilder().NewQuery("UPDATE " + table + " SET embed_art_hash={:h} WHERE id={:id}"). + Bind(dbx.Params{"h": hash, "id": id}).Execute() + Expect(err).ToNot(HaveOccurred()) + } + + clearEmbedHashes := func() { + for _, table := range []string{"media_file", "album"} { + _, err := GetDBXBuilder().NewQuery("UPDATE " + table + " SET embed_art_hash=''").Execute() + Expect(err).ToNot(HaveOccurred()) + } + } + getByID := func() map[string]model.MediaFile { all, err := repo.GetAll() Expect(err).ToNot(HaveOccurred()) @@ -267,6 +280,41 @@ var _ = Describe("Artwork hydration", func() { Expect(byID["2002"].ImageAbsent).To(BeFalse()) }) + It("skips the album lookup when no track on the page embeds a picture", func() { + mfs := model.MediaFiles{{ID: "x1", AlbumID: "101"}, {ID: "x2", AlbumID: "102", HasCoverArt: true}} + // A nil builder panics on any query, so reaching the assertions proves no lookup ran. + Expect(func() { hydrateAlbumEmbedArtHashes(ctx, nil, mfs) }).ToNot(Panic()) + Expect(mfs[0].AlbumEmbedArtHash).To(BeEmpty()) + Expect(mfs[1].AlbumEmbedArtHash).To(BeEmpty()) + }) + + It("defers a track to its album when its embedded picture is the album's cover", func() { + setCover("1001", true) + setCover("1002", true) + setEmbedHash("media_file", "1001", "samepicxxxxxxxxx") + setEmbedHash("album", "101", "samepicxxxxxxxxx") + setEmbedHash("media_file", "1002", "ownpicxxxxxxxxxx") + setEmbedHash("album", "102", "otherpicxxxxxxxx") + DeferCleanup(func() { setCover("1001", false); setCover("1002", false); clearEmbedHashes() }) + + putInfo("al", "101", "alh101xxxxxxxxxx") + putInfo("al", "102", "alh102xxxxxxxxxx") + putInfo("mf", "1001", "mfh1001xxxxxxxx") // resolved, but it is the album's picture + putInfo("mf", "1002", "mfh1002xxxxxxxx") + + byID := getByID() + + Expect(byID["1001"].AlbumEmbedArtHash).To(Equal("samepicxxxxxxxxx")) + Expect(byID["1001"].HasOwnCoverArt()).To(BeFalse()) + Expect(byID["1001"].ImageHash).To(Equal("alh101xxxxxxxxxx")) + Expect(byID["1001"].CoverArtID().Kind).To(Equal(model.KindAlbumArtwork)) + + Expect(byID["1002"].AlbumEmbedArtHash).To(Equal("otherpicxxxxxxxx")) + Expect(byID["1002"].HasOwnCoverArt()).To(BeTrue()) + Expect(byID["1002"].ImageHash).To(Equal("mfh1002xxxxxxxx")) + Expect(byID["1002"].CoverArtID().Kind).To(Equal(model.KindMediaFileArtwork)) + }) + It("populates AlbumImage from hydrateArtwork regardless of which continue branch a track takes", func() { setCover("1001", true) // eligible, resolves its own art -> own-art-wins continue DeferCleanup(func() { setCover("1001", false) }) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 8542b3ac6..1f9529d03 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -445,6 +445,69 @@ var _ = Describe("Scanner", Ordered, func() { }) }) + Context("Album art only in an external file", func() { + BeforeEach(func() { + revolver := template(_t{"albumartist": "The Beatles", "album": "Revolver", "year": 1966}) + createFS(fstest.MapFS{ + "The Beatles/Revolver/01 - Taxman.mp3": revolver(track(1, "Taxman")), + "The Beatles/Revolver/02 - Eleanor Rigby.mp3": revolver(track(2, "Eleanor Rigby")), + "The Beatles/Revolver/cover.jpg": &fstest.MapFile{Data: []byte("jpeg bytes")}, + }) + }) + + It("stores no picture hash and keeps every track on the album art", func() { + conf.Server.EnableMediaFileCoverArt = true + Expect(runScanner(ctx, true)).To(Succeed()) + + albums, err := ds.Album(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(albums).To(HaveLen(1)) + Expect(albums[0].EmbedArtPath).To(BeEmpty()) + Expect(albums[0].EmbedArtHash).To(BeEmpty()) + + mfs, err := ds.MediaFile(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(mfs).To(HaveLen(2)) + for _, mf := range mfs { + Expect(mf.HasCoverArt).To(BeFalse()) + Expect(mf.EmbedArtHash).To(BeEmpty()) + Expect(mf.CoverArtID().Kind).To(Equal(model.KindAlbumArtwork)) + } + }) + }) + + Context("Tracks embedding the album cover", func() { + BeforeEach(func() { + revolver := template(_t{"albumartist": "The Beatles", "album": "Revolver", "year": 1966, "disc": 1}) + createFS(fstest.MapFS{ + "The Beatles/Revolver/01 - Taxman.mp3": revolver(track(1, "Taxman", _t{"picture_hash": "aaaaaaaaaaaaaaaa"})), + "The Beatles/Revolver/02 - Eleanor Rigby.mp3": revolver(track(2, "Eleanor Rigby", _t{"picture_hash": "aaaaaaaaaaaaaaaa"})), + "The Beatles/Revolver/03 - Love You To.mp3": revolver(track(3, "Love You To", _t{"picture_hash": "bbbbbbbbbbbbbbbb"})), + }) + }) + + It("stores the cover picture hash on the album and points matching tracks at the album art", func() { + conf.Server.EnableMediaFileCoverArt = true + Expect(runScanner(ctx, true)).To(Succeed()) + + albums, err := ds.Album(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(albums).To(HaveLen(1)) + Expect(albums[0].EmbedArtPath).To(Equal("The Beatles/Revolver/01 - Taxman.mp3")) + Expect(albums[0].EmbedArtHash).To(Equal("aaaaaaaaaaaaaaaa")) + + mfs, err := ds.MediaFile(ctx).GetAll() + Expect(err).ToNot(HaveOccurred()) + byTitle := slice.ToMap(mfs, func(mf model.MediaFile) (string, model.MediaFile) { return mf.Title, mf }) + Expect(byTitle).To(HaveLen(3)) + Expect(byTitle["Taxman"].EmbedArtHash).To(Equal("aaaaaaaaaaaaaaaa")) + // Matching tracks defer to the disc, which falls back to the album; the odd one keeps its own id. + Expect(byTitle["Taxman"].CoverArtID().Kind).To(Equal(model.KindDiscArtwork)) + Expect(byTitle["Eleanor Rigby"].CoverArtID().Kind).To(Equal(model.KindDiscArtwork)) + Expect(byTitle["Love You To"].CoverArtID().Kind).To(Equal(model.KindMediaFileArtwork)) + }) + }) + Context("Ignored entries", func() { BeforeEach(func() { revolver := template(_t{"albumartist": "The Beatles", "album": "Revolver", "year": 1966}) diff --git a/server/jellyfin/dto/mappers.go b/server/jellyfin/dto/mappers.go index 1ebf66ca1..ae0654219 100644 --- a/server/jellyfin/dto/mappers.go +++ b/server/jellyfin/dto/mappers.go @@ -224,8 +224,7 @@ func SongToBaseItem(mf model.MediaFile, fields Fields) BaseItemDto { } func embeddedArtPending(mf model.MediaFile) bool { - return mf.HasCoverArt && conf.Server.EnableMediaFileCoverArt && - mf.ImageHash == "" && !mf.ItemImage.ImageAbsent + return mf.HasOwnCoverArt() && mf.ImageHash == "" && !mf.ItemImage.ImageAbsent } // primaryImage never fakes a blurhash: clients key their cover cache on the value, which would diff --git a/server/jellyfin/dto/mappers_test.go b/server/jellyfin/dto/mappers_test.go index 576482e61..87a3d463a 100644 --- a/server/jellyfin/dto/mappers_test.go +++ b/server/jellyfin/dto/mappers_test.go @@ -542,6 +542,16 @@ var _ = Describe("mappers", func() { Expect(item.ImageTags).To(BeEmpty()) Expect(item.AlbumPrimaryImageTag).To(Equal("0123456789abcdef")) }) + + It("falls back to the album when the track's picture is the album's embedded cover", func() { + mf := model.MediaFile{ID: testID("mf-5"), AlbumID: testID("alb-1"), HasCoverArt: true, + EmbedArtHash: "samepicxxxxxxxxx", AlbumEmbedArtHash: "samepicxxxxxxxxx"} + mf.AlbumImage.ImageHash = "0123456789abcdef" + + item := SongToBaseItem(mf, nil) + Expect(item.ImageTags).To(BeEmpty()) + Expect(item.AlbumPrimaryImageTag).To(Equal("0123456789abcdef")) + }) }) Describe("primary image tags", func() {