From 8c7c6e536f66051dae0beb95d39ddd8d88700729 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 9 Feb 2026 09:40:23 -0500 Subject: [PATCH] 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 | 3 +- core/transcode/codec.go | 2 +- core/transcode/transcode_test.go | 4 +- server/public/handle_streams.go | 1 + server/subsonic/transcode.go | 15 +++-- server/subsonic/transcode_test.go | 101 ++++++++++++++++++++++++++++++ 6 files changed, 116 insertions(+), 10 deletions(-) diff --git a/core/media_streamer.go b/core/media_streamer.go index 9d642de55..6dbad4bcd 100644 --- a/core/media_streamer.go +++ b/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) }() diff --git a/core/transcode/codec.go b/core/transcode/codec.go index 641c7c124..3e7dd3578 100644 --- a/core/transcode/codec.go +++ b/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 diff --git a/core/transcode/transcode_test.go b/core/transcode/transcode_test.go index 0ee3aba77..e7a0e635e 100644 --- a/core/transcode/transcode_test.go +++ b/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, diff --git a/server/public/handle_streams.go b/server/public/handle_streams.go index 27240b1fe..2baec52ed 100644 --- a/server/public/handle_streams.go +++ b/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 diff --git a/server/subsonic/transcode.go b/server/subsonic/transcode.go index c1d195249..020406c45 100644 --- a/server/subsonic/transcode.go +++ b/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 diff --git a/server/subsonic/transcode_test.go b/server/subsonic/transcode_test.go index 032d80051..ede6d92fe 100644 --- a/server/subsonic/transcode_test.go +++ b/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 +}