Complete Jukebox playback support (continues #300) - #1014
Open
systemfreund wants to merge 12 commits into
Open
Conversation
systemfreund
marked this pull request as ready for review
August 14, 2026 12:03
RemotePlayers() now appends a Jukebox device to the cast device list whenever the connected server implements mediaprovider.JukeboxProvider, alongside the network-discovered DLNA renderers. Selecting it instantiates a jukebox.JukeboxPlayer bound to the server's provider, using the existing SetRemotePlayer/rp.new() mechanism. Tracking upstream supersonic-app#300.
ShowCastMenu already iterates generically over PlaybackManager.RemotePlayers(), so the Jukebox device added there shows up automatically. This adds a translated, non-device-derived label for it (unlike DLNA devices, whose names come from the network device itself) via a new exported JukeboxProtocol constant so the UI can special-case it without string-matching on the device name. Tracking upstream supersonic-app#300.
JukeboxPlayer never actually started its stopwatch (only Reset/Start calls lived in the still-stubbed handleOnTrackChange), so curPlayPos() was always effectively 0 while playing, and PlayTrack() never set state to playing or fired InvokeOnPlaying at all - Continue() was the only path that did. Bring PlayTrack/Continue/Pause/Stop in line with the DLNAPlayer's established lastStartTime+stopwatch pattern: - PlayTrack and Continue now start the stopwatch and set state=playing - PlayTrack now honors startTime (needed for resuming a paused track loaded at startup) by seeking after JukeboxStart, matching how DLNAPlayer.PlayFile handles it - Pause() now stops the stopwatch instead of leaving a TODO; the Stopwatch type already accumulates elapsed time correctly across Start/Stop pairs, so no extra bookkeeping is needed - Stop() resets lastStartTime/stopwatch like DLNAPlayer.Stop() does Also removed startTrackTime/startedAtUnixMilli and the startAndUpdateTime() helper: they were a half-built, never-wired-up attempt at round-trip-latency compensation for the start command. That compensation only makes sense once the position is reconciled against the server's own authoritative status (JukeboxGetStatus), which belongs with the status-polling work, not the basic start path. GetStatus() now also reports Duration (curTrackDuration), previously a bare TODO comment. Tracking upstream supersonic-app#300.
The Subsonic jukebox API has no push/event mechanism, so all of this
is driven by polling JukeboxGetStatus() at two points:
- handleOnTrackChange(): fires when the local trackChangeTimer
estimates the current track has ended. Since that's only an
estimate, it always reconciles against JukeboxGetStatus first:
- server reports !Playing -> queue exhausted, report stopped
- server's CurrentTrack index unchanged -> our timer fired early
(clock drift); resync position and re-arm for the real
remaining time instead of treating it as a track change
- server's CurrentTrack index advanced -> genuine track change;
adopt the queued next-track duration (captured via
SetNextTrack), reset local position tracking to the server's
reported position, and fire InvokeOnTrackChange
A failed status call retries after a short delay rather than giving
up silently.
- scheduleSync(): run once, some time after PlayTrack/Continue/
SeekSeconds, to correct for command round-trip latency the same way
DLNAPlayer's syncPlaybackTime() does after PlayFile/SeekSeconds.
Along the way:
- SetNextTrack(nil) is now handled (previously nil-derefed on
track.ID); the engine calls it with nil to clear a queued next
track, exactly like DLNAPlayer.SetNextFile("", ...) does for DLNA.
- trackChangeTimer is now actually part of the Pause/Stop/Continue/
PlayTrack/SeekSeconds lifecycle (armed with the remaining time on
start/resume/seek, cancelled on pause/stop), rather than only ever
touched from the still-stubbed track-change handler.
- Fixed remainingTrackTime() to account for elapsed time since the
last position sync (via curPlayPos()), not just the start offset -
otherwise Continue() after a Pause would re-arm the timer for the
full track duration instead of what's actually left.
- PlayTrack now honors startTime (see previous commit) combined with
the new timer arithmetic.
- destroyed-guards added to all provider-calling methods, and
Destroy() now actually cancels the pending timer, matching
DLNAPlayer's lifecycle handling.
Tracking upstream supersonic-app#300.
Not every server exposing a Subsonic-compatible API actually allows jukebox playback (it can be disabled globally or per-user), and the Subsonic API has no capability-negotiation mechanism for it the way OpenSubsonic extensions provide for other optional features. Without this, any server that type-asserts to mediaprovider.JukeboxProvider (currently: all Subsonic servers) would show a "Jukebox" cast option that fails outright for users whose server doesn't support/allow it. - mediaprovider.JukeboxProvider gains JukeboxSupported() bool. subsonicMediaProvider implements it with a side-effect-free "status" probe call, cached for the provider's lifetime via sync.Once (same pattern already used for playbackReportSupported). - PlaybackManager caches the result separately, refreshed by a background goroutine kicked off from ServerManager.OnServerConnected (and cleared on OnLogout/reconnect). RemotePlayers() - called synchronously from the UI cast menu - only ever reads this cached flag, so opening the cast menu can never block on the network probe. Tracking upstream supersonic-app#300.
Since RemotePlayers() silently omits the Jukebox cast entry when JukeboxSupported() comes back false, there was previously no way to tell from the running app why it isn't showing up (wrong server type, jukebox disabled, no permission, etc). Log the probe error once.
Reproduced the freeze reported when switching from Jukebox back to "This computer" mid-playback: the cast button stayed disabled and playback controls stopped responding entirely. Root cause was in TrackChangeTimer (shared by DLNAPlayer and JukeboxPlayer), not in either player itself. Its old Reset() implementation handed off cancel/reschedule requests to a dispatcher goroutine over an unbuffered channel. If two Reset() calls raced - e.g. Stop()'s Reset(0) (cancel) against JukeboxPlayer's scheduleSync background goroutine calling Reset(nonzero) roughly 2s after a prior Continue/PlayTrack/SeekSeconds - and the dispatcher received the Reset(0) first, it would exit immediately, permanently orphaning the other, still-blocked sender. That caller (Stop(), called from PlaybackManager.SetRemotePlayer while switching players) would then hang forever, so SetRemotePlayer never returns, its deferred re-enable-cast-button callback never runs, and since JukeboxPlayer's Continue()/Pause() also call trackChangeTimer.Reset(), any playback command processed afterward by the single-goroutine command queue hangs the same way, freezing all playback controls. JukeboxPlayer's scheduleSync (added for the status-polling work) made this much easier to hit than in DLNAPlayer, since it fires reliably ~2s after every start/seek, but the bug was latent in DLNAPlayer's equivalent syncPlaybackTime() pattern too. Replaced the channel/atomic.Bool dispatcher-goroutine design with a mutex-protected *time.Timer + time.AfterFunc, which has no handoff race to begin with. Same public API (NewTrackChangeTimer/Reset), so no changes needed in either player. Added a regression test that reliably deadlocks against the old implementation (verified manually) and passes against the new one.
The engine only advances its own now-playing index (and therefore the UI's "now playing" display) in response to the player's OnTrackChange callback - both for user-initiated track changes (skip/select) and for a player-internal auto-advance. PlayTrack was only invoking OnPlaying, so skipping to a track while on the Jukebox player would audibly start the new track on the server but leave the UI showing the previous one, since the engine never got the signal to update nowPlayingIdx. DLNAPlayer.PlayFile already does this correctly (OnPlaying, then OnTrackChange, then OnSeek if resuming at a nonzero position) - brought PlayTrack in line with the same sequence.
j.volume was purely a local cache, only ever populated by prior calls to SetVolume() on this exact JukeboxPlayer instance. Right after switching to Jukebox, PlaybackManager immediately calls GetVolume() to decide whether to fire a volume-changed UI callback (see playbackEngine.SetPlayer) - on a freshly constructed JukeboxPlayer that field is still its zero value, so this always reported 0 and yanked the volume slider down to 0 regardless of the jukebox's actual volume on the server. GetVolume now queries JukeboxGetStatus for the real current value (falling back to the last-known one on error), matching how DLNAPlayer.GetVolume queries the actual render control endpoint rather than trusting a local cache.
… index Reported bug: playing through an album via Jukebox, a track would finish, the UI would correctly advance to the next track, but the audio would restart the *previous* track instead of continuing to the one after. SetNextTrack (called by the engine ~10s before the current track ends, to gapless-preload the next one) computed which server-queue index to remove/replace using the locally-cached j.curTrack. That cache is only updated when the track-change timer fires and reconciles against JukeboxGetStatus - and that timer is armed from our own estimate of the current track's duration, which can run behind the server's actual timing (e.g. metadata duration slightly longer than what's actually played). In that window, the server has already auto-advanced to the track we previously queued via SetNextTrack, but j.curTrack still points at the now-finished track. SetNextTrack then computed JukeboxRemove(j.curTrack + 1) against that stale index, which lands on the currently-*playing* entry rather than a queued-but-unplayed one - removing the active track apparently made the server jukebox fall back to the previous one. Fixed by having SetNextTrack fetch a fresh JukeboxGetStatus first and reconcile j.curTrack against it before computing any index, via a new reconcileWithStatus() helper shared with handleOnTrackChange. If that reconciliation reveals the server already advanced, it performs the same bookkeeping as a natural track change (including InvokeOnTrackChange, so the engine's now-playing index/UI stays correct) *before* SetNextTrack acts on the now-current curTrack - rather than SetNextTrack silently correcting its local copy and leaving the engine to find out later (which would just move the "stuck on the previous track" UI bug from supersonic-app#300's original report to a different trigger). Also replaced the queueLength-derived "is something already queued" check with the existing hasNextTrack bool, which is more direct and was already the field actually driving next-track-duration lookup; queueLength was otherwise unused and never itself reconciled against the server, so it was just another source of drift. Not independently verified against a live server for this specific fix (multi-track queue timing is hard to reproduce without one) - please re-test an album playthrough via Jukebox.
Reported: playing an album via Jukebox, track 1 finishes and track 2
audibly starts, but the UI shows nothing playing at all; when track 2
then ends, track 3 never plays - playback stops completely.
handleOnTrackChange() treated any JukeboxGetStatus response with
Playing=false as "the queue is exhausted", unconditionally setting
state=stopped and firing InvokeOnStopped. But the Subsonic jukebox
backend doesn't switch tracks instantaneously - Navidrome's logs show
it spinning up a fresh mpv process per track ("Starting trackSwitcher
goroutine" / "Found mpv") - so there's a real window right at a track
boundary where the server legitimately reports not-playing even though
it's about to continue to the next queued track. If our poll happens
to land in that window, we had no way to tell that apart from the
queue genuinely being empty.
That misdiagnosis is doubly damaging: InvokeOnStopped makes the engine
drop nowPlayingIdx to -1 (NowPlaying() UI goes blank even though audio
is audibly still playing), and since nothing else re-arms
trackChangeTimer once state is stopped, the reconciliation loop this
whole feature depends on is now permanently dead - every subsequent
track in the queue silently never plays.
Fixed by checking hasNextTrack: if we know a track was queued to
follow this one, a not-playing status is treated as "server hasn't
started it yet" and retried shortly, rather than as the queue being
done. Only declare stopped when hasNextTrack is false, i.e. we never
queued anything further (last track, or SetNextTrack(nil) was called).
Not independently verified against a live server (this exact timing
window is inherently racy/hard to force) - please re-test an album
playthrough via Jukebox, in particular letting 3+ tracks play through
naturally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of all I'm happy I've found this lightweight no-BS player. However, I really miss the jukebox feature and luckily there's already a branch for it, so I decided to spin up my agent and finish it.
I'm already testing the code and it works fine for me. I can freely switch between "This computer" and "Jukebox" and the playback resumes seamlessly.
The following is the AI summary of the changes:
Summary
Continues/completes the
jukeboxbranch (started by @dweymouth) toward closing #300. As noted in that issue's comments, the branch had a working backend API layer (mediaprovider.JukeboxProvider+ Subsonic implementation) but was never wired up to be reachable or fully functional.This PR:
JukeboxPlayerintoPlaybackManager's cast-device list, so it's selectable alongside DLNA devicesJukeboxPlayer(the stopwatch was never actually started,PlayTracknever setstate=playing,GetStatus()never reportedDuration)JukeboxGetStatus, and track-change detection (the Subsonic jukebox API has no push/event mechanism, so this is all poll-driven, mirroringDLNAPlayer'ssyncPlaybackTime/handleOnTrackChangepattern)common.TrackChangeTimer(shared byDLNAPlayerandJukeboxPlayer) that could deadlock the entire playback command queue - found via manual testing of this branch, see commitcommon: fix TrackChangeTimer deadlock on concurrent Reset callsfor the full root-cause writeupInvokeOnTrackChangewas never fired fromPlayTrack)GetVolume()reporting 0 right after switching to Jukebox instead of the server's actual current volumeEach commit is self-contained with a detailed message; probably easiest to review commit-by-commit rather than as a squashed diff.
Testing
Manually tested end-to-end against a real Navidrome instance (0.63.2): connecting, switching to/from Jukebox mid-playback, play/pause/seek/skip, volume control, and multi-track queue playback. Added a regression test for the
TrackChangeTimerdeadlock fix (go test ./backend/player/common/... -race).Not yet covered (opening as draft for this reason):
JukeboxPlayer's state machineen.json)Notes for review
JukeboxSupported()probes via astatusjukeboxControlcall and caches the result for the provider's lifetime - happy to adjust the caching strategy if there's a preference (e.g. re-probing after some interval).