fix: address review findings for OpenSubsonic transcoding PR

Fix multiple issues identified during code review of the transcoding
extension: add missing return after error in shared stream handler
preventing nil pointer panic, replace dead r.Body nil check with
MaxBytesReader size limit, distinguish not-found from other DB errors,
fix bpsToKbps integer truncation with rounding, add "pcm" to
isLosslessFormat for consistency with model.IsLossless(), add
sampleRate/bitDepth/channels to streaming log, fix outdated test
comment, and add tests for conversion functions and GetTranscodeStream
parameter passing.

core/media_streamer.go

@@ -77,7 +77,8 @@ func (ms *mediaStreamer) DoStream(ctx context.Context, mf *model.MediaFile, req
 	var cached bool
 	defer func() {
 		log.Info(ctx, "Streaming file", "title", mf.Title, "artist", mf.Artist, "format", format, "cached", cached,
-			"bitRate", bitRate, "user", userName(ctx), "transcoding", format != "raw",
+			"bitRate", bitRate, "sampleRate", req.SampleRate, "bitDepth", req.BitDepth, "channels", req.Channels,
+			"user", userName(ctx), "transcoding", format != "raw",
 			"originalFormat", mf.Suffix, "originalBitRate", mf.BitRate)
 	}()
 

core/transcode/codec.go

@@ -8,7 +8,7 @@ import "strings"
 // for transcoding decision purposes.
 func isLosslessFormat(format string) bool {
 	switch strings.ToLower(format) {
-	case "flac", "alac", "wav", "aiff", "ape", "wv", "tta", "tak", "shn", "dsd":
+	case "flac", "alac", "wav", "aiff", "ape", "wv", "tta", "tak", "shn", "dsd", "pcm":
 		return true
 	}
 	return false

core/transcode/transcode_test.go

@@ -294,8 +294,8 @@ var _ = Describe("Decider", func() {
 				ds.MockedTranscoding = mockTranscoding
 				svc = NewDecider(ds)
 
-				// MockTranscodingRepo doesn't support flac, so this will skip lossless profile.
-				// Use mp3 which is supported as the fallback.
+				// Transcoding to mp3 (lossy) should result in IsLossless=false.
+				// Use mp3 profile to test that lossy output is correctly identified.
 				mf := &model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 96000, BitDepth: 24}
 				ci := &ClientInfo{
 					MaxTranscodingAudioBitrate: 320,

server/public/handle_streams.go

@@ -29,6 +29,7 @@ func (pub *Router) handleStream(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		log.Error(ctx, "Error starting shared stream", err)
 		http.Error(w, "invalid request", http.StatusInternalServerError)
+		return
 	}
 
 	// Make sure the stream will be closed at the end, to avoid leakage

server/subsonic/transcode.go

@@ -2,6 +2,7 @@ package subsonic
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"slices"
@@ -10,6 +11,7 @@ import (
 	"github.com/navidrome/navidrome/core"
 	"github.com/navidrome/navidrome/core/transcode"
 	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/server/subsonic/responses"
 	"github.com/navidrome/navidrome/utils/req"
 )
@@ -110,9 +112,9 @@ func (r *clientInfoRequest) toCoreClientInfo() *transcode.ClientInfo {
 	return ci
 }
 
-// bpsToKbps converts bits per second to kilobits per second.
+// bpsToKbps converts bits per second to kilobits per second (rounded).
 func bpsToKbps(bps int) int {
-	return bps / 1000
+	return (bps + 500) / 1000
 }
 
 // kbpsToBps converts kilobits per second to bits per second.
@@ -233,9 +235,7 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request)
 
 	// Parse and validate ClientInfo from request body (required per OpenSubsonic spec)
 	var clientInfoReq clientInfoRequest
-	if r.Body == nil {
-		return nil, newError(responses.ErrorMissingParameter, "missing required JSON request body")
-	}
+	r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1 MB limit
 	if err := json.NewDecoder(r.Body).Decode(&clientInfoReq); err != nil {
 		return nil, newError(responses.ErrorGeneric, "invalid JSON request body")
 	}
@@ -247,7 +247,10 @@ func (api *Router) GetTranscodeDecision(w http.ResponseWriter, r *http.Request)
 	// Get media file
 	mf, err := api.ds.MediaFile(ctx).Get(mediaID)
 	if err != nil {
-		return nil, newError(responses.ErrorDataNotFound, "media file not found: %s", mediaID)
+		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)
 	}
 
 	// Make the decision

server/subsonic/transcode_test.go

@@ -3,9 +3,11 @@ package subsonic
 import (
 	"bytes"
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 
+	"github.com/navidrome/navidrome/core"
 	"github.com/navidrome/navidrome/core/transcode"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/tests"
@@ -230,6 +232,87 @@ var _ = Describe("Transcode endpoints", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(err.Error()).To(ContainSubstring("does not match"))
 		})
+
+		It("builds correct StreamRequest for direct play", func() {
+			fakeStreamer := &fakeMediaStreamer{}
+			router = New(ds, nil, fakeStreamer, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, mockTD)
+			mockTD.params = &transcode.Params{MediaID: "song-1", DirectPlay: true}
+
+			r := newGetRequest("mediaId=song-1", "mediaType=song", "transcodeParams=valid-token")
+			_, _ = router.GetTranscodeStream(w, r)
+
+			Expect(fakeStreamer.captured).ToNot(BeNil())
+			Expect(fakeStreamer.captured.ID).To(Equal("song-1"))
+			Expect(fakeStreamer.captured.Format).To(BeEmpty())
+			Expect(fakeStreamer.captured.BitRate).To(BeZero())
+			Expect(fakeStreamer.captured.SampleRate).To(BeZero())
+			Expect(fakeStreamer.captured.BitDepth).To(BeZero())
+			Expect(fakeStreamer.captured.Channels).To(BeZero())
+		})
+
+		It("builds correct StreamRequest for transcoding", func() {
+			fakeStreamer := &fakeMediaStreamer{}
+			router = New(ds, nil, fakeStreamer, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, mockTD)
+			mockTD.params = &transcode.Params{
+				MediaID:          "song-2",
+				DirectPlay:       false,
+				TargetFormat:     "mp3",
+				TargetBitrate:    256,
+				TargetSampleRate: 44100,
+				TargetBitDepth:   16,
+				TargetChannels:   2,
+			}
+
+			r := newGetRequest("mediaId=song-2", "mediaType=song", "transcodeParams=valid-token", "offset=10")
+			_, _ = router.GetTranscodeStream(w, r)
+
+			Expect(fakeStreamer.captured).ToNot(BeNil())
+			Expect(fakeStreamer.captured.ID).To(Equal("song-2"))
+			Expect(fakeStreamer.captured.Format).To(Equal("mp3"))
+			Expect(fakeStreamer.captured.BitRate).To(Equal(256))
+			Expect(fakeStreamer.captured.SampleRate).To(Equal(44100))
+			Expect(fakeStreamer.captured.BitDepth).To(Equal(16))
+			Expect(fakeStreamer.captured.Channels).To(Equal(2))
+			Expect(fakeStreamer.captured.Offset).To(Equal(10))
+		})
+	})
+
+	Describe("bpsToKbps", func() {
+		It("converts standard bitrates", func() {
+			Expect(bpsToKbps(128000)).To(Equal(128))
+			Expect(bpsToKbps(320000)).To(Equal(320))
+			Expect(bpsToKbps(256000)).To(Equal(256))
+		})
+		It("returns 0 for 0", func() {
+			Expect(bpsToKbps(0)).To(Equal(0))
+		})
+		It("rounds instead of truncating", func() {
+			Expect(bpsToKbps(999)).To(Equal(1))
+			Expect(bpsToKbps(500)).To(Equal(1))
+			Expect(bpsToKbps(499)).To(Equal(0))
+		})
+	})
+
+	Describe("kbpsToBps", func() {
+		It("converts standard bitrates", func() {
+			Expect(kbpsToBps(128)).To(Equal(128000))
+			Expect(kbpsToBps(320)).To(Equal(320000))
+		})
+		It("returns 0 for 0", func() {
+			Expect(kbpsToBps(0)).To(Equal(0))
+		})
+	})
+
+	Describe("convertBitrateValues", func() {
+		It("converts valid bps strings to kbps", func() {
+			Expect(convertBitrateValues([]string{"128000", "320000"})).To(Equal([]string{"128", "320"}))
+		})
+		It("preserves unparseable values", func() {
+			Expect(convertBitrateValues([]string{"128000", "bad", "320000"})).To(Equal([]string{"128", "bad", "320"}))
+		})
+		It("handles empty slice", func() {
+			Expect(convertBitrateValues([]string{})).To(Equal([]string{}))
+		})
 	})
 })
 
@@ -266,3 +349,21 @@ func (m *mockTranscodeDecision) ParseTranscodeParams(_ string) (*transcode.Param
 	}
 	return m.params, nil
 }
+
+// fakeMediaStreamer captures the StreamRequest and returns a sentinel error,
+// allowing tests to verify parameter passing without constructing a real Stream.
+var errStreamCaptured = errors.New("stream request captured")
+
+type fakeMediaStreamer struct {
+	captured *core.StreamRequest
+}
+
+func (f *fakeMediaStreamer) NewStream(_ context.Context, req core.StreamRequest) (*core.Stream, error) {
+	f.captured = &req
+	return nil, errStreamCaptured
+}
+
+func (f *fakeMediaStreamer) DoStream(_ context.Context, _ *model.MediaFile, req core.StreamRequest) (*core.Stream, error) {
+	f.captured = &req
+	return nil, errStreamCaptured
+}