From da56df3160aa0d0c452d5ad66b7154919e335c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 13 Jun 2026 13:15:20 -0400 Subject: [PATCH 1/3] feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable in the criteria field map, then extend missingExpr to handle string columns where absence is encoded as NULL or empty string (plus '[]' for lyrics). The Numeric/Boolean path (ReplayGain) is preserved via an explicit type check. * refactor(model): make MediaFile BPM and BitDepth nullable pointers Convert BPM and BitDepth fields in model.MediaFile from int to *int so that 'tag absent' is distinguishable from zero. The metadata mapper now uses NullableFloat for BPM (nil when absent or zero/unparseable) and only sets BitDepth when the audio property is non-zero (lossy codecs report 0). All read sites use gg.V() for zero-fallback deref so Subsonic API output and transcoding behaviour are byte-identical to before. The persistence layer bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0 back to nil on read in PostMapArgs/PostScan; a later migration task will drop those constraints. Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil *int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue, so no files will be spuriously re-imported after this change. Extra files touched beyond the plan's list: core/stream/legacy_client_test.go (BitDepth in model.MediaFile literals), persistence/mediafile_repository.go (NOT NULL bridge). * test(model): pin pre-conversion golden hashes for BPM/BitDepth * feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth * feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL Drop the NOT NULL constraint on media_file.bpm and bit_depth via a lossless migration that converts legacy 0-means-absent values to real NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging the old NOT NULL columns to the *int model fields. Add round-trip persistence tests asserting NULL storage for nil pointers and correct value round-trip for non-nil pointers. * test(e2e): verify isMissing/isPresent partition for nullable fields Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id: for each field, isMissing + isPresent song counts must equal the total library count, proving the nullable-column SQL is exhaustive and correct. * test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial * fix(model): omit bitDepth from JSON when absent instead of emitting null * feat(smartplaylist): support isMissing/isPresent on more string fields Enable isMissing/isPresent operators for album, comment, catalognumber, discsubtitle, albumcomment, sorttitle, sortalbum, sortartist, sortalbumartist, and explicitstatus by marking them Nullable in fieldMap. * refactor(smartplaylist): unify missingExpr column logic into one flow Collapse the numeric/string fork in missingExpr into a single empties-driven loop (numeric/boolean fields simply have no empties), and replace the duplicated IsTag/IsRole guard with a three-way switch that expresses the dispatch model once. No SQL semantics change for string fields; numeric/boolean fields now emit a single-element Or/And which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare `col IS NULL`) — update the affected test expectations accordingly. --- core/stream/decider.go | 3 +- core/stream/decider_test.go | 65 ++++++++++--------- core/stream/legacy_client_test.go | 14 ++-- ...60612222838_make_bpm_bitdepth_nullable.sql | 35 ++++++++++ model/criteria/fields.go | 52 ++++++++------- model/criteria/fields_test.go | 12 ++++ model/mediafile.go | 7 +- model/mediafile_test.go | 21 +++++- model/metadata/map_mediafile.go | 10 ++- model/metadata/map_mediafile_test.go | 28 ++++++++ persistence/criteria_sql.go | 51 ++++++++++----- persistence/criteria_sql_test.go | 62 ++++++++++++++++-- persistence/mediafile_repository_test.go | 45 +++++++++++++ server/e2e/e2e_suite_test.go | 2 +- server/e2e/subsonic_playlists_test.go | 38 +++++++++++ server/subsonic/helpers.go | 5 +- server/subsonic/transcode_test.go | 2 +- 17 files changed, 352 insertions(+), 100 deletions(-) create mode 100644 db/migrations/20260612222838_make_bpm_bitdepth_nullable.sql 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_test.go b/core/stream/legacy_client_test.go index ce7b38650..2ddd74ce0 100644 --- a/core/stream/legacy_client_test.go +++ b/core/stream/legacy_client_test.go @@ -138,7 +138,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 +147,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 +169,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 +179,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 +259,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 +270,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) @@ -332,7 +332,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/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 36206712f..5c9ec898d 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -4,12 +4,14 @@ import "strings" // FieldInfo contains semantic metadata about a criteria field. type FieldInfo struct { - Alias string // If set, this field is a backward-compat alias for another canonical name - IsTag bool - IsRole bool - Numeric bool - Boolean bool - Nullable bool // If set, this column field can be NULL, so isMissing/isPresent are supported on it + Alias string // If set, this field is a backward-compat alias for another canonical name + IsTag bool + 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.yaml that resolves to this field name string // Canonical name, populated by LookupField from the map key @@ -22,7 +24,7 @@ func (f FieldInfo) Name() string { var fieldMap = map[string]FieldInfo{ "title": {}, - "album": {}, + "album": {Nullable: true}, "hascoverart": {Boolean: true}, "tracknumber": {}, "discnumber": {}, @@ -35,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": {}, @@ -75,12 +77,12 @@ 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": {}, + "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}, diff --git a/model/criteria/fields_test.go b/model/criteria/fields_test.go index 3dc0c7b90..2367101a8 100644 --- a/model/criteria/fields_test.go +++ b/model/criteria/fields_test.go @@ -75,5 +75,17 @@ var _ = Describe("fields", func() { 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/mediafile.go b/model/mediafile.go index 6be8402ae..718f0443d 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" 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 824cad7c2..966a545be 100644 --- a/model/metadata/map_mediafile.go +++ b/model/metadata/map_mediafile.go @@ -35,7 +35,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() @@ -63,7 +67,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 16142f526..75a7ed358 100644 --- a/model/metadata/map_mediafile_test.go +++ b/model/metadata/map_mediafile_test.go @@ -117,4 +117,32 @@ var _ = Describe("ToMediaFile", func() { Expect(actual).To(Equal(expected)) }) }) + + 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/criteria_sql.go b/persistence/criteria_sql.go index ee0baca18..fa769ef40 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"}, @@ -218,30 +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 && !info.Nullable { - return nil, fmt.Errorf("isMissing/isPresent operator is not supported for field: %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 - // Nullable column fields (e.g. ReplayGain) are stored in dedicated columns, not in the tags - // JSON, so "missing" maps to a NULL check on the column rather than a json_tree lookup. - if info.Nullable && !info.IsTag && !info.IsRole { - col, ok := fieldExpr(info.Name()) - if !ok || col == "" { + 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) } - if negate { - return squirrel.Eq{col: nil}, nil + col := f.expr + var empties []string + if !info.Numeric && !info.Boolean { + empties = append([]string{""}, f.emptyValues...) } - return squirrel.NotEq{col: nil}, nil + 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) } - - return jsonExpr(info, nil, negate), nil } func mapExpr(values map[string]any, makeCond func(map[string]any) squirrel.Sqlizer, negateJSON bool) (squirrel.Sqlizer, error) { diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index 8e801a703..0257fa0cf 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -87,18 +87,68 @@ var _ = Describe("Smart playlist criteria SQL", func() { "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"), + "(media_file.rg_album_gain IS NULL)"), Entry("isMissing rgAlbumGain [false]", criteria.IsMissing{"rgAlbumGain": false}, - "media_file.rg_album_gain IS NOT NULL"), + "(media_file.rg_album_gain IS NOT NULL)"), Entry("isPresent rgTrackPeak [true]", criteria.IsPresent{"rgTrackPeak": true}, - "media_file.rg_track_peak IS NOT NULL"), + "(media_file.rg_track_peak IS NOT NULL)"), Entry("isPresent rgTrackPeak [false]", criteria.IsPresent{"rgTrackPeak": false}, - "media_file.rg_track_peak IS NULL"), + "(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"), + "(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"), + "(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() { diff --git a/persistence/mediafile_repository_test.go b/persistence/mediafile_repository_test.go index 532e9c10f..4c2363e43 100644 --- a/persistence/mediafile_repository_test.go +++ b/persistence/mediafile_repository_test.go @@ -824,4 +824,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/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/subsonic/helpers.go b/server/subsonic/helpers.go index e4c39e373..e6c6f9114 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_test.go b/server/subsonic/transcode_test.go index adc7b7600..29a883ac1 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -205,7 +205,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", From af78bdeb3a8b42089d7a35080cffe6ece125c894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 13 Jun 2026 13:29:29 -0400 Subject: [PATCH 2/3] fix(artwork): never serve artist folder images as album art (#5596) * test(artwork): add failing e2e tests for artist image leaking as album art Reproduces a v0.62.0 regression (#5451/#5457): the album cover-art parent-folder fallback can include the artist folder, serving the artist thumbnail (e.g. Artist/folder.jpg) as album art for any album without image files in its own folder(s). Covers three scenarios: a plain Artist/Album layout with no album images, a single-disc album spread across sibling folders under the artist folder, and a spread album whose own front.jpg is shadowed by the artist's cover.jpg via CoverArtPriority order. Also adds an albumByName test helper for multi-album layouts. The tests are expected to fail until the parent-folder inclusion is gated by a structural check (skip the common parent when audio from other albums lives under it). * fix(artwork): never serve artist folder images as album art The album cover-art parent-folder fallback (introduced in #5451/#5457) could include the artist folder as a source of album images, serving the artist thumbnail (e.g. Artist/folder.jpg) as cover art for any album without image files in its own folder(s). This affected both plain Artist/Album layouts and single-disc albums spread across sibling folders under the artist folder. Gate the common-parent inclusion with a structural check: the parent only qualifies as an album root when no audio belonging to other albums lives in it or anywhere beneath it. An artist folder contains other albums' tracks, while an album root above disc subfolders contains only this album's, so the check works for any disc folder naming scheme and never affects the multi-disc fixes from #5376/#5456. A single-album artist with no images anywhere remains structurally indistinguishable from an album root and is a known residual case. * refactor(artwork): move album-root audio check into folder repository Replace the raw subtree SQL (LIKE/ESCAPE expression and wildcard escaping) that lived in core/artwork with an explicit FolderRepository.HasAudioOutsideFolders method, implemented in the persistence layer next to the existing folder-subtree query pattern. This also removes the test mock's brittle dispatch that sniffed the generated SQL to recognize the query; the fake now overrides the new method directly. Extract the whole parent-folder resolution from loadAlbumFoldersPaths into an albumRootParent helper, flattening four levels of nesting back into a linear flow. Behavior is unchanged; the unit test for a parent containing audio moved to the persistence suite, with added coverage for subtree boundaries, missing folders, and LIKE-wildcard escaping in folder paths. * refactor(persistence): use exists helper in HasAudioOutsideFolders Replace the hand-rolled count(*) query with the repository's canonical exists helper, as suggested in PR review. --- core/artwork/e2e/album_test.go | 92 +++++++++++++++++++++++++++ core/artwork/e2e/suite_test.go | 14 ++++ core/artwork/reader_album.go | 70 ++++++++++++++------ core/artwork/reader_album_test.go | 55 ++++++++++++++++ core/artwork/reader_artist_test.go | 8 +++ model/folder.go | 4 ++ persistence/folder_repository.go | 28 ++++++++ persistence/folder_repository_test.go | 61 ++++++++++++++++++ 8 files changed, 311 insertions(+), 21 deletions(-) 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/model/folder.go b/model/folder.go index 7a769735e..39cb6db84 100644 --- a/model/folder.go +++ b/model/folder.go @@ -86,6 +86,10 @@ 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) diff --git a/persistence/folder_repository.go b/persistence/folder_repository.go index f7bb6a4fe..624ef21b8 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) diff --git a/persistence/folder_repository_test.go b/persistence/folder_repository_test.go index ebc08fd04..413a6b38f 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: From c466f6b612a89b7b80a27d24a48a2f8cd1e2a2d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 13 Jun 2026 13:58:26 -0400 Subject: [PATCH 3/3] fix(artwork): prevent WebP segfault on 32-bit and disable WebP-by-default in Docker (#5606) * fix(artwork): avoid WebP segfault on 32-bit ARM On 32-bit ARM, the gen2brain/webp native libwebp path uses ebitengine/purego reverse callbacks, which purego does not support on that architecture. Selecting it crashes the process with a SIGSEGV when encoding or decoding WebP cover art, taking down the whole server on the first web UI artwork request (issue #5597). Force the safe WASM path on armv7/v6 in two layers: build the Docker arm binary with the gen2brain/webp "nodynamic" tag so purego is never linked, and add a runtime GOARCH guard in the init hook so source builds on 32-bit ARM are also protected. arm64 keeps the native libwebp path. * fix(artwork): also disable native WebP on 32-bit x86 purego's callback implementation is built with the constraint !386 && !arm, so 32-bit x86 (386) crashes with the same SIGSEGV as 32-bit ARM when the native libwebp path is used. Navidrome ships linux/386 and windows/386 builds, so guard 386 alongside arm: extend the runtime GOARCH check and the Docker nodynamic build tag to cover both. 64-bit arches keep the native libwebp path. * fix(artwork): rely on nodynamic build tag, drop ineffective runtime guard The previous runtime GOARCH guard did not actually prevent the crash: gen2brain/webp selects the native (purego) vs WASM backend in its own package init() and registers the purego write callback at import time, before any Navidrome hook runs. webp.Dynamic() is only a status getter, and Decode/Encode branch on the library's unexported flag, so the guard merely skipped a log line while the native path stayed active. The effective fix is the nodynamic build tag (applied for 32-bit ARM and x86 in the Dockerfile), which compiles gen2brain/webp WASM-only so purego is never linked. Drop the misleading guard and document that source builds on 32-bit architectures must be built with -tags nodynamic. * fix(artwork): don't enable WebP encoding by default in Docker The Docker image set ND_ENABLEWEBPENCODING=true, which (a) forced cover-art thumbnails through WebP for every install and (b) overrode any EnableWebPEncoding=false set in the user's navidrome.toml, since env vars take precedence over the config file in Viper. On 32-bit platforms the only available WebP backend is the WASM encoder, which is slow on the underpowered hardware those builds typically run on, so enabling it by default is the wrong tradeoff there. Remove the env default and leave EnableWebPEncoding off unless the user opts in. Combined with the nodynamic build tag, 32-bit images neither crash nor pay the WASM cost out of the box. A smarter automatic policy (use WebP only when native libwebp is available) can be revisited separately. --- Dockerfile | 11 +++++++++-- core/artwork/reader_resized.go | 6 ++++++ 2 files changed, 15 insertions(+), 2 deletions(-) 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/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 {