From 70376a882d6c4ac99e0065d179e516193d8d76a5 Mon Sep 17 00:00:00 2001 From: palchrb Date: Sun, 2 Aug 2026 13:52:38 +0000 Subject: [PATCH] feat: debounce rapid next/prev into a single deferred track load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Holding down next on a Connect controller currently loads every track it passes through: each press resolves a track, requests an audio key and opens a stream, only to throw all of it away when the next press arrives milliseconds later. Skipping ten tracks costs ten audio-key requests, and Spotify starts answering 429 well before a listener has found what they were looking for. Split the advance into a pointer move and a load. A skip that arrives inside skip_debounce_ms of the previous one moves the pointer and arms a settle timer instead of loading; the timer is reset by every further skip, so a burst of any length resolves to exactly one load of the track the user actually stopped on. The first skip of a burst still loads immediately, so a single press is as responsive as before, and setting skip_debounce_ms to 0 disables debouncing entirely. The settle timer is a case in the existing Run loop select, alongside the prefetch and state timers, so the deferred load runs on the same goroutine as every other player mutation and needs no locking. Clients are not left guessing during the wait: every pointer move emits will_play with the pending track, and the daemon exposes the pending position so a UI can follow the burst in real time. The window defaults to 600ms, chosen from measurement rather than taste. Burst detection measures the gap between one skip finishing and the next arriving, and that gap is far wider than the interval between presses when the controller is the Spotify app, which paces its Connect commands. Cached tracks widen it further rather than narrowing it: they load in a fraction of the time, so the daemon spends longer idle between skips and each press starts to look like the first press of a new burst. Across two measured sessions of continuous mashing from the app, the gaps ranged from 74ms to 495ms — a 400ms window classified a third of one burst as fresh presses. The costs of mis-sizing are asymmetric: a window that is slightly too small splits a burst and costs one extra load, while a window that is too large adds its full length to the silence after the last press of every burst. 600ms covers everything observed with headroom without making the settle noticeably laggy. Deferred pointer moves do not publish the connect state. Each one is superseded within the window, so PUTting them describes a track that is never loaded and outruns the endpoint's coalescing interval — the same 429 the feature exists to avoid, on a different endpoint. Only the first deferral publishes, which is what tells the app that skipping has started, and the settled load publishes the track that was chosen. A client driving its own UI from the websocket is unaffected, since will_play still fires on every move. Several paths need to know a load is outstanding. Prefetching is suppressed during a settle, since it would perform the very requests being deferred. The not-playing and stop player events are ignored, because the pointer has already moved past the track that ended and the output will be reopened by the settle. A seek flushes the pending load first, so it acts on the track the user chose rather than the one still playing. The wait itself is not counted as playback, so the settled track starts at 0:00 instead of skip_debounce_ms in. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019FHWG4ossSBydy7jbVFWho --- README.md | 1 + api-spec.yml | 10 ++ cmd/daemon/cli_config.go | 5 + cmd/daemon/cli_config_test.go | 9 ++ config_schema.json | 6 + daemon/api_gen.go | 3 + daemon/app.go | 3 + daemon/config.go | 8 ++ daemon/controls.go | 245 +++++++++++++++++++++++++++++----- daemon/controls_test.go | 225 +++++++++++++++++++++++++++++++ daemon/player.go | 47 +++++++ 11 files changed, 527 insertions(+), 35 deletions(-) create mode 100644 daemon/controls_test.go diff --git a/README.md b/README.md index 902f9422..decd2ef1 100644 --- a/README.md +++ b/README.md @@ -252,6 +252,7 @@ audio_output_pipe: '' # Path to a named pipe for audio output audio_output_pipe_format: s16le # Audio output pipe format (s16le, s32le, f32le) bitrate: 160 # Playback bitrate (96, 160, 320) crossfade_duration: 0 # Crossfade duration between tracks in milliseconds (0 to disable) +skip_debounce_ms: 600 # Coalesce rapid next/prev presses; the selected track loads once presses stop (0 to disable) volume_steps: 100 # Volume steps count initial_volume: 100 # Initial volume in steps (not applied to the mixer device) ignore_last_volume: false # Whether to ignore the last saved volume and always use initial_volume diff --git a/api-spec.yml b/api-spec.yml index 9306e1aa..5ff27940 100644 --- a/api-spec.yml +++ b/api-spec.yml @@ -170,6 +170,16 @@ components: allOf: - $ref: '#/components/schemas/track' nullable: true + pending_track_uri: + description: >- + The track selected by a not-yet-settled skip: rapid next/prev + presses move the selection immediately but defer loading it until + the presses stop (skip_debounce_ms), so while browsing this holds + the target track URI and the track object still describes the last + loaded one. Absent when no skip is pending. + type: string + nullable: true + x-omitempty: true root: x-go-name: ApiRoot description: API reachability and playback readiness diff --git a/cmd/daemon/cli_config.go b/cmd/daemon/cli_config.go index 6a5ec777..dd8ead94 100644 --- a/cmd/daemon/cli_config.go +++ b/cmd/daemon/cli_config.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strconv" "strings" + "time" "github.com/devgianlu/go-librespot/daemon" "github.com/gofrs/flock" @@ -53,6 +54,7 @@ type cliConfig struct { NormalisationUseAlbumGain bool `koanf:"normalisation_use_album_gain"` NormalisationPregain float32 `koanf:"normalisation_pregain"` CrossfadeDuration int `koanf:"crossfade_duration"` + SkipDebounceMs int `koanf:"skip_debounce_ms"` ExternalVolume bool `koanf:"external_volume"` ZeroconfEnabled bool `koanf:"zeroconf_enabled"` ZeroconfPort int `koanf:"zeroconf_port"` @@ -119,6 +121,7 @@ func (c *cliConfig) toDaemonConfig() *daemon.Config { NormalisationUseAlbumGain: c.NormalisationUseAlbumGain, NormalisationPregain: c.NormalisationPregain, CrossfadeDuration: c.CrossfadeDuration, + SkipDebounce: time.Duration(c.SkipDebounceMs) * time.Millisecond, ExternalVolume: c.ExternalVolume, DisableAutoplay: c.DisableAutoplay, @@ -199,6 +202,8 @@ func loadCLIConfig(cfg *cliConfig) error { "volume_steps": 100, "initial_volume": 100, + "skip_debounce_ms": 600, + "credentials.type": "zeroconf", "cache.enabled": false, diff --git a/cmd/daemon/cli_config_test.go b/cmd/daemon/cli_config_test.go index 0fd6b6b9..55149070 100644 --- a/cmd/daemon/cli_config_test.go +++ b/cmd/daemon/cli_config_test.go @@ -8,6 +8,15 @@ import ( "github.com/stretchr/testify/require" ) +func TestSkipDebounceMapping(t *testing.T) { + var c cliConfig + c.SkipDebounceMs = 400 + require.Equal(t, int64(400_000_000), int64(c.toDaemonConfig().SkipDebounce)) + + c.SkipDebounceMs = 0 + require.Zero(t, c.toDaemonConfig().SkipDebounce) +} + func TestParseSize(t *testing.T) { cases := []struct { in string diff --git a/config_schema.json b/config_schema.json index a840072a..bb795bd7 100644 --- a/config_schema.json +++ b/config_schema.json @@ -308,6 +308,12 @@ "default": 0, "minimum": 0 }, + "skip_debounce_ms": { + "type": "integer", + "description": "How long to wait after a burst of next/prev commands before loading the selected track, in milliseconds, so rapid skipping costs one audio-key request instead of one per press. The first skip of a burst always loads immediately, so this only affects how long the daemon waits to see whether another skip follows. 0 disables debouncing", + "default": 600, + "minimum": 0 + }, "external_volume": { "type": "boolean", "description": "Whether volume is controlled externally, if true the app will not pre-multiply samples", diff --git a/daemon/api_gen.go b/daemon/api_gen.go index a469a937..c53277f8 100644 --- a/daemon/api_gen.go +++ b/daemon/api_gen.go @@ -118,6 +118,9 @@ type ApiStatus struct { // Paused Whether the player is paused Paused bool `json:"paused"` + // PendingTrackUri The track selected by a not-yet-settled skip: rapid next/prev presses move the selection immediately but defer loading it until the presses stop (skip_debounce_ms), so while browsing this holds the target track URI and the track object still describes the last loaded one. Absent when no skip is pending. + PendingTrackUri *string `json:"pending_track_uri,omitempty"` + // PlayOrigin Who started the playback, "go-librespot" identifies the API as the play origin, everything else is Spotify own stuff PlayOrigin string `json:"play_origin"` diff --git a/daemon/app.go b/daemon/app.go index 3ec79882..270ef043 100644 --- a/daemon/app.go +++ b/daemon/app.go @@ -223,6 +223,9 @@ func (app *App) newAppPlayer(ctx context.Context, creds any) (_ *AppPlayer, err appPlayer.prefetchTimer = time.NewTimer(math.MaxInt64) appPlayer.prefetchTimer.Stop() + appPlayer.settleTimer = time.NewTimer(math.MaxInt64) + appPlayer.settleTimer.Stop() + if appPlayer.sess, err = session.NewSessionFromOptions(ctx, &session.Options{ Log: app.log, DeviceType: app.deviceType, diff --git a/daemon/config.go b/daemon/config.go index bf067c4f..820d417f 100644 --- a/daemon/config.go +++ b/daemon/config.go @@ -1,5 +1,7 @@ package daemon +import "time" + // Config carries the runtime configuration for a daemon instance. type Config struct { DeviceId string @@ -28,6 +30,12 @@ type Config struct { ExternalVolume bool DisableAutoplay bool + // SkipDebounce is how long to wait after a burst of next/prev commands + // before actually loading the track the pointer landed on, so mashing the + // skip button costs one audio-key request instead of one per press. Zero + // disables debouncing (every skip loads immediately). + SkipDebounce time.Duration + ZeroconfEnabled bool ZeroconfPort int ZeroconfBackend string diff --git a/daemon/controls.go b/daemon/controls.go index 50b9e4df..42b65b29 100644 --- a/daemon/controls.go +++ b/daemon/controls.go @@ -23,6 +23,12 @@ import ( ) func (p *AppPlayer) prefetchNext(ctx context.Context) { + // A deferred skip is waiting: prefetching would perform the exact network + // requests (audio key included) the settle is deferring. + if p.settlePending { + return + } + // Limit ourselves to 30 seconds for prefetching ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() @@ -193,6 +199,13 @@ func (p *AppPlayer) handlePlayerEvent(ctx context.Context, ev *player.Event) { }, }) case player.EventTypeNotPlaying: + // A deferred skip already moved the pointer past this track; the settle + // timer will load it. Advancing here would advance a second time. + if p.settlePending { + p.app.log.Tracef("ignoring not playing event during pending settle") + break + } + p.sess.Events().OnPlayerEnd(p.primaryStream, p.state.trackPosition()) // Played to the end: clear the resume point before advancing, so that @@ -225,6 +238,13 @@ func (p *AppPlayer) handlePlayerEvent(ctx context.Context, ev *player.Event) { p.emitMprisUpdate(mpris.Stopped) } case player.EventTypeStop: + // The output closed while a deferred load is pending; the settle will + // re-open it, so this is not a terminal stop. + if p.settlePending { + p.app.log.Tracef("ignoring stop event during pending settle") + break + } + p.app.server.Emit(&ApiEvent{ Type: ApiEventTypeStopped, Data: ApiEventDataStopped{ @@ -240,6 +260,9 @@ func (p *AppPlayer) handlePlayerEvent(ctx context.Context, ev *player.Event) { type skipToFunc func(*connectpb.ContextTrack) bool func (p *AppPlayer) loadContext(ctx context.Context, spotCtx *connectpb.Context, skipTo skipToFunc, paused, drop bool) error { + // A new context supersedes any deferred skip or key-retry backoff. + p.cancelSettle() + ctxTracks, err := tracks.NewTrackListFromContext(ctx, p.app.log, p.sess.Spclient(), spotCtx) if err != nil { return fmt.Errorf("failed creating track list: %w", err) @@ -595,6 +618,14 @@ func (p *AppPlayer) pause(ctx context.Context) error { } func (p *AppPlayer) seek(ctx context.Context, position int64) error { + // A deferred skip is waiting: load it now so the seek targets the track the + // pointer is on, not the stale stream from before the burst. + if p.settlePending { + if err := p.settleNow(ctx); err != nil { + return fmt.Errorf("failed settling before seek: %w", err) + } + } + if p.primaryStream == nil { return fmt.Errorf("no primary stream") } @@ -634,8 +665,129 @@ func (p *AppPlayer) seek(ctx context.Context, position int64) error { return nil } +// shouldDeferSkip reports whether this skip is part of a burst and its load should be +// debounced: a settle is already pending, or the previous skip finished less than +// SkipDebounce ago. The first skip of a burst always loads immediately (leading edge), +// so single presses keep today's behavior exactly; a zero SkipDebounce disables +// debouncing entirely. +func (p *AppPlayer) shouldDeferSkip() bool { + // Without a context there is no pointer to move and no track to publish + // (deferSettle needs state.player.Track); the inline path handles the + // no-context case safely. + if p.state.tracks == nil { + return false + } + + d := p.app.cfg.SkipDebounce + if d <= 0 { + return false + } + + // Log the decision with the gap that produced it: the useful signal when + // tuning skip_debounce_ms is how far apart skips actually arrive, which + // varies a lot by controller (a physical button mashes far faster than the + // Spotify app, which paces its Connect commands) and by how fast loads + // complete (a cached track loads in a fraction of the time, which widens + // the gap between one skip finishing and the next arriving). + since := time.Since(p.lastSkipDone) + defer_ := p.settlePending || since < d + if p.lastSkipDone.IsZero() { + p.app.log.Debugf("skip: loading inline (first skip, window %s)", d) + } else { + p.app.log.Debugf("skip: %s (%dms since last skip, window %s, settle pending: %t)", + map[bool]string{true: "deferring", false: "loading inline"}[defer_], + since.Milliseconds(), d, p.settlePending) + } + + return defer_ +} + +// deferSettle publishes the pending pointer position (will_play event, plus a connect-state +// PUT on the first deferral) and (re)arms the settle timer. Runs after every deferred +// pointer move. +func (p *AppPlayer) deferSettle(ctx context.Context) { + first := !p.settlePending + + p.settlePending = true + p.settleTimer.Reset(p.app.cfg.SkipDebounce) + p.lastSkipDone = time.Now() + + p.app.log.WithField("uri", p.state.player.Track.GetUri()). + Debugf("deferred skip, settling in %s unless another skip arrives", p.app.cfg.SkipDebounce) + + // Publish the pending track without a stream: buffering, position 0. + p.state.updateTimestamp() + p.state.player.IsPlaying = true + p.state.player.IsBuffering = true + p.state.player.PlaybackSpeed = 0 + + // Only the first deferral of a burst PUTs the connect state. Every later pointer + // move is superseded within skip_debounce_ms, so PUTting each one buys nothing but + // rate-limit pressure: a long burst moves the pointer faster than the 200ms + // coalescing interval and the endpoint starts answering 429. Local subscribers + // still see every move through the will_play event below, and the settled load + // publishes the state that actually matters. + if first { + p.updateState(ctx) + } + + p.app.server.Emit(&ApiEvent{ + Type: ApiEventTypeWillPlay, + Data: ApiEventDataWillPlay{ + ContextUri: p.state.player.ContextUri, + Uri: p.state.player.Track.Uri, + PlayOrigin: p.state.playOrigin(), + }, + }) +} + +// settleNow loads whatever track the pointer currently points at. Called from the Run +// loop when the settle timer fires, or inline for non-burst skips and seek flushes. +func (p *AppPlayer) settleNow(ctx context.Context) error { + p.settleTimer.Stop() + p.settlePending = false + atEnd := p.settleAtEnd + p.settleAtEnd = false + + // The time spent waiting for the settle timer is not playback: refresh the + // timestamp (PlaybackSpeed is 0 while pending, so the position stays put) + // so the track loads at its actual position instead of position+debounce. + p.state.updateTimestamp() + + if p.state.tracks == nil && !atEnd { + return nil + } + + hasNextTrack, err := p.finishAdvance(ctx, !atEnd, true) + if err != nil { + return err + } + + // if no track to be played, just stop + if !hasNextTrack { + p.app.server.Emit(&ApiEvent{ + Type: ApiEventTypeStopped, + Data: ApiEventDataStopped{ + PlayOrigin: p.state.playOrigin(), + }, + }) + } + + return nil +} + +// cancelSettle abandons a pending deferred load: whatever superseded it (new +// context, transfer, stop) owns playback now. +func (p *AppPlayer) cancelSettle() { + p.settleTimer.Stop() + p.settlePending = false + p.settleAtEnd = false +} + func (p *AppPlayer) skipPrev(ctx context.Context, allowSeeking bool) error { - if allowSeeking && p.player.PositionMs() > 3000 { + // The old stream's position is meaningless while a settle is pending: a + // restart-current-track seek must not fire mid-burst. + if allowSeeking && !p.settlePending && p.player.PositionMs() > 3000 { return p.seek(ctx, 0) } @@ -654,13 +806,18 @@ func (p *AppPlayer) skipPrev(ctx context.Context, allowSeeking bool) error { p.state.player.Timestamp = time.Now().UnixMilli() p.state.player.PositionAsOfTimestamp = 0 - p.resumeCurrentEpisode(ctx) + p.settleAtEnd = false - // load current track into stream - if err := p.loadCurrentTrack(ctx, p.state.player.IsPaused, true); err != nil { - return fmt.Errorf("failed loading current track (skip prev): %w", err) + if p.shouldDeferSkip() { + p.deferSettle(ctx) + return nil } + err := p.settleNow(ctx) + p.lastSkipDone = time.Now() + if err != nil { + return fmt.Errorf("failed loading current track (skip prev): %w", err) + } return nil } @@ -681,30 +838,23 @@ func (p *AppPlayer) skipNext(ctx context.Context, track *connectpb.ContextTrack) p.state.player.NextTracks = p.state.tracks.NextTracks(ctx, nil) p.state.player.Index = p.state.tracks.Index() - p.resumeCurrentEpisode(ctx) - - if err := p.loadCurrentTrack(ctx, p.state.player.IsPaused, true); err != nil { - return err - } - return nil + p.settleAtEnd = false } else { - hasNextTrack, err := p.advanceNext(ctx, true, true) - if err != nil { - return fmt.Errorf("failed skipping to next track: %w", err) - } - - // if no track to be played, just stop - if !hasNextTrack { - p.app.server.Emit(&ApiEvent{ - Type: ApiEventTypeStopped, - Data: ApiEventDataStopped{ - PlayOrigin: p.state.playOrigin(), - }, - }) - } + hasNextTrack := p.advancePointerNext(ctx, true) + p.settleAtEnd = !hasNextTrack + } + if p.shouldDeferSkip() { + p.deferSettle(ctx) return nil } + + err := p.settleNow(ctx) + p.lastSkipDone = time.Now() + if err != nil { + return fmt.Errorf("failed skipping to next track: %w", err) + } + return nil } // maxConsecutiveUnplayableSkips caps how many refused/restricted tracks advanceNext will skip @@ -712,7 +862,14 @@ func (p *AppPlayer) skipNext(ctx context.Context, track *connectpb.ContextTrack) const maxConsecutiveUnplayableSkips = 50 func (p *AppPlayer) advanceNext(ctx context.Context, forceNext, drop bool) (bool, error) { - var uri string + hasNextTrack := p.advancePointerNext(ctx, forceNext) + return p.finishAdvance(ctx, hasNextTrack, drop) +} + +// advancePointerNext moves the track pointer forward (repeat-track, queue and +// end-of-context rules) without loading anything, and reports whether a next +// track is available to play. +func (p *AppPlayer) advancePointerNext(ctx context.Context, forceNext bool) bool { var hasNextTrack bool if p.state.tracks != nil { if !forceNext && p.state.player.Options.RepeatingTrack { @@ -739,13 +896,22 @@ func (p *AppPlayer) advanceNext(ctx context.Context, forceNext, drop bool) (bool p.state.player.PrevTracks = p.state.tracks.PrevTracks() p.state.player.NextTracks = p.state.tracks.NextTracks(ctx, nil) p.state.player.Index = p.state.tracks.Index() - - uri = p.state.player.Track.Uri } p.state.player.Timestamp = time.Now().UnixMilli() p.state.player.PositionAsOfTimestamp = 0 + return hasNextTrack +} + +// finishAdvance loads the track the pointer is currently on: autoplay resolution when +// the context ended and the bounded unplayable walk. +func (p *AppPlayer) finishAdvance(ctx context.Context, hasNextTrack, drop bool) (bool, error) { + var uri string + if p.state.tracks != nil { + uri = p.state.player.Track.Uri + } + if !hasNextTrack && !p.app.cfg.DisableAutoplay && !strings.HasPrefix(p.state.player.ContextUri, "spotify:station:") { p.state.player.Suppressions = &connectpb.Suppressions{} @@ -787,17 +953,27 @@ func (p *AppPlayer) advanceNext(ctx context.Context, forceNext, drop bool) (bool p.state.player.IsBuffering = false } + // Look up the episode resume point at the settle, not per pointer move: a + // deferred skip that browses across episodes must not pay a resumption + // lookup per press, only the episode that actually loads does. p.resumeCurrentEpisode(ctx) - // load current track into stream. - // + // load current track into stream. The paused state was already decided by the pointer + // move (advancePointerNext sets IsPaused, skip/settle paths preserve the user's), so + // respect it; a context that ended without repeat additionally loads paused. + err := p.loadCurrentTrack(ctx, p.state.player.IsPaused || !hasNextTrack, drop) + if err == nil { + p.consecutiveUnplayableSkips = 0 + return hasNextTrack, nil + } + // BAND-AID: Spotify makes a per-track, context-dependent decision on granting the legacy // AES audio key. License-gated tracks are refused (AesKeyError, e.g. code 1) in ordinary // playlist playback — even though they play on official clients, which establish a licensed // context. We cannot decrypt a refused track, so skip it instead of freezing the player. // Remove once proper key licensing (PlayPlay) is implemented — tracked separately. var keyErr *audio.KeyProviderError - if err := p.loadCurrentTrack(ctx, !hasNextTrack, drop); errors.Is(err, librespot.ErrMediaRestricted) || errors.Is(err, librespot.ErrNoSupportedFormats) || errors.As(err, &keyErr) { + if errors.Is(err, librespot.ErrMediaRestricted) || errors.Is(err, librespot.ErrNoSupportedFormats) || errors.As(err, &keyErr) { if keyErr != nil { p.app.log.WithError(err).Warnf("skipping track: Spotify refused the audio key (code %d) for this playback context: %s", keyErr.Code, uri) } else { @@ -814,13 +990,10 @@ func (p *AppPlayer) advanceNext(ctx context.Context, forceNext, drop bool) (bool return false, err } return p.advanceNext(ctx, true, drop) - } else if err != nil { - p.consecutiveUnplayableSkips = 0 - return false, fmt.Errorf("failed loading current track (advance to %s): %w", uri, err) } p.consecutiveUnplayableSkips = 0 - return hasNextTrack, nil + return false, fmt.Errorf("failed loading current track (advance to %s): %w", uri, err) } // Return the volume as an integer in the range 0..player.MaxStateVolume, as @@ -877,6 +1050,8 @@ func (p *AppPlayer) volumeUpdated(ctx context.Context) { } func (p *AppPlayer) stopPlayback(ctx context.Context) error { + p.cancelSettle() + p.player.Stop() p.primaryStream = nil p.secondaryStream = nil diff --git a/daemon/controls_test.go b/daemon/controls_test.go new file mode 100644 index 00000000..eaa54920 --- /dev/null +++ b/daemon/controls_test.go @@ -0,0 +1,225 @@ +package daemon + +import ( + "context" + "math" + "sync" + "testing" + "time" + + librespot "github.com/devgianlu/go-librespot" + connectpb "github.com/devgianlu/go-librespot/proto/spotify/connectstate" + "github.com/devgianlu/go-librespot/tracks" +) + +// recordingApiServer is an ApiServer that records emitted events, so tests can +// assert on the event sequence without a websocket. +type recordingApiServer struct { + mu sync.Mutex + events []ApiEventType +} + +func (s *recordingApiServer) Emit(ev *ApiEvent) { + s.mu.Lock() + s.events = append(s.events, ev.Type) + s.mu.Unlock() +} + +func (s *recordingApiServer) Receive() <-chan ApiRequest { return make(<-chan ApiRequest) } +func (s *recordingApiServer) Close() error { return nil } + +func (s *recordingApiServer) snapshot() []ApiEventType { + s.mu.Lock() + defer s.mu.Unlock() + return append([]ApiEventType(nil), s.events...) +} + +// newSettleTestPlayer builds the minimal AppPlayer needed by the settle +// bookkeeping helpers (shouldDeferSkip, deferSettle, settleNow's no-context +// path, cancelSettle): config, recording event server, state and stopped +// timers. lastStatePut is set to now so updateState defers its PUT to the +// state timer instead of reaching for a live session. +func newSettleTestPlayer(t *testing.T, debounce time.Duration) (*AppPlayer, *recordingApiServer) { + t.Helper() + + server := &recordingApiServer{} + p := &AppPlayer{ + app: &App{ + cfg: &Config{SkipDebounce: debounce}, + server: server, + log: &librespot.NullLogger{}, + }, + state: &State{ + player: &connectpb.PlayerState{ + PlayOrigin: &connectpb.PlayOrigin{FeatureIdentifier: "go-librespot"}, + Track: &connectpb.ProvidedTrack{Uri: "spotify:track:pending"}, + }, + }, + lastStatePut: time.Now(), + } + + p.settleTimer = time.NewTimer(math.MaxInt64) + p.settleTimer.Stop() + p.stateTimer = time.NewTimer(math.MaxInt64) + p.stateTimer.Stop() + + return p, server +} + +func TestShouldDeferSkip(t *testing.T) { + cases := []struct { + name string + debounce time.Duration + hasContext bool + settlePending bool + lastSkipDone time.Time + want bool + }{ + {"disabled never defers", 0, true, true, time.Now(), false}, + {"no context never defers", 400 * time.Millisecond, false, true, time.Now(), false}, + {"first skip is immediate", 400 * time.Millisecond, true, false, time.Time{}, false}, + {"skip after quiet period is immediate", 400 * time.Millisecond, true, false, time.Now().Add(-time.Second), false}, + {"pending settle defers", 400 * time.Millisecond, true, true, time.Time{}, true}, + {"skip right after previous defers", 400 * time.Millisecond, true, false, time.Now(), true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p, _ := newSettleTestPlayer(t, tc.debounce) + if tc.hasContext { + p.state.tracks = &tracks.List{} + } + p.settlePending = tc.settlePending + p.lastSkipDone = tc.lastSkipDone + + if got := p.shouldDeferSkip(); got != tc.want { + t.Fatalf("shouldDeferSkip() = %t, want %t", got, tc.want) + } + }) + } +} + +func TestDeferSettlePublishesPendingTrack(t *testing.T) { + p, server := newSettleTestPlayer(t, 400*time.Millisecond) + + p.deferSettle(context.Background()) + + if !p.settlePending { + t.Fatal("expected settlePending after deferSettle") + } + if !p.state.player.IsPlaying || !p.state.player.IsBuffering { + t.Fatalf("expected the pending track to be published as playing+buffering, got playing=%t buffering=%t", + p.state.player.IsPlaying, p.state.player.IsBuffering) + } + if time.Since(p.lastSkipDone) > time.Second { + t.Fatal("expected lastSkipDone to be refreshed") + } + + events := server.snapshot() + if len(events) != 1 || events[0] != ApiEventTypeWillPlay { + t.Fatalf("expected exactly one will_play event, got %v", events) + } +} + +// A burst moves the pointer far faster than the connect-state endpoint tolerates, +// so only the first deferral publishes; the rest are visible locally through +// will_play and are superseded before anyone could have seen them remotely. +func TestDeferSettlePutsConnectStateOncePerBurst(t *testing.T) { + p, server := newSettleTestPlayer(t, 400*time.Millisecond) + + p.deferSettle(context.Background()) + if !p.statePutScheduled { + t.Fatal("expected the first deferral of a burst to publish the connect state") + } + + // Pretend the scheduled PUT went out, then keep skipping within the window. + p.statePutScheduled = false + p.stateDirty = false + + for i := 0; i < 5; i++ { + p.deferSettle(context.Background()) + } + + if p.statePutScheduled || p.stateDirty { + t.Fatalf("expected no further connect-state PUTs during the burst, got scheduled=%t dirty=%t", + p.statePutScheduled, p.stateDirty) + } + + if events := server.snapshot(); len(events) != 6 { + t.Fatalf("expected a will_play event for every pointer move, got %v", events) + } +} + +func TestSettleNowWithoutContextIsNoop(t *testing.T) { + p, server := newSettleTestPlayer(t, 400*time.Millisecond) + p.settlePending = true + p.settleAtEnd = false + + if err := p.settleNow(context.Background()); err != nil { + t.Fatalf("settleNow without context failed: %v", err) + } + if p.settlePending || p.settleAtEnd { + t.Fatal("expected settle flags cleared") + } + if events := server.snapshot(); len(events) != 0 { + t.Fatalf("expected no events, got %v", events) + } +} + +// TestSettleNowAtEndWithoutContext locks in the no-context end-of-context +// path: a deferred skip past the end with no track list must not panic and +// must tell clients playback stopped. +func TestSettleNowAtEndWithoutContext(t *testing.T) { + p, server := newSettleTestPlayer(t, 400*time.Millisecond) + p.state.player.Track = nil // fresh state: no track ever loaded + p.settlePending = true + p.settleAtEnd = true + + if err := p.settleNow(context.Background()); err != nil { + t.Fatalf("settleNow at end without context failed: %v", err) + } + if p.settlePending || p.settleAtEnd { + t.Fatal("expected settle flags cleared") + } + + events := server.snapshot() + if len(events) != 1 || events[0] != ApiEventTypeStopped { + t.Fatalf("expected exactly one stopped event, got %v", events) + } +} + +// TestSettleWaitIsNotPlayback locks in the position fix: the time spent +// waiting for the settle timer must not count as playback, otherwise the +// settled track starts ~debounce ms into the song instead of at 0:00. +func TestSettleWaitIsNotPlayback(t *testing.T) { + p, _ := newSettleTestPlayer(t, 400*time.Millisecond) + + // State as a deferred skip leaves it, with the settle wait elapsed. + p.deferSettle(context.Background()) + p.state.player.Timestamp = time.Now().Add(-450 * time.Millisecond).UnixMilli() + p.state.player.PositionAsOfTimestamp = 0 + + // No context: settleNow returns early, but must have refreshed the + // timestamp first so the wait is discarded. + if err := p.settleNow(context.Background()); err != nil { + t.Fatalf("settleNow failed: %v", err) + } + + if pos := p.state.trackPosition(); pos > 50 { + t.Fatalf("expected the settle wait to be discarded from the position, got %dms", pos) + } +} + +func TestCancelSettleClearsEverything(t *testing.T) { + p, _ := newSettleTestPlayer(t, 400*time.Millisecond) + p.settlePending = true + p.settleAtEnd = true + p.settleTimer.Reset(time.Hour) + + p.cancelSettle() + + if p.settlePending || p.settleAtEnd { + t.Fatalf("expected all settle state cleared, got pending=%t atEnd=%t", + p.settlePending, p.settleAtEnd) + } +} diff --git a/daemon/player.go b/daemon/player.go index 428e51ef..b0c7aad3 100644 --- a/daemon/player.go +++ b/daemon/player.go @@ -69,6 +69,19 @@ type AppPlayer struct { prefetchTimer *time.Timer + // settleTimer defers the expensive part of a skip (audio key + stream load) while the + // user is still browsing: every next/prev in a burst just moves the track pointer and + // re-arms this timer, and only the track the pointer lands on gets loaded. Fires in + // Run's select loop. + settleTimer *time.Timer + // settlePending is true while a pointer move (or key retry) awaits its deferred load. + settlePending bool + // settleAtEnd records that the pointer ran past the end of the context during the + // settle window, so the deferred load runs the end-of-context (autoplay/stop) path. + settleAtEnd bool + // lastSkipDone is when the previous skip command finished processing; a skip arriving + // within SkipDebounce of it is part of a burst and gets deferred. + lastSkipDone time.Time // consecutiveUnplayableSkips bounds how many unplayable tracks in a row advanceNext will // skip past (Spotify-refused audio keys / restricted media) before giving up — so a run // of refused tracks (even at the very start of a context) advances to the first playable @@ -206,6 +219,9 @@ func (p *AppPlayer) handlePlayerCommand(ctx context.Context, req dealer.RequestP } p.state.lastTransferTimestamp = transferState.Playback.Timestamp + // The transferred remote state supersedes any deferred skip. + p.cancelSettle() + ctxTracks, err := tracks.NewTrackListFromContext(ctx, p.app.log, p.sess.Spclient(), transferState.CurrentSession.Context) if err != nil { return fmt.Errorf("failed creating track list: %w", err) @@ -492,6 +508,12 @@ func (p *AppPlayer) handleApiRequest(ctx context.Context, req ApiRequest) (any, resp.Track = p.newApiResponseStatusTrack(p.primaryStream, p.state.trackPosition()) } + // While a deferred skip awaits its load, the track object above still + // describes the last loaded stream; expose the pointer's target too. + if p.settlePending && p.state.player.Track != nil { + resp.PendingTrackUri = &p.state.player.Track.Uri + } + return resp, nil case ApiRequestTypeResume: _ = p.play(ctx) @@ -851,6 +873,31 @@ func (p *AppPlayer) Run(ctx context.Context, apiRecv <-chan ApiRequest, mprisRec p.handlePlayerEvent(ctx, &ev) case <-p.prefetchTimer.C: p.prefetchNext(ctx) + case <-p.settleTimer.C: + // e.g. cancelled between Reset and fire + if !p.settlePending { + break + } + + // Limit ourselves to 30 seconds like every other load path: a + // hanging network call here would block the whole control loop. + settleCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + if err := p.settleNow(settleCtx); err != nil { + p.app.log.WithError(err).Error("failed loading settled track") + + // No caller sees this error: tell clients audio is not coming + // instead of stranding them on a forever-buffering state. + if p.state.player.IsPlaying { + p.state.player.IsPlaying = false + p.state.player.IsBuffering = false + p.updateState(settleCtx) + p.app.server.Emit(&ApiEvent{ + Type: ApiEventTypeStopped, + Data: ApiEventDataStopped{PlayOrigin: p.state.playOrigin()}, + }) + } + } + cancel() case volume := <-p.volumeUpdate: // Received a new volume: from Spotify Connect, from the REST API, // or from the system volume mixer.