From af984bc81713d055b6b90474cb4e48ba893ceed2 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 00:35:21 -0400 Subject: [PATCH 01/88] feat(artwork): advertise album art for tracks whose embedded cover is the album's Most albums embed the same picture in every track, so clients requesting per-track cover art (mf- ids) download and cache the same image once per song. This change hashes the embedded picture at scan time (XXH3 of the bytes, the same digest the artwork pipeline uses) and stores it on media_file.embed_art_hash. ToAlbum stores the hash of the track it already picks as the album's embedded cover (EmbedArtPath) on album.embed_art_hash. A new MediaFile.HasOwnCoverArt replaces the scattered "HasCoverArt && EnableMediaFileCoverArt" checks: a track whose hash equals its album's now returns the disc/album artwork id from CoverArtID, so the Subsonic, Jellyfin and native responses point every matching song at one cached image. Serving a stale mf- URL follows the same rule, so what is advertised and what is served agree. Tracks with a different embedded picture (compilations, per-track art) keep their own ids. Rows scanned before this change have an empty hash and keep the previous behavior until a full scan; no config option is added. The album hash is hydrated onto tracks with one batched PK lookup per page, only when the page has hashed tracks. BestImageIndex is exported from core/artwork so the scanner hashes the same picture the resolver serves. --- adapters/gotaglib/gotaglib.go | 14 ++++- adapters/gotaglib/gotaglib_test.go | 6 ++ core/artwork/artwork.go | 42 ++++++-------- core/artwork/artwork_test.go | 12 ++++ core/artwork/image_store.go | 3 +- core/artwork/sources.go | 23 ++++---- core/artwork/sources_internal_test.go | 12 ++++ core/storage/storagetest/fake_storage.go | 4 +- .../20260907041357_add_embed_art_hash.sql | 9 +++ model/album.go | 1 + model/artwork_id.go | 5 ++ model/mediafile.go | 55 ++++++++++--------- model/mediafile_test.go | 25 ++++++++- model/metadata/map_mediafile.go | 1 + model/metadata/metadata.go | 24 ++++---- model/metadata/metadata_test.go | 6 +- persistence/artwork_hydration.go | 39 ++++++++++++- persistence/artwork_hydration_test.go | 40 ++++++++++++++ scanner/scanner_test.go | 32 +++++++++++ server/jellyfin/dto/mappers.go | 3 +- server/jellyfin/dto/mappers_test.go | 10 ++++ 21 files changed, 282 insertions(+), 84 deletions(-) create mode 100644 db/migrations/20260907041357_add_embed_art_hash.sql diff --git a/adapters/gotaglib/gotaglib.go b/adapters/gotaglib/gotaglib.go index 7ea98a442..412f1a968 100644 --- a/adapters/gotaglib/gotaglib.go +++ b/adapters/gotaglib/gotaglib.go @@ -21,9 +21,12 @@ 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" "github.com/navidrome/navidrome/model/metadata" + "github.com/zeebo/xxh3" "go.senan.xyz/taglib" ) @@ -112,13 +115,18 @@ 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 { + if data, err := f.Image(artwork.BestImageIndex(props.Images)); err == nil && len(data) > 0 { + pictureHash = model.FormatImageHash(xxh3.Hash(data)) + } + } 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..4a31d8a58 100644 --- a/adapters/gotaglib/gotaglib_test.go +++ b/adapters/gotaglib/gotaglib_test.go @@ -1,6 +1,7 @@ package gotaglib import ( + "fmt" "io/fs" "os" "strings" @@ -9,6 +10,8 @@ import ( "github.com/navidrome/navidrome/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/zeebo/xxh3" + "go.senan.xyz/taglib" ) var _ = Describe("Extractor", func() { @@ -35,6 +38,9 @@ var _ = Describe("Extractor", func() { Expect(m.Tags).To(HaveKeyWithValue("albumartist", []string{"Album Artist"})) Expect(m.HasPicture).To(BeTrue()) + img, err := taglib.ReadImage("tests/fixtures/test.mp3") + Expect(err).NotTo(HaveOccurred()) + Expect(m.PictureHash).To(Equal(fmt.Sprintf("%016x", xxh3.Hash(img)))) 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/image_store.go b/core/artwork/image_store.go index 5dbe727e4..a15cd1ce3 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" "github.com/zeebo/xxh3" ) @@ -50,7 +51,7 @@ func hashImage(r io.Reader) (string, error) { if _, err := io.Copy(d, r); err != nil { return "", err } - return fmt.Sprintf("%016x", d.Sum64()), nil + return model.FormatImageHash(d.Sum64()), nil } // validHash guards path sharding: a malformed hash would slice-panic or inject path separators. 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/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/artwork_id.go b/model/artwork_id.go index e827e935a..0854e6a65 100644 --- a/model/artwork_id.go +++ b/model/artwork_id.go @@ -110,6 +110,11 @@ func ParseArtworkID(id string) (ArtworkID, error) { return parsedID, nil } +// FormatImageHash renders an XXH3-64 digest as the 16-hex image hash used in artwork ids and rows. +func FormatImageHash(sum uint64) string { + return fmt.Sprintf("%016x", sum) +} + // isImageHash reports whether s is a 16-char lowercase-hex XXH3-64 content hash. func isImageHash(s string) bool { if len(s) != 16 { diff --git a/model/mediafile.go b/model/mediafile.go index 2669018f3..c0334fb43 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:"-"` // XXH3 of the embedded picture bytes 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..9e06719b0 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,33 @@ var _ = Describe("Artwork hydration", func() { Expect(byID["2002"].ImageAbsent).To(BeFalse()) }) + 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..3387a5a7f 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -445,6 +445,38 @@ var _ = Describe("Scanner", Ordered, func() { }) }) + 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() { From 7b12c41fb144cb541baa64ecde22fe4cb6f2a31f Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 00:59:32 -0400 Subject: [PATCH 02/88] perf(scanner): take the picture fingerprint from go-taglib instead of hashing in Go Reading the embedded picture out of the WASM module to hash it in Go cost +28% in tag extraction and doubled allocations (93 real MP3s: 32.5ms -> 41.7ms, 91MiB -> 188MiB per pass), because the fork instantiates a module per file and the image read grows its memory and copies the bytes twice. go-taglib now fingerprints each embedded picture inside the module during the existing properties call (length, head, tail and 64 sampled stripes) and exposes it as ImageDesc.Hash. The extractor stores that value, so no picture bytes cross the boundary. Extraction is back to master speed (32.0ms, p=0.24) and the scanner benchmark is unchanged (10.88ms vs 10.89ms). Also adds tests for albums whose only art is an external file: no fingerprint is stored, tracks keep the album id, and hydration skips the album lookup. --- adapters/gotaglib/gotaglib.go | 6 +----- adapters/gotaglib/gotaglib_test.go | 7 +++--- core/artwork/image_store.go | 3 +-- go.mod | 2 +- go.sum | 4 ++-- model/artwork_id.go | 5 ----- persistence/artwork_hydration_test.go | 8 +++++++ scanner/scanner_test.go | 31 +++++++++++++++++++++++++++ 8 files changed, 47 insertions(+), 19 deletions(-) diff --git a/adapters/gotaglib/gotaglib.go b/adapters/gotaglib/gotaglib.go index 412f1a968..ab0b18bfb 100644 --- a/adapters/gotaglib/gotaglib.go +++ b/adapters/gotaglib/gotaglib.go @@ -24,9 +24,7 @@ import ( "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/storage/local" "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/metadata" - "github.com/zeebo/xxh3" "go.senan.xyz/taglib" ) @@ -117,9 +115,7 @@ func (e extractor) extractMetadata(filePath string) (info *metadata.Info, err er var pictureHash string if len(props.Images) > 0 { - if data, err := f.Image(artwork.BestImageIndex(props.Images)); err == nil && len(data) > 0 { - pictureHash = model.FormatImageHash(xxh3.Hash(data)) - } + pictureHash = props.Images[artwork.BestImageIndex(props.Images)].Hash } return &metadata.Info{ diff --git a/adapters/gotaglib/gotaglib_test.go b/adapters/gotaglib/gotaglib_test.go index 4a31d8a58..4f1d7cc39 100644 --- a/adapters/gotaglib/gotaglib_test.go +++ b/adapters/gotaglib/gotaglib_test.go @@ -1,7 +1,6 @@ package gotaglib import ( - "fmt" "io/fs" "os" "strings" @@ -10,7 +9,6 @@ import ( "github.com/navidrome/navidrome/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/zeebo/xxh3" "go.senan.xyz/taglib" ) @@ -38,9 +36,10 @@ var _ = Describe("Extractor", func() { Expect(m.Tags).To(HaveKeyWithValue("albumartist", []string{"Album Artist"})) Expect(m.HasPicture).To(BeTrue()) - img, err := taglib.ReadImage("tests/fixtures/test.mp3") + props, err := taglib.ReadProperties("tests/fixtures/test.mp3") Expect(err).NotTo(HaveOccurred()) - Expect(m.PictureHash).To(Equal(fmt.Sprintf("%016x", xxh3.Hash(img)))) + 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/image_store.go b/core/artwork/image_store.go index a15cd1ce3..5dbe727e4 100644 --- a/core/artwork/image_store.go +++ b/core/artwork/image_store.go @@ -14,7 +14,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" "github.com/zeebo/xxh3" ) @@ -51,7 +50,7 @@ func hashImage(r io.Reader) (string, error) { if _, err := io.Copy(d, r); err != nil { return "", err } - return model.FormatImageHash(d.Sum64()), nil + return fmt.Sprintf("%016x", d.Sum64()), nil } // validHash guards path sharding: a malformed hash would slice-panic or inject path separators. 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/artwork_id.go b/model/artwork_id.go index 0854e6a65..e827e935a 100644 --- a/model/artwork_id.go +++ b/model/artwork_id.go @@ -110,11 +110,6 @@ func ParseArtworkID(id string) (ArtworkID, error) { return parsedID, nil } -// FormatImageHash renders an XXH3-64 digest as the 16-hex image hash used in artwork ids and rows. -func FormatImageHash(sum uint64) string { - return fmt.Sprintf("%016x", sum) -} - // isImageHash reports whether s is a 16-char lowercase-hex XXH3-64 content hash. func isImageHash(s string) bool { if len(s) != 16 { diff --git a/persistence/artwork_hydration_test.go b/persistence/artwork_hydration_test.go index 9e06719b0..f510ed0b0 100644 --- a/persistence/artwork_hydration_test.go +++ b/persistence/artwork_hydration_test.go @@ -280,6 +280,14 @@ 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) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 3387a5a7f..1f9529d03 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -445,6 +445,37 @@ 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}) From 026d7d34ffa5177275af958331fe192d75e2a07c Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 01:07:30 -0400 Subject: [PATCH 03/88] docs: describe embed_art_hash as the tag reader's fingerprint The field comment still said XXH3 of the picture bytes, which stopped being true when fingerprinting moved into go-taglib. --- model/mediafile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/model/mediafile.go b/model/mediafile.go index c0334fb43..f3140a3bb 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -50,7 +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:"-"` // XXH3 of the embedded picture bytes + 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"` From 48af781b82524fcb1dd9e4b1a0a3d35c7405428b Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 13:47:45 -0400 Subject: [PATCH 04/88] fix(reflex): exclude .worktrees from the reflex configuration regex --- reflex.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex.conf b/reflex.conf index 47dd775ab..1cbaa7bb7 100644 --- a/reflex.conf +++ b/reflex.conf @@ -1 +1 @@ --s -r "(\.go$$|\.cpp$$|\.h$$|navidrome.toml|resources|token_received.html)" -R "(^ui|^data|^db/migrations)" -R "_test\.go$$" -- go run -race -tags netgo,sqlite_fts5 . +-s -r "(\.go$$|\.cpp$$|\.h$$|navidrome.toml|resources|token_received.html)" -R "(^ui|^data|^db/migrations)" -R "_test\.go$$" -R "^\.worktrees" -- go run -race -tags netgo,sqlite_fts5 . From 404837799b9e7fc378625a1e8e1834df5c3533a3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 13:43:26 -0400 Subject: [PATCH 05/88] fix(subsonic): don't re-encode a source already in the player's forced format When a player has a forced transcoding format, ClientInfo.ForceFormat cleared DirectPlayProfiles unconditionally. A FLAC source on a player configured to transcode to FLAC was therefore re-encoded to FLAC, wasting CPU and bandwidth for no gain. Worse, the transcoder pipes ffmpeg output to stdout, so the resulting FLAC has total_samples=0 and no seek table -- an offline copy of it can never be seeked. Reported against getTranscodeDecision by the Symfonium author. ForceFormat now rebuilds DirectPlayProfiles from the matching transcoding profiles instead of dropping them: a client declaring a transcoding profile for a format is proof it can consume that format, so a source already in it is served as-is. Container and codec come from resolveTargetFormat, so a legacy "oga" target_format yields an ogg/opus profile, and the profile's MaxAudioChannels is carried across. DirectPlayProfile has no bitrate field, so restoring direct play needs a ceiling to keep an over-bitrate source out of it. GetTranscodeDecision now seeds that ceiling from the transcoding row's DefaultBitRate when a format was successfully forced, with the player's own MaxBitRate still taking precedence. This also closes a gap where the new endpoint ignored DefaultBitRate entirely: an mp3 320 source on a player forced to mp3@192 was served at 320, while the legacy /rest/stream path correctly gave 192. Applied via CapBitrate, which only ever lowers, so a client declaring a stricter limit keeps it. The legacy path (applyServerOverride) is untouched -- ForceFormat has no other callers. --- core/stream/decider_test.go | 76 +++++++++++++++++++++++++++++++ core/stream/types.go | 26 +++++++---- core/stream/types_test.go | 32 ++++++++++++- server/subsonic/transcode.go | 25 ++++++---- server/subsonic/transcode_test.go | 45 +++++++++++++++++- 5 files changed, 183 insertions(+), 21 deletions(-) diff --git a/core/stream/decider_test.go b/core/stream/decider_test.go index 577207636..01fef1249 100644 --- a/core/stream/decider_test.go +++ b/core/stream/decider_test.go @@ -1144,6 +1144,82 @@ var _ = Describe("Decider", func() { }) }) + Context("Player-forced format", func() { + symfonium := func() *ClientInfo { + return &ClientInfo{ + Name: "Symfonium", + DirectPlayProfiles: []DirectPlayProfile{ + {Containers: []string{"mp3", "flac", "ogg"}, Protocols: []string{ProtocolHTTP}}, + }, + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + } + + It("direct plays a flac source forced to flac", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1026, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + ci := symfonium() + Expect(ci.ForceFormat("flac")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeTrue()) + }) + + It("still transcodes a 24-bit flac when the client caps bit depth", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 4600, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) + ci := symfonium() + ci.CodecProfiles = []CodecProfile{{ + Type: CodecProfileTypeAudio, Name: "flac", + Limitations: []Limitation{{Name: LimitationAudioBitdepth, Comparison: ComparisonLessThanEqual, Values: []string{"16"}, Required: true}}, + }} + Expect(ci.ForceFormat("flac")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TranscodeStream.BitDepth).To(Equal(16)) + }) + + It("still transcodes a 320 mp3 forced to mp3 at a lower bitrate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + ci.CapBitrate(192) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TargetBitrate).To(Equal(192)) + }) + + It("direct plays a 128 mp3 forced to mp3 at a higher bitrate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 128, Channels: 2, SampleRate: 44100}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + ci.CapBitrate(192) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeTrue()) + }) + + It("transcodes a flac source forced to mp3", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1026, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + ci := symfonium() + Expect(ci.ForceFormat("mp3")).To(BeTrue()) + + decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.CanDirectPlay).To(BeFalse()) + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TargetFormat).To(Equal("mp3")) + }) + }) }) Describe("ensureProbed", func() { diff --git a/core/stream/types.go b/core/stream/types.go index 19474dd91..300017c01 100644 --- a/core/stream/types.go +++ b/core/stream/types.go @@ -59,28 +59,38 @@ func (ci *ClientInfo) CapBitrate(maxKbps int) bool { return changed } -// ForceFormat narrows the client to transcoding to targetFormat and suppresses -// direct play, but only if the client already declares a profile for that -// format. All matching profiles are kept so negotiation can still pick among -// them (e.g. by protocol). Returns false (no-op) when targetFormat is empty or -// unsupported. +// ForceFormat narrows the client to transcoding to targetFormat, but only if the +// client already declares a profile for it. All matching profiles are kept so +// negotiation can still pick among them (e.g. by protocol). Direct play is rebuilt +// from those profiles rather than dropped, since declaring a transcoding profile +// for a format is proof the client can play it. Returns false when unsupported. func (ci *ClientInfo) ForceFormat(targetFormat string) bool { if targetFormat == "" { return false } var matched []Profile + var directPlay []DirectPlayProfile for i := range ci.TranscodingProfiles { + p := &ci.TranscodingProfiles[i] // matchesContainer is alias-aware, so a forced "oga" (legacy Opus // target_format) still matches a resolved "opus" profile. - if _, format := resolveTargetFormat(&ci.TranscodingProfiles[i]); matchesContainer(format, []string{targetFormat}) { - matched = append(matched, ci.TranscodingProfiles[i]) + container, format := resolveTargetFormat(p) + if !matchesContainer(format, []string{targetFormat}) { + continue } + matched = append(matched, *p) + directPlay = append(directPlay, DirectPlayProfile{ + Containers: []string{container}, + AudioCodecs: []string{format}, + Protocols: []string{ProtocolHTTP}, + MaxAudioChannels: p.MaxAudioChannels, + }) } if len(matched) == 0 { return false } ci.TranscodingProfiles = matched - ci.DirectPlayProfiles = nil + ci.DirectPlayProfiles = directPlay return true } diff --git a/core/stream/types_test.go b/core/stream/types_test.go index eff408362..88ad904a7 100644 --- a/core/stream/types_test.go +++ b/core/stream/types_test.go @@ -58,7 +58,7 @@ var _ = Describe("ClientInfo", func() { }) Describe("ForceFormat", func() { - It("restricts to the forced format and clears direct play when supported", func() { + It("restricts direct play to the forced format when supported", func() { ci := &ClientInfo{ DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}, AudioCodecs: []string{"flac"}}}, TranscodingProfiles: []Profile{ @@ -71,7 +71,35 @@ var _ = Describe("ClientInfo", func() { Expect(ok).To(BeTrue()) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) - Expect(ci.DirectPlayProfiles).To(BeEmpty()) + Expect(ci.DirectPlayProfiles).To(ConsistOf(DirectPlayProfile{ + Containers: []string{"ogg"}, AudioCodecs: []string{"opus"}, Protocols: []string{ProtocolHTTP}, + })) + }) + + It("keeps direct play for a source already in the forced format", func() { + ci := &ClientInfo{ + DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}, AudioCodecs: []string{"flac"}}}, + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + ok := ci.ForceFormat("flac") + Expect(ok).To(BeTrue()) + Expect(ci.DirectPlayProfiles).To(ConsistOf(DirectPlayProfile{ + Containers: []string{"flac"}, AudioCodecs: []string{"flac"}, Protocols: []string{ProtocolHTTP}, + })) + }) + + It("carries the channel limit of the forced profile into direct play", func() { + ci := &ClientInfo{ + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP, MaxAudioChannels: 2}, + }, + } + Expect(ci.ForceFormat("flac")).To(BeTrue()) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + Expect(ci.DirectPlayProfiles[0].MaxAudioChannels).To(Equal(2)) }) It("matches a container-only forced format (mp3)", func() { diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index 9eb2af160..d64bce605 100644 --- a/server/subsonic/transcode.go +++ b/server/subsonic/transcode.go @@ -280,12 +280,19 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) return stream.IsAACCodec(p.Container) }) + player, hasPlayer := request.PlayerFrom(ctx) + // Honor the player's forced transcoding format, falling back to normal // negotiation when the client can't play it (issue #5583). + maxBitRate := 0 if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { - if !clientInfo.ForceFormat(trc.TargetFormat) { + if clientInfo.ForceFormat(trc.TargetFormat) { + // DirectPlayProfile carries no bitrate, so this ceiling is the only + // thing keeping an over-bitrate source out of direct play. + maxBitRate = trc.DefaultBitRate + } else { clientName := clientInfo.Name - if player, ok := request.PlayerFrom(ctx); ok && player.Client != "" { + if hasPlayer && player.Client != "" { clientName = player.Client } log.Debug(ctx, "Player forced format not supported by client; falling back to negotiation", @@ -293,13 +300,13 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) } } - // Apply the player's MaxBitRate as a ceiling on the client's declared - // limits (issue #5583). Both fields are capped because the client sends - // them independently here; capping only MaxAudioBitrate would let an - // independent MaxTranscodingAudioBitrate slip through computeBitrate. - if player, ok := request.PlayerFrom(ctx); ok && clientInfo.CapBitrate(player.MaxBitRate) { - log.Debug(ctx, "Applied player MaxBitRate cap to transcode decision", - "playerMaxBitRate", player.MaxBitRate, "client", clientInfo.Name) + // The player's own MaxBitRate outranks the forced-format default (issue #5583). + if hasPlayer && player.MaxBitRate > 0 { + maxBitRate = player.MaxBitRate + } + if clientInfo.CapBitrate(maxBitRate) { + log.Debug(ctx, "Applied bitrate ceiling to transcode decision", + "maxBitRate", maxBitRate, "client", clientInfo.Name) } // Get media file diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index 8d5cbb974..0f3c24832 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -369,7 +369,7 @@ var _ = Describe("Transcode endpoints", func() { mockTD.token = "token" }) - It("forces a supported format and clears direct play", func() { + It("forces a supported format and narrows direct play to it", func() { body := `{"directPlayProfiles":[{"containers":["flac"],"audioCodecs":["flac"],"protocols":["http"]}], "transcodingProfiles":[{"container":"ogg","audioCodec":"opus","protocol":"http"}, {"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` @@ -380,7 +380,11 @@ var _ = Describe("Transcode endpoints", func() { Expect(err).ToNot(HaveOccurred()) Expect(mockTD.capturedClient.TranscodingProfiles).To(HaveLen(1)) Expect(mockTD.capturedClient.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) - Expect(mockTD.capturedClient.DirectPlayProfiles).To(BeEmpty()) + Expect(mockTD.capturedClient.DirectPlayProfiles).To(ConsistOf(stream.DirectPlayProfile{ + Containers: []string{"ogg"}, + AudioCodecs: []string{"opus"}, + Protocols: []string{"http"}, + })) }) It("falls back to negotiation when the forced format is unsupported", func() { @@ -416,6 +420,43 @@ var _ = Describe("Transcode endpoints", func() { Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(128)) Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(128)) }) + + withForcedBitRate := func(r *http.Request, format string, defaultBitRate, playerMaxBitRate int) *http.Request { + ctx := request.WithTranscoding(r.Context(), model.Transcoding{TargetFormat: format, DefaultBitRate: defaultBitRate}) + ctx = request.WithPlayer(ctx, model.Player{Client: "NavidromeUI", MaxBitRate: playerMaxBitRate}) + return r.WithContext(ctx) + } + + It("applies the transcoding default bitrate when the player sets no maxBitRate", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "mp3", 192, 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(192)) + Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(192)) + }) + + It("prefers the player maxBitRate over the transcoding default bitrate", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "mp3", 192, 320) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(320)) + }) + + It("ignores the transcoding default bitrate when the forced format is unsupported", func() { + body := `{"transcodingProfiles":[{"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedBitRate(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "opus", 192, 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(BeZero()) + }) }) }) From 89026012ab3d9aeddcd7bbdf965fa9d6e0806b62 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 14:19:45 -0400 Subject: [PATCH 06/88] fix(transcoding): make piped FLAC transcodes seekable The FLAC muxer writes STREAMINFO before it knows the stream length, then rewinds at the end to fill total_samples in. Navidrome pipes ffmpeg's stdout (-f flac -), which is not seekable, so ffmpeg logs "unable to rewrite FLAC header" and the field stays 0. A decoder needs total_samples to turn a timestamp into a byte offset, so it reports an unknown duration and refuses to seek. Online playback hides this because the client re-requests with a new offset each time, but an offline copy is permanently unseekable, the symptom reported against Symfonium where seeking a downloaded track jumps back to the start. Transcode now wraps its own output and rewrites total_samples as the first bytes flow past. This lives in core/ffmpeg because the unseekable pipe is that package's doing: buildDynamicArgs is what appends the trailing '-'. core/stream only learns a target format and hands back an io.ReadCloser, so compensating there leaked a transcoder implementation detail one layer up. TranscodeOptions grows a Duration field alongside the existing Offset, which also puts the duration-minus-offset arithmetic in the same function that emits -ss. The wrapper runs on every transcode rather than only FLAC targets: the format on a transcoding row is a declared target that nothing validates against the command's actual -f, so a custom command can emit FLAC under any target_format. The magic-byte check inside the wrapper is the authoritative test and costs a 26-byte peek. The output sample rate is read back out of the header ffmpeg just wrote rather than taken from the transcode options, so a resampled (-ar) output still gets the right count. Anything that is not a FLAC stream with an unset total_samples passes through byte for byte. Measured on a 177s source: before, total_samples=0 and ffprobe reported duration N/A; after, total_samples=7807023 and duration 177.03s, with the audio payload byte-identical. This affects every piped FLAC regardless of the source format; only FLAC stores an authoritative "unknown", which is why mp3, opus and aac survive the same pipe. No SEEKTABLE is synthesised and the MD5 is left zero: both are optional, and decoders binary-search using total_samples alone. --- core/ffmpeg/ffmpeg.go | 17 ++-- core/ffmpeg/ffmpeg_test.go | 35 +++++++ core/ffmpeg/flac_streaminfo.go | 66 +++++++++++++ core/ffmpeg/flac_streaminfo_test.go | 142 ++++++++++++++++++++++++++++ core/stream/media_streamer.go | 1 + 5 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 core/ffmpeg/flac_streaminfo.go create mode 100644 core/ffmpeg/flac_streaminfo_test.go diff --git a/core/ffmpeg/ffmpeg.go b/core/ffmpeg/ffmpeg.go index af2dab647..cc38dd9de 100644 --- a/core/ffmpeg/ffmpeg.go +++ b/core/ffmpeg/ffmpeg.go @@ -27,11 +27,12 @@ type TranscodeOptions struct { Command string // DB command template (used to detect custom vs default) Format string // Target format (mp3, opus, aac, flac) FilePath string - BitRate int // kbps, 0 = codec default - SampleRate int // 0 = no constraint - Channels int // 0 = no constraint - BitDepth int // 0 = no constraint; valid values: 16, 24, 32 - Offset int // seconds + BitRate int // kbps, 0 = codec default + SampleRate int // 0 = no constraint + Channels int // 0 = no constraint + BitDepth int // 0 = no constraint; valid values: 16, 24, 32 + Offset int // seconds + Duration float32 // seconds; 0 = unknown. Only used to repair a piped FLAC header. } // AudioProbeResult contains authoritative audio stream properties from ffprobe. @@ -86,7 +87,11 @@ func (e *ffmpeg) Transcode(ctx context.Context, opts TranscodeOptions) (io.ReadC } else { args = buildTemplateArgs(opts) } - return e.start(ctx, args) + out, err := e.start(ctx, args) + if err != nil { + return nil, err + } + return patchFLACDuration(out, opts.Duration-float32(opts.Offset)), nil } func (e *ffmpeg) ConvertAnimatedImage(ctx context.Context, reader io.Reader, maxSize int, quality int) (io.ReadCloser, error) { diff --git a/core/ffmpeg/ffmpeg_test.go b/core/ffmpeg/ffmpeg_test.go index 0fa3de111..dbc8fa3c8 100644 --- a/core/ffmpeg/ffmpeg_test.go +++ b/core/ffmpeg/ffmpeg_test.go @@ -3,6 +3,7 @@ package ffmpeg import ( "context" "errors" + "io" "os" "os/exec" "path/filepath" @@ -684,6 +685,40 @@ var _ = Describe("ffmpeg", func() { }) Expect(err).To(MatchError(context.Canceled)) }) + + It("fills in total_samples on a piped FLAC transcode", func() { + stream, err := ff.Transcode(GinkgoT().Context(), TranscodeOptions{ + Command: "ffmpeg -i %s -map 0:a:0 -v 0 -c:a flac -f flac -", + Format: "flac", + FilePath: "tests/fixtures/test.flac", + Duration: 1, // the fixture is exactly 1s at 44100Hz + }) + Expect(err).ToNot(HaveOccurred()) + defer stream.Close() + + out, err := io.ReadAll(stream) + Expect(err).ToNot(HaveOccurred()) + Expect(string(out[:4])).To(Equal("fLaC")) + Expect(readTotalSamples(out)).To(Equal(uint64(44100))) + }) + + It("patches the duration net of the requested offset", func() { + // The command has no %t, so ffmpeg still emits the whole fixture. + // What is under test is the header arithmetic, not the audio. + stream, err := ff.Transcode(GinkgoT().Context(), TranscodeOptions{ + Command: "ffmpeg -i %s -map 0:a:0 -v 0 -c:a flac -f flac -", + Format: "flac", + FilePath: "tests/fixtures/test.flac", + Duration: 3, + Offset: 1, + }) + Expect(err).ToNot(HaveOccurred()) + defer stream.Close() + + out, err := io.ReadAll(stream) + Expect(err).ToNot(HaveOccurred()) + Expect(readTotalSamples(out)).To(Equal(uint64(2 * 44100))) + }) }) Context("stderr capture", func() { diff --git a/core/ffmpeg/flac_streaminfo.go b/core/ffmpeg/flac_streaminfo.go new file mode 100644 index 000000000..878c28718 --- /dev/null +++ b/core/ffmpeg/flac_streaminfo.go @@ -0,0 +1,66 @@ +package ffmpeg + +import ( + "bytes" + "encoding/binary" + "errors" + "io" + "math" +) + +const ( + flacPrefixLen = 26 // through the last total_samples byte + flacMaxTotalSamples = 1<<36 - 1 +) + +// patchFLACDuration fills in the STREAMINFO total_samples that ffmpeg leaves at 0 +// when writing to a pipe, since a decoder cannot seek a cached FLAC without it. +func patchFLACDuration(r io.ReadCloser, duration float32) io.ReadCloser { + if duration <= 0 { + return r + } + return &flacPatcher{ReadCloser: r, duration: duration} +} + +type flacPatcher struct { + io.ReadCloser + duration float32 + // Peeking here rather than in the constructor keeps Transcode from blocking + // until ffmpeg has emitted its first bytes. + stream io.Reader +} + +func (f *flacPatcher) Read(p []byte) (int, error) { + if f.stream == nil { + prefix := make([]byte, flacPrefixLen) + n, err := io.ReadFull(f.ReadCloser, prefix) + if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { + return 0, err + } + prefix = prefix[:n] + if err == nil { + setFLACTotalSamples(prefix, f.duration) + } + f.stream = io.MultiReader(bytes.NewReader(prefix), f.ReadCloser) + } + return f.stream.Read(p) +} + +// setFLACTotalSamples takes the rate from the header rather than the transcode +// options, so a resampled (-ar) output still gets the right count. +func setFLACTotalSamples(prefix []byte, duration float32) { + if string(prefix[:4]) != "fLaC" || prefix[4]&0x7F != 0 { + return + } + // 20-bit rate | 3-bit channels | 5-bit depth | 36-bit total_samples + info := binary.BigEndian.Uint64(prefix[18:]) + rate := info >> 44 + if rate == 0 || info&flacMaxTotalSamples != 0 { + return + } + total := math.Round(float64(duration) * float64(rate)) + if total > flacMaxTotalSamples { + return + } + binary.BigEndian.PutUint64(prefix[18:], info|uint64(total)) +} diff --git a/core/ffmpeg/flac_streaminfo_test.go b/core/ffmpeg/flac_streaminfo_test.go new file mode 100644 index 000000000..6bf3503d7 --- /dev/null +++ b/core/ffmpeg/flac_streaminfo_test.go @@ -0,0 +1,142 @@ +package ffmpeg + +import ( + "bytes" + "errors" + "io" + "os" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Decoded independently so the specs do not mirror the production bit-twiddling. +func readSampleRate(b []byte) int { + return int(b[18])<<12 | int(b[19])<<4 | int(b[20])>>4 +} + +func readTotalSamples(b []byte) uint64 { + return uint64(b[21]&0x0F)<<32 | uint64(b[22])<<24 | uint64(b[23])<<16 | uint64(b[24])<<8 | uint64(b[25]) +} + +var _ = Describe("patchFLACDuration", func() { + var fileFLAC []byte + + // Zeroing total_samples reproduces what a piped transcode emits. + pipedFLAC := func() []byte { + b := bytes.Clone(fileFLAC) + b[21] &= 0xF0 + clear(b[22:26]) + return b + } + + readAll := func(in []byte, duration float32) []byte { + out, err := io.ReadAll(patchFLACDuration(io.NopCloser(bytes.NewReader(in)), duration)) + Expect(err).ToNot(HaveOccurred()) + return out + } + + BeforeEach(func() { + var err error + fileFLAC, err = os.ReadFile("tests/fixtures/test.flac") + Expect(err).ToNot(HaveOccurred()) + Expect(readSampleRate(fileFLAC)).To(Equal(44100)) // specs below hard-code this rate + }) + + It("fills in total_samples from the duration", func() { + out := readAll(pipedFLAC(), 1.0) + Expect(readTotalSamples(out)).To(Equal(uint64(44100))) + }) + + It("takes the sample rate from the header, not from the source file", func() { + in := pipedFLAC() + // Rewrite the header's rate to 48000, as -ar would. + in[18], in[19] = 0x0B, 0xB8 + in[20] &= 0x0F + + out := readAll(in, 2.0) + + Expect(readSampleRate(out)).To(Equal(48000)) + Expect(readTotalSamples(out)).To(Equal(uint64(96000))) + }) + + It("rounds to the nearest sample rather than truncating", func() { + // float32(0.7)*44100 is 30869.9995, so truncation would lose a sample. + out := readAll(pipedFLAC(), 0.7) + Expect(readTotalSamples(out)).To(Equal(uint64(30870))) + }) + + It("passes through when the duration overflows the 36-bit field", func() { + in := pipedFLAC() + Expect(readAll(in, 2e6)).To(Equal(in)) + }) + + It("leaves everything after the header untouched", func() { + in := pipedFLAC() + out := readAll(in, 1.0) + Expect(out).To(HaveLen(len(in))) + Expect(out[26:]).To(Equal(in[26:])) + Expect(out[:18]).To(Equal(in[:18])) + }) + + It("leaves an already-populated total_samples alone", func() { + out := readAll(fileFLAC, 99.0) + Expect(out).To(Equal(fileFLAC)) + }) + + It("passes through a stream that is not FLAC", func() { + in := []byte("ID3\x04\x00\x00\x00\x00\x00\x00 not a flac stream at all, just bytes") + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through when the first metadata block is not STREAMINFO", func() { + in := pipedFLAC() + in[4] = 0x04 // VORBIS_COMMENT + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through a stream shorter than the STREAMINFO fields it patches", func() { + in := pipedFLAC()[:20] + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("passes through an empty stream", func() { + Expect(readAll(nil, 1.0)).To(BeEmpty()) + }) + + It("passes through when the duration is zero or negative", func() { + in := pipedFLAC() + Expect(readAll(in, 0)).To(Equal(in)) + Expect(readAll(in, -5)).To(Equal(in)) + }) + + It("passes through when the header declares no sample rate", func() { + in := pipedFLAC() + in[18], in[19] = 0, 0 + in[20] &= 0x0F + Expect(readAll(in, 1.0)).To(Equal(in)) + }) + + It("propagates a read error from the underlying stream", func() { + _, err := io.ReadAll(patchFLACDuration(io.NopCloser(io.MultiReader( + bytes.NewReader(pipedFLAC()[:10]), &errReader{})), 1.0)) + Expect(err).To(MatchError("boom")) + }) + + It("closes the underlying stream", func() { + c := &closeSpy{Reader: bytes.NewReader(pipedFLAC())} + Expect(patchFLACDuration(c, 1.0).Close()).To(Succeed()) + Expect(c.closed).To(BeTrue()) + }) +}) + +type errReader struct{} + +func (e *errReader) Read([]byte) (int, error) { return 0, errors.New("boom") } + +type closeSpy struct { + io.Reader + closed bool +} + +func (c *closeSpy) Close() error { c.closed = true; return nil } diff --git a/core/stream/media_streamer.go b/core/stream/media_streamer.go index aaa3126b4..6db2f6338 100644 --- a/core/stream/media_streamer.go +++ b/core/stream/media_streamer.go @@ -268,6 +268,7 @@ func NewTranscodingCache() TranscodingCache { BitDepth: job.bitDepth, Channels: job.channels, Offset: job.offset, + Duration: job.mf.Duration, }) if err != nil { release() From bea9715001abc956c712f76bf87cd93fd41bd6f1 Mon Sep 17 00:00:00 2001 From: Deluan Date: Tue, 8 Sep 2026 18:51:49 -0400 Subject: [PATCH 07/88] refactor(ui): replace icons in LibraryScanButton with react-icons --- ui/src/library/LibraryScanButton.jsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/src/library/LibraryScanButton.jsx b/ui/src/library/LibraryScanButton.jsx index 50d90e615..b793844fd 100644 --- a/ui/src/library/LibraryScanButton.jsx +++ b/ui/src/library/LibraryScanButton.jsx @@ -8,8 +8,8 @@ import { useUnselectAll, } from 'react-admin' import { useSelector } from 'react-redux' -import SyncIcon from '@material-ui/icons/Sync' -import CachedIcon from '@material-ui/icons/Cached' +import { GiMagnifyingGlass } from 'react-icons/gi' +import { VscSync } from 'react-icons/vsc' import subsonic from '../subsonic' const LibraryScanButton = ({ fullScan, selectedIds, className }) => { @@ -54,7 +54,7 @@ const LibraryScanButton = ({ fullScan, selectedIds, className }) => { ? translate('resources.library.actions.fullScan') : translate('resources.library.actions.quickScan') - const icon = fullScan ? : + const icon = fullScan ? : return (