From b935a7a4770094f68abe3ea5f22c3b54734dba52 Mon Sep 17 00:00:00 2001 From: hungryandgrumpy <87514617+hungryandgrumpy@users.noreply.github.com> Date: Tue, 11 Aug 2026 09:37:40 +0200 Subject: [PATCH] refactor(playback): decouple Jukebox track creation Signed-off-by: hungryandgrumpy <87514617+hungryandgrumpy@users.noreply.github.com> --- core/playback/device.go | 7 +- core/playback/device_test.go | 355 +++++++++++++++++++++++++++ core/playback/factory.go | 24 ++ core/playback/playbackserver.go | 17 +- core/playback/playbackserver_test.go | 93 +++++++ 5 files changed, 487 insertions(+), 9 deletions(-) create mode 100644 core/playback/device_test.go create mode 100644 core/playback/factory.go create mode 100644 core/playback/playbackserver_test.go diff --git a/core/playback/device.go b/core/playback/device.go index fd08b340e..6b9f4c925 100644 --- a/core/playback/device.go +++ b/core/playback/device.go @@ -6,7 +6,6 @@ import ( "fmt" "sync" - "github.com/navidrome/navidrome/core/playback/mpv" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" ) @@ -33,6 +32,7 @@ type playbackDevice struct { Gain float32 PlaybackDone chan bool ActiveTrack Track + trackFactory TrackFactory startTrackSwitcher sync.Once } @@ -61,7 +61,7 @@ func (pd *playbackDevice) getStatus() DeviceStatus { // NewPlaybackDevice creates a new playback device which implements all the basic Jukebox mode commands defined here: // http://www.subsonic.org/pages/api.jsp#jukeboxControl // Starts the trackSwitcher goroutine for the device. -func NewPlaybackDevice(ctx context.Context, playbackServer PlaybackServer, name string, deviceName string) *playbackDevice { +func NewPlaybackDevice(ctx context.Context, playbackServer PlaybackServer, name string, deviceName string, trackFactory TrackFactory) *playbackDevice { return &playbackDevice{ serviceCtx: ctx, ParentPlaybackServer: playbackServer, @@ -71,6 +71,7 @@ func NewPlaybackDevice(ctx context.Context, playbackServer PlaybackServer, name Gain: DefaultGain, PlaybackQueue: NewQueue(), PlaybackDone: make(chan bool), + trackFactory: trackFactory, } } @@ -289,7 +290,7 @@ func (pd *playbackDevice) switchActiveTrackByIndex(index int) error { return errors.New("could not get current track") } - track, err := mpv.NewTrack(pd.serviceCtx, pd.PlaybackDone, pd.DeviceName, *currentTrack) + track, err := pd.trackFactory(pd.serviceCtx, pd.PlaybackDone, pd.DeviceName, *currentTrack) if err != nil { return err } diff --git a/core/playback/device_test.go b/core/playback/device_test.go new file mode 100644 index 000000000..cb4eb9ba7 --- /dev/null +++ b/core/playback/device_test.go @@ -0,0 +1,355 @@ +package playback + +import ( + "context" + "errors" + "fmt" + "sync" + + "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +type fakeTrack struct { + name string + playing bool + position int + positionErr error + pauseCalls int + unpauseCalls int + closeCalls int + setVolumeCalls []float32 +} + +func (t *fakeTrack) IsPlaying() bool { return t.playing } + +func (t *fakeTrack) SetVolume(value float32) { + t.setVolumeCalls = append(t.setVolumeCalls, value) +} + +func (t *fakeTrack) Pause() { + t.pauseCalls++ + t.playing = false +} + +func (t *fakeTrack) Unpause() { + t.unpauseCalls++ + t.playing = true +} + +func (t *fakeTrack) Position() int { return t.position } + +func (t *fakeTrack) SetPosition(offset int) error { + if t.positionErr != nil { + return t.positionErr + } + t.position = offset + return nil +} + +func (t *fakeTrack) Close() { t.closeCalls++ } + +func (t *fakeTrack) String() string { return t.name } + +type trackFactoryRecorder struct { + mu sync.Mutex + calls []trackFactoryCall + tracks []*fakeTrack + err error + nextErr []error +} + +type trackFactoryCall struct { + ctx context.Context + playbackDone chan bool + deviceName string + mf model.MediaFile +} + +func (r *trackFactoryRecorder) factory(ctx context.Context, playbackDone chan bool, deviceName string, mf model.MediaFile) (Track, error) { + r.mu.Lock() + defer r.mu.Unlock() + + r.calls = append(r.calls, trackFactoryCall{ctx: ctx, playbackDone: playbackDone, deviceName: deviceName, mf: mf}) + if len(r.nextErr) > 0 { + err := r.nextErr[0] + r.nextErr = r.nextErr[1:] + if err != nil { + return nil, err + } + } + if r.err != nil { + return nil, r.err + } + track := &fakeTrack{name: fmt.Sprintf("track-%s", mf.ID)} + r.tracks = append(r.tracks, track) + return track, nil +} + +func (r *trackFactoryRecorder) trackCount() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.tracks) +} + +func (r *trackFactoryRecorder) trackAt(i int) *fakeTrack { + r.mu.Lock() + defer r.mu.Unlock() + return r.tracks[i] +} + +type fakePlaybackServer struct { + mediaFiles map[string]model.MediaFile +} + +func (s *fakePlaybackServer) Run(context.Context) error { return nil } + +func (s *fakePlaybackServer) GetDeviceForUser(string) (*playbackDevice, error) { return nil, nil } + +func (s *fakePlaybackServer) GetMediaFile(id string) (*model.MediaFile, error) { + mf, ok := s.mediaFiles[id] + if !ok { + return nil, model.ErrNotFound + } + copy := mf + return ©, nil +} + +func makeMediaFiles(ids ...string) model.MediaFiles { + items := make(model.MediaFiles, len(ids)) + for i, id := range ids { + items[i] = model.MediaFile{ID: id, Path: "/music/" + id + ".mp3"} + } + return items +} + +var _ = Describe("playbackDevice", func() { + var ( + ctx context.Context + cancel context.CancelFunc + parent *fakePlaybackServer + rec *trackFactoryRecorder + device *playbackDevice + ) + + BeforeEach(func() { + ctx, cancel = context.WithCancel(context.Background()) + parent = &fakePlaybackServer{} + rec = &trackFactoryRecorder{} + device = NewPlaybackDevice(ctx, parent, "jukebox", "auto", rec.factory) + }) + + AfterEach(func() { + cancel() + }) + + It("starts with a queue and no active track", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + + status, err := device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.calls).To(HaveLen(1)) + Expect(rec.calls[0].deviceName).To(Equal("auto")) + Expect(rec.calls[0].playbackDone).To(BeIdenticalTo(device.PlaybackDone)) + Expect(rec.calls[0].mf.ID).To(Equal("1")) + Expect(device.ActiveTrack).To(BeIdenticalTo(rec.tracks[0])) + Expect(rec.tracks[0].unpauseCalls).To(Equal(1)) + Expect(rec.tracks[0].setVolumeCalls).To(Equal([]float32{DefaultGain})) + Expect(status.CurrentIndex).To(Equal(0)) + Expect(status.Playing).To(BeTrue()) + }) + + It("starts a paused existing track without creating a new one", func() { + track := &fakeTrack{name: "existing", playing: false} + device.ActiveTrack = track + + status, err := device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.calls).To(BeEmpty()) + Expect(track.unpauseCalls).To(Equal(1)) + Expect(status.Playing).To(BeTrue()) + }) + + It("keeps an already playing track running", func() { + track := &fakeTrack{name: "existing", playing: true} + device.ActiveTrack = track + + status, err := device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.calls).To(BeEmpty()) + Expect(track.unpauseCalls).To(Equal(0)) + Expect(status.Playing).To(BeTrue()) + }) + + It("stops by pausing the active track", func() { + track := &fakeTrack{name: "existing", playing: true} + device.ActiveTrack = track + + status, err := device.Stop(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(track.pauseCalls).To(Equal(1)) + Expect(status.Playing).To(BeFalse()) + }) + + It("applies gain to an active track", func() { + track := &fakeTrack{name: "existing"} + device.ActiveTrack = track + + status, err := device.SetGain(ctx, 0.4) + Expect(err).ToNot(HaveOccurred()) + Expect(device.Gain).To(Equal(float32(0.4))) + Expect(track.setVolumeCalls).To(Equal([]float32{0.4})) + Expect(status.Gain).To(Equal(float32(0.4))) + }) + + It("applies prior gain when creating a track later", func() { + device.PlaybackQueue.Add(makeMediaFiles("1")) + _, err := device.SetGain(ctx, 0.6) + Expect(err).ToNot(HaveOccurred()) + + _, err = device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.tracks).To(HaveLen(1)) + Expect(rec.tracks[0].setVolumeCalls).To(Equal([]float32{0.6})) + }) + + It("seeks within the same track", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + track := &fakeTrack{name: "existing", playing: true, position: 10} + device.ActiveTrack = track + device.PlaybackQueue.Index = 0 + + status, err := device.Skip(ctx, 0, 42) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.calls).To(BeEmpty()) + Expect(track.pauseCalls).To(Equal(1)) + Expect(track.position).To(Equal(42)) + Expect(track.closeCalls).To(Equal(0)) + Expect(track.unpauseCalls).To(Equal(1)) + Expect(status.CurrentIndex).To(Equal(0)) + Expect(status.Position).To(Equal(42)) + Expect(status.Playing).To(BeTrue()) + }) + + It("skips to another queue index", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + current := &fakeTrack{name: "current", playing: true, position: 3} + device.ActiveTrack = current + device.PlaybackQueue.Index = 0 + + status, err := device.Skip(ctx, 1, 25) + Expect(err).ToNot(HaveOccurred()) + Expect(current.pauseCalls).To(Equal(1)) + Expect(current.closeCalls).To(Equal(1)) + Expect(rec.calls).To(HaveLen(1)) + Expect(rec.calls[0].mf.ID).To(Equal("2")) + Expect(rec.tracks[0].position).To(Equal(25)) + Expect(rec.tracks[0].unpauseCalls).To(Equal(1)) + Expect(device.ActiveTrack).To(BeIdenticalTo(rec.tracks[0])) + Expect(status.CurrentIndex).To(Equal(1)) + Expect(status.Position).To(Equal(25)) + Expect(status.Playing).To(BeTrue()) + }) + + It("clears the queue and closes the active track", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + track := &fakeTrack{name: "existing", playing: true} + device.ActiveTrack = track + + status, err := device.Clear(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(track.pauseCalls).To(Equal(1)) + Expect(track.closeCalls).To(Equal(1)) + Expect(device.ActiveTrack).To(BeNil()) + Expect(device.PlaybackQueue.Index).To(Equal(-1)) + Expect(device.PlaybackQueue.Items).To(BeNil()) + Expect(status.CurrentIndex).To(Equal(-1)) + Expect(status.Playing).To(BeFalse()) + }) + + It("removes the current playing item while preserving existing behavior", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + track := &fakeTrack{name: "existing", playing: true, position: 17} + device.ActiveTrack = track + device.PlaybackQueue.Index = 0 + + status, err := device.Remove(ctx, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(track.pauseCalls).To(Equal(1)) + Expect(track.closeCalls).To(Equal(0)) + Expect(device.ActiveTrack).To(BeIdenticalTo(track)) + Expect(device.PlaybackQueue.Size()).To(Equal(1)) + Expect(device.PlaybackQueue.Index).To(Equal(-1)) + Expect(status.CurrentIndex).To(Equal(-1)) + Expect(status.Position).To(Equal(17)) + Expect(status.Playing).To(BeFalse()) + }) + + It("advances to the next track on natural completion", func() { + device.PlaybackQueue.Add(makeMediaFiles("1", "2")) + + _, err := device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + first := rec.tracks[0] + + delivered := make(chan struct{}) + go func() { + device.PlaybackDone <- true + close(delivered) + }() + + Eventually(delivered).Should(BeClosed()) + Eventually(func() int { return rec.trackCount() }).Should(Equal(2)) + Eventually(func() int { return first.closeCalls }).Should(Equal(1)) + Eventually(func() int { return device.PlaybackQueue.Index }).Should(Equal(1)) + Eventually(func() Track { return device.ActiveTrack }).Should(BeIdenticalTo(rec.trackAt(1))) + Eventually(func() int { return rec.trackAt(1).unpauseCalls }).Should(Equal(1)) + }) + + It("finishes on the final track after natural completion", func() { + device.PlaybackQueue.Add(makeMediaFiles("1")) + + _, err := device.Start(ctx) + Expect(err).ToNot(HaveOccurred()) + first := rec.tracks[0] + + delivered := make(chan struct{}) + go func() { + device.PlaybackDone <- true + close(delivered) + }() + + Eventually(delivered).Should(BeClosed()) + Eventually(func() int { return first.closeCalls }).Should(Equal(1)) + Eventually(func() Track { return device.ActiveTrack }).Should(BeNil()) + Consistently(func() int { return rec.trackCount() }).Should(Equal(1)) + Expect(device.PlaybackQueue.Index).To(Equal(0)) + }) + + It("returns a factory error when creating a track fails", func() { + device.PlaybackQueue.Add(makeMediaFiles("1")) + rec.err = errors.New("factory failed") + + status, err := device.Start(ctx) + Expect(err).To(MatchError("factory failed")) + Expect(device.ActiveTrack).To(BeNil()) + Expect(status.CurrentIndex).To(Equal(0)) + Expect(status.Playing).To(BeFalse()) + }) + + It("returns a seek error from SetPosition", func() { + seekErr := errors.New("seek failed") + track := &fakeTrack{name: "existing", playing: true, positionErr: seekErr} + device.PlaybackQueue.Add(makeMediaFiles("1")) + device.ActiveTrack = track + device.PlaybackQueue.Index = 0 + + status, err := device.Skip(ctx, 0, 99) + Expect(err).To(MatchError(seekErr)) + Expect(track.pauseCalls).To(Equal(1)) + Expect(track.unpauseCalls).To(Equal(0)) + Expect(status.Playing).To(BeFalse()) + Expect(status.Position).To(Equal(0)) + }) +}) diff --git a/core/playback/factory.go b/core/playback/factory.go new file mode 100644 index 000000000..643b74a49 --- /dev/null +++ b/core/playback/factory.go @@ -0,0 +1,24 @@ +package playback + +import ( + "context" + + "github.com/navidrome/navidrome/core/playback/mpv" + "github.com/navidrome/navidrome/model" +) + +type TrackFactory func( + ctx context.Context, + playbackDone chan bool, + deviceName string, + mf model.MediaFile, +) (Track, error) + +func defaultTrackFactory( + ctx context.Context, + playbackDone chan bool, + deviceName string, + mf model.MediaFile, +) (Track, error) { + return mpv.NewTrack(ctx, playbackDone, deviceName, mf) +} diff --git a/core/playback/playbackserver.go b/core/playback/playbackserver.go index 7dd02dcb1..280effe8b 100644 --- a/core/playback/playbackserver.go +++ b/core/playback/playbackserver.go @@ -25,12 +25,13 @@ type playbackServer struct { ctx *context.Context datastore model.DataStore playbackDevices []playbackDevice + trackFactory TrackFactory } // GetInstance returns the playback-server singleton func GetInstance(ds model.DataStore) PlaybackServer { return singleton.GetInstance(func() *playbackServer { - return &playbackServer{datastore: ds} + return &playbackServer{datastore: ds, trackFactory: defaultTrackFactory} }) } @@ -38,7 +39,11 @@ func GetInstance(ds model.DataStore) PlaybackServer { func (ps *playbackServer) Run(ctx context.Context) error { ps.ctx = &ctx - devices, err := ps.initDeviceStatus(ctx, conf.Server.Jukebox.Devices, conf.Server.Jukebox.Default) + if ps.trackFactory == nil { + ps.trackFactory = defaultTrackFactory + } + + devices, err := ps.initDeviceStatus(ctx, conf.Server.Jukebox.Devices, conf.Server.Jukebox.Default, ps.trackFactory) if err != nil { return err } @@ -55,14 +60,14 @@ func (ps *playbackServer) Run(ctx context.Context) error { return nil } -func (ps *playbackServer) initDeviceStatus(ctx context.Context, devices []conf.AudioDeviceDefinition, defaultDevice string) ([]playbackDevice, error) { +func (ps *playbackServer) initDeviceStatus(ctx context.Context, devices []conf.AudioDeviceDefinition, defaultDevice string, trackFactory TrackFactory) ([]playbackDevice, error) { pbDevices := make([]playbackDevice, max(1, len(devices))) defaultDeviceFound := false if defaultDevice == "" { // if there are no devices given and no default device, we create a synthetic device named "auto" if len(devices) == 0 { - pbDevices[0] = *NewPlaybackDevice(ctx, ps, "auto", "auto") + pbDevices[0] = *NewPlaybackDevice(ctx, ps, "auto", "auto", trackFactory) } // if there is but only one entry and no default given, just use that. @@ -70,7 +75,7 @@ func (ps *playbackServer) initDeviceStatus(ctx context.Context, devices []conf.A if len(devices[0]) != 2 { return []playbackDevice{}, fmt.Errorf("audio device definition ought to contain 2 fields, found: %d ", len(devices[0])) } - pbDevices[0] = *NewPlaybackDevice(ctx, ps, devices[0][0], devices[0][1]) + pbDevices[0] = *NewPlaybackDevice(ctx, ps, devices[0][0], devices[0][1], trackFactory) } if len(devices) > 1 { @@ -86,7 +91,7 @@ func (ps *playbackServer) initDeviceStatus(ctx context.Context, devices []conf.A return []playbackDevice{}, fmt.Errorf("audio device definition ought to contain 2 fields, found: %d ", len(audioDevice)) } - pbDevices[idx] = *NewPlaybackDevice(ctx, ps, audioDevice[0], audioDevice[1]) + pbDevices[idx] = *NewPlaybackDevice(ctx, ps, audioDevice[0], audioDevice[1], trackFactory) if audioDevice[0] == defaultDevice { pbDevices[idx].Default = true diff --git a/core/playback/playbackserver_test.go b/core/playback/playbackserver_test.go new file mode 100644 index 000000000..eecac8a05 --- /dev/null +++ b/core/playback/playbackserver_test.go @@ -0,0 +1,93 @@ +package playback + +import ( + "context" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("playbackServer", func() { + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + }) + + It("injects the configured factory into created devices", func() { + rec := &trackFactoryRecorder{} + ps := &playbackServer{trackFactory: rec.factory} + + devices, err := ps.initDeviceStatus(ctx, []conf.AudioDeviceDefinition{{"living", "hw:0"}, {"kitchen", "hw:1"}}, "living", ps.trackFactory) + Expect(err).ToNot(HaveOccurred()) + Expect(devices).To(HaveLen(2)) + + devices[1].PlaybackQueue.Add(makeMediaFiles("track-1")) + _, err = devices[1].Start(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(rec.calls).To(HaveLen(1)) + Expect(rec.calls[0].deviceName).To(Equal("hw:1")) + Expect(rec.calls[0].playbackDone).To(BeIdenticalTo(devices[1].PlaybackDone)) + Expect(rec.calls[0].mf.ID).To(Equal("track-1")) + }) + + It("creates the synthetic default device when no devices are configured", func() { + ps := &playbackServer{trackFactory: defaultTrackFactory} + + devices, err := ps.initDeviceStatus(ctx, nil, "", ps.trackFactory) + Expect(err).ToNot(HaveOccurred()) + Expect(devices).To(HaveLen(1)) + Expect(devices[0].Name).To(Equal("auto")) + Expect(devices[0].DeviceName).To(Equal("auto")) + Expect(devices[0].Default).To(BeTrue()) + Expect(devices[0].trackFactory).ToNot(BeNil()) + }) + + It("creates a single configured device as default when no explicit default is set", func() { + ps := &playbackServer{trackFactory: defaultTrackFactory} + + devices, err := ps.initDeviceStatus(ctx, []conf.AudioDeviceDefinition{{"office", "hw:2"}}, "", ps.trackFactory) + Expect(err).ToNot(HaveOccurred()) + Expect(devices).To(HaveLen(1)) + Expect(devices[0].Name).To(Equal("office")) + Expect(devices[0].DeviceName).To(Equal("hw:2")) + Expect(devices[0].Default).To(BeTrue()) + }) + + It("creates configured devices and preserves default target selection behavior", func() { + ps := &playbackServer{trackFactory: defaultTrackFactory} + devices, err := ps.initDeviceStatus(ctx, []conf.AudioDeviceDefinition{{"living", "hw:0"}, {"kitchen", "hw:1"}}, "kitchen", ps.trackFactory) + Expect(err).ToNot(HaveOccurred()) + ps.playbackDevices = devices + + device, err := ps.GetDeviceForUser("alice") + Expect(err).ToNot(HaveOccurred()) + Expect(device.Name).To(Equal("kitchen")) + Expect(device.DeviceName).To(Equal("hw:1")) + Expect(device.Default).To(BeTrue()) + Expect(device.User).To(Equal("alice")) + }) + + It("wires the default track factory during Run when none is preset", func() { + oldDevices := conf.Server.Jukebox.Devices + oldDefault := conf.Server.Jukebox.Default + DeferCleanup(func() { + conf.Server.Jukebox.Devices = oldDevices + conf.Server.Jukebox.Default = oldDefault + }) + conf.Server.Jukebox.Devices = nil + conf.Server.Jukebox.Default = "" + + ps := &playbackServer{datastore: &tests.MockDataStore{}} + runCtx, cancel := context.WithCancel(context.Background()) + cancel() + + err := ps.Run(runCtx) + Expect(err).ToNot(HaveOccurred()) + Expect(ps.trackFactory).ToNot(BeNil()) + Expect(ps.playbackDevices).To(HaveLen(1)) + Expect(ps.playbackDevices[0].trackFactory).ToNot(BeNil()) + }) +})