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.