fix(transcode): improve code review findings across transcode implementation

- 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

@@ -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))
 	}

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

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),

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"))

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.

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 {

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")
 		}
 	}
 

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() {