From b99092a69780cbf74359cad59454acb6a36f2333 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 7 Mar 2026 20:20:29 -0500 Subject: [PATCH] fix(transcode): improve code review findings across transcode implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix parseProbeData to return nil on JSON unmarshal failure instead of a zero-valued struct, preventing silent degradation of source stream details - Use probe-resolved codec for lossless detection in buildSourceStream instead of the potentially stale scanner data - Remove MediaFile.IsLossless() (dead code) and consolidate lossless detection in isLosslessFormat(), using codec name only — bit depth is not reliable since lossy codecs like ADPCM report non-zero values - Add "wavpack" to lossless codec list (ffprobe codec_name for WavPack) - Guard bpsToKbps against negative input values - Fix misleading comment in buildTemplateArgs about conditional injection - Avoid leaking internal error details in Subsonic API responses - Add missing test for ErrNotFound branch in GetTranscodeDecision - Add TODO for hardcoded protocol in toResponseStreamDetails --- core/ffmpeg/ffmpeg.go | 6 ++- core/transcode/codec.go | 10 +++-- core/transcode/transcode.go | 8 ++-- core/transcode/transcode_test.go | 74 +++++++++++++++++++++++++++++++ model/mediafile.go | 19 -------- model/mediafile_test.go | 36 --------------- server/subsonic/transcode.go | 14 ++++-- server/subsonic/transcode_test.go | 20 +++++++-- 8 files changed, 116 insertions(+), 71 deletions(-) diff --git a/core/ffmpeg/ffmpeg.go b/core/ffmpeg/ffmpeg.go index 2bd4e4b7d..7202d02dd 100644 --- a/core/ffmpeg/ffmpeg.go +++ b/core/ffmpeg/ffmpeg.go @@ -349,11 +349,13 @@ func buildDynamicArgs(opts TranscodeOptions) []string { } // buildTemplateArgs handles user-customized command templates, with dynamic injection -// of sample rate and channels when the template doesn't already include them. +// of sample rate, channels, and bit depth when requested by the transcode decision. +// Note: these flags are injected unconditionally when non-zero, even if the template +// already includes them. FFmpeg uses the last occurrence of duplicate flags. func buildTemplateArgs(opts TranscodeOptions) []string { args := createFFmpegCommand(opts.Command, opts.FilePath, opts.BitRate, opts.Offset) - // Dynamically inject -ar, -ac, and -sample_fmt for custom templates that don't include them + // Dynamically inject -ar, -ac, and -sample_fmt before the output target if opts.SampleRate > 0 { args = injectBeforeOutput(args, "-ar", strconv.Itoa(opts.SampleRate)) } diff --git a/core/transcode/codec.go b/core/transcode/codec.go index 00635b860..aa276d43f 100644 --- a/core/transcode/codec.go +++ b/core/transcode/codec.go @@ -18,13 +18,15 @@ func normalizeProbeCodec(codec string) string { return c } -// isLosslessFormat returns true if the format is a lossless audio codec/format. +// isLosslessFormat returns true if the format is a known lossless audio codec/format. +// Detection is based on codec name only, not bit depth — some lossy codecs (e.g. ADPCM) +// report non-zero bits_per_sample in ffprobe, so bit depth alone is not a reliable signal. +// // Note: core/ffmpeg has a separate isLosslessOutputFormat that covers only formats -// ffmpeg can produce as output (a smaller set). This function covers all known lossless formats -// for transcoding decision purposes. +// ffmpeg can produce as output (a smaller set). func isLosslessFormat(format string) bool { switch strings.ToLower(format) { - case "flac", "alac", "wav", "aiff", "ape", "wv", "tta", "tak", "shn", "dsd", "pcm": + case "flac", "alac", "wav", "aiff", "ape", "wv", "wavpack", "tta", "tak", "shn", "dsd", "pcm": return true } return false diff --git a/core/transcode/transcode.go b/core/transcode/transcode.go index 074f070e6..8ac7436fa 100644 --- a/core/transcode/transcode.go +++ b/core/transcode/transcode.go @@ -134,8 +134,8 @@ func buildSourceStream(mf *model.MediaFile, probe *ffmpeg.AudioProbeResult) Stre sd.BitDepth = mf.BitDepth sd.Channels = mf.Channels } + sd.IsLossless = isLosslessFormat(sd.Codec) - sd.IsLossless = mf.IsLossless() return sd } @@ -144,8 +144,10 @@ func parseProbeData(data string) (*ffmpeg.AudioProbeResult, error) { return nil, nil } var result ffmpeg.AudioProbeResult - err := json.Unmarshal([]byte(data), &result) - return &result, err + if err := json.Unmarshal([]byte(data), &result); err != nil { + return nil, err + } + return &result, nil } // checkDirectPlayProfile returns "" if the profile matches (direct play OK), diff --git a/core/transcode/transcode_test.go b/core/transcode/transcode_test.go index 111d30d2d..63016984a 100644 --- a/core/transcode/transcode_test.go +++ b/core/transcode/transcode_test.go @@ -721,6 +721,51 @@ 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} + probe := ffmpeg.AudioProbeResult{ + Codec: "wavpack", BitRate: 1000, SampleRate: 44100, BitDepth: 16, Channels: 2, + } + data, _ := json.Marshal(probe) + mf.ProbeData = string(data) + + ci := &ClientInfo{ + TranscodingProfiles: []Profile{ + {Container: "mp3", AudioCodec: "mp3", Protocol: "http"}, + }, + MaxTranscodingAudioBitrate: 256, + } + decision, err := svc.MakeDecision(ctx, mf, ci) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.SourceStream.IsLossless).To(BeTrue()) + Expect(decision.SourceStream.Codec).To(Equal("wavpack")) + // Lossless source transcoding to MP3 should use MaxTranscodingAudioBitrate + Expect(decision.CanTranscode).To(BeTrue()) + Expect(decision.TranscodeStream.Bitrate).To(Equal(256)) + }) + + It("detects lossy from probe codec name", func() { + mf := &model.MediaFile{ID: "1", Suffix: "ogg", BitRate: 192, Channels: 2, SampleRate: 48000} + probe := ffmpeg.AudioProbeResult{ + Codec: "vorbis", BitRate: 192, SampleRate: 48000, BitDepth: 0, Channels: 2, + } + data, _ := json.Marshal(probe) + mf.ProbeData = string(data) + + ci := &ClientInfo{ + DirectPlayProfiles: []DirectPlayProfile{ + {Containers: []string{"ogg"}, AudioCodecs: []string{"vorbis"}, Protocols: []string{"http"}}, + }, + } + decision, err := svc.MakeDecision(ctx, mf, ci) + Expect(err).ToNot(HaveOccurred()) + Expect(decision.SourceStream.IsLossless).To(BeFalse()) + Expect(decision.CanDirectPlay).To(BeTrue()) + }) + }) + 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}) @@ -1164,6 +1209,35 @@ var _ = Describe("Decider", func() { }) }) + Describe("isLosslessFormat", func() { + It("returns true for known lossless codecs", func() { + Expect(isLosslessFormat("flac")).To(BeTrue()) + Expect(isLosslessFormat("alac")).To(BeTrue()) + Expect(isLosslessFormat("pcm")).To(BeTrue()) + Expect(isLosslessFormat("wav")).To(BeTrue()) + Expect(isLosslessFormat("dsd")).To(BeTrue()) + Expect(isLosslessFormat("ape")).To(BeTrue()) + Expect(isLosslessFormat("wv")).To(BeTrue()) + Expect(isLosslessFormat("wavpack")).To(BeTrue()) // ffprobe codec_name for WavPack + }) + + It("returns false for lossy codecs", func() { + Expect(isLosslessFormat("mp3")).To(BeFalse()) + Expect(isLosslessFormat("aac")).To(BeFalse()) + Expect(isLosslessFormat("opus")).To(BeFalse()) + Expect(isLosslessFormat("vorbis")).To(BeFalse()) + }) + + It("returns false for unknown codecs", func() { + Expect(isLosslessFormat("unknown_codec")).To(BeFalse()) + }) + + It("is case-insensitive", func() { + Expect(isLosslessFormat("FLAC")).To(BeTrue()) + Expect(isLosslessFormat("Alac")).To(BeTrue()) + }) + }) + Describe("normalizeProbeCodec", func() { It("passes through common codec names unchanged", func() { Expect(normalizeProbeCodec("mp3")).To(Equal("mp3")) diff --git a/model/mediafile.go b/model/mediafile.go index d04060473..20532bfb9 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -14,7 +14,6 @@ import ( "github.com/gohugoio/hashstructure" "github.com/navidrome/navidrome/conf" - confmime "github.com/navidrome/navidrome/conf/mime" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/slice" @@ -228,24 +227,6 @@ func (mf MediaFile) inferCodecFromSuffix() string { } } -// IsLossless returns true if this file uses a lossless codec. -func (mf MediaFile) IsLossless() bool { - codec := mf.AudioCodec() - // Primary: codec-based check (most accurate for containers like M4A) - switch codec { - case "flac", "alac", "pcm", "ape", "wv", "tta", "tak", "shn", "dsd": - return true - } - // Secondary: suffix-based check using configurable list from YAML - if slices.Contains(confmime.LosslessFormats, mf.Suffix) { - return true - } - // Fallback heuristic: if BitDepth is set, it's likely lossless. - // This may produce false positives for lossy formats that report bit depth, - // but it becomes irrelevant once the Codec column is populated after a full rescan. - return mf.BitDepth > 0 -} - type MediaFiles []MediaFile // ToAlbum creates an Album object based on the attributes of this MediaFiles collection. diff --git a/model/mediafile_test.go b/model/mediafile_test.go index 31f5c53be..207d3c155 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -570,42 +570,6 @@ var _ = Describe("MediaFile", func() { }) }) - Describe("IsLossless", func() { - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - }) - - DescribeTable("detects lossless codecs", - func(codec string, suffix string, bitDepth int, expected bool) { - mf := MediaFile{Codec: codec, Suffix: suffix, BitDepth: bitDepth} - Expect(mf.IsLossless()).To(Equal(expected)) - }, - Entry("flac", "FLAC", "flac", 16, true), - Entry("alac", "ALAC", "m4a", 24, true), - Entry("pcm via wav", "", "wav", 16, true), - Entry("pcm via aiff", "", "aiff", 24, true), - Entry("ape", "", "ape", 16, true), - Entry("wv", "", "wv", 0, true), - Entry("tta", "", "tta", 0, true), - Entry("tak", "", "tak", 0, true), - Entry("shn", "", "shn", 0, true), - Entry("dsd", "", "dsf", 0, true), - Entry("mp3 is lossy", "MP3", "mp3", 0, false), - Entry("aac is lossy", "AAC", "m4a", 0, false), - Entry("vorbis is lossy", "", "ogg", 0, false), - Entry("opus is lossy", "", "opus", 0, false), - ) - - It("detects lossless via BitDepth fallback when codec is unknown", func() { - mf := MediaFile{Suffix: "xyz", BitDepth: 24} - Expect(mf.IsLossless()).To(BeTrue()) - }) - - It("returns false for unknown with no BitDepth", func() { - mf := MediaFile{Suffix: "xyz", BitDepth: 0} - Expect(mf.IsLossless()).To(BeFalse()) - }) - }) }) func t(v string) time.Time { diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index ce0a0f4be..2aaec3e9a 100644 --- a/server/subsonic/transcode.go +++ b/server/subsonic/transcode.go @@ -114,6 +114,9 @@ func (r *clientInfoRequest) toCoreClientInfo() *transcode.ClientInfo { // bpsToKbps converts bits per second to kilobits per second (rounded). func bpsToKbps(bps int) int { + if bps < 0 { + return 0 + } return (bps + 500) / 1000 } @@ -218,7 +221,7 @@ func isValidComparison(c string) bool { // toResponseStreamDetails converts a core StreamDetails to the API response type. func toResponseStreamDetails(sd *transcode.StreamDetails) *responses.StreamDetails { return &responses.StreamDetails{ - Protocol: transcode.ProtocolHTTP, + Protocol: transcode.ProtocolHTTP, // TODO: derive from decision when HLS support is added Container: sd.Container, Codec: sd.Codec, AudioBitrate: int32(kbpsToBps(sd.Bitrate)), @@ -272,13 +275,15 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) if errors.Is(err, model.ErrNotFound) { return nil, newError(responses.ErrorDataNotFound, "media file not found: %s", mediaID) } - return nil, newError(responses.ErrorGeneric, "error retrieving media file: %v", err) + log.Error(ctx, "Error retrieving media file", "mediaID", mediaID, err) + return nil, newError(responses.ErrorGeneric, "error retrieving media file") } // Make the decision decision, err := api.transcodeDecision.MakeDecision(ctx, mf, clientInfo) if err != nil { - return nil, newError(responses.ErrorGeneric, "failed to make transcode decision: %v", err) + log.Error(ctx, "Failed to make transcode decision", "mediaID", mediaID, err) + return nil, newError(responses.ErrorGeneric, "failed to make transcode decision") } // Only create a token when there is a valid playback path @@ -286,7 +291,8 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request) if decision.CanDirectPlay || decision.CanTranscode { transcodeParams, err = api.transcodeDecision.CreateTranscodeParams(decision) if err != nil { - return nil, newError(responses.ErrorGeneric, "failed to create transcode token: %v", err) + log.Error(ctx, "Failed to create transcode token", "mediaID", mediaID, err) + return nil, newError(responses.ErrorGeneric, "failed to create transcode token") } } diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index 88c65d9ce..fbc4a6f43 100644 --- a/server/subsonic/transcode_test.go +++ b/server/subsonic/transcode_test.go @@ -61,11 +61,20 @@ var _ = Describe("Transcode endpoints", func() { Expect(err.Error()).To(ContainSubstring("not yet supported")) }) - It("returns error when media file not found", func() { - mockMFRepo.SetError(true) - r := newJSONPostRequest("mediaId=notfound&mediaType=song", "{}") + It("returns ErrorDataNotFound when media file does not exist", func() { + // mockMFRepo has no data set, so Get() returns model.ErrNotFound + r := newJSONPostRequest("mediaId=nonexistent&mediaType=song", "{}") _, err := router.GetTranscodeDecision(w, r) Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("media file not found")) + }) + + It("returns error when media file retrieval fails", func() { + mockMFRepo.SetError(true) + r := newJSONPostRequest("mediaId=song-1&mediaType=song", "{}") + _, err := router.GetTranscodeDecision(w, r) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("error retrieving media file")) }) It("returns error when body is empty", func() { @@ -318,6 +327,11 @@ var _ = Describe("Transcode endpoints", func() { Expect(bpsToKbps(500)).To(Equal(1)) Expect(bpsToKbps(499)).To(Equal(0)) }) + It("returns 0 for negative values", func() { + Expect(bpsToKbps(-1)).To(Equal(0)) + Expect(bpsToKbps(-1000)).To(Equal(0)) + Expect(bpsToKbps(-1000000)).To(Equal(0)) + }) }) Describe("kbpsToBps", func() {