diff --git a/Dockerfile b/Dockerfile index ad1e2a41c..e8a00f470 100644 --- a/Dockerfile +++ b/Dockerfile @@ -69,8 +69,16 @@ RUN --mount=type=bind,source=. \ set -e xx-go --wrap export CGO_ENABLED=1 + # Native libwebp (gen2brain/webp) uses ebitengine/purego reverse callbacks, + # which purego does not support on 32-bit ARM or x86 and crash with a SIGSEGV + # (issue #5597). Build those arches with the "nodynamic" tag so gen2brain/webp + # is WASM-only and never links the purego path. 64-bit arches keep native libwebp. + BUILD_TAGS=netgo,sqlite_fts5 + if [ "$(xx-info arch)" = "arm" ] || [ "$(xx-info arch)" = "386" ]; then + BUILD_TAGS=${BUILD_TAGS},nodynamic + fi # -latomic is required on 32-bit arm (arm/v6, arm/v7) so SQLite's 64-bit atomics resolve. - go build -tags=netgo,sqlite_fts5 -ldflags="-w -s \ + go build -tags=${BUILD_TAGS} -ldflags="-w -s \ -linkmode=external -extldflags '-latomic' \ -X github.com/navidrome/navidrome/consts.gitSha=${GIT_SHA} \ -X github.com/navidrome/navidrome/consts.gitTag=${GIT_TAG}" \ @@ -159,7 +167,6 @@ ENV ND_MUSICFOLDER=/music ENV ND_DATAFOLDER=/data ENV ND_CONFIGFILE=/data/navidrome.toml ENV ND_PORT=4533 -ENV ND_ENABLEWEBPENCODING=true RUN touch /.nddockerenv EXPOSE ${ND_PORT} diff --git a/consts/consts.go b/consts/consts.go index edd8f2b54..4baf4610d 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -14,6 +14,9 @@ const ( DefaultDbPath = "navidrome.db?cache=shared&_busy_timeout=15000&_journal_mode=WAL&_foreign_keys=on&synchronous=normal" InitialSetupFlagKey = "InitialSetup" FullScanAfterMigrationFlagKey = "FullScanAfterMigration" + // PlaylistsImportPendingFlagKey marks that playlist import was deferred because + // no admin user existed yet; the next scan with an admin imports them. + PlaylistsImportPendingFlagKey = "PlaylistsImportPending" LastScanErrorKey = "LastScanError" LastScanTypeKey = "LastScanType" LastScanStartTimeKey = "LastScanStartTime" diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index e765e1b1b..5e61684cc 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -357,6 +357,98 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // Regression introduced in v0.62.0 (#5451 + #5457): the parent-folder + // fallback can pick up images from the ARTIST folder, serving the artist + // thumbnail as album art for any album without its own image files. + When("an album has no images and the artist folder has folder.jpg", func() { + // Artist/ + // ├── folder.jpg ← artist thumbnail, must NOT become album art + // ├── Album A/ + // │ └── 01 - Track.mp3 (no images) + // └── Album B/ + // ├── 01 - Track.mp3 + // └── cover.jpg + It("does not use the artist image as album art", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/folder.jpg": imageFile("artist-thumbnail"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + "Artist/Album B/cover.jpg": imageFile("album-b"), + }) + scan() + + alA := albumByName("Album A") + _, err := readArtworkOrErr(alA.CoverArtID()) + Expect(err).To(HaveOccurred(), + "Album A has no images of its own, so it must fall through to the placeholder "+ + "instead of inheriting the artist folder's folder.jpg") + + alB := albumByName("Album B") + Expect(readArtwork(alB.CoverArtID())).To(Equal(imageBytes("album-b"))) + }) + }) + + When("a single-disc album is spread across sibling folders under the artist folder", func() { + // Artist/ + // ├── folder.jpg ← artist thumbnail, must NOT become album art + // ├── Album A/ + // │ └── 01 - Track.mp3 (album: "Album A") + // ├── Album A bonus/ + // │ └── 02 - Track.mp3 (album: "Album A" — same album, second folder) + // └── Album B/ + // ├── 01 - Track.mp3 + // └── cover.jpg + It("does not use the artist image as album art for the spread album", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/folder.jpg": imageFile("artist-thumbnail"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + "Artist/Album B/cover.jpg": imageFile("album-b"), + }) + scan() + + alA := albumByName("Album A") + Expect(alA.FolderIDs).To(HaveLen(2), + "sanity check: scanner should treat the two sibling folders as one spread album") + _, err := readArtworkOrErr(alA.CoverArtID()) + Expect(err).To(HaveOccurred(), + "the spread album has no images of its own, so it must fall through to the "+ + "placeholder instead of inheriting the artist folder's folder.jpg") + }) + }) + + When("a spread album has its own front.jpg but the artist folder has cover.jpg", func() { + // Artist/ + // ├── cover.jpg ← artist image; matches cover.* (first pattern), + // │ must NOT shadow the album's own front.jpg + // ├── Album A/ + // │ ├── 01 - Track.mp3 (album: "Album A") + // │ └── front.jpg ← should win + // ├── Album A bonus/ + // │ └── 02 - Track.mp3 (album: "Album A") + // └── Album B/ + // └── 01 - Track.mp3 + It("prefers the album's own art over the artist image", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Artist/cover.jpg": imageFile("artist-image"), + "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album A/front.jpg": imageFile("album-a-front"), + "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), + "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), + }) + scan() + + alA := albumByName("Album A") + Expect(alA.FolderIDs).To(HaveLen(2), + "sanity check: scanner should treat the two sibling folders as one spread album") + Expect(readArtwork(alA.CoverArtID())).To(Equal(imageBytes("album-a-front"))) + }) + }) + When("embedded is first in CoverArtPriority but the track has no embedded art", func() { // Artist/ // └── Album/ diff --git a/core/artwork/e2e/suite_test.go b/core/artwork/e2e/suite_test.go index 733e2e98c..06cc05b6f 100644 --- a/core/artwork/e2e/suite_test.go +++ b/core/artwork/e2e/suite_test.go @@ -2,6 +2,7 @@ package artworke2e_test import ( "context" + "fmt" "path/filepath" "testing" @@ -104,3 +105,16 @@ func firstAlbum() model.Album { Expect(albums).To(HaveLen(1), "expected exactly one album, got %d", len(albums)) return albums[0] } + +func albumByName(name string) model.Album { + GinkgoHelper() + albums, err := ds.Album(ctx).GetAll(model.QueryOptions{}) + Expect(err).ToNot(HaveOccurred()) + for _, al := range albums { + if al.Name == name { + return al + } + } + Fail(fmt.Sprintf("album %q not found among %d albums", name, len(albums))) + return model.Album{} +} diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 73ba9b5ee..8ad07773b 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -113,28 +113,12 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return nil, nil, nil, err } - folderIDSet := make(map[string]bool, len(folderIDs)) - for _, id := range folderIDs { - folderIDSet[id] = true + parent, err := albumRootParent(ctx, ds, folders, folderIDs) + if err != nil { + return nil, nil, nil, err } - - // Check if all folders share a common parent that is not already included. - // This finds cover art in the album root folder (e.g., "Artist/Album/cover.jpg" - // when tracks are in disc subfolders like "Artist/Album/CD1/" and "Artist/Album/CD2/"). - // For single-folder albums, the parent is only included when the folder has no - // images of its own (indicating a disc subfolder needing parent artwork). - if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" { - if len(folders) >= 2 || !anyFolderHasImages(folders) { - parentFolder, err := ds.Folder(ctx).Get(commonParentID) - if errors.Is(err, model.ErrNotFound) { - log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) - } else if err != nil { - return nil, nil, nil, err - } - if parentFolder != nil && parentFolder.ParentID != "" { - folders = append(folders, *parentFolder) - } - } + if parent != nil { + folders = append(folders, *parent) } var paths []string @@ -159,6 +143,50 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return paths, imgFiles, &updatedAt, nil } +// albumRootParent returns the common parent of the album's folders when it +// qualifies as the album's root folder (e.g. "Artist/Album" above disc +// subfolders), or nil when there is no such parent. This finds cover art in +// the album root folder when tracks live in disc subfolders, like +// "Artist/Album/cover.jpg" with tracks in "Artist/Album/CD1/" and +// "Artist/Album/CD2/". The parent must look like an album root, not an +// artist-level folder — it qualifies only when it holds no audio belonging to +// other albums — so artist images are never served as album art. +func albumRootParent(ctx context.Context, ds model.DataStore, folders []model.Folder, folderIDs []string) (*model.Folder, error) { + folderIDSet := make(map[string]bool, len(folderIDs)) + for _, id := range folderIDs { + folderIDSet[id] = true + } + commonParentID := commonParentFolder(folders, folderIDSet) + if commonParentID == "" { + return nil, nil + } + // Single-folder albums only use the parent when the folder has no images + // of its own (indicating a disc subfolder needing parent artwork). + if len(folders) < 2 && anyFolderHasImages(folders) { + return nil, nil + } + parent, err := ds.Folder(ctx).Get(commonParentID) + if errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) + return nil, nil + } + if err != nil { + return nil, err + } + if parent.ParentID == "" { + // The library root can never be an album root + return nil, nil + } + hasOtherAudio, err := ds.Folder(ctx).HasAudioOutsideFolders(*parent, folderIDs) + if err != nil { + return nil, err + } + if hasOtherAudio { + return nil, nil + } + return parent, nil +} + func anyFolderHasImages(folders []model.Folder) bool { for _, f := range folders { if len(f.ImageFiles) > 0 { diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index 1cf039bee..fe4a1a545 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -339,6 +339,61 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("does not include parent images when other albums' audio lives under the parent", func() { + // Simulates: Artist/folder.jpg with Artist/Album (no images) and + // another album's tracks elsewhere under the artist folder + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "artistFolder", + Path: ".", + Name: "Artist", + ParentID: "libraryRoot", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"folder.jpg"}, + } + repo.hasOtherAudio = true + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(BeEmpty()) + }) + + It("propagates errors from the album-root check", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + repo.otherAudioErr = errors.New("db connection failed") + + _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).To(MatchError("db connection failed")) + }) + It("propagates non-ErrNotFound errors from parent folder lookup", func() { repo.result = []model.Folder{ { diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 50ca3a2ce..6d6d58fc5 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -702,12 +702,20 @@ type fakeFolderRepo struct { getErr error getCallCount int err error + // hasOtherAudio is returned by HasAudioOutsideFolders (the album-root + // check). False means the parent qualifies as an album root. + hasOtherAudio bool + otherAudioErr error } func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { return f.result, f.err } +func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) { + return f.hasOtherAudio, f.otherAudioErr +} + func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { f.getCallCount++ if f.getErr != nil { diff --git a/core/artwork/reader_resized.go b/core/artwork/reader_resized.go index 85a19a4c3..08f42f130 100644 --- a/core/artwork/reader_resized.go +++ b/core/artwork/reader_resized.go @@ -21,6 +21,12 @@ import ( func init() { conf.AddHook(func() { + // gen2brain/webp selects native (purego/libwebp) vs WASM in its own + // package init() and exposes the result only via webp.Dynamic(); there is + // no runtime way to switch back. On 32-bit ARM/x86 the purego callback path + // crashes (issue #5597), so those builds must be compiled with the + // "nodynamic" tag (see Dockerfile), which makes webp.Dynamic() report an + // error here and forces the safe WASM path. if err := webp.Dynamic(); err != nil { log.Debug("Using WASM WebP encoder/decoder", "reason", err) } else { diff --git a/core/stream/decider.go b/core/stream/decider.go index d6e48497c..7940c6862 100644 --- a/core/stream/decider.go +++ b/core/stream/decider.go @@ -12,6 +12,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/utils/gg" ) const fallbackBitrate = 256 // kbps @@ -142,7 +143,7 @@ func buildSourceStream(mf *model.MediaFile, probe *ffmpeg.AudioProbeResult) Deta sd.Codec = mf.AudioCodec() sd.Bitrate = mf.BitRate sd.SampleRate = mf.SampleRate - sd.BitDepth = mf.BitDepth + sd.BitDepth = gg.V(mf.BitDepth) sd.Channels = mf.Channels } sd.IsLossless = isLosslessFormat(sd.Codec) diff --git a/core/stream/decider_test.go b/core/stream/decider_test.go index f74953258..03c4ea437 100644 --- a/core/stream/decider_test.go +++ b/core/stream/decider_test.go @@ -12,6 +12,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" + "github.com/navidrome/navidrome/utils/gg" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -23,7 +24,7 @@ func withProbe(mf *model.MediaFile) *model.MediaFile { Codec: mf.AudioCodec(), BitRate: mf.BitRate, SampleRate: mf.SampleRate, - BitDepth: mf.BitDepth, + BitDepth: gg.V(mf.BitDepth), Channels: mf.Channels, } data, _ := json.Marshal(probe) @@ -243,7 +244,7 @@ var _ = Describe("Decider", func() { Context("Transcoding", func() { It("selects transcoding when direct play isn't possible", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 256, // kbps DirectPlayProfiles: []DirectPlayProfile{ @@ -278,7 +279,7 @@ var _ = Describe("Decider", func() { }) It("uses default bitrate when client doesn't specify", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, BitDepth: new(16)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "mp3", Protocol: ProtocolHTTP}, @@ -331,7 +332,7 @@ var _ = Describe("Decider", func() { }) It("selects first valid transcoding profile in order", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 48000, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 48000, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, DirectPlayProfiles: []DirectPlayProfile{ @@ -351,7 +352,7 @@ var _ = Describe("Decider", func() { Context("Lossless to lossless transcoding", func() { It("allows lossless to lossless when samplerate needs downsampling", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 176400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 176400, BitDepth: new(1)}) ci := &ClientInfo{ MaxAudioBitrate: 1000, DirectPlayProfiles: []DirectPlayProfile{ @@ -369,7 +370,7 @@ var _ = Describe("Decider", func() { It("sets IsLossless=true on transcoded stream when target is lossless", func() { // Transcoding to mp3 (lossy) should result in IsLossless=false. - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -526,7 +527,7 @@ var _ = Describe("Decider", func() { }) It("rejects direct play due to samplerate limitation", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ DirectPlayProfiles: []DirectPlayProfile{ {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, @@ -573,7 +574,7 @@ var _ = Describe("Decider", func() { }) It("applies channel limitation to transcoded stream", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 48000, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 48000, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -596,7 +597,7 @@ var _ = Describe("Decider", func() { }) It("applies samplerate limitation to transcoded stream", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -619,7 +620,7 @@ var _ = Describe("Decider", func() { }) It("applies bitdepth limitation to transcoded stream", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, @@ -642,7 +643,7 @@ var _ = Describe("Decider", func() { }) It("preserves source bit depth when no limitation applies", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(24)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, @@ -656,7 +657,7 @@ var _ = Describe("Decider", func() { }) It("rejects transcoding profile when GreaterThanEqual cannot be satisfied", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -680,7 +681,7 @@ var _ = Describe("Decider", func() { Context("DSD sample rate conversion", func() { It("converts DSD sample rate to PCM-equivalent in decision", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: new(1)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -700,7 +701,7 @@ var _ = Describe("Decider", func() { }) It("converts DSD sample rate for FLAC target without codec limit", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: new(1)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, @@ -719,7 +720,7 @@ var _ = Describe("Decider", func() { }) It("applies codec profile limit to DSD-converted FLAC sample rate", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: new(1)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, @@ -746,7 +747,7 @@ var _ = Describe("Decider", func() { }) It("applies audioBitdepth limitation to DSD-converted bit depth", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: new(1)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, @@ -775,7 +776,7 @@ var _ = Describe("Decider", func() { // Regression test for #5336: ffmpeg's mp3 encoder rejects >2 channels. // The decider must clamp to the codec's hard limit even when no // transcoding profile MaxAudioChannels is configured. - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -791,7 +792,7 @@ var _ = Describe("Decider", func() { }) It("honors a stricter profile MaxAudioChannels over the codec clamp", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -806,7 +807,7 @@ var _ = Describe("Decider", func() { }) It("applies the codec clamp when the profile limit is looser", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -821,7 +822,7 @@ var _ = Describe("Decider", func() { }) It("passes channels through unchanged for codecs with no hard limit", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 6, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -840,7 +841,7 @@ var _ = Describe("Decider", func() { Context("Probe-based lossless detection", func() { It("uses probe codec name for lossless detection", func() { // WavPack files: ffprobe reports codec as "wavpack", suffix is ".wv" - mf := &model.MediaFile{ID: "1", Suffix: "wv", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16} + mf := &model.MediaFile{ID: "1", Suffix: "wv", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)} probe := ffmpeg.AudioProbeResult{ Codec: "wavpack", BitRate: 1000, SampleRate: 44100, BitDepth: 16, Channels: 2, } @@ -884,7 +885,7 @@ var _ = Describe("Decider", func() { Context("Opus fixed sample rate", func() { It("sets Opus output to 48000Hz regardless of input", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 128, TranscodingProfiles: []Profile{ @@ -901,7 +902,7 @@ var _ = Describe("Decider", func() { }) It("sets Opus output to 48000Hz even for 96kHz input", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1500, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1500, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 128, TranscodingProfiles: []Profile{ @@ -917,7 +918,7 @@ var _ = Describe("Decider", func() { Context("Container vs format separation", func() { It("preserves mp4 container when falling back to aac format", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 256, TranscodingProfiles: []Profile{ @@ -935,7 +936,7 @@ var _ = Describe("Decider", func() { }) It("uses container as format when container matches transcoding config", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 256, TranscodingProfiles: []Profile{ @@ -952,7 +953,7 @@ var _ = Describe("Decider", func() { Context("MP3 max sample rate", func() { It("caps sample rate at 48000 for MP3", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1500, Channels: 2, SampleRate: 96000, BitDepth: 24}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1500, Channels: 2, SampleRate: 96000, BitDepth: new(24)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -966,7 +967,7 @@ var _ = Describe("Decider", func() { }) It("preserves sample rate at 44100 for MP3", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -982,7 +983,7 @@ var _ = Describe("Decider", func() { Context("AAC max sample rate", func() { It("caps sample rate at 96000 for AAC", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: 1}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "dsf", Codec: "DSD", BitRate: 5644, Channels: 2, SampleRate: 2822400, BitDepth: new(1)}) ci := &ClientInfo{ MaxTranscodingAudioBitrate: 320, TranscodingProfiles: []Profile{ @@ -1025,7 +1026,7 @@ var _ = Describe("Decider", func() { Context("Source stream details", func() { It("populates source stream correctly with kbps bitrate", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24, Duration: 300.5, Size: 50000000}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24), Duration: 300.5, Size: 50000000}) ci := &ClientInfo{ DirectPlayProfiles: []DirectPlayProfile{ {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, @@ -1058,7 +1059,7 @@ var _ = Describe("Decider", func() { }) It("ignores player MaxBitRate in context", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ Name: "TestClient", DirectPlayProfiles: []DirectPlayProfile{ @@ -1074,7 +1075,7 @@ var _ = Describe("Decider", func() { Context("Format-aware default bitrate", func() { It("uses opus default bitrate from DB", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 48000, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 48000, BitDepth: new(16)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "opus", AudioCodec: "opus", Protocol: ProtocolHTTP}, @@ -1087,7 +1088,7 @@ var _ = Describe("Decider", func() { }) It("uses aac default bitrate from DB", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) ci := &ClientInfo{ TranscodingProfiles: []Profile{ {Container: "aac", AudioCodec: "aac", Protocol: ProtocolHTTP}, diff --git a/core/stream/legacy_client.go b/core/stream/legacy_client.go index 9dd6179a0..652e42eba 100644 --- a/core/stream/legacy_client.go +++ b/core/stream/legacy_client.go @@ -12,7 +12,7 @@ import ( // buildLegacyClientInfo translates legacy Subsonic stream/download parameters // into a ClientInfo for use with MakeDecision. -func buildLegacyClientInfo(mf *model.MediaFile, reqFormat string, reqBitRate int) *ClientInfo { +func buildLegacyClientInfo(mf *model.MediaFile, reqFormat string, reqBitRate int, playerMaxBitRate int) *ClientInfo { ci := &ClientInfo{Name: "legacy"} // Determine target format for transcoding @@ -22,6 +22,10 @@ func buildLegacyClientInfo(mf *model.MediaFile, reqFormat string, reqBitRate int targetFormat = reqFormat case reqBitRate > 0 && reqBitRate < mf.BitRate && conf.Server.DefaultDownsamplingFormat != "": targetFormat = conf.Server.DefaultDownsamplingFormat + case playerMaxBitRate > 0 && playerMaxBitRate < mf.BitRate && conf.Server.DefaultDownsamplingFormat != "": + // Server-side player MaxBitRate alone forces downsampling, even when the + // client sent no format/bitrate params (issue #5583, legacy /stream path). + targetFormat = conf.Server.DefaultDownsamplingFormat } if targetFormat != "" { @@ -63,15 +67,19 @@ func (s *deciderService) ResolveRequest(ctx context.Context, mf *model.MediaFile return req } - clientInfo := buildLegacyClientInfo(mf, reqFormat, reqBitRate) + playerMaxBitRate := 0 + if player, ok := request.PlayerFrom(ctx); ok { + playerMaxBitRate = player.MaxBitRate + } + + clientInfo := buildLegacyClientInfo(mf, reqFormat, reqBitRate, playerMaxBitRate) // Apply server-side player transcoding override before making the decision if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { clientInfo = applyServerOverride(ctx, clientInfo, &trc) - } else if player, ok := request.PlayerFrom(ctx); ok && player.MaxBitRate > 0 { - if clientInfo.MaxAudioBitrate == 0 || player.MaxBitRate < clientInfo.MaxAudioBitrate { - modified := *clientInfo - modified.MaxAudioBitrate = player.MaxBitRate + } else if player, ok := request.PlayerFrom(ctx); ok { + modified := *clientInfo + if modified.CapBitrate(player.MaxBitRate) { clientInfo = &modified log.Debug(ctx, "Applied player MaxBitRate cap", "playerMaxBitRate", player.MaxBitRate, "client", clientInfo.Name) } diff --git a/core/stream/legacy_client_test.go b/core/stream/legacy_client_test.go index ce7b38650..bc8405976 100644 --- a/core/stream/legacy_client_test.go +++ b/core/stream/legacy_client_test.go @@ -21,7 +21,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("sets transcoding profile for explicit format without bitrate", func() { - ci := buildLegacyClientInfo(mf, "mp3", 0) + ci := buildLegacyClientInfo(mf, "mp3", 0, 0) Expect(ci.Name).To(Equal("legacy")) Expect(ci.TranscodingProfiles).To(HaveLen(1)) @@ -34,7 +34,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("does not add direct play profile when explicit format differs from source (no bitrate)", func() { - ci := buildLegacyClientInfo(mf, "opus", 0) + ci := buildLegacyClientInfo(mf, "opus", 0, 0) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].Container).To(Equal("opus")) @@ -42,7 +42,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("adds direct play profile when explicit format matches source format", func() { - ci := buildLegacyClientInfo(mf, "flac", 0) + ci := buildLegacyClientInfo(mf, "flac", 0, 0) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].Container).To(Equal("flac")) @@ -52,7 +52,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("sets transcoding profile and bitrate for explicit format with bitrate", func() { - ci := buildLegacyClientInfo(mf, "mp3", 192) + ci := buildLegacyClientInfo(mf, "mp3", 192, 0) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].Container).To(Equal("mp3")) @@ -63,7 +63,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("returns direct play profile when no format and no bitrate", func() { - ci := buildLegacyClientInfo(mf, "", 0) + ci := buildLegacyClientInfo(mf, "", 0, 0) Expect(ci.DirectPlayProfiles).To(HaveLen(1)) Expect(ci.DirectPlayProfiles[0].Containers).To(BeEmpty()) @@ -77,7 +77,7 @@ var _ = Describe("buildLegacyClientInfo", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.DefaultDownsamplingFormat = "opus" - ci := buildLegacyClientInfo(mf, "", 128) + ci := buildLegacyClientInfo(mf, "", 128, 0) Expect(ci.TranscodingProfiles).To(HaveLen(1)) Expect(ci.TranscodingProfiles[0].Container).To(Equal("opus")) @@ -91,7 +91,7 @@ var _ = Describe("buildLegacyClientInfo", func() { }) It("returns direct play when bitrate >= source bitrate", func() { - ci := buildLegacyClientInfo(mf, "", 960) + ci := buildLegacyClientInfo(mf, "", 960, 0) Expect(ci.DirectPlayProfiles).To(HaveLen(1)) Expect(ci.DirectPlayProfiles[0].Containers).To(BeEmpty()) @@ -100,6 +100,51 @@ var _ = Describe("buildLegacyClientInfo", func() { Expect(ci.TranscodingProfiles).To(BeEmpty()) Expect(ci.MaxAudioBitrate).To(BeZero()) }) + + It("uses default downsampling format when player MaxBitRate is below source and no format/bitrate", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "opus" + + ci := buildLegacyClientInfo(mf, "", 0, 256) + + Expect(ci.TranscodingProfiles).To(HaveLen(1)) + Expect(ci.TranscodingProfiles[0].Container).To(Equal("opus")) + Expect(ci.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + Expect(ci.DirectPlayProfiles[0].Containers).To(Equal([]string{"flac"})) + }) + + It("does not downsample when player MaxBitRate is >= source bitrate", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "opus" + + ci := buildLegacyClientInfo(mf, "", 0, 960) + + Expect(ci.TranscodingProfiles).To(BeEmpty()) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + Expect(ci.DirectPlayProfiles[0].Containers).To(BeEmpty()) + }) + + It("does not downsample when DefaultDownsamplingFormat is empty even with player cap", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "" + + ci := buildLegacyClientInfo(mf, "", 0, 256) + + Expect(ci.TranscodingProfiles).To(BeEmpty()) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + Expect(ci.DirectPlayProfiles[0].Containers).To(BeEmpty()) + }) + + It("prefers explicit request format over player cap", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "opus" + + ci := buildLegacyClientInfo(mf, "mp3", 0, 256) + + Expect(ci.TranscodingProfiles).To(HaveLen(1)) + Expect(ci.TranscodingProfiles[0].Container).To(Equal("mp3")) + }) }) var _ = Describe("ResolveRequest", func() { @@ -138,7 +183,7 @@ var _ = Describe("ResolveRequest", func() { }) It("transcodes to requested format", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) decider := svc.(*deciderService) req := decider.ResolveRequest(ctx, mf, "opus", 0, 0) @@ -147,7 +192,7 @@ var _ = Describe("ResolveRequest", func() { }) It("transcodes to requested format with bitrate limit", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) decider := svc.(*deciderService) req := decider.ResolveRequest(ctx, mf, "mp3", 128, 0) @@ -169,7 +214,7 @@ var _ = Describe("ResolveRequest", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.DefaultDownsamplingFormat = "opus" - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) decider := svc.(*deciderService) req := decider.ResolveRequest(ctx, mf, "", 128, 0) @@ -179,7 +224,7 @@ var _ = Describe("ResolveRequest", func() { }) It("passes offset through", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) decider := svc.(*deciderService) req := decider.ResolveRequest(ctx, mf, "opus", 128, 30) @@ -259,7 +304,7 @@ var _ = Describe("ResolveRequest", func() { Context("Player MaxBitRate cap", func() { It("applies player MaxBitRate cap when client has no limit", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 320}) decider := svc.(*deciderService) @@ -270,7 +315,7 @@ var _ = Describe("ResolveRequest", func() { }) It("uses client limit when it is more restrictive than player MaxBitRate", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 500}) decider := svc.(*deciderService) @@ -289,6 +334,33 @@ var _ = Describe("ResolveRequest", func() { Expect(req.Format).To(Equal("raw")) }) + + It("downsamples using DefaultDownsamplingFormat when only player MaxBitRate is set (no format/bitrate)", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "opus" + + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 256}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(playerCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("opus")) + Expect(req.BitRate).To(Equal(256)) + }) + + It("serves raw when only player MaxBitRate is set but no DefaultDownsamplingFormat", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DefaultDownsamplingFormat = "" + + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) + playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 256}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(playerCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("raw")) + }) }) Context("fallback for unknown format", func() { @@ -332,7 +404,7 @@ var _ = Describe("ResolveRequest", func() { DeferCleanup(configtest.SetupConfig()) conf.Server.DefaultDownsamplingFormat = "opus" - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: new(16)}) decider := svc.(*deciderService) req := decider.ResolveRequest(ctx, mf, "xyz", 128, 0) diff --git a/core/stream/token.go b/core/stream/token.go index 24a154b54..21a26ca93 100644 --- a/core/stream/token.go +++ b/core/stream/token.go @@ -92,17 +92,10 @@ func paramsFromToken(token jwt.Token) (*params, error) { return &p, nil } -// getIntClaim extracts an int claim from a JWT token, handling the case where -// the value may be stored as int64 or float64 (common in JSON-based JWT libraries). +// getIntClaim extracts a numeric claim from a JWT token. Numeric claims in a +// parsed token are always deserialized as float64, regardless of the type used +// when encoding. func getIntClaim(token jwt.Token, key string) int { - var v int - if err := token.Get(key, &v); err == nil { - return v - } - var v64 int64 - if err := token.Get(key, &v64); err == nil { - return int(v64) - } var f float64 if err := token.Get(key, &f); err == nil { return int(f) diff --git a/core/stream/types.go b/core/stream/types.go index bd8ce292c..19474dd91 100644 --- a/core/stream/types.go +++ b/core/stream/types.go @@ -40,6 +40,50 @@ type ClientInfo struct { CodecProfiles []CodecProfile } +// CapBitrate lowers the client's declared audio bitrate limits to maxKbps, +// never raising them. A zero limit means "unlimited" and is set to maxKbps. +// Returns true if anything changed. No-op when maxKbps <= 0. +func (ci *ClientInfo) CapBitrate(maxKbps int) bool { + if maxKbps <= 0 { + return false + } + changed := false + if ci.MaxAudioBitrate == 0 || maxKbps < ci.MaxAudioBitrate { + ci.MaxAudioBitrate = maxKbps + changed = true + } + if ci.MaxTranscodingAudioBitrate == 0 || maxKbps < ci.MaxTranscodingAudioBitrate { + ci.MaxTranscodingAudioBitrate = maxKbps + changed = true + } + 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. +func (ci *ClientInfo) ForceFormat(targetFormat string) bool { + if targetFormat == "" { + return false + } + var matched []Profile + for i := range ci.TranscodingProfiles { + // 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]) + } + } + if len(matched) == 0 { + return false + } + ci.TranscodingProfiles = matched + ci.DirectPlayProfiles = nil + return true +} + // DirectPlayProfile describes a format the client can play directly type DirectPlayProfile struct { Containers []string diff --git a/core/stream/types_test.go b/core/stream/types_test.go new file mode 100644 index 000000000..eff408362 --- /dev/null +++ b/core/stream/types_test.go @@ -0,0 +1,133 @@ +package stream + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ClientInfo", func() { + Describe("CapBitrate", func() { + It("is a no-op when maxKbps is zero", func() { + ci := &ClientInfo{MaxAudioBitrate: 320, MaxTranscodingAudioBitrate: 320} + Expect(ci.CapBitrate(0)).To(BeFalse()) + Expect(ci.MaxAudioBitrate).To(Equal(320)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(320)) + }) + + It("is a no-op when maxKbps is negative", func() { + ci := &ClientInfo{MaxAudioBitrate: 320, MaxTranscodingAudioBitrate: 320} + Expect(ci.CapBitrate(-1)).To(BeFalse()) + Expect(ci.MaxAudioBitrate).To(Equal(320)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(320)) + }) + + It("sets both limits when both are zero (unlimited)", func() { + ci := &ClientInfo{} + Expect(ci.CapBitrate(256)).To(BeTrue()) + Expect(ci.MaxAudioBitrate).To(Equal(256)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(256)) + }) + + It("lowers limits higher than maxKbps", func() { + ci := &ClientInfo{MaxAudioBitrate: 320, MaxTranscodingAudioBitrate: 500} + Expect(ci.CapBitrate(192)).To(BeTrue()) + Expect(ci.MaxAudioBitrate).To(Equal(192)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(192)) + }) + + It("does not raise limits lower than maxKbps", func() { + ci := &ClientInfo{MaxAudioBitrate: 128, MaxTranscodingAudioBitrate: 96} + Expect(ci.CapBitrate(320)).To(BeFalse()) + Expect(ci.MaxAudioBitrate).To(Equal(128)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(96)) + }) + + It("reports changed when only one limit is lowered", func() { + ci := &ClientInfo{MaxAudioBitrate: 320, MaxTranscodingAudioBitrate: 128} + Expect(ci.CapBitrate(192)).To(BeTrue()) + Expect(ci.MaxAudioBitrate).To(Equal(192)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(128)) + }) + + It("caps only the zero (unlimited) limit", func() { + ci := &ClientInfo{MaxAudioBitrate: 128, MaxTranscodingAudioBitrate: 0} + Expect(ci.CapBitrate(192)).To(BeTrue()) + Expect(ci.MaxAudioBitrate).To(Equal(128)) + Expect(ci.MaxTranscodingAudioBitrate).To(Equal(192)) + }) + }) + + Describe("ForceFormat", func() { + It("restricts to the forced format and clears direct play when supported", func() { + ci := &ClientInfo{ + DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}, AudioCodecs: []string{"flac"}}}, + TranscodingProfiles: []Profile{ + {Container: "flac", AudioCodec: "flac", Protocol: ProtocolHTTP}, + {Container: "ogg", AudioCodec: "opus", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + ok := ci.ForceFormat("opus") + Expect(ok).To(BeTrue()) + Expect(ci.TranscodingProfiles).To(HaveLen(1)) + Expect(ci.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) + Expect(ci.DirectPlayProfiles).To(BeEmpty()) + }) + + It("matches a container-only forced format (mp3)", func() { + ci := &ClientInfo{ + TranscodingProfiles: []Profile{ + {Container: "ogg", AudioCodec: "opus", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + ok := ci.ForceFormat("mp3") + Expect(ok).To(BeTrue()) + Expect(ci.TranscodingProfiles).To(HaveLen(1)) + Expect(ci.TranscodingProfiles[0].Container).To(Equal("mp3")) + }) + + It("matches the forced format against codec aliases (oga/opus)", func() { + ci := &ClientInfo{ + TranscodingProfiles: []Profile{ + {Container: "ogg", AudioCodec: "opus", Protocol: ProtocolHTTP}, + {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + }, + } + // Legacy DBs may store the Opus transcoding as target_format "oga". + ok := ci.ForceFormat("oga") + Expect(ok).To(BeTrue()) + Expect(ci.TranscodingProfiles).To(HaveLen(1)) + Expect(ci.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) + }) + + It("is a no-op when the forced format is not supported by the client", func() { + original := []Profile{{Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}} + ci := &ClientInfo{ + DirectPlayProfiles: []DirectPlayProfile{{Containers: []string{"flac"}}}, + TranscodingProfiles: original, + } + ok := ci.ForceFormat("opus") + Expect(ok).To(BeFalse()) + Expect(ci.TranscodingProfiles).To(Equal(original)) + Expect(ci.DirectPlayProfiles).To(HaveLen(1)) + }) + + It("is a no-op for an empty target format", func() { + ci := &ClientInfo{TranscodingProfiles: []Profile{{Container: "mp3", AudioCodec: "mp3"}}} + Expect(ci.ForceFormat("")).To(BeFalse()) + }) + + It("keeps all matching profiles when multiple resolve to the forced format", func() { + first := Profile{Container: "ogg", AudioCodec: "opus", Protocol: ProtocolHTTP} + second := Profile{Container: "ogg", AudioCodec: "opus", Protocol: ProtocolHTTP, MaxAudioChannels: 2} + other := Profile{Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP} + ci := &ClientInfo{TranscodingProfiles: []Profile{first, other, second}} + + ok := ci.ForceFormat("opus") + + Expect(ok).To(BeTrue()) + Expect(ci.TranscodingProfiles).To(ConsistOf(first, second)) + }) + }) +}) diff --git a/db/db.go b/db/db.go index 168c12122..6e5b2f569 100644 --- a/db/db.go +++ b/db/db.go @@ -51,7 +51,6 @@ func Db() *sql.DB { _, err = db.Exec("PRAGMA optimize=0x10002") if err != nil { log.Error("Error applying PRAGMA optimize", err) - return nil } } return db diff --git a/db/migrations/20260612171826_add_media_file_missing_library_id_index.sql b/db/migrations/20260612171826_add_media_file_missing_library_id_index.sql new file mode 100644 index 000000000..8bd8ad5b8 --- /dev/null +++ b/db/migrations/20260612171826_add_media_file_missing_library_id_index.sql @@ -0,0 +1,13 @@ +-- +goose Up +-- Covering index for the rowid-only pagination query used by search3 with an empty query +-- (full library sync). It must cover both `missing` and `library_id` so SQLite never touches +-- the (wide) media_file rows while skipping over large offsets. +-- Replaces media_file_missing: the composite serves all `missing = ?` lookups via its prefix. +create index if not exists media_file_missing_library_id + on media_file(missing, library_id); +drop index if exists media_file_missing; + +-- +goose Down +create index if not exists media_file_missing + on media_file(missing); +drop index if exists media_file_missing_library_id; diff --git a/db/migrations/20260612222838_make_bpm_bitdepth_nullable.sql b/db/migrations/20260612222838_make_bpm_bitdepth_nullable.sql new file mode 100644 index 000000000..c84323158 --- /dev/null +++ b/db/migrations/20260612222838_make_bpm_bitdepth_nullable.sql @@ -0,0 +1,35 @@ +-- +goose Up +drop index if exists media_file_bpm; + +alter table media_file add column bpm_new integer; +alter table media_file add column bit_depth_new integer; + +update media_file set + bpm_new = nullif(bpm, 0), + bit_depth_new = nullif(bit_depth, 0); + +alter table media_file drop column bpm; +alter table media_file drop column bit_depth; + +alter table media_file rename column bpm_new to bpm; +alter table media_file rename column bit_depth_new to bit_depth; + +create index if not exists media_file_bpm on media_file (bpm); + +-- +goose Down +drop index if exists media_file_bpm; + +alter table media_file add column bpm_old integer default 0 not null; +alter table media_file add column bit_depth_old integer default 0 not null; + +update media_file set + bpm_old = coalesce(bpm, 0), + bit_depth_old = coalesce(bit_depth, 0); + +alter table media_file drop column bpm; +alter table media_file drop column bit_depth; + +alter table media_file rename column bpm_old to bpm; +alter table media_file rename column bit_depth_old to bit_depth; + +create index if not exists media_file_bpm on media_file (bpm); diff --git a/model/criteria/fields.go b/model/criteria/fields.go index 9eafff7ab..5c9ec898d 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -9,8 +9,11 @@ type FieldInfo struct { IsRole bool Numeric bool Boolean bool + // Nullable: isMissing/isPresent are supported on this column field. For numeric/boolean + // fields, missing means NULL; for string fields it means NULL or empty string. + Nullable bool - tagAlias string // If set, a tag name from mappings.yml that resolves to this field + tagAlias string // If set, a tag name from mappings.yaml that resolves to this field name string // Canonical name, populated by LookupField from the map key } @@ -21,7 +24,7 @@ func (f FieldInfo) Name() string { var fieldMap = map[string]FieldInfo{ "title": {}, - "album": {}, + "album": {Nullable: true}, "hascoverart": {Boolean: true}, "tracknumber": {}, "discnumber": {}, @@ -34,26 +37,26 @@ var fieldMap = map[string]FieldInfo{ "size": {}, "compilation": {Boolean: true}, "missing": {Boolean: true}, - "explicitstatus": {}, + "explicitstatus": {Nullable: true}, "dateadded": {}, "datemodified": {}, - "discsubtitle": {}, - "comment": {}, - "lyrics": {}, - "sorttitle": {}, - "sortalbum": {}, - "sortartist": {}, - "sortalbumartist": {}, - "albumcomment": {}, - "catalognumber": {}, + "discsubtitle": {Nullable: true}, + "comment": {Nullable: true}, + "lyrics": {Nullable: true}, + "sorttitle": {Nullable: true}, + "sortalbum": {Nullable: true}, + "sortartist": {Nullable: true}, + "sortalbumartist": {Nullable: true}, + "albumcomment": {Nullable: true}, + "catalognumber": {Nullable: true}, "filepath": {}, "filetype": {}, "codec": {}, "duration": {}, "bitrate": {}, - "bitdepth": {}, + "bitdepth": {Numeric: true, Nullable: true}, "samplerate": {}, - "bpm": {}, + "bpm": {Numeric: true, Nullable: true}, "channels": {}, "loved": {Boolean: true}, "dateloved": {}, @@ -74,21 +77,29 @@ var fieldMap = map[string]FieldInfo{ "artistlastplayed": {}, "artistdateloved": {}, "artistdaterated": {}, - "mbz_album_id": {}, - "mbz_album_artist_id": {}, - "mbz_artist_id": {}, - "mbz_recording_id": {}, - "mbz_release_track_id": {}, - "mbz_release_group_id": {}, - "rgalbumgain": {Numeric: true}, - "rgalbumpeak": {Numeric: true}, - "rgtrackgain": {Numeric: true}, - "rgtrackpeak": {Numeric: true}, + "mbz_album_id": {Nullable: true}, + "mbz_album_artist_id": {Nullable: true}, + "mbz_artist_id": {Nullable: true}, + "mbz_recording_id": {Nullable: true}, + "mbz_release_track_id": {Nullable: true}, + "mbz_release_group_id": {Nullable: true}, + "rgalbumgain": {Numeric: true, Nullable: true}, + "rgalbumpeak": {Numeric: true, Nullable: true}, + "rgtrackgain": {Numeric: true, Nullable: true}, + "rgtrackpeak": {Numeric: true, Nullable: true}, "library_id": {Numeric: true}, // Backward compatibility: albumtype is an alias for the releasetype tag. "albumtype": {Alias: "releasetype", IsTag: true}, + // Backward compatibility: the replaygain_* tag names (as written in metadata and in the + // PR #5256 example) are aliases for the canonical rg* column fields. Without these, the tag + // names would be registered as empty tags from mappings.yaml and isMissing would always match. + "replaygain_album_gain": {Alias: "rgalbumgain", Numeric: true, Nullable: true}, + "replaygain_album_peak": {Alias: "rgalbumpeak", Numeric: true, Nullable: true}, + "replaygain_track_gain": {Alias: "rgtrackgain", Numeric: true, Nullable: true}, + "replaygain_track_peak": {Alias: "rgtrackpeak", Numeric: true, Nullable: true}, + // Pseudo-field for random sorting "random": {}, } @@ -128,7 +139,7 @@ func AddRoles(roles []string) { } } -// AddTagNames adds tag names to the field map. This is used to add all tags mapped in the `mappings.yml` +// AddTagNames adds tag names to the field map. This is used to add all tags mapped in the `mappings.yaml` // configuration file. func AddTagNames(tagNames []string) { for _, tagName := range tagNames { diff --git a/model/criteria/fields_test.go b/model/criteria/fields_test.go index 5b6f53341..2367101a8 100644 --- a/model/criteria/fields_test.go +++ b/model/criteria/fields_test.go @@ -53,5 +53,39 @@ var _ = Describe("fields", func() { gomega.Expect(field.IsRole).To(gomega.BeTrue()) }) + It("marks ReplayGain column fields as nullable", func() { + field, ok := LookupField("rgAlbumGain") + + gomega.Expect(ok).To(gomega.BeTrue()) + gomega.Expect(field.Name()).To(gomega.Equal("rgalbumgain")) + gomega.Expect(field.Nullable).To(gomega.BeTrue()) + gomega.Expect(field.IsTag).To(gomega.BeFalse()) + }) + + It("resolves replaygain_* tag names as aliases to nullable column fields", func() { + // AddTagNames skips names already in the field map, so the startup tag registration + // (from mappings.yaml) must not convert the pre-registered alias into a tag field. + AddTagNames([]string{"replaygain_album_gain"}) + + field, ok := LookupField("replaygain_album_gain") + + gomega.Expect(ok).To(gomega.BeTrue()) + gomega.Expect(field.Name()).To(gomega.Equal("rgalbumgain")) + gomega.Expect(field.Nullable).To(gomega.BeTrue()) + gomega.Expect(field.IsTag).To(gomega.BeFalse()) + }) + + It("marks mbz_* and lyrics string fields as nullable (empty means missing)", func() { + for _, name := range []string{"mbz_album_id", "mbz_album_artist_id", "mbz_artist_id", + "mbz_recording_id", "mbz_release_track_id", "mbz_release_group_id", "lyrics", + "album", "comment", "catalognumber", "discsubtitle", "albumcomment", + "sorttitle", "sortalbum", "sortartist", "sortalbumartist", "explicitstatus"} { + field, ok := LookupField(name) + gomega.Expect(ok).To(gomega.BeTrue(), name) + gomega.Expect(field.Nullable).To(gomega.BeTrue(), name) + gomega.Expect(field.Numeric).To(gomega.BeFalse(), name) + } + }) + }) }) diff --git a/model/folder.go b/model/folder.go index 7a769735e..81800c072 100644 --- a/model/folder.go +++ b/model/folder.go @@ -86,7 +86,14 @@ type FolderRepository interface { GetAll(...QueryOptions) ([]Folder, error) CountAll(...QueryOptions) (int64, error) GetFolderUpdateInfo(lib Library, targetPaths ...string) (map[string]FolderUpdateInfo, error) + // HasAudioOutsideFolders reports whether any folder in parent's subtree + // (including parent itself) contains audio files and is not one of the + // given folder IDs. + HasAudioOutsideFolders(parent Folder, excludeFolderIDs []string) (bool, error) Put(*Folder) error MarkMissing(missing bool, ids ...string) error GetTouchedWithPlaylists() (FolderCursor, error) + // GetAllWithPlaylists returns all non-missing folders with playlists, ignoring + // the scan-timestamp gate used by GetTouchedWithPlaylists. + GetAllWithPlaylists() (FolderCursor, error) } diff --git a/model/mediafile.go b/model/mediafile.go index 6be8402ae..6a489bcd5 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/utils" + "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/slice" ) @@ -54,7 +55,7 @@ type MediaFile struct { Duration float32 `structs:"duration" json:"duration"` BitRate int `structs:"bit_rate" json:"bitRate"` SampleRate int `structs:"sample_rate" json:"sampleRate"` - BitDepth int `structs:"bit_depth" json:"bitDepth"` + BitDepth *int `structs:"bit_depth" json:"bitDepth,omitempty"` Channels int `structs:"channels" json:"channels"` Codec string `structs:"codec" json:"codec"` ProbeData string `structs:"probe_data" json:"-" hash:"ignore"` @@ -71,7 +72,7 @@ type MediaFile struct { Compilation bool `structs:"compilation" json:"compilation"` Comment string `structs:"comment" json:"comment,omitempty"` Lyrics string `structs:"lyrics" json:"lyrics"` - BPM int `structs:"bpm" json:"bpm,omitempty"` + BPM *int `structs:"bpm" json:"bpm,omitempty"` ExplicitStatus string `structs:"explicit_status" json:"explicitStatus"` CatalogNum string `structs:"catalog_num" json:"catalogNum,omitempty"` MbzRecordingID string `structs:"mbz_recording_id" json:"mbzRecordingID,omitempty"` @@ -225,7 +226,7 @@ func (mf MediaFile) inferCodecFromSuffix() string { return "dsd" case "m4a": // AAC if BitDepth==0, ALAC if BitDepth>0 - if mf.BitDepth > 0 { + if gg.V(mf.BitDepth) > 0 { return "alac" } return "aac" @@ -438,6 +439,9 @@ type MediaFileRepository interface { Get(id string) (*MediaFile, error) GetWithParticipants(id string) (*MediaFile, error) GetAll(options ...QueryOptions) (MediaFiles, error) + // GetRandom returns up to options.Max media files in random order, applying the same + // filters as GetAll. Sort/Order are ignored. + GetRandom(options ...QueryOptions) (MediaFiles, error) GetAllByTags(tag TagName, values []string, options ...QueryOptions) (MediaFiles, error) GetCursor(options ...QueryOptions) (MediaFileCursor, error) Delete(id string) error diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 3547ec4ef..65c5a0652 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -564,7 +564,7 @@ var _ = Describe("MediaFile", func() { DescribeTable("infers codec from suffix when Codec field is empty", func(suffix string, bitDepth int, expected string) { - mf := MediaFile{Suffix: suffix, BitDepth: bitDepth} + mf := MediaFile{Suffix: suffix, BitDepth: new(bitDepth)} Expect(mf.AudioCodec()).To(Equal(expected)) }, Entry("mp3", "mp3", 0, "mp3"), @@ -597,13 +597,30 @@ var _ = Describe("MediaFile", func() { ) It("prefers stored codec over suffix inference", func() { - mf := MediaFile{Codec: "ALAC", Suffix: "m4a", BitDepth: 0} + mf := MediaFile{Codec: "ALAC", Suffix: "m4a"} Expect(mf.AudioCodec()).To(Equal("alac")) }) }) }) +var _ = Describe("MediaFile.Hash", func() { + // Guards the upgrade guarantee: converting BPM/BitDepth from int to *int must not change hashes, + // or every file would be spuriously re-imported on the next scan. + // Golden hashes were captured at 46221d516 when those fields were plain ints. + It("keeps hashes identical to the pre-pointer-conversion values", func() { + // Golden hashes computed at 46221d516, when BPM/BitDepth were plain ints — pinning + // them guarantees the pointer conversion cannot trigger a full-library re-import. + Expect(MediaFile{Title: "Song"}.Hash()).To(Equal("1d856ced42cb96db39e354a4bac9a622")) + Expect(MediaFile{Title: "Song", BPM: new(120), BitDepth: new(16)}.Hash()).To(Equal("b2b0b1d1dd7fd767093588e4af3a0689")) + }) + It("changes the hash when a pointer field has a value", func() { + base := MediaFile{Title: "Song"} + Expect(base.Equals(MediaFile{Title: "Song", BPM: new(120)})).To(BeFalse()) + Expect(base.Equals(MediaFile{Title: "Song", BitDepth: new(24)})).To(BeFalse()) + }) +}) + func t(v string) time.Time { var timeFormats = []string{"2006-01-02", "2006-01-02 15:04", "2006-01-02 15:04:05", "2006-01-02T15:04:05", "2006-01-02T15:04", "2006-01-02 15:04:05.999999999 -0700 MST"} for _, f := range timeFormats { diff --git a/model/metadata/map_mediafile.go b/model/metadata/map_mediafile.go index ecb0a9195..52258b938 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -36,7 +36,11 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf.DiscSubtitle = md.String(model.TagDiscSubtitle) mf.CatalogNum = md.String(model.TagCatalogNumber) mf.Comment = md.String(model.TagComment) - mf.BPM = int(math.Round(md.Float(model.TagBPM))) + if f := md.NullableFloat(model.TagBPM); f != nil { + if v := int(math.Round(*f)); v != 0 { + mf.BPM = new(v) + } + } mf.Lyrics = md.mapLyrics() mf.ExplicitStatus = md.mapExplicitStatusTag() @@ -64,7 +68,9 @@ func (md Metadata) ToMediaFile(libID int, folderID string) model.MediaFile { mf.Duration = md.Length() mf.BitRate = md.AudioProperties().BitRate mf.SampleRate = md.AudioProperties().SampleRate - mf.BitDepth = md.AudioProperties().BitDepth + if bd := md.AudioProperties().BitDepth; bd > 0 { + mf.BitDepth = new(bd) + } mf.Channels = md.AudioProperties().Channels mf.Codec = md.AudioProperties().Codec mf.Path = md.FilePath() diff --git a/model/metadata/map_mediafile_test.go b/model/metadata/map_mediafile_test.go index 486386718..35afe3915 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -199,4 +199,32 @@ Estamos nas legendas`}, })) }) }) + + Describe("BPM", func() { + It("maps the BPM tag rounded to the nearest integer", func() { + mf = toMediaFile(model.RawTags{"BPM": {"120.6"}}) + Expect(mf.BPM).To(Equal(new(121))) + }) + It("leaves BPM nil when the tag is absent", func() { + mf = toMediaFile(model.RawTags{}) + Expect(mf.BPM).To(BeNil()) + }) + It("leaves BPM nil when the tag is zero or unparseable", func() { + Expect(toMediaFile(model.RawTags{"BPM": {"0"}}).BPM).To(BeNil()) + Expect(toMediaFile(model.RawTags{"BPM": {"fast"}}).BPM).To(BeNil()) + }) + }) + + Describe("BitDepth", func() { + It("maps the bit depth when present", func() { + props.AudioProperties = metadata.AudioProperties{BitDepth: 24} + mf = toMediaFile(model.RawTags{}) + Expect(mf.BitDepth).To(Equal(new(24))) + }) + It("leaves BitDepth nil when zero (lossy codecs have no bit depth)", func() { + props.AudioProperties = metadata.AudioProperties{BitDepth: 0} + mf = toMediaFile(model.RawTags{}) + Expect(mf.BitDepth).To(BeNil()) + }) + }) }) diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index cfdc499e0..aa3bc0776 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -540,13 +540,28 @@ func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) { return totalRowsAffected, nil } +// applyLibraryFilterToSearchQuery is applyLibraryFilterToArtistQuery with the join order +// pinned via CROSS JOIN (SQLite's explicit join-order override): the search Phase 1 paginates +// rowids by artist.id, and when the planner drives from library_artist it must sort every +// junction row on every page (temp b-tree over the whole table). Keeping artist as the outer +// table streams rows in artist.id order from its primary key index, so LIMIT/OFFSET +// short-circuits. Search-only: other artist queries keep the planner's freedom. +func (r *artistRepository) applyLibraryFilterToSearchQuery(query SelectBuilder) SelectBuilder { + user := loggedUser(r.ctx) + query = query.CrossJoin("library_artist on library_artist.artist_id = artist.id") + if user.ID != invalidUserId && !user.IsAdmin { + query = query.Join("user_library on user_library.library_id = library_artist.library_id AND user_library.user_id = ?", user.ID) + } + return query +} + func (r *artistRepository) searchCfg() searchConfig { return searchConfig{ // Natural order for artists is more performant by ID, due to GROUP BY clause in selectArtist NaturalOrder: "artist.id", OrderBy: []string{"sum(json_extract(stats, '$.total.m')) desc", "name"}, MBIDFields: []string{"mbz_artist_id"}, - LibraryFilter: r.applyLibraryFilterToArtistQuery, + LibraryFilter: r.applyLibraryFilterToSearchQuery, } } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 076a9da3b..7003efec3 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -3,6 +3,7 @@ package persistence import ( "context" "encoding/json" + "fmt" "os" "path/filepath" @@ -594,6 +595,66 @@ var _ = Describe("ArtistRepository", func() { }) }) + Context("Empty Query (sync pagination)", func() { + It("does not duplicate artists that belong to multiple libraries", func() { + // An artist in two libraries has two library_artist rows; pagination + // must still enumerate it exactly once, at a stable offset. + Expect(lr.AddArtist(lib2.ID, artistBeatles.ID)).To(Succeed()) + + all, err := repo.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + + seen := map[string]bool{} + var paged model.Artists + for offset := range len(all) { + page, err := repo.Search("", model.QueryOptions{Max: 1, Offset: offset}) + Expect(err).ToNot(HaveOccurred()) + for _, a := range page { + Expect(seen[a.ID]).To(BeFalse(), fmt.Sprintf("artist %s returned twice", a.ID)) + seen[a.ID] = true + } + paged = append(paged, page...) + } + Expect(paged).To(HaveLen(len(all))) + }) + + It("paginates all artists in natural order without overlaps or gaps", func() { + all, err := repo.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(all)).To(BeNumerically(">", 1)) + + var paged model.Artists + pageSize := 2 + for offset := 0; offset < len(all); offset += pageSize { + page, err := repo.Search("", model.QueryOptions{Max: pageSize, Offset: offset}) + Expect(err).ToNot(HaveOccurred()) + paged = append(paged, page...) + } + Expect(paged).To(HaveLen(len(all))) + for i := range all { + Expect(paged[i].ID).To(Equal(all[i].ID)) + } + }) + + It("respects library filtering for restricted users", func() { + // Create an artist only in library 2 (not accessible to restricted user) + lib2Artist := model.Artist{ID: "empty-query-lib2-artist", Name: "Empty Query Lib2 Artist"} + Expect(repo.Put(&lib2Artist)).To(Succeed()) + Expect(lr.AddArtist(lib2.ID, lib2Artist.ID)).To(Succeed()) + + results, err := restrictedRepo.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + for _, a := range results { + Expect(a.ID).ToNot(Equal(lib2Artist.ID), "Empty query search should respect library filtering") + } + + // Clean up + if raw, ok := repo.(*artistRepository); ok { + _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": lib2Artist.ID})) + } + }) + }) + Context("Headless Processes (No User Context)", func() { It("should see all artists from all libraries when no user is in context", func() { // Add artists to different libraries diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 37e4ae340..b74f498d0 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -28,9 +28,10 @@ func (j smartPlaylistJoinType) has(other smartPlaylistJoinType) bool { } type smartPlaylistField struct { - expr string - order string - joinType smartPlaylistJoinType + expr string + order string + joinType smartPlaylistJoinType + emptyValues []string // additional values that encode "missing" for string columns (e.g. '[]' for lyrics) } type smartPlaylistCriteria struct { @@ -72,7 +73,7 @@ var smartPlaylistFields = map[string]smartPlaylistField{ "datemodified": {expr: "media_file.updated_at"}, "discsubtitle": {expr: "media_file.disc_subtitle"}, "comment": {expr: "media_file.comment"}, - "lyrics": {expr: "media_file.lyrics"}, + "lyrics": {expr: "media_file.lyrics", emptyValues: []string{"[]"}}, "sorttitle": {expr: "media_file.sort_title"}, "sortalbum": {expr: "media_file.sort_album_name"}, "sortartist": {expr: "media_file.sort_artist_name"}, @@ -139,7 +140,7 @@ func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqliz } and = append(and, cond) } - return and, nil + return mergeNegatedJsonConds(and), nil case criteria.Any: or := squirrel.Or{} for _, child := range e { @@ -218,16 +219,44 @@ func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, er } return nil, fmt.Errorf("invalid field in criteria: %s", field) } - if !info.IsTag && !info.IsRole { - return nil, fmt.Errorf("isMissing/isPresent operator is only supported for tag and role fields, got: %s", field) - } - b, ok := value.(bool) if !ok { return nil, fmt.Errorf("invalid boolean value for 'missing' expression: %s: %v", field, value) } negate := checkAbsence == b - return jsonExpr(info, nil, negate), nil + + switch { + case info.IsTag || info.IsRole: + return jsonExpr(info, nil, negate), nil + case info.Nullable: + // Nullable column fields are stored in dedicated columns, not in the tags JSON, so + // "missing" maps to a column check rather than a json_tree lookup. Numeric/boolean + // columns (e.g. ReplayGain, BPM) encode absence as NULL only; string columns (e.g. + // mbz_* IDs, lyrics) additionally treat empty string — and any field-specific empty + // encodings (e.g. '[]' for lyrics) — as missing. The unified flow below handles both: + // numeric/boolean fields simply have no empties, so the loops are no-ops. + f, ok := smartPlaylistFields[info.Name()] + if !ok || f.expr == "" { + return nil, fmt.Errorf("invalid field in criteria: %s", field) + } + col := f.expr + var empties []string + if !info.Numeric && !info.Boolean { + empties = append([]string{""}, f.emptyValues...) + } + missing := squirrel.Or{squirrel.Eq{col: nil}} + present := squirrel.And{squirrel.NotEq{col: nil}} + for _, e := range empties { + missing = append(missing, squirrel.Eq{col: e}) + present = append(present, squirrel.NotEq{col: e}) + } + if negate { + return missing, nil + } + return present, nil + default: + return nil, fmt.Errorf("isMissing/isPresent operator is not supported for field: %s", field) + } } func mapExpr(values map[string]any, makeCond func(map[string]any) squirrel.Sqlizer, negateJSON bool) (squirrel.Sqlizer, error) { @@ -425,6 +454,25 @@ const jsonCondBatchSize = 350 // This turns N separate correlated subqueries into ceil(N/batchSize), dramatically // improving performance for smart playlists with many patterns. func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { + if merged, ok := mergeSameFieldConds(or, false); ok { + return squirrel.Or(merged) + } + return or +} + +// mergeNegatedJsonConds is the AND-group counterpart to mergeJsonConds, merging negated +// conditions. By De Morgan, "NOT EXISTS(X) AND NOT EXISTS(Y)" == "NOT EXISTS(X OR Y)". +func mergeNegatedJsonConds(and squirrel.And) squirrel.Sqlizer { + if merged, ok := mergeSameFieldConds(and, true); ok { + return squirrel.And(merged) + } + return and +} + +// mergeSameFieldConds groups roleCond/tagCond entries that share a field and the requested +// polarity, replacing each group of 2+ with batched roleCondGroup/tagCondGroup subqueries. +// Returns the rewritten conditions and whether any merge happened. +func mergeSameFieldConds(conds []squirrel.Sqlizer, negated bool) ([]squirrel.Sqlizer, bool) { type condEntry struct { index int cond squirrel.Sqlizer @@ -436,10 +484,10 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { tag string } groups := make(map[string]*group) - for i, s := range or { + for i, s := range conds { switch c := s.(type) { case roleCond: - if c.not || c.cond == nil { + if c.not != negated || c.cond == nil { continue } g, exists := groups["role:"+c.role] @@ -449,7 +497,7 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { } g.entries = append(g.entries, condEntry{index: i, cond: c.cond}) case tagCond: - if c.not || c.cond == nil { + if c.not != negated || c.cond == nil { continue } g, exists := groups["tag:"+c.tag] @@ -461,7 +509,6 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { } } - merged := false remove := make(map[int]bool) var additions []squirrel.Sqlizer for _, key := range slices.Sorted(maps.Keys(groups)) { @@ -469,45 +516,42 @@ func mergeJsonConds(or squirrel.Or) squirrel.Sqlizer { if len(g.entries) < 2 { continue } - merged = true - for _, e := range g.entries { - remove[e.index] = true - } - conds := make([]squirrel.Sqlizer, len(g.entries)) + batchConds := make([]squirrel.Sqlizer, len(g.entries)) for i, e := range g.entries { - conds[i] = e.cond + remove[e.index] = true + batchConds[i] = e.cond } if g.isRole { role := key[len("role:"):] - for batch := range slices.Chunk(conds, jsonCondBatchSize) { - additions = append(additions, roleCondGroup{role: role, conds: batch}) + for batch := range slices.Chunk(batchConds, jsonCondBatchSize) { + additions = append(additions, roleCondGroup{role: role, conds: batch, not: negated}) } } else { - for batch := range slices.Chunk(conds, jsonCondBatchSize) { - additions = append(additions, tagCondGroup{tag: g.tag, numeric: g.numeric, conds: batch}) + for batch := range slices.Chunk(batchConds, jsonCondBatchSize) { + additions = append(additions, tagCondGroup{tag: g.tag, numeric: g.numeric, conds: batch, not: negated}) } } } - if !merged { - return or + if len(remove) == 0 { + return conds, false } - result := make(squirrel.Or, 0, len(or)-len(remove)+len(additions)) - for i, s := range or { + result := make([]squirrel.Sqlizer, 0, len(conds)-len(remove)+len(additions)) + for i, s := range conds { if !remove[i] { result = append(result, s) } } - result = append(result, additions...) - return result + return append(result, additions...), true } -// roleCondGroup represents multiple role conditions for the same role, merged into -// a single EXISTS subquery for performance. +// roleCondGroup represents multiple role conditions for the same role, merged into a single +// (optionally negated) EXISTS subquery for performance. type roleCondGroup struct { role string conds []squirrel.Sqlizer + not bool } func (g roleCondGroup) ToSql() (string, []any, error) { @@ -522,15 +566,19 @@ func (g roleCondGroup) ToSql() (string, []any, error) { allArgs = append(allArgs, args...) } cond := roleExistsSQL("(" + strings.Join(innerParts, " OR ") + ")") + if g.not { + cond = "not " + cond + } return cond, allArgs, nil } -// tagCondGroup represents multiple tag conditions for the same tag, merged into -// a single EXISTS subquery for performance. +// tagCondGroup represents multiple tag conditions for the same tag, merged into a single +// (optionally negated) EXISTS subquery for performance. type tagCondGroup struct { tag string numeric bool conds []squirrel.Sqlizer + not bool } func (g tagCondGroup) ToSql() (string, []any, error) { @@ -549,6 +597,9 @@ func (g tagCondGroup) ToSql() (string, []any, error) { } cond := fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and (%s))", g.tag, strings.Join(innerParts, " OR ")) + if g.not { + cond = "not " + cond + } return cond, allArgs, nil } diff --git a/persistence/criteria_sql_benchmark_test.go b/persistence/criteria_sql_benchmark_test.go index d901e9eda..1dcf97871 100644 --- a/persistence/criteria_sql_benchmark_test.go +++ b/persistence/criteria_sql_benchmark_test.go @@ -60,6 +60,61 @@ func BenchmarkSmartPlaylistRole(b *testing.B) { }) } +// BenchmarkSmartPlaylistNegatedRole compares performance for smart playlists with many +// negated role conditions ANDed together (e.g. 500 "isNot artist" rules, issue #5511) +// between the current implementation (merged NOT EXISTS via criteria pipeline) and the +// old baseline (one separate NOT EXISTS subquery per pattern). +func BenchmarkSmartPlaylistNegatedRole(b *testing.B) { + configtest.SetupConfig() + tmpDir := b.TempDir() + conf.Server.DbPath = filepath.Join(tmpDir, "bench-smartpl-neg.db") + cleanup := db.Init(context.Background()) + defer cleanup() + log.SetLevel(log.LevelFatal) + + conn := dbx.NewFromDB(db.Db(), db.Dialect) + ctx := log.NewContext(context.Background()) + user := model.User{ID: "bench-user", UserName: "bench", Name: "Bench User", IsAdmin: true} + ctx = request.WithUser(ctx, user) + + setupBenchData(b, ctx, conn, user) + criteria.AddRoles([]string{"artist"}) + + // Build the criteria expression: 500 "isNot artist" patterns in an AND group + allExprs := make(criteria.All, benchNumPatterns) + for i := range benchNumPatterns { + allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist %04d", i)} + } + expr := criteria.Criteria{Expression: allExprs, Sort: "title", Limit: 500} + + b.Run("Current", func(b *testing.B) { + benchmarkCriteriaPipeline(b, ctx, expr) + }) + b.Run("Baseline_UnmergedNotExists", func(b *testing.B) { + benchmarkUnmergedNegatedJSONTree(b, ctx) + }) +} + +// benchmarkUnmergedNegatedJSONTree builds the old-style query with N separate negated +// json_tree EXISTS subqueries ANDed together (the pre-optimization baseline). +func benchmarkUnmergedNegatedJSONTree(b *testing.B, ctx context.Context) { + b.Helper() + + var sb strings.Builder + sb.WriteString("SELECT media_file.id FROM media_file WHERE (") + args := make([]any, 0, benchNumPatterns) + for i := range benchNumPatterns { + if i > 0 { + sb.WriteString(" AND ") + } + sb.WriteString("not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value = ?)") + args = append(args, fmt.Sprintf("Artist %04d", i)) + } + sb.WriteString(") ORDER BY media_file.title LIMIT 500") + + runBenchQuery(b, ctx, sb.String(), args) +} + // benchmarkCriteriaPipeline runs the criteria through the actual production code path: // newSmartPlaylistCriteria → Where() → ToSql(), then executes the resulting query. func benchmarkCriteriaPipeline(b *testing.B, ctx context.Context, expr criteria.Criteria) { diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 5c8909e1c..9ff7f1f07 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -14,7 +14,7 @@ import ( var _ = Describe("Smart playlist criteria SQL", func() { BeforeEach(func() { criteria.AddRoles([]string{"artist", "composer", "producer"}) - criteria.AddTagNames([]string{"genre", "mood", "releasetype", "recordingdate"}) + criteria.AddTagNames([]string{"genre", "mood", "releasetype", "recordingdate", "replaygain_album_gain"}) criteria.AddNumericTags([]string{"rate"}) }) @@ -85,6 +85,70 @@ var _ = Describe("Smart playlist criteria SQL", func() { "exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"), Entry("isPresent role [false]", criteria.IsPresent{"composer": false}, "not exists (select 1 from media_file_artists mfa where mfa.media_file_id = media_file.id and mfa.role = ?)", "composer"), + // isMissing/isPresent — nullable column fields (ReplayGain) + Entry("isMissing rgAlbumGain [true]", criteria.IsMissing{"rgAlbumGain": true}, + "(media_file.rg_album_gain IS NULL)"), + Entry("isMissing rgAlbumGain [false]", criteria.IsMissing{"rgAlbumGain": false}, + "(media_file.rg_album_gain IS NOT NULL)"), + Entry("isPresent rgTrackPeak [true]", criteria.IsPresent{"rgTrackPeak": true}, + "(media_file.rg_track_peak IS NOT NULL)"), + Entry("isPresent rgTrackPeak [false]", criteria.IsPresent{"rgTrackPeak": false}, + "(media_file.rg_track_peak IS NULL)"), + // isMissing — replaygain_* tag-name alias resolves to the nullable column (issue #5584) + Entry("isMissing replaygain_album_gain alias [true]", criteria.IsMissing{"replaygain_album_gain": true}, + "(media_file.rg_album_gain IS NULL)"), + Entry("isPresent replaygain_album_gain alias [true]", criteria.IsPresent{"replaygain_album_gain": true}, + "(media_file.rg_album_gain IS NOT NULL)"), + // isMissing/isPresent — string column fields (empty string means missing) + Entry("isMissing mbz_recording_id [true]", criteria.IsMissing{"mbz_recording_id": true}, + "(media_file.mbz_recording_id IS NULL OR media_file.mbz_recording_id = ?)", ""), + Entry("isMissing mbz_recording_id [false]", criteria.IsMissing{"mbz_recording_id": false}, + "(media_file.mbz_recording_id IS NOT NULL AND media_file.mbz_recording_id <> ?)", ""), + Entry("isPresent mbz_album_id [true]", criteria.IsPresent{"mbz_album_id": true}, + "(media_file.mbz_album_id IS NOT NULL AND media_file.mbz_album_id <> ?)", ""), + Entry("isPresent mbz_album_id [false]", criteria.IsPresent{"mbz_album_id": false}, + "(media_file.mbz_album_id IS NULL OR media_file.mbz_album_id = ?)", ""), + // lyrics: absence is encoded as '' or '[]' (empty serialized LyricList) + Entry("isMissing lyrics [true]", criteria.IsMissing{"lyrics": true}, + "(media_file.lyrics IS NULL OR media_file.lyrics = ? OR media_file.lyrics = ?)", "", "[]"), + Entry("isPresent lyrics [true]", criteria.IsPresent{"lyrics": true}, + "(media_file.lyrics IS NOT NULL AND media_file.lyrics <> ? AND media_file.lyrics <> ?)", "", "[]"), + Entry("isMissing lyrics [false]", criteria.IsMissing{"lyrics": false}, + "(media_file.lyrics IS NOT NULL AND media_file.lyrics <> ? AND media_file.lyrics <> ?)", "", "[]"), + Entry("isPresent lyrics [false]", criteria.IsPresent{"lyrics": false}, + "(media_file.lyrics IS NULL OR media_file.lyrics = ? OR media_file.lyrics = ?)", "", "[]"), + // isMissing/isPresent — nullable numeric columns (BPM, BitDepth) + Entry("isMissing bpm [true]", criteria.IsMissing{"bpm": true}, + "(media_file.bpm IS NULL)"), + Entry("isPresent bpm [true]", criteria.IsPresent{"bpm": true}, + "(media_file.bpm IS NOT NULL)"), + Entry("isMissing bitdepth [true]", criteria.IsMissing{"bitdepth": true}, + "(media_file.bit_depth IS NULL)"), + Entry("isPresent bitdepth [false]", criteria.IsPresent{"bitdepth": false}, + "(media_file.bit_depth IS NULL)"), + // isMissing/isPresent — more string column fields (empty string means missing) + Entry("isMissing album [true]", criteria.IsMissing{"album": true}, + "(media_file.album IS NULL OR media_file.album = ?)", ""), + Entry("isMissing comment [true]", criteria.IsMissing{"comment": true}, + "(media_file.comment IS NULL OR media_file.comment = ?)", ""), + Entry("isMissing catalognumber [true]", criteria.IsMissing{"catalognumber": true}, + "(media_file.catalog_num IS NULL OR media_file.catalog_num = ?)", ""), + Entry("isMissing discsubtitle [true]", criteria.IsMissing{"discsubtitle": true}, + "(media_file.disc_subtitle IS NULL OR media_file.disc_subtitle = ?)", ""), + Entry("isMissing albumcomment [true]", criteria.IsMissing{"albumcomment": true}, + "(media_file.mbz_album_comment IS NULL OR media_file.mbz_album_comment = ?)", ""), + Entry("isMissing sorttitle [true]", criteria.IsMissing{"sorttitle": true}, + "(media_file.sort_title IS NULL OR media_file.sort_title = ?)", ""), + Entry("isMissing sortalbum [true]", criteria.IsMissing{"sortalbum": true}, + "(media_file.sort_album_name IS NULL OR media_file.sort_album_name = ?)", ""), + Entry("isMissing sortartist [true]", criteria.IsMissing{"sortartist": true}, + "(media_file.sort_artist_name IS NULL OR media_file.sort_artist_name = ?)", ""), + Entry("isMissing sortalbumartist [true]", criteria.IsMissing{"sortalbumartist": true}, + "(media_file.sort_album_artist_name IS NULL OR media_file.sort_album_artist_name = ?)", ""), + Entry("isMissing explicitstatus [true]", criteria.IsMissing{"explicitstatus": true}, + "(media_file.explicit_status IS NULL OR media_file.explicit_status = ?)", ""), + Entry("isPresent comment [true]", criteria.IsPresent{"comment": true}, + "(media_file.comment IS NOT NULL AND media_file.comment <> ?)", ""), ) Describe("playlist permissions", func() { @@ -143,12 +207,12 @@ var _ = Describe("Smart playlist criteria SQL", func() { It("returns an error when isMissing is used with a regular field", func() { _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).Where() - Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields"))) + Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field"))) }) It("returns an error when isPresent is used with a regular field", func() { _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).Where() - Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields"))) + Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is not supported for field"))) }) It("returns an error when isMissing has a non-boolean value", func() { @@ -344,6 +408,97 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?")) Expect(args).To(HaveLen(2 + 2 + 1)) // 2 tag patterns + 2 role patterns + 1 role name }) + + It("merges negated role conditions in an AND group into a single NOT EXISTS", func() { + expr := criteria.All{ + criteria.IsNot{"artist": "Beatles"}, + criteria.IsNot{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // A single NOT EXISTS with both names ORed inside (De Morgan) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("artist.name = ? OR artist.name = ?")) + Expect(args).To(HaveExactElements("artist", "Beatles", "Kraftwerk")) + }) + + It("merges negated notContains role conditions in an AND group", func() { + expr := criteria.All{ + criteria.NotContains{"artist": "Beatles"}, + criteria.NotContains{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("artist.name LIKE ? OR artist.name LIKE ?")) + Expect(args).To(HaveExactElements("artist", "%Beatles%", "%Kraftwerk%")) + }) + + It("merges negated tag conditions in an AND group into a single NOT EXISTS", func() { + expr := criteria.All{ + criteria.NotContains{"genre": "Rock"}, + criteria.NotContains{"genre": "Metal"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(sql).To(ContainSubstring("value LIKE ? OR value LIKE ?")) + Expect(args).To(HaveExactElements("%Rock%", "%Metal%")) + }) + + It("does not merge a single negated condition with a positive one of the same role in AND", func() { + // AND of mixed polarity must not be collapsed: NOT EXISTS(a) AND EXISTS(b) + // is not equivalent to any single merged subquery. + expr := criteria.All{ + criteria.Contains{"artist": "Beatles"}, + criteria.IsNot{"artist": "Kraftwerk"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, _, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // One positive EXISTS and one negated NOT EXISTS, kept separate + Expect(strings.Count(sql, "not exists")).To(Equal(1)) + Expect(strings.Count(sql, "exists")).To(Equal(2)) // "not exists" contains "exists" + }) + + It("does not merge negated conditions of different roles in AND", func() { + expr := criteria.All{ + criteria.IsNot{"artist": "Beatles"}, + criteria.IsNot{"composer": "Lennon"}, + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: expr}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, _, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(strings.Count(sql, "not exists")).To(Equal(2)) + }) + + It("batches large negated AND groups to avoid SQLite expression tree depth limit", func() { + allExprs := make(criteria.All, jsonCondBatchSize+1) + for i := range allExprs { + allExprs[i] = criteria.IsNot{"artist": fmt.Sprintf("Artist%d", i)} + } + sqlizer, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: allExprs}).Where() + Expect(err).ToNot(HaveOccurred()) + + sql, args, err := sqlizer.ToSql() + Expect(err).ToNot(HaveOccurred()) + // Two NOT EXISTS subqueries (one batch of jsonCondBatchSize, one of 1) + Expect(strings.Count(sql, "not exists")).To(Equal(2)) + Expect(args).To(HaveLen(2 + jsonCondBatchSize + 1)) + }) }) Describe("joins", func() { diff --git a/persistence/e2e/e2e_suite_test.go b/persistence/e2e/e2e_suite_test.go index f42292f02..1ff2139e6 100644 --- a/persistence/e2e/e2e_suite_test.go +++ b/persistence/e2e/e2e_suite_test.go @@ -70,11 +70,13 @@ var ( func buildTestFS() { abbeyRoad := template(_t{ - "albumartist": "The Beatles", - "artist": "The Beatles", - "album": "Abbey Road", - "year": 1969, - "genre": "Rock;Blues", + "albumartist": "The Beatles", + "artist": "The Beatles", + "album": "Abbey Road", + "year": 1969, + "genre": "Rock;Blues", + "replaygain_album_gain": "-6.5 dB", + "replaygain_album_peak": "0.98", }) ledZepIV := template(_t{ "albumartist": "Led Zeppelin", @@ -116,12 +118,16 @@ func buildTestFS() { fs := storagetest.FakeFS{} fs.SetFiles(fstest.MapFS{ "Rock/The Beatles/Abbey Road/01 - Come Together.mp3": abbeyRoad(track(1, "Come Together", - _t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120, "grouping": "Beatles Tracks"})), + _t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120, "grouping": "Beatles Tracks", + "replaygain_track_gain": "-7.1 dB", "replaygain_track_peak": "0.95"})), "Rock/The Beatles/Abbey Road/02 - Something.mp3": abbeyRoad(track(2, "Something", - _t{"genre": "Rock", "composer": "Harrison", "bpm": 100, "grouping": "Beatles Tracks"})), + _t{"genre": "Rock", "composer": "Harrison", "bpm": 100, "grouping": "Beatles Tracks", + "replaygain_track_gain": "-6.0 dB", "replaygain_track_peak": "0.92"})), + // Stairway To Heaven has track gain but no album gain, to distinguish the two fields "Rock/Led Zeppelin/IV/01 - Stairway To Heaven.flac": ledZepIV(track(1, "Stairway To Heaven", _t{"genre": "Rock;Folk", "composer": "Page/Plant", "bpm": 82, "suffix": "flac", - "bitrate": 900, "samplerate": 44100, "bitdepth": 16})), + "bitrate": 900, "samplerate": 44100, "bitdepth": 16, + "replaygain_track_gain": "-8.25 dB", "replaygain_track_peak": "0.99"})), "Rock/Led Zeppelin/IV/02 - Black Dog.flac": ledZepIV(track(2, "Black Dog", _t{"genre": "Rock;Blues", "composer": "Page/Plant/Jones", "bpm": 150, "suffix": "flac", "bitrate": 900, "samplerate": 44100, "bitdepth": 16})), diff --git a/persistence/e2e/smartplaylist_test.go b/persistence/e2e/smartplaylist_test.go index a844dc982..4966d3b8a 100644 --- a/persistence/e2e/smartplaylist_test.go +++ b/persistence/e2e/smartplaylist_test.go @@ -371,4 +371,61 @@ var _ = Describe("Smart Playlists", func() { Expect(results).To(ConsistOf("Black Dog", "All Along the Watchtower")) }) }) + + // ReplayGain values are stored in nullable media_file columns (not in the tags JSON), so + // isMissing/isPresent translate to IS [NOT] NULL checks on those columns (issue #5584). + Describe("isMissing/isPresent on ReplayGain fields", func() { + It("isMissing finds tracks without album gain", func() { + results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":true}}]}`) + Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What", + "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("isMissing false finds tracks with album gain", func() { + results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":false}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("isPresent finds tracks with album gain", func() { + results := evaluateRule(`{"all":[{"isPresent":{"rgalbumgain":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("isPresent finds tracks with album peak", func() { + results := evaluateRule(`{"all":[{"isPresent":{"rgalbumpeak":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("isMissing distinguishes track gain from album gain", func() { + results := evaluateRule(`{"all":[{"isMissing":{"rgtrackgain":true}}]}`) + Expect(results).To(ConsistOf("Black Dog", "So What", "Bohemian Rhapsody", + "All Along the Watchtower", "We Are the Champions")) + }) + + It("isPresent finds tracks with track gain", func() { + results := evaluateRule(`{"all":[{"isPresent":{"rgtrackgain":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven")) + }) + + It("resolves the replaygain_album_gain alias to the rgalbumgain column", func() { + results := evaluateRule(`{"all":[{"isMissing":{"replaygain_album_gain":true}}]}`) + Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What", + "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("resolves the replaygain_track_gain alias to the rgtrackgain column", func() { + results := evaluateRule(`{"all":[{"isPresent":{"replaygain_track_gain":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven")) + }) + + It("supports numeric comparisons through the replaygain_* alias", func() { + results := evaluateRule(`{"all":[{"gt":{"replaygain_track_gain":-7.5}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("combines isMissing on ReplayGain with other operators", func() { + results := evaluateRule(`{"all":[{"isMissing":{"rgalbumgain":true}},{"is":{"genre":"Blues"}}]}`) + Expect(results).To(ConsistOf("Black Dog", "All Along the Watchtower")) + }) + }) }) diff --git a/persistence/folder_repository.go b/persistence/folder_repository.go index f7bb6a4fe..8fb7f0296 100644 --- a/persistence/folder_repository.go +++ b/persistence/folder_repository.go @@ -7,6 +7,7 @@ import ( "iter" "maps" "os" + "path" "path/filepath" "slices" "strings" @@ -188,6 +189,33 @@ func (r folderRepository) queryFolderUpdateInfo(where And) (map[string]model.Fol return m, nil } +// HasAudioOutsideFolders reports whether any folder in parent's subtree +// (including parent itself) contains audio files and is not one of the given +// folder IDs. LIKE wildcards in the parent path are escaped, so it is always +// matched as a literal prefix. +func (r folderRepository) HasAudioOutsideFolders(parent model.Folder, excludeFolderIDs []string) (bool, error) { + if parent.NumAudioFiles > 0 { + return true, nil + } + parentPath := strings.TrimPrefix(path.Join(parent.Path, parent.Name), "/") + return r.exists(And{ + Eq{"library_id": parent.LibraryID, "missing": false}, + Gt{"num_audio_files": 0}, + NotEq{"id": excludeFolderIDs}, + Or{ + // Direct children have path = parentPath; deeper descendants match the prefix + Eq{"path": parentPath}, + Expr(`path LIKE ? ESCAPE '\'`, escapeLikePrefix(parentPath)+"/%"), + }, + }) +} + +// escapeLikePrefix escapes SQL LIKE wildcards so a string can be used as a +// literal prefix in a LIKE pattern (with ESCAPE '\'). +func escapeLikePrefix(s string) string { + return strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`).Replace(s) +} + func (r folderRepository) Put(f *model.Folder) error { dbf := dbFolder{Folder: f} _, err := r.put(dbf.ID, &dbf) @@ -222,6 +250,18 @@ func (r folderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error) return wrapFolderCursor(cursor), nil } +func (r folderRepository) GetAllWithPlaylists() (model.FolderCursor, error) { + query := r.selectFolder().Where(And{ + Eq{"missing": false}, + Gt{"num_playlists": 0}, + }) + cursor, err := queryWithStableResults[dbFolder](r.sqlRepository, query) + if err != nil { + return nil, err + } + return wrapFolderCursor(cursor), nil +} + func wrapFolderCursor(cursor iter.Seq2[dbFolder, error]) model.FolderCursor { return func(yield func(model.Folder, error) bool) { for f, err := range cursor { diff --git a/persistence/folder_repository_test.go b/persistence/folder_repository_test.go index ebc08fd04..a8945dfee 100644 --- a/persistence/folder_repository_test.go +++ b/persistence/folder_repository_test.go @@ -217,6 +217,67 @@ var _ = Describe("FolderRepository", func() { }) }) + Describe("HasAudioOutsideFolders", func() { + var albumRoot, disc1, disc2 *model.Folder + + // TestHasAudio/Album/ + // ├── CD1/ (audio, belongs to the album) + // └── CD2/ (audio, belongs to the album) + BeforeEach(func() { + albumRoot = model.NewFolder(testLib, "TestHasAudio/Album") + disc1 = model.NewFolder(testLib, "TestHasAudio/Album/CD1") + disc1.NumAudioFiles = 5 + disc2 = model.NewFolder(testLib, "TestHasAudio/Album/CD2") + disc2.NumAudioFiles = 5 + for _, f := range []*model.Folder{albumRoot, disc1, disc2} { + Expect(repo.Put(f)).To(Succeed()) + } + }) + + It("returns false when all audio under the parent belongs to the given folders", func() { + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("returns true when another folder under the parent has audio", func() { + bonus := model.NewFolder(testLib, "TestHasAudio/Album/Bonus") + bonus.NumAudioFiles = 1 + Expect(repo.Put(bonus)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue()) + }) + + It("returns true when the parent itself contains audio files", func() { + albumRoot.NumAudioFiles = 2 + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue()) + }) + + It("ignores audio outside the parent's subtree", func() { + other := model.NewFolder(testLib, "TestHasAudio/Other Album") + other.NumAudioFiles = 10 + Expect(repo.Put(other)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("ignores missing folders", func() { + gone := model.NewFolder(testLib, "TestHasAudio/Album/Gone") + gone.NumAudioFiles = 3 + gone.Missing = true + Expect(repo.Put(gone)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse()) + }) + + It("does not treat LIKE wildcards in the parent path as patterns", func() { + // "TestHas_udio" would LIKE-match "TestHasAudio" if "_" were not escaped + wildcardRoot := model.NewFolder(testLib, "TestHas_udio/Album") + Expect(repo.Put(wildcardRoot)).To(Succeed()) + + Expect(repo.HasAudioOutsideFolders(*wildcardRoot, []string{"none"})).To(BeFalse()) + }) + }) + Describe("wrapFolderCursor", func() { It("does not panic when the cursor yields a dbFolder with nil Folder", func() { // Simulate what queryWithStableResults does on the rows.Err() path: @@ -256,4 +317,36 @@ var _ = Describe("FolderRepository", func() { Expect(folders[0].ID).To(Equal("f1")) }) }) + + Describe("GetAllWithPlaylists", func() { + It("returns all non-missing folders with playlists, ignoring the scan-timestamp gate", func() { + withPls := model.NewFolder(testLib, "TestAllPls/WithPls") + withPls.NumPlaylists = 2 + noPls := model.NewFolder(testLib, "TestAllPls/NoPls") + noPls.NumPlaylists = 0 + missingWithPls := model.NewFolder(testLib, "TestAllPls/Missing") + missingWithPls.NumPlaylists = 1 + missingWithPls.Missing = true + + Expect(repo.Put(withPls)).To(Succeed()) + Expect(repo.Put(noPls)).To(Succeed()) + Expect(repo.Put(missingWithPls)).To(Succeed()) + + // Force the folder's updated_at to the past so GetTouchedWithPlaylists + // (which gates on updated_at > last_scan_at) would NOT return it. + _, err := conn.NewQuery("UPDATE folder SET updated_at = {:t} WHERE id = {:id}"). + Bind(dbx.Params{"t": "2000-01-01 00:00:00", "id": withPls.ID}).Execute() + Expect(err).ToNot(HaveOccurred()) + + var ids []string + cursor, err := repo.GetAllWithPlaylists() + Expect(err).ToNot(HaveOccurred()) + for f, err := range cursor { + Expect(err).ToNot(HaveOccurred()) + ids = append(ids, f.ID) + } + + Expect(ids).To(ConsistOf(withPls.ID)) // only the non-missing folder with playlists + }) + }) }) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 559378262..dd8145eae 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -207,6 +207,40 @@ func (r *mediaFileRepository) GetAll(options ...model.QueryOptions) (model.Media return res.toModels(), nil } +// GetRandom uses two passes so the random sort runs over a narrow rowid index instead of the +// wide media_file row: pick random rowids first, then hydrate only those. +func (r *mediaFileRepository) GetRandom(options ...model.QueryOptions) (model.MediaFiles, error) { + var opt model.QueryOptions + if len(options) > 0 { + opt = options[0] + } + + rowidQuery := Select("media_file.rowid").From(r.tableName) + rowidQuery = r.applyFilters(rowidQuery, model.QueryOptions{Filters: opt.Filters}) + rowidQuery = r.applyLibraryFilter(rowidQuery) + rowidQuery = rowidQuery.OrderBy("random()") + if opt.Max > 0 { + rowidQuery = rowidQuery.Limit(uint64(opt.Max)) + } + + var rowids []int64 + if err := r.queryAllSlice(rowidQuery, &rowids); err != nil { + return nil, err + } + if len(rowids) == 0 { + return model.MediaFiles{}, nil + } + + // Re-shuffle in Phase 2: `WHERE rowid IN (...)` returns rows in ascending rowid order, not + // the random order from Phase 1. Sorting only the (<=Max) hydrated rows is negligible. + sq := r.selectMediaFile().Where(Eq{"media_file.rowid": rowids}).OrderBy("random()") + var res dbMediaFiles + if err := r.queryAll(sq, &res); err != nil { + return nil, err + } + return res.toModels(), nil +} + func (r *mediaFileRepository) GetAllByTags(tag model.TagName, values []string, options ...model.QueryOptions) (model.MediaFiles, error) { placeholders := make([]string, len(values)) args := make([]any, len(values)) diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 2bc9d0267..7989dc25a 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "reflect" "time" "github.com/Masterminds/squirrel" @@ -106,6 +107,102 @@ var _ = Describe("MediaRepository", func() { } }) + Describe("GetRandom", func() { + It("returns the requested number of distinct, fully-hydrated media files", func() { + results, err := mr.GetRandom(model.QueryOptions{Max: 5}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(5)) + + // Each returned row must match its GetAll counterpart exactly — proves Phase 2 + // hydrates full rows (not bare rowids) — and ids must be distinct. + byID := map[string]model.MediaFile{} + all, err := mr.GetAll() + Expect(err).ToNot(HaveOccurred()) + for _, mf := range all { + byID[mf.ID] = mf + } + seen := map[string]bool{} + for _, mf := range results { + expected, ok := byID[mf.ID] + Expect(ok).To(BeTrue(), "returned id must be a real media file") + Expect(mf.Title).To(Equal(expected.Title), "row must be fully hydrated") + Expect(seen[mf.ID]).To(BeFalse(), "no duplicate rows") + seen[mf.ID] = true + } + }) + + It("returns all matching files when Max exceeds the total", func() { + results, err := mr.GetRandom(model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(HaveLen(13)) + }) + + It("honors filters", func() { + results, err := mr.GetRandom(model.QueryOptions{ + Max: 10, + Filters: squirrel.Eq{"media_file.title": "Antenna"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(results).ToNot(BeEmpty()) + for _, mf := range results { + Expect(mf.Title).To(Equal("Antenna")) + } + }) + + It("returns varying results across calls", func() { + // Retry a few times: two random draws of 5 from 13 rows differ with near-certainty. + first, err := mr.GetRandom(model.QueryOptions{Max: 5}) + Expect(err).ToNot(HaveOccurred()) + firstIDs := func() []string { + ids := make([]string, len(first)) + for i, mf := range first { + ids[i] = mf.ID + } + return ids + }() + differed := false + for range 10 { + next, err := mr.GetRandom(model.QueryOptions{Max: 5}) + Expect(err).ToNot(HaveOccurred()) + nextIDs := make([]string, len(next)) + for i, mf := range next { + nextIDs[i] = mf.ID + } + if !reflect.DeepEqual(firstIDs, nextIDs) { + differed = true + break + } + } + Expect(differed).To(BeTrue(), "GetRandom should not return an identical set every call") + }) + + It("randomizes order even when Max exceeds the total", func() { + // Same set of rows every time (all 13), but the order must still be shuffled — + // guards against Phase 2's `rowid IN (...)` returning rows in rowid order. + first, err := mr.GetRandom(model.QueryOptions{Max: 100}) + Expect(err).ToNot(HaveOccurred()) + Expect(first).To(HaveLen(13)) + firstIDs := make([]string, len(first)) + for i, mf := range first { + firstIDs[i] = mf.ID + } + differed := false + for range 10 { + next, err := mr.GetRandom(model.QueryOptions{Max: 100}) + Expect(err).ToNot(HaveOccurred()) + nextIDs := make([]string, len(next)) + for i, mf := range next { + nextIDs[i] = mf.ID + } + if !reflect.DeepEqual(firstIDs, nextIDs) { + differed = true + break + } + } + Expect(differed).To(BeTrue(), "order must vary even when returning all rows") + }) + }) + Describe("Put CreatedAt behavior (#5050)", func() { It("sets CreatedAt to now when inserting a new file with zero CreatedAt", func() { before := time.Now().Add(-time.Second) @@ -652,6 +749,49 @@ var _ = Describe("MediaRepository", func() { _, _ = raw.executeSQL(squirrel.Delete(raw.tableName).Where(squirrel.Eq{"id": missingMediaFile.ID})) }) }) + + Context("empty query (natural order pagination)", func() { + It("returns all non-missing files in natural order", func() { + results, err := mr.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).ToNot(BeEmpty()) + for _, result := range results { + Expect(result.Missing).To(BeFalse()) + } + }) + + It(`treats quoted empty query ("") the same as empty`, func() { + all, err := mr.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + quoted, err := mr.Search(`""`, model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + Expect(quoted).To(HaveLen(len(all))) + }) + + It("paginates without overlaps or gaps", func() { + all, err := mr.Search("", model.QueryOptions{Max: 1000}) + Expect(err).ToNot(HaveOccurred()) + Expect(len(all)).To(BeNumerically(">", 3)) + + var paged model.MediaFiles + pageSize := 3 + for offset := 0; offset < len(all); offset += pageSize { + page, err := mr.Search("", model.QueryOptions{Max: pageSize, Offset: offset}) + Expect(err).ToNot(HaveOccurred()) + paged = append(paged, page...) + } + Expect(paged).To(HaveLen(len(all))) + for i := range all { + Expect(paged[i].ID).To(Equal(all[i].ID), fmt.Sprintf("row %d differs", i)) + } + }) + + It("returns empty page when offset is beyond the total", func() { + results, err := mr.Search("", model.QueryOptions{Max: 10, Offset: 100000}) + Expect(err).ToNot(HaveOccurred()) + Expect(results).To(BeEmpty()) + }) + }) }) Describe("FindByPaths", func() { @@ -781,4 +921,49 @@ var _ = Describe("MediaRepository", func() { Expect(mediafiles[0].ID).To(Equal("mf1")) }) }) + + Describe("BPM and BitDepth nullable round-trip", func() { + It("stores nil BPM and BitDepth as NULL and retrieves them as nil", func() { + newID := id.NewRandom() + mf := model.MediaFile{LibraryID: 1, ID: newID, Path: "test/bpm-nil.mp3"} + Expect(mr.Put(&mf)).To(Succeed()) + + retrieved, err := mr.Get(newID) + Expect(err).ToNot(HaveOccurred()) + Expect(retrieved.BPM).To(BeNil()) + Expect(retrieved.BitDepth).To(BeNil()) + + // Also verify via raw SQL that the columns are truly NULL (not 0) + db := GetDBXBuilder() + var row struct { + BPM *int `db:"bpm"` + BitDepth *int `db:"bit_depth"` + } + err = db.NewQuery("SELECT bpm, bit_depth FROM media_file WHERE id={:id}"). + Bind(dbx.Params{"id": newID}). + One(&row) + Expect(err).ToNot(HaveOccurred()) + Expect(row.BPM).To(BeNil(), "bpm should be stored as NULL in the database") + Expect(row.BitDepth).To(BeNil(), "bit_depth should be stored as NULL in the database") + + _ = mr.Delete(newID) + }) + + It("stores non-nil BPM and BitDepth and retrieves correct values", func() { + newID := id.NewRandom() + bpm := 120 + bitDepth := 24 + mf := model.MediaFile{LibraryID: 1, ID: newID, Path: "test/bpm-set.mp3", BPM: &bpm, BitDepth: &bitDepth} + Expect(mr.Put(&mf)).To(Succeed()) + + retrieved, err := mr.Get(newID) + Expect(err).ToNot(HaveOccurred()) + Expect(retrieved.BPM).ToNot(BeNil()) + Expect(*retrieved.BPM).To(Equal(120)) + Expect(retrieved.BitDepth).ToNot(BeNil()) + Expect(*retrieved.BitDepth).To(Equal(24)) + + _ = mr.Delete(newID) + }) + }) }) diff --git a/persistence/sql_search.go b/persistence/sql_search.go index 43965ebb7..19cbaf24f 100644 --- a/persistence/sql_search.go +++ b/persistence/sql_search.go @@ -1,6 +1,7 @@ package persistence import ( + "fmt" "strings" . "github.com/Masterminds/squirrel" @@ -20,8 +21,10 @@ type searchConfig struct { NaturalOrder string // ORDER BY for empty-query results (e.g. "album.rowid") OrderBy []string // ORDER BY for text search results (e.g. ["name"]) MBIDFields []string // columns to match when query is a UUID - // LibraryFilter overrides the default applyLibraryFilter for FTS Phase 1. - // Needed when library access requires a junction table (e.g. artist → library_artist). + // LibraryFilter overrides the default applyLibraryFilter for the rowid Phase 1 of + // two-phase searches (FTS and empty-query). Needed when library access goes through a + // junction table (e.g. artist → library_artist), whose JOIN can fan out rowids for + // entities in multiple libraries — Phase 1 dedups whenever this is set. LibraryFilter func(sq SelectBuilder) SelectBuilder } @@ -57,8 +60,8 @@ func (r sqlRepository) doSearch(sq SelectBuilder, q string, results any, cfg sea // Empty query (OpenSubsonic `search3?query=""`) — return all in natural order. if q == "" || q == `""` { - sq = sq.OrderBy(cfg.NaturalOrder) - return r.queryAll(sq, results, options) + rowidCore := Select(r.tableName + ".rowid").From(r.tableName).OrderBy(cfg.NaturalOrder) + return r.executeTwoPhase(sq, results, rowidCore, cfg, options) } // MBID search: if query is a valid UUID, search by MBID fields instead @@ -82,6 +85,53 @@ func (r sqlRepository) doSearch(sq SelectBuilder, q string, results any, cfg sea return strategy.execute(r, sq, results, cfg, options) } +// executeTwoPhase runs a search in two phases: +// - Phase 1: rowidCore (strategy-specific FROM/JOINs and ORDER BY) plus the shared search +// contract applied here — non-missing rows only, library access, options.Filters, and +// pagination. Keeping Phase 1 free of the full SELECT's JOINs lets SQLite paginate via a +// covering index; with those JOINs, large offsets degrade to O(offset) join probes — +// multi-second responses on 100k+ libraries. +// - Phase 2: full SELECT with all JOINs, scoped to Phase 1's rowid page. +func (r sqlRepository) executeTwoPhase(sq SelectBuilder, results any, rowidCore SelectBuilder, cfg searchConfig, options model.QueryOptions) error { + rowidQuery := rowidCore. + Where(Eq{r.tableName + ".missing": false}) + if options.Max > 0 { + rowidQuery = rowidQuery.Limit(uint64(options.Max)) + } + if options.Offset > 0 { + rowidQuery = rowidQuery.Offset(uint64(options.Offset)) + } + if cfg.LibraryFilter != nil { + // Junction-table library filters can repeat rowids for entities in multiple + // libraries, which would corrupt offset-based pagination — dedup before paginating. + // (DISTINCT, not GROUP BY: bm25() can't be evaluated in a grouped query.) + rowidQuery = cfg.LibraryFilter(rowidQuery).Distinct() + } else { + rowidQuery = r.applyLibraryFilter(rowidQuery) + } + if options.Filters != nil { + rowidQuery = rowidQuery.Where(options.Filters) + } + return r.hydrateRowidPage(sq, rowidQuery, results) +} + +// hydrateRowidPage joins sq to the ordered rowid set produced by rowidQuery, preserving its +// ordering. rowidQuery must handle pagination itself; sq's LIMIT/OFFSET are stripped. +func (r sqlRepository) hydrateRowidPage(sq SelectBuilder, rowidQuery SelectBuilder, results any) error { + rowidSQL, rowidArgs, err := rowidQuery.ToSql() + if err != nil { + return fmt.Errorf("building rowid query: %w", err) + } + sq = sq.RemoveLimit().RemoveOffset() + rankedSubquery := fmt.Sprintf( + "(SELECT rowid as _rid, row_number() OVER () AS _rn FROM (%s)) AS _ranked", + rowidSQL, + ) + sq = sq.Join(rankedSubquery+" ON "+r.tableName+".rowid = _ranked._rid", rowidArgs...) + sq = sq.OrderBy("_ranked._rn") + return r.queryAll(sq, results) +} + func mbidExpr(tableName, mbid string, mbidFields ...string) Sqlizer { if uuid.Validate(mbid) != nil || len(mbidFields) == 0 { return nil diff --git a/persistence/sql_search_fts.go b/persistence/sql_search_fts.go index b90dc937b..bbae47fe8 100644 --- a/persistence/sql_search_fts.go +++ b/persistence/sql_search_fts.go @@ -284,11 +284,9 @@ func (s *ftsSearch) ToSql() (string, []any, error) { return sql, []any{s.matchExpr}, nil } -// execute runs a two-phase FTS5 search: -// - Phase 1: lightweight rowid query (main table + FTS + library filter) for ranking and pagination. -// - Phase 2: full SELECT with all JOINs, scoped to Phase 1's rowid set. -// -// Complex ORDER BY (function calls, aggregations) are dropped from Phase 1. +// execute runs a two-phase FTS5 search (see executeTwoPhase): Phase 1 here contributes the +// FTS MATCH join and BM25 rank ordering. Complex ORDER BY (function calls, aggregations) are +// dropped from Phase 1. func (s *ftsSearch) execute(r sqlRepository, sq SelectBuilder, dest any, cfg searchConfig, options model.QueryOptions) error { qualifiedOrderBys := []string{s.rankExpr} for _, ob := range cfg.OrderBy { @@ -297,45 +295,11 @@ func (s *ftsSearch) execute(r sqlRepository, sq SelectBuilder, dest any, cfg sea } } - // Phase 1: fresh query — must set LIMIT/OFFSET from options explicitly. - // Mirror applyOptions behavior: Max=0 means no limit, not LIMIT 0. - rowidQuery := Select(s.tableName+".rowid"). + rowidCore := Select(s.tableName+".rowid"). From(s.tableName). Join(s.ftsTable+" ON "+s.ftsTable+".rowid = "+s.tableName+".rowid AND "+s.ftsTable+" MATCH ?", s.matchExpr). - Where(Eq{s.tableName + ".missing": false}). OrderBy(qualifiedOrderBys...) - if options.Max > 0 { - rowidQuery = rowidQuery.Limit(uint64(options.Max)) - } - if options.Offset > 0 { - rowidQuery = rowidQuery.Offset(uint64(options.Offset)) - } - - // Library filter + musicFolderId must be applied here, before pagination. - if cfg.LibraryFilter != nil { - rowidQuery = cfg.LibraryFilter(rowidQuery) - } else { - rowidQuery = r.applyLibraryFilter(rowidQuery) - } - if options.Filters != nil { - rowidQuery = rowidQuery.Where(options.Filters) - } - - rowidSQL, rowidArgs, err := rowidQuery.ToSql() - if err != nil { - return fmt.Errorf("building FTS rowid query: %w", err) - } - - // Phase 2: strip LIMIT/OFFSET from sq (Phase 1 handled pagination), - // join on the ranked rowid set to hydrate with full columns. - sq = sq.RemoveLimit().RemoveOffset() - rankedSubquery := fmt.Sprintf( - "(SELECT rowid as _rid, row_number() OVER () AS _rn FROM (%s)) AS _ranked", - rowidSQL, - ) - sq = sq.Join(rankedSubquery+" ON "+s.tableName+".rowid = _ranked._rid", rowidArgs...) - sq = sq.OrderBy("_ranked._rn") - return r.queryAll(sq, dest) + return r.executeTwoPhase(sq, dest, rowidCore, cfg, options) } // qualifyOrderBy prepends tableName to a simple column name. Returns empty string for diff --git a/release/goreleaser.yml b/release/goreleaser.yml index e5035adda..103f2beaf 100644 --- a/release/goreleaser.yml +++ b/release/goreleaser.yml @@ -114,7 +114,7 @@ release: ## Where to go next? * Read installation instructions on our [website](https://www.navidrome.org/docs/installation/). - * Host Navidrome on [PikaPods](https://www.pikapods.com/pods/navidrome) for a simple cloud solution. + * Host Navidrome on [PikaPods](https://www.pikapods.com/pods/navidrome) or [Danian](https://danian.co/navidrome?nd) for a simple cloud solution. * Reach out on [Discord](https://discord.gg/xh7j7yF), [Reddit](https://www.reddit.com/r/navidrome/) and [Twitter](https://twitter.com/navidrome)! # Add the MSI installers to the release diff --git a/resources/i18n/et.json b/resources/i18n/et.json index d511c1246..b0131f4ea 100644 --- a/resources/i18n/et.json +++ b/resources/i18n/et.json @@ -154,12 +154,12 @@ "currentPassword": "Senine salasõna", "newPassword": "Uus salasõna", "token": "Tunnusluba", - "lastAccessAt": "Viimasti avatud", + "lastAccessAt": "Viimati avatud", "libraries": "Kogumikud" }, "helperTexts": { "name": "Sinu nime muudatused on näha järgmisel sisselogimisel", - "libraries": "Vali selle kasutaja jaoks konkreetsed kogumikus või jäta vaikimisi väärtuse kasutamiseks tühjaks" + "libraries": "Vali selle kasutaja jaoks konkreetsed kogumikud või jäta vaikimisi väärtuse kasutamiseks tühjaks" }, "notifications": { "created": "Kasutaja on lisatud", @@ -413,10 +413,10 @@ }, "ra": { "auth": { - "welcome1": "Aitäh, et paigaldasite Navidrome'i!", + "welcome1": "Aitäh, et paigaldasid Navidrome'i!", "welcome2": "Alustamiseks lisa peakasutaja", "confirmPassword": "Korda salasõna", - "buttonCreateAdmin": "Loo admin", + "buttonCreateAdmin": "Lisa peakasutaja", "auth_check_error": "Jätkamiseks palun logi sisse", "user_menu": "Profiil", "username": "Kasutajanimi", @@ -427,7 +427,7 @@ "insightsCollectionNote": "Navidrome kogub anonüümset kasutustusstatistikat, mille alusel on võimalik projekti paremaks muuta. Klõpsides [siin], saad lugeda lisateavet ning soovi korral sellest kogumisest loobuda" }, "validation": { - "invalidChars": "Palun kasutage ainult tähti ja numbreid", + "invalidChars": "Palun kasuta ainult tähti ja numbreid", "passwordDoesNotMatch": "Salasõnad ei kattu", "required": "Nõutav", "minLength": "Pikkus peab olema vähemalt %{min} tähemärki", @@ -558,8 +558,8 @@ }, "message": { "note": "MÄRGE", - "transcodingDisabled": "Transkodeeringu seadistuse muutmine läbi veebiliidese ei ole turvariskide tõttu saadaval. Kui soovite muuta või lisada transkodeerimisega seotud seadistusi, taaskäivitage server %{config} valikuga.", - "transcodingEnabled": "Navidrome käivitati %{config} valikuga, mis lubab läbi veebiliidese transkodeerimisseadistuste jooksutada süsteemikäsklusi. Turvakaalutlustel on soovitatav kasutada seda valikut ainult transkodeerimisseadete muutmiseks.", + "transcodingDisabled": "Teisendusseadistuste muutmine läbi veebiliidese ei ole turvariskide tõttu saadaval. Kui soovid muuta või lisada teisendamisega seotud seadistusi, taaskäivita server %{config} valikuga.", + "transcodingEnabled": "Navidrome käivitati %{config} valikuga, mis lubab läbi veebiliidese teisendusseadistuste käivitada süsteemikäsklusi. Turvakaalutlustel on soovitatav kasutada seda valikut ainult teisendusvalikute muutmiseks.", "songsAddedToPlaylist": "Lisasin ühe loo esitusloendisse |||| Lisasin %{smart_count} lugu esitusloendisse", "noPlaylistsAvailable": "Pole saadaval", "delete_user_title": "Kustuta kasutaja „%{name}“", @@ -603,13 +603,13 @@ }, "menu": { "library": "Kogumik", - "settings": "Seaded", + "settings": "Seadistused", "version": "Versioon", - "theme": "Teema", + "theme": "Kujundus", "personal": { "name": "Isiklik", "options": { - "theme": "Teema", + "theme": "Kujundus", "language": "Keel", "defaultView": "Vaikimisi vaade", "desktop_notifications": "Teavitused töölaual", diff --git a/resources/i18n/id.json b/resources/i18n/id.json index cdba66663..762ce4ebb 100644 --- a/resources/i18n/id.json +++ b/resources/i18n/id.json @@ -37,7 +37,10 @@ "sampleRate": "Sample rate", "missing": "Hilang", "libraryName": "Pustaka", - "composer": "Komposer" + "composer": "Komposer", + "disc": "Disk %{discNumber}", + "albumGain": "Album gain", + "trackGain": "Trek gain" }, "actions": { "addToQueue": "Tambah ke antrean", @@ -353,7 +356,8 @@ "allUsers": "Izinkan semua pengguna", "selectedUsers": "Pengguna yang dipilih", "allLibraries": "Izinkan semua pustaka", - "selectedLibraries": "Pustaka dipilih" + "selectedLibraries": "Pustaka dipilih", + "allowWriteAccess": "Izinkan akses tulis" }, "sections": { "status": "Status", @@ -398,7 +402,8 @@ "librariesRequired": "Plugin ini membutuhkan akses ke informasi pustaka. Pilih beberapa pustaka yang bisa diakses, atau aktifkan 'Izinkan semua pustaka'.", "requiredHosts": "Hosts diperlukan", "configValidationError": "Validasi konfigurasi gagal:", - "schemaRenderError": "Tidak dapat menampilkan form konfigurasi. Skema plugin mungkin tidak valid." + "schemaRenderError": "Tidak dapat menampilkan form konfigurasi. Skema plugin mungkin tidak valid.", + "allowWriteAccessHelp": "Ketika diaktifkan, plugin dapat mengubah file di direktori pustaka. Bawaannya, plugin hanya memiliki akses read-only" }, "placeholders": { "configKey": "key", @@ -588,7 +593,13 @@ "remove_all_missing_content": "Apa kamu yakin ingin menghapus semua file dari database? Ini akan menghapus permanen dan apapun referensi ke mereka, termasuk hitungan pemutaran dan rating mereka.", "noSimilarSongsFound": "Tidak ada lagu yang serupa ditemukan", "noTopSongsFound": "Tidak ada lagu teratas ditemukan", - "startingInstantMix": "Memuat Mix Instan..." + "startingInstantMix": "Memuat Mix Instan...", + "uploadCover": "Unggah Sampul", + "removeCover": "Hapus Sampul", + "coverUploaded": "Sampul diperbarui", + "coverRemoved": "Sampul dihapus", + "coverUploadError": "Kesalahan mengunggah sampul", + "coverRemoveError": "Kesalahan menghapus sampul" }, "menu": { "library": "Pustaka", @@ -674,7 +685,8 @@ "exportSuccess": "Konfigurasi sudah diekspor ke papan klip dalam bentuk format TOML", "exportFailed": "Gagal menyalin konfigurasi", "devFlagsHeader": "Flag Pengembangan (subyek untuk perubahan/pemindahan)", - "devFlagsComment": "Ini adalan pengaturan eksperimen dan mungkin akan dihapus di versi mendatang" + "devFlagsComment": "Ini adalan pengaturan eksperimen dan mungkin akan dihapus di versi mendatang", + "downloadToml": "Unduh Konfigurasi (TOML)" } }, "activity": { diff --git a/resources/i18n/pt-br.json b/resources/i18n/pt-br.json index d9f29f5d4..b3b3bab2f 100644 --- a/resources/i18n/pt-br.json +++ b/resources/i18n/pt-br.json @@ -4,7 +4,7 @@ "song": { "name": "Música |||| Músicas", "fields": { - "albumArtist": "Artista", + "albumArtist": "Artista do Álbum", "duration": "Duração", "trackNumber": "#", "playCount": "Execuções", @@ -57,7 +57,7 @@ "album": { "name": "Álbum |||| Álbuns", "fields": { - "albumArtist": "Artista", + "albumArtist": "Artista do Álbum", "artist": "Artista", "duration": "Duração", "songCount": "Músicas", diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index d52743966..8ba014235 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -2,6 +2,7 @@ package scanner import ( "context" + "errors" "fmt" "os" "strings" @@ -10,6 +11,7 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" @@ -18,12 +20,13 @@ import ( ) type phasePlaylists struct { - ctx context.Context - scanState *scanState - ds model.DataStore - pls playlists.Playlists - cw artwork.CacheWarmer - refreshed atomic.Uint32 + ctx context.Context + scanState *scanState + ds model.DataStore + pls playlists.Playlists + cw artwork.CacheWarmer + refreshed atomic.Uint32 + pendingImport bool } func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls playlists.Playlists, cw artwork.CacheWarmer) *phasePlaylists { @@ -49,22 +52,41 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error { log.Info(p.ctx, "Playlists will not be imported, AutoImportPlaylists is set to false") return nil } - u, _ := request.UserFrom(p.ctx) - if !u.IsAdmin || u.ID == "" { - log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet, "+ - "Please create an admin user first, and then update the playlists for them to be imported") - return nil + + // Resolve the admin at phase time (the producer runs late in the scan), so an + // admin created while the scan was in progress is picked up. Assigned once, + // before any put() below, so the channel send synchronizes it with the stages. + admin, err := p.ds.User(p.ctx).FindFirstAdmin() + if err != nil && !errors.Is(err, model.ErrNotFound) { + return fmt.Errorf("finding admin user: %w", err) + } + noAdmin := admin == nil || admin.ID == "" + if noAdmin { + return p.deferImport() + } + p.ctx = request.WithUser(p.ctx, *admin) + + // When recovering a deferred import, scan all playlist folders, not just touched ones. + pending, err := p.importPending() + if err != nil { + return fmt.Errorf("checking pending playlist import: %w", err) + } + p.pendingImport = pending + var cursor model.FolderCursor + if p.pendingImport { + cursor, err = p.ds.Folder(p.ctx).GetAllWithPlaylists() + } else { + cursor, err = p.ds.Folder(p.ctx).GetTouchedWithPlaylists() + } + if err != nil { + return fmt.Errorf("loading folders with playlists: %w", err) } count := 0 - cursor, err := p.ds.Folder(p.ctx).GetTouchedWithPlaylists() - if err != nil { - return fmt.Errorf("loading touched folders: %w", err) - } - log.Debug(p.ctx, "Scanner: Checking playlists that may need refresh") + log.Debug(p.ctx, "Scanner: Checking playlists that may need refresh", "pendingImport", p.pendingImport) for folder, err := range cursor { if err != nil { - return fmt.Errorf("loading touched folder: %w", err) + return fmt.Errorf("loading folder with playlists: %w", err) } count++ put(&folder) @@ -78,6 +100,23 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error { return nil } +// deferImport records the pending-import flag so a later scan with an admin can +// import the playlists, and returns an error if the flag can't be persisted (so +// the scan does not complete as successful without recording the recovery). +func (p *phasePlaylists) deferImport() error { + if err := p.ds.Property(p.ctx).Put(consts.PlaylistsImportPendingFlagKey, "1"); err != nil { + return fmt.Errorf("recording pending playlist import: %w", err) + } + log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet. "+ + "They will be imported automatically once an admin user is created.") + return nil +} + +func (p *phasePlaylists) importPending() (bool, error) { + v, err := p.ds.Property(p.ctx).DefaultGet(consts.PlaylistsImportPendingFlagKey, "0") + return v == "1", err +} + func (p *phasePlaylists) stages() []ppl.Stage[*model.Folder] { return []ppl.Stage[*model.Folder]{ ppl.NewStage(p.processPlaylistsInFolder, ppl.Name("process playlists in folder"), ppl.Concurrency(3)), @@ -123,6 +162,11 @@ func (p *phasePlaylists) finalize(err error) error { } else { p.scanState.changesDetected.Store(true) } + if p.pendingImport && err == nil { + if derr := p.ds.Property(p.ctx).Delete(consts.PlaylistsImportPendingFlagKey); derr != nil { + log.Warn(p.ctx, "Scanner: Could not clear pending playlist-import flag", derr) + } + } logF(p.ctx, "Scanner: Finished refreshing playlists", "refreshed", refreshed, err) return err } diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 0e01a7549..49ffc7fb7 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -9,10 +9,10 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -30,14 +30,22 @@ var _ = Describe("phasePlaylists", func() { cw artwork.CacheWarmer ) + var userRepo *tests.MockedUserRepo + var propRepo *tests.MockedPropertyRepo + BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) conf.Server.AutoImportPlaylists = true ctx = context.Background() - ctx = request.WithUser(ctx, model.User{ID: "123", IsAdmin: true}) folderRepo = &mockFolderRepository{} + userRepo = tests.CreateMockUserRepo() + // An admin user exists by default, so playlist import proceeds. + Expect(userRepo.Put(&model.User{ID: "123", UserName: "admin", IsAdmin: true})).To(Succeed()) + propRepo = &tests.MockedPropertyRepo{} ds = &tests.MockDataStore{ - MockedFolder: folderRepo, + MockedFolder: folderRepo, + MockedUser: userRepo, + MockedProperty: propRepo, } pls = &mockPlaylists{} cw = artwork.NoopCacheWarmer() @@ -84,6 +92,81 @@ var _ = Describe("phasePlaylists", func() { Expect(called).To(BeFalse()) Expect(err).To(MatchError(ContainSubstring("error loading folders"))) }) + + It("sets the pending flag and imports nothing when no admin user exists", func() { + // Remove the admin user; produce resolves the admin at phase time. + userRepo.Data = map[string]*model.User{} + folderRepo.SetData(map[*model.Folder]error{ + {Path: "/path/to/folder1"}: nil, + }) + + called := false + err := phase.produce(func(folder *model.Folder) { called = true }) + + Expect(err).ToNot(HaveOccurred()) + Expect(called).To(BeFalse()) + v, _ := propRepo.Get(consts.PlaylistsImportPendingFlagKey) + Expect(v).To(Equal("1")) + }) + + It("returns an error (not a silent defer) on a datastore failure resolving the admin", func() { + userRepo.Error = errors.New("db is locked") + + err := phase.produce(func(folder *model.Folder) {}) + + Expect(err).To(MatchError(ContainSubstring("finding admin user"))) + // Must NOT have set the pending flag on a real error. + _, getErr := propRepo.Get(consts.PlaylistsImportPendingFlagKey) + Expect(getErr).To(HaveOccurred()) + }) + + It("returns an error when the pending flag cannot be persisted", func() { + userRepo.Data = map[string]*model.User{} // no admin -> defer path + propRepo.Error = errors.New("property table unavailable") + + err := phase.produce(func(folder *model.Folder) {}) + + Expect(err).To(MatchError(ContainSubstring("recording pending playlist import"))) + }) + + It("imports all playlist folders when the pending flag is set", func() { + Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed()) + folderRepo.SetAllData(map[*model.Folder]error{ + {Path: "/path/to/folder1"}: nil, + {Path: "/path/to/folder2"}: nil, + }) + // Touched set is empty: proves selection used GetAllWithPlaylists. + folderRepo.SetData(map[*model.Folder]error{}) + + var produced []*model.Folder + err := phase.produce(func(folder *model.Folder) { produced = append(produced, folder) }) + + Expect(err).ToNot(HaveOccurred()) + Expect(produced).To(HaveLen(2)) + Expect(phase.pendingImport).To(BeTrue()) + }) + }) + + Describe("finalize", func() { + It("clears the pending flag after a successful pending import", func() { + Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed()) + phase.pendingImport = true + + Expect(phase.finalize(nil)).To(Succeed()) + + _, err := propRepo.Get(consts.PlaylistsImportPendingFlagKey) + Expect(err).To(HaveOccurred()) // deleted + }) + + It("keeps the pending flag when the import failed", func() { + Expect(propRepo.Put(consts.PlaylistsImportPendingFlagKey, "1")).To(Succeed()) + phase.pendingImport = true + + Expect(phase.finalize(errors.New("boom"))).To(HaveOccurred()) + + v, _ := propRepo.Get(consts.PlaylistsImportPendingFlagKey) + Expect(v).To(Equal("1")) + }) }) Describe("processPlaylistsInFolder", func() { @@ -141,12 +224,13 @@ func (p *mockPlaylists) ImportFromFolder(ctx context.Context, folder *model.Fold type mockFolderRepository struct { model.FolderRepository - data map[*model.Folder]error + data map[*model.Folder]error + allData map[*model.Folder]error } -func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error) { +func cursorFromData(data map[*model.Folder]error) model.FolderCursor { return func(yield func(model.Folder, error) bool) { - for folder, err := range f.data { + for folder, err := range data { if err != nil { if !yield(model.Folder{}, err) { return @@ -157,9 +241,21 @@ func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, er return } } - }, nil + } +} + +func (f *mockFolderRepository) GetTouchedWithPlaylists() (model.FolderCursor, error) { + return cursorFromData(f.data), nil +} + +func (f *mockFolderRepository) GetAllWithPlaylists() (model.FolderCursor, error) { + return cursorFromData(f.allData), nil } func (f *mockFolderRepository) SetData(m map[*model.Folder]error) { f.data = m } + +func (f *mockFolderRepository) SetAllData(m map[*model.Folder]error) { + f.allData = m +} diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 7ce8de2e6..12a7c95e0 100644 --- a/server/e2e/e2e_suite_test.go +++ b/server/e2e/e2e_suite_test.go @@ -133,7 +133,7 @@ func buildTestFS() storagetest.FakeFS { // Note: "musicbrainz_trackid" is an alias for the musicbrainz_recordingid tag (populates MbzRecordingID), // "musicbrainz_releasetrackid" is an alias for the musicbrainz_trackid tag (populates MbzReleaseTrackID). "Rock/The Beatles/Abbey Road/01 - Come Together.mp3": abbeyRoad(track(1, "Come Together", - _t{"musicbrainz_releasetrackid": mbidComeTogether, "musicbrainz_trackid": mbidComeTogetherRec})), + _t{"musicbrainz_releasetrackid": mbidComeTogether, "musicbrainz_trackid": mbidComeTogetherRec, "bpm": 120})), "Rock/The Beatles/Abbey Road/02 - Something.mp3": abbeyRoad(track(2, "Something", _t{"musicbrainz_releasetrackid": mbidSomething, "musicbrainz_trackid": mbidSomethingRec})), // Rock / The Beatles / Help! (no MBIDs) diff --git a/server/e2e/subsonic_playlists_test.go b/server/e2e/subsonic_playlists_test.go index 466e68cf0..467535df7 100644 --- a/server/e2e/subsonic_playlists_test.go +++ b/server/e2e/subsonic_playlists_test.go @@ -646,5 +646,43 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { stringResp := doReq("getPlaylist", "id", stringPls.ID) Expect(stringResp.Playlist.SongCount).To(Equal(boolResp.Playlist.SongCount)) }) + + DescribeTable("isMissing/isPresent partition all songs for nullable column fields", + func(fieldName string) { + allPls := &model.Playlist{ + Name: "All Songs " + fieldName, + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.Contains{"title": ""}}, + } + Expect(ds.Playlist(ctx).Put(allPls)).To(Succeed()) + missingPls := &model.Playlist{ + Name: "Missing " + fieldName, + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{fieldName: true}}}, + } + Expect(ds.Playlist(ctx).Put(missingPls)).To(Succeed()) + presentPls := &model.Playlist{ + Name: "Present " + fieldName, + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsPresent{fieldName: true}}}, + } + Expect(ds.Playlist(ctx).Put(presentPls)).To(Succeed()) + + allResp := doReq("getPlaylist", "id", allPls.ID) + missingResp := doReq("getPlaylist", "id", missingPls.ID) + presentResp := doReq("getPlaylist", "id", presentPls.ID) + + Expect(allResp.Status).To(Equal(responses.StatusOK)) + Expect(allResp.Playlist.SongCount).To(BeNumerically(">", int32(0))) + Expect(missingResp.Playlist.SongCount + presentResp.Playlist.SongCount). + To(Equal(allResp.Playlist.SongCount)) + }, + Entry("bpm", "bpm"), + Entry("bitdepth", "bitdepth"), + Entry("lyrics", "lyrics"), + Entry("mbz_recording_id", "mbz_recording_id"), + Entry("album", "album"), + Entry("comment", "comment"), + ) }) }) diff --git a/server/e2e/subsonic_transcode_test.go b/server/e2e/subsonic_transcode_test.go index ae3d6208c..afe7d52ca 100644 --- a/server/e2e/subsonic_transcode_test.go +++ b/server/e2e/subsonic_transcode_test.go @@ -159,11 +159,22 @@ var _ = Describe("Transcode Endpoints", Ordered, func() { Expect(ds.Player(ctx).Put(player)).To(Succeed()) } + setPlayerForcedFormat := func(format string) { + doReq("ping") + player, err := ds.Player(ctx).FindMatch(adminUser.ID, "test-client", "") + Expect(err).ToNot(HaveOccurred()) + trc, err := ds.Transcoding(ctx).FindByFormat(format) + Expect(err).ToNot(HaveOccurred()) + player.TranscodingId = trc.ID + Expect(ds.Player(ctx).Put(player)).To(Succeed()) + } + AfterEach(func() { // Reset player MaxBitRate to 0 after each test player, err := ds.Player(ctx).FindMatch(adminUser.ID, "test-client", "") if err == nil { player.MaxBitRate = 0 + player.TranscodingId = "" _ = ds.Player(ctx).Put(player) } }) @@ -396,30 +407,34 @@ var _ = Describe("Transcode Endpoints", Ordered, func() { }) }) - Describe("player MaxBitRate cap is ignored", func() { - It("allows direct play even when source bitrate exceeds player MaxBitRate", func() { + Describe("player MaxBitRate cap is enforced", func() { + It("forces transcode when source bitrate exceeds player MaxBitRate", func() { setPlayerMaxBitRate(320) // 320 kbps cap - // FLAC is 900kbps, player cap is 320, but getTranscodeDecision - // ignores server-side overrides — client profiles are used as-is + // FLAC is 900kbps. Player cap (320) < source → direct play is + // rejected and the file is transcoded down. resp := doPostReq("getTranscodeDecision", flacAndMp3Client, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) - Expect(resp.TranscodeDecision.CanDirectPlay).To(BeTrue()) + Expect(resp.TranscodeDecision.CanDirectPlay).To(BeFalse()) + Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) + Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) + // Target bitrate is capped at the player MaxBitRate (320kbps). + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) }) - It("uses only client limit, not player MaxBitRate", func() { + It("uses the player cap when it is more restrictive than the client limit", func() { setPlayerMaxBitRate(192) // 192 kbps player cap - // Client caps at 320kbps (bitrateCapClient), player is more restrictive at 192 - // but getTranscodeDecision ignores player cap → client limit (320kbps) applies + // Client caps at 320kbps (bitrateCapClient); player is more + // restrictive at 192 → player cap wins. resp := doPostReq("getTranscodeDecision", bitrateCapClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - // Only client limit (320kbps) applies → 320000 bps - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) + // Player cap (192kbps) applies → 192000 bps. + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(192000))) }) }) @@ -475,38 +490,73 @@ var _ = Describe("Transcode Endpoints", Ordered, func() { }) }) - Describe("player MaxBitRate is ignored by getTranscodeDecision", func() { - It("does not inject maxAudioBitrate from player cap", func() { + Describe("player MaxBitRate injected by getTranscodeDecision", func() { + It("injects the player cap as the transcode target when the client declares none", func() { setPlayerMaxBitRate(320) - // opusTranscodeClient has no client bitrate limits - // Player cap is 320, but getTranscodeDecision ignores it - // FLAC (900kbps) → can't direct play → transcode to opus using format default + // opusTranscodeClient has no client bitrate limits. The player + // cap (320) is injected, so FLAC (900kbps) → opus is capped at 320. resp := doPostReq("getTranscodeDecision", opusTranscodeClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) Expect(resp.TranscodeDecision.TranscodeStream.Codec).To(Equal("opus")) - // Bitrate should be opus format default (128kbps), not player cap (320kbps) - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(128000))) + // Bitrate is the player cap (320kbps), not the opus format default. + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) }) - It("uses only client maxTranscodingAudioBitrate, ignoring player cap", func() { + It("keeps the lower client maxTranscodingAudioBitrate over a higher player cap", func() { setPlayerMaxBitRate(320) - // maxTranscodeBitrateClient: maxTranscodingAudioBitrate=192000 (192kbps) - // Player cap is 320, but getTranscodeDecision ignores it - // Only client maxTranscodingAudioBitrate=192 applies + // maxTranscodeBitrateClient: maxTranscodingAudioBitrate=192000 (192kbps). + // Player cap (320) is higher → the lower client limit wins. resp := doPostReq("getTranscodeDecision", maxTranscodeBitrateClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - // maxTranscodingAudioBitrate=192 → 192000 bps + // Client limit (192kbps) wins → 192000 bps. Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(192000))) }) }) + + Describe("player forced format", func() { + It("transcodes a FLAC to the forced opus format when the client supports it", func() { + setPlayerForcedFormat("opus") + + resp := doPostReq("getTranscodeDecision", opusTranscodeClient, "mediaId", flacTrackID, "mediaType", "song") + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TranscodeDecision).ToNot(BeNil()) + Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) + Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) + Expect(resp.TranscodeDecision.TranscodeStream.Codec).To(Equal("opus")) + }) + + It("falls back to negotiation when the client does not support the forced format", func() { + setPlayerForcedFormat("opus") + + resp := doPostReq("getTranscodeDecision", mp3OnlyClient, "mediaId", flacTrackID, "mediaType", "song") + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TranscodeDecision).ToNot(BeNil()) + Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) + Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) + Expect(resp.TranscodeDecision.TranscodeStream.Container).To(Equal("mp3")) + }) + + It("applies maxBitRate on top of the forced format", func() { + setPlayerForcedFormat("opus") + setPlayerMaxBitRate(96) + + resp := doPostReq("getTranscodeDecision", opusTranscodeClient, "mediaId", flacTrackID, "mediaType", "song") + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TranscodeDecision).ToNot(BeNil()) + Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) + Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) + Expect(resp.TranscodeDecision.TranscodeStream.Codec).To(Equal("opus")) + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(96000))) + }) + }) }) Describe("getTranscodeStream", func() { diff --git a/server/subsonic/album_lists.go b/server/subsonic/album_lists.go index 0d82c8be9..24bbca960 100644 --- a/server/subsonic/album_lists.go +++ b/server/subsonic/album_lists.go @@ -240,10 +240,11 @@ func (api *Router) GetRandomSongs(r *http.Request) (*responses.Subsonic, error) if err != nil { return nil, err } - opts := filter.SongsByRandom(genre, fromYear, toYear) + opts := filter.SongsByGenreAndYearRange(genre, fromYear, toYear) opts = filter.ApplyLibraryFilter(opts, musicFolderIds) + opts.Max = size - songs, err := api.getSongs(r.Context(), 0, size, opts) + songs, err := api.ds.MediaFile(r.Context()).GetRandom(opts) if err != nil { log.Error(r, "Error retrieving random songs", err) return nil, err diff --git a/server/subsonic/filter/filters.go b/server/subsonic/filter/filters.go index 856870a6c..c3710394f 100644 --- a/server/subsonic/filter/filters.go +++ b/server/subsonic/filter/filters.go @@ -90,10 +90,8 @@ func SongsByAlbum(albumId string) Options { }) } -func SongsByRandom(genre string, fromYear, toYear int) Options { - options := Options{ - Sort: "random()", - } +func SongsByGenreAndYearRange(genre string, fromYear, toYear int) Options { + options := Options{} ff := And{} if genre != "" { ff = append(ff, filterByGenre(genre)) diff --git a/server/subsonic/helpers.go b/server/subsonic/helpers.go index 565b064a2..c75d38fdc 100644 --- a/server/subsonic/helpers.go +++ b/server/subsonic/helpers.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" + "github.com/navidrome/navidrome/utils/gg" "github.com/navidrome/navidrome/utils/number" "github.com/navidrome/navidrome/utils/req" "github.com/navidrome/navidrome/utils/slice" @@ -250,7 +251,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op } child.Comment = mf.Comment child.SortName = sortName(mf.SortTitle, mf.OrderTitle) - child.BPM = int32(mf.BPM) + child.BPM = int32(gg.V(mf.BPM)) child.MediaType = responses.MediaTypeSong child.MusicBrainzId = mf.MbzRecordingID child.Isrc = mf.Tags.Values(model.TagISRC) @@ -262,7 +263,7 @@ func osChildFromMediaFile(ctx context.Context, mf model.MediaFile) *responses.Op } child.ChannelCount = int32(mf.Channels) child.SamplingRate = int32(mf.SampleRate) - child.BitDepth = int32(mf.BitDepth) + child.BitDepth = int32(gg.V(mf.BitDepth)) child.Genres = toItemGenres(mf.Genres) child.Moods = mf.Tags.Values(model.TagMood) child.Groupings = mf.Tags.Values(model.TagGrouping) diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index 4e494b324..511db2b85 100644 --- a/server/subsonic/transcode.go +++ b/server/subsonic/transcode.go @@ -11,6 +11,7 @@ import ( "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/req" ) @@ -278,6 +279,28 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) return stream.IsAACCodec(p.Container) }) + // Honor the player's forced transcoding format, falling back to normal + // negotiation when the client can't play it (issue #5583). + if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { + if !clientInfo.ForceFormat(trc.TargetFormat) { + clientName := clientInfo.Name + if player, ok := request.PlayerFrom(ctx); ok && player.Client != "" { + clientName = player.Client + } + log.Debug(ctx, "Player forced format not supported by client; falling back to negotiation", + "forcedFormat", trc.TargetFormat, "client", clientName) + } + } + + // 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) + } + // Get media file mf, err := api.ds.MediaFile(ctx).Get(mediaID) if err != nil { @@ -370,6 +393,7 @@ func (api *Router) GetTranscodeStream(w http.ResponseWriter, r *http.Request) (* if err != nil { switch { case errors.Is(err, stream.ErrTokenInvalid), errors.Is(err, stream.ErrTokenStale): + log.Warn(ctx, "Invalid or stale transcode token", "mediaID", mediaID, err) http.Error(w, "Gone", http.StatusGone) default: log.Error(ctx, "Error validating transcode params", err) diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index adc7b7600..7e36ab243 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -9,6 +9,7 @@ import ( "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -205,7 +206,7 @@ var _ = Describe("Transcode endpoints", func() { It("includes transcode stream when transcoding", func() { mockMFRepo.SetData(model.MediaFiles{ - {ID: "song-2", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}, + {ID: "song-2", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: new(24)}, }) mockTD.decision = &stream.TranscodeDecision{ MediaID: "song-2", @@ -234,6 +235,143 @@ var _ = Describe("Transcode endpoints", func() { Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) Expect(resp.TranscodeDecision.TranscodeStream.Container).To(Equal("mp3")) }) + + Describe("player MaxBitRate cap", func() { + withPlayer := func(r *http.Request, maxBitRate int) *http.Request { + ctx := request.WithPlayer(r.Context(), model.Player{Client: "NavidromeUI", MaxBitRate: maxBitRate}) + return r.WithContext(ctx) + } + + BeforeEach(func() { + mockMFRepo.SetData(model.MediaFiles{ + {ID: "song-1", Suffix: "flac", Codec: "FLAC", BitRate: 900, Channels: 2, SampleRate: 44100}, + }) + mockTD.decision = &stream.TranscodeDecision{MediaID: "song-1", CanDirectPlay: true} + mockTD.token = "token" + }) + + It("caps client MaxAudioBitrate at the player MaxBitRate when client declares none", func() { + body := `{"directPlayProfiles":[{"containers":["flac"],"protocols":["http"]}]}` + r := withPlayer(newJSONPostRequest("mediaId=song-1&mediaType=song", body), 320) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient).ToNot(BeNil()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(320)) + Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(320)) + }) + + It("does not raise a lower client-declared limit", func() { + // Client declares 192 kbps (192000 bps); player cap is 320 — client wins. + body := `{"maxAudioBitrate":192000,"directPlayProfiles":[{"containers":["flac"],"protocols":["http"]}]}` + r := withPlayer(newJSONPostRequest("mediaId=song-1&mediaType=song", body), 320) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(192)) + }) + + It("lowers a higher client-declared limit to the player cap", func() { + // Client declares 320 kbps (320000 bps); player cap is 192 — player wins. + body := `{"maxAudioBitrate":320000,"directPlayProfiles":[{"containers":["flac"],"protocols":["http"]}]}` + r := withPlayer(newJSONPostRequest("mediaId=song-1&mediaType=song", body), 192) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(192)) + Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(192)) + }) + + It("does nothing when no player is in context", func() { + body := `{"maxAudioBitrate":320000,"directPlayProfiles":[{"containers":["flac"],"protocols":["http"]}]}` + r := newJSONPostRequest("mediaId=song-1&mediaType=song", body) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(320)) + }) + + It("does nothing when player MaxBitRate is 0", func() { + body := `{"maxAudioBitrate":320000,"directPlayProfiles":[{"containers":["flac"],"protocols":["http"]}]}` + r := withPlayer(newJSONPostRequest("mediaId=song-1&mediaType=song", body), 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(320)) + }) + }) + + Describe("player forced format", func() { + withForcedFormat := func(r *http.Request, format string, maxBitRate int) *http.Request { + ctx := r.Context() + ctx = request.WithTranscoding(ctx, model.Transcoding{TargetFormat: format}) + if maxBitRate > 0 { + ctx = request.WithPlayer(ctx, model.Player{Client: "NavidromeUI", MaxBitRate: maxBitRate}) + } + return r.WithContext(ctx) + } + + BeforeEach(func() { + mockMFRepo.SetData(model.MediaFiles{ + {ID: "song-1", Suffix: "flac", Codec: "FLAC", BitRate: 900, Channels: 2, SampleRate: 44100}, + }) + mockTD.decision = &stream.TranscodeDecision{MediaID: "song-1", CanTranscode: true} + mockTD.token = "token" + }) + + It("forces a supported format and clears direct play", func() { + body := `{"directPlayProfiles":[{"containers":["flac"],"audioCodecs":["flac"],"protocols":["http"]}], + "transcodingProfiles":[{"container":"ogg","audioCodec":"opus","protocol":"http"}, + {"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedFormat(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "opus", 0) + + _, err := router.GetTranscodeDecision(w, r) + + 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()) + }) + + It("falls back to negotiation when the forced format is unsupported", func() { + // Forced format is opus, but the client only declares mp3 and flac. + // Should fall back to negotiating among the client's own profiles. + body := `{"directPlayProfiles":[{"containers":["flac"],"audioCodecs":["flac"],"protocols":["http"]}], + "transcodingProfiles":[ + {"container":"flac","audioCodec":"flac","protocol":"http"}, + {"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedFormat(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "opus", 0) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + // Profiles left intact for normal negotiation (forced format not applied). + Expect(mockTD.capturedClient.TranscodingProfiles).To(HaveLen(2)) + Expect(mockTD.capturedClient.DirectPlayProfiles).ToNot(BeEmpty()) + }) + + It("applies the maxBitRate cap on top of the forced format", func() { + // Client supports opus + mp3; forced format opus must be selected, + // and the maxBitRate cap applied on top. + body := `{"transcodingProfiles":[ + {"container":"ogg","audioCodec":"opus","protocol":"http"}, + {"container":"mp3","audioCodec":"mp3","protocol":"http"}]}` + r := withForcedFormat(newJSONPostRequest("mediaId=song-1&mediaType=song", body), "opus", 128) + + _, err := router.GetTranscodeDecision(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(mockTD.capturedClient.TranscodingProfiles).To(HaveLen(1)) + Expect(mockTD.capturedClient.TranscodingProfiles[0].AudioCodec).To(Equal("opus")) + Expect(mockTD.capturedClient.MaxAudioBitrate).To(Equal(128)) + Expect(mockTD.capturedClient.MaxTranscodingAudioBitrate).To(Equal(128)) + }) + }) }) Describe("GetTranscodeStream", func() { diff --git a/tests/mock_mediafile_repo.go b/tests/mock_mediafile_repo.go index 01eacae30..f15ba1bc6 100644 --- a/tests/mock_mediafile_repo.go +++ b/tests/mock_mediafile_repo.go @@ -98,6 +98,17 @@ func (m *MockMediaFileRepo) GetAll(qo ...model.QueryOptions) (model.MediaFiles, return result, nil } +func (m *MockMediaFileRepo) GetRandom(qo ...model.QueryOptions) (model.MediaFiles, error) { + res, err := m.GetAll(qo...) + if err != nil { + return nil, err + } + if len(qo) > 0 && qo[0].Max > 0 && len(res) > qo[0].Max { + res = res[:qo[0].Max] + } + return res, nil +} + func (m *MockMediaFileRepo) Put(mf *model.MediaFile) error { if m.Err { return errors.New("error") diff --git a/tests/mock_user_repo.go b/tests/mock_user_repo.go index 7c7dadbc4..2d6ff3c02 100644 --- a/tests/mock_user_repo.go +++ b/tests/mock_user_repo.go @@ -57,6 +57,18 @@ func (u *MockedUserRepo) FindByUsernameWithPassword(username string) (*model.Use return u.FindByUsername(username) } +func (u *MockedUserRepo) FindFirstAdmin() (*model.User, error) { + if u.Error != nil { + return nil, u.Error + } + for _, usr := range u.Data { + if usr.IsAdmin { + return usr, nil + } + } + return nil, model.ErrNotFound +} + func (u *MockedUserRepo) Get(id string) (*model.User, error) { if u.Error != nil { return nil, u.Error diff --git a/ui/public/fonts/Unbounded-Variable.woff2 b/ui/public/fonts/Unbounded-Variable.woff2 index 96d8ff5fa..a2f74491f 100644 Binary files a/ui/public/fonts/Unbounded-Variable.woff2 and b/ui/public/fonts/Unbounded-Variable.woff2 differ diff --git a/ui/src/App.jsx b/ui/src/App.jsx index 35eaee3eb..d10aa5a33 100644 --- a/ui/src/App.jsx +++ b/ui/src/App.jsx @@ -1,7 +1,12 @@ import ReactGA from 'react-ga' import { Provider } from 'react-redux' import { createHashHistory } from 'history' -import { Admin as RAAdmin, Resource } from 'react-admin' +import { + Admin as RAAdmin, + Resource, + useSetLocale, + useRefresh, +} from 'react-admin' import { HotKeys } from 'react-hotkeys' import dataProvider from './dataProvider' import authProvider from './authProvider' @@ -36,7 +41,7 @@ import { transcodingReducer, } from './reducers' import createAdminStore from './store/createAdminStore' -import { i18nProvider } from './i18n' +import { i18nProvider, retrieveTranslation } from './i18n' import config, { shareInfo } from './config' import { keyMap } from './hotkeys' import useChangeThemeColor from './useChangeThemeColor' @@ -44,6 +49,7 @@ import SharePlayer from './share/SharePlayer' import { HTML5Backend } from 'react-dnd-html5-backend' import { DndProvider } from 'react-dnd' import missing from './missing/index.js' +import { useEffect } from 'react' const history = createHashHistory() @@ -84,6 +90,24 @@ const App = () => ( ) const Admin = (props) => { + const setLocale = useSetLocale() + const refresh = useRefresh() + useEffect(() => { + if (config.defaultLanguage !== '' && !localStorage.getItem('locale')) { + retrieveTranslation(config.defaultLanguage) + .then(() => setLocale(config.defaultLanguage)) + .then(() => { + localStorage.setItem('locale', config.defaultLanguage) + refresh(true) + }) + .catch((e) => { + // eslint-disable-next-line no-console + console.error( + 'Cannot load language "' + config.defaultLanguage + '": ' + e, + ) + }) + } + }, [setLocale, refresh]) useChangeThemeColor() /* eslint-disable react/jsx-key */ return ( diff --git a/ui/src/layout/Login.jsx b/ui/src/layout/Login.jsx index a84e01f4d..a7763cff3 100644 --- a/ui/src/layout/Login.jsx +++ b/ui/src/layout/Login.jsx @@ -1,4 +1,4 @@ -import React, { useState, useCallback, useEffect } from 'react' +import React, { useState, useCallback } from 'react' import PropTypes from 'prop-types' import { Field, Form } from 'react-final-form' import { useDispatch } from 'react-redux' @@ -13,8 +13,6 @@ import { createMuiTheme, useLogin, useNotify, - useRefresh, - useSetLocale, useTranslate, useVersion, } from 'react-admin' @@ -24,7 +22,6 @@ import Notification from './Notification' import useCurrentTheme from '../themes/useCurrentTheme' import config from '../config' import { clearQueue } from '../actions' -import { retrieveTranslation } from '../i18n' import { INSIGHTS_DOC_URL } from '../consts.js' const useStyles = makeStyles( @@ -407,27 +404,8 @@ Login.propTypes = { // the right theme const LoginWithTheme = (props) => { const theme = useCurrentTheme() - const setLocale = useSetLocale() - const refresh = useRefresh() const version = useVersion() - useEffect(() => { - if (config.defaultLanguage !== '' && !localStorage.getItem('locale')) { - retrieveTranslation(config.defaultLanguage) - .then(() => { - setLocale(config.defaultLanguage).then(() => { - localStorage.setItem('locale', config.defaultLanguage) - }) - refresh(true) - }) - .catch((e) => { - throw new Error( - 'Cannot load language "' + config.defaultLanguage + '": ' + e, - ) - }) - } - }, [refresh, setLocale]) - return ( diff --git a/ui/src/themes/nautiline.js b/ui/src/themes/nautiline.js index 65ded5fc5..0c13dc0ec 100644 --- a/ui/src/themes/nautiline.js +++ b/ui/src/themes/nautiline.js @@ -627,7 +627,6 @@ const NautilineTheme = { root: { [`@media (max-width: ${breakpoints.xs}px)`]: { padding: '0.7em', - width: '100%', minWidth: 'unset', }, },