From 753f135989f79eabd4ef1c5369d58da8cc7ff55b Mon Sep 17 00:00:00 2001 From: devgianlu Date: Fri, 16 Oct 2026 10:22:22 +0200 Subject: [PATCH 1/2] fix: start an external-volume session from the real volume Under external_volume a new session advertised 100% (MaxStateVolume) in its first state, and kept advertising it until the controlling application pushed its level. Every zeroconf takeover and every session restored after a logout opened that window; an idle restored session stayed at 100% until playback started. A phone that steps its volume buttons from what the device advertised sent 96% or 100% from there, and the speaker jumped to it. The app now holds the volume across sessions: every change, from the API or a Spotify client, sets it, and POST /player/volume is accepted while there is no session, including during login. Under external_volume a new session starts at that volume: the last one set in this process, else the saved one unless ignore_last_volume is set, else initial_volume. Co-Authored-By: Claude Opus 5.5 (1M context) --- api-spec.yml | 4 +- daemon/api_server_test.go | 22 +++++++++++ daemon/app.go | 17 ++++++-- daemon/controls.go | 54 ++++++++++++++++++++++--- daemon/controls_test.go | 79 +++++++++++++++++++++++++++++++++++++ daemon/player.go | 13 +----- daemon/player_state.go | 10 ++++- daemon/player_state_test.go | 16 ++++++++ 8 files changed, 192 insertions(+), 23 deletions(-) diff --git a/api-spec.yml b/api-spec.yml index 74749cc2..c741a361 100644 --- a/api-spec.yml +++ b/api-spec.yml @@ -1018,7 +1018,9 @@ paths: $ref: '#/components/schemas/volume' post: operationId: playerSetVolume - description: Set player volume + description: | + Set player volume. Accepted without a session too: the volume is kept, and + with external volume every new session starts from it. requestBody: required: true content: diff --git a/daemon/api_server_test.go b/daemon/api_server_test.go index ba1c7b1a..3e63f8c5 100644 --- a/daemon/api_server_test.go +++ b/daemon/api_server_test.go @@ -17,6 +17,7 @@ import ( "github.com/coder/websocket" librespot "github.com/devgianlu/go-librespot" + "github.com/devgianlu/go-librespot/player" "github.com/devgianlu/go-librespot/spclient" "github.com/stretchr/testify/require" ) @@ -905,6 +906,27 @@ func TestAnswerWithoutSession(t *testing.T) { require.Contains(t, body(t, resp), "someone") } +// A volume set while there is no session is kept for the next one, so its first +// state carries the real level instead of a placeholder. +func TestAnswerWithoutSessionTakesVolume(t *testing.T) { + ts := newTestServer(t, nil) + app, saves := newVolumeTestApp(t, &Config{VolumeSteps: 100, InitialVolume: 100}, nil) + app.server = ts.server + + stop := app.answerWithoutSession(context.Background()) + t.Cleanup(stop) + + resp := ts.do(http.MethodPost, "/player/volume", map[string]any{"volume": 37}) + require.Equal(t, http.StatusOK, resp.StatusCode) + requireSaved(t, saves, player.MaxStateVolume*37/100) + + resp = ts.do(http.MethodPost, "/player/volume", map[string]any{"volume": -5, "relative": true}) + require.Equal(t, http.StatusOK, resp.StatusCode) + requireSaved(t, saves, player.MaxStateVolume*32/100) + + require.Equal(t, uint32(player.MaxStateVolume*32/100), app.sessionVolume()) +} + func TestApiUnknownPathIsNotFound(t *testing.T) { ts := newTestServer(t, okReply) diff --git a/daemon/app.go b/daemon/app.go index 32bb42f7..b8b61094 100644 --- a/daemon/app.go +++ b/daemon/app.go @@ -52,13 +52,18 @@ type App struct { metaCache *trackMetaCache contextLists *contextListCache - // stateMu guards state and the store behind it. Two AppPlayers overlap - // briefly whenever a zeroconf session is replaced, and volume changes are - // written back from off the player loop. + // stateMu guards state and the store behind it, and volume. Two AppPlayers + // overlap briefly whenever a zeroconf session is replaced, and volume changes + // are written back from off the player loop. stateMu sync.Mutex saving bool saveDirty bool + // volume is the last volume set while this process runs, in the range + // 0..player.MaxStateVolume. Unlike a session's own, it outlives sessions, + // and an external controller can set it while there is none. + volume *uint32 + closed bool } @@ -479,6 +484,9 @@ func (app *App) answerWithoutSession(ctx context.Context) (stop func()) { switch req.Type { case ApiRequestTypeRoot: req.Reply(&ApiRoot{}, nil) + case ApiRequestTypeSetVolume: + app.setVolumeWithoutSession(req.Data.(ApiSetVolume)) + req.Reply(nil, nil) default: // Renames included, unlike the zeroconf branch: the login // running meanwhile reads the device name. @@ -634,6 +642,9 @@ func (app *App) withAppPlayer(ctx context.Context, appPlayerFunc func(context.Co switch req.Type { case ApiRequestTypeRoot: req.Reply(&ApiRoot{}, nil) + case ApiRequestTypeSetVolume: + app.setVolumeWithoutSession(req.Data.(ApiSetVolume)) + req.Reply(nil, nil) case ApiRequestSetDeviceName: // The device name drives the zeroconf advertisement, which // runs independently of any player session, so handle it diff --git a/daemon/controls.go b/daemon/controls.go index 4f87502e..cd654b18 100644 --- a/daemon/controls.go +++ b/daemon/controls.go @@ -1628,12 +1628,7 @@ func (p *AppPlayer) updateVolume(newVal uint32) { p.app.log.Debugf("update volume requested to %d/%d", newVal, player.MaxStateVolume) p.player.SetVolume(newVal) - - // Save the volume to the state - p.app.stateMu.Lock() - p.app.state.LastVolume = &newVal - p.app.stateMu.Unlock() - p.app.requestPersist() + p.app.setVolume(newVal) // Replace whatever is already queued. The mixer writes to this channel too, // so the send has to tolerate losing the race for the freed slot: this @@ -1649,6 +1644,53 @@ func (p *AppPlayer) updateVolume(newVal uint32) { } } +// setVolume records newVal, in the range 0..player.MaxStateVolume, as the +// volume that outlives sessions, and saves it. +func (app *App) setVolume(newVal uint32) { + app.stateMu.Lock() + app.volume = &newVal + app.state.LastVolume = &newVal + app.stateMu.Unlock() + app.requestPersist() +} + +// setVolumeWithoutSession takes a volume set through the API while there is no +// session, so that the next one starts from it. +func (app *App) setVolumeWithoutSession(data ApiSetVolume) { + newVal := app.volumeFromApi(data, app.sessionVolume()) + app.log.Debugf("volume set to %d/%d without a session", newVal, player.MaxStateVolume) + app.setVolume(newVal) +} + +// sessionVolume is the volume a session starts at when the volume is external: +// the last one set while this process runs, else the one saved by an earlier +// run unless ignore_last_volume is set, else initial_volume. +func (app *App) sessionVolume() uint32 { + app.stateMu.Lock() + defer app.stateMu.Unlock() + + switch { + case app.volume != nil: + return *app.volume + case !app.cfg.IgnoreLastVolume && app.state.LastVolume != nil: + return *app.state.LastVolume + default: + return min(app.cfg.InitialVolume, app.cfg.VolumeSteps) * player.MaxStateVolume / app.cfg.VolumeSteps + } +} + +// volumeFromApi resolves a set_volume request, given in steps, against current, +// and returns the result in the range 0..player.MaxStateVolume. +func (app *App) volumeFromApi(data ApiSetVolume, current uint32) uint32 { + steps := int32(app.cfg.VolumeSteps) + target := data.Volume + if data.Relative { + target += int32(math.Round(float64(current) * float64(steps) / player.MaxStateVolume)) + } + target = max(min(target, steps), 0) + return uint32(target) * player.MaxStateVolume / uint32(steps) +} + // Send notification that the volume changed. // The original change can come from anywhere: from Spotify Connect, from the // REST API, or from a volume mixer. diff --git a/daemon/controls_test.go b/daemon/controls_test.go index 68858638..0548bb83 100644 --- a/daemon/controls_test.go +++ b/daemon/controls_test.go @@ -6,6 +6,9 @@ import ( "testing" "time" + librespot "github.com/devgianlu/go-librespot" + "github.com/devgianlu/go-librespot/player" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) @@ -33,3 +36,79 @@ func TestSkipDelay(t *testing.T) { }) } } + +// newVolumeTestApp is an App with a saved volume of saved (nil for none) whose +// saves are reported on the returned channel. +func newVolumeTestApp(t *testing.T, cfg *Config, saved *uint32) (*App, <-chan uint32) { + t.Helper() + + saves := make(chan uint32, 8) + store := librespot.NewMockStateStore(t) + store.EXPECT().Save(mock.Anything).RunAndReturn(func(s *librespot.AppState) error { + saves <- *s.LastVolume + return nil + }).Maybe() + + return &App{ + cfg: cfg, + log: &librespot.NullLogger{}, + stateStore: store, + state: &librespot.AppState{LastVolume: saved}, + }, saves +} + +func requireSaved(t *testing.T, saves <-chan uint32, want uint32) { + t.Helper() + select { + case got := <-saves: + require.Equal(t, want, got) + case <-time.After(time.Second): + t.Fatal("the volume was not saved") + } +} + +func TestSessionVolume(t *testing.T) { + saved := uint32(player.MaxStateVolume / 4) + cfg := func(ignoreLast bool) *Config { + return &Config{VolumeSteps: 100, InitialVolume: 50, IgnoreLastVolume: ignoreLast} + } + + app, _ := newVolumeTestApp(t, cfg(false), nil) + require.Equal(t, uint32(player.MaxStateVolume/2), app.sessionVolume(), "nothing saved: initial_volume") + + app, _ = newVolumeTestApp(t, cfg(false), &saved) + require.Equal(t, saved, app.sessionVolume(), "the volume an earlier run saved") + + app, saves := newVolumeTestApp(t, cfg(true), &saved) + require.Equal(t, uint32(player.MaxStateVolume/2), app.sessionVolume(), "ignore_last_volume skips the saved one") + + set := uint32(player.MaxStateVolume / 3) + app.setVolume(set) + requireSaved(t, saves, set) + require.Equal(t, set, app.sessionVolume(), "a volume set while running wins, ignore_last_volume or not") + + app, _ = newVolumeTestApp(t, &Config{VolumeSteps: 100, InitialVolume: 150}, nil) + require.Equal(t, uint32(player.MaxStateVolume), app.sessionVolume(), "initial_volume above the steps is capped") +} + +func TestVolumeFromApi(t *testing.T) { + app := &App{cfg: &Config{VolumeSteps: 100}} + current := uint32(player.MaxStateVolume * 37 / 100) + + cases := []struct { + name string + data ApiSetVolume + want uint32 + }{ + {"absolute", ApiSetVolume{Volume: 64}, player.MaxStateVolume * 64 / 100}, + {"relative up", ApiSetVolume{Volume: 5, Relative: true}, player.MaxStateVolume * 42 / 100}, + {"relative down past zero", ApiSetVolume{Volume: -50, Relative: true}, 0}, + {"absolute above the steps", ApiSetVolume{Volume: 150}, player.MaxStateVolume}, + {"absolute below zero", ApiSetVolume{Volume: -1}, 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.want, app.volumeFromApi(tc.data, current)) + }) + } +} diff --git a/daemon/player.go b/daemon/player.go index e991197e..12e2af9f 100644 --- a/daemon/player.go +++ b/daemon/player.go @@ -842,18 +842,7 @@ func (p *AppPlayer) handleApiRequest(req ApiRequest) (any, error) { Value: p.apiVolume(), }, nil case ApiRequestTypeSetVolume: - data := req.Data.(ApiSetVolume) - - var volume int32 - if data.Relative { - volume = int32(p.apiVolume()) - volume += data.Volume - volume = max(min(volume, int32(p.app.cfg.VolumeSteps)), 0) - } else { - volume = data.Volume - } - - p.updateVolume(uint32(volume) * player.MaxStateVolume / p.app.cfg.VolumeSteps) + p.updateVolume(p.app.volumeFromApi(req.Data.(ApiSetVolume), p.state.device.Volume)) return nil, nil case ApiRequestTypeSetRepeatingContext: val := req.Data.(bool) diff --git a/daemon/player_state.go b/daemon/player_state.go index d7970ba5..11787c6d 100644 --- a/daemon/player_state.go +++ b/daemon/player_state.go @@ -201,11 +201,19 @@ func deviceAddressMask() string { } func (p *AppPlayer) initState() { + // The first state pushed to Spotify must carry the real level: a controller + // steps its volume buttons from what it was shown. Without external volume + // the session sets its own once the dealer connects. + volume := uint32(player.MaxStateVolume) + if p.app.cfg.ExternalVolume { + volume = p.app.sessionVolume() + } + p.state = &State{ lastCommand: nil, device: &connectpb.DeviceInfo{ CanPlay: true, - Volume: player.MaxStateVolume, + Volume: volume, Name: p.app.cfg.DeviceName, DeviceId: p.app.deviceId, DeviceType: p.app.deviceType, diff --git a/daemon/player_state_test.go b/daemon/player_state_test.go index 41a8edd2..d3bc8d1b 100644 --- a/daemon/player_state_test.go +++ b/daemon/player_state_test.go @@ -9,6 +9,7 @@ import ( "time" librespot "github.com/devgianlu/go-librespot" + "github.com/devgianlu/go-librespot/player" connectpb "github.com/devgianlu/go-librespot/proto/spotify/connectstate" metadatapb "github.com/devgianlu/go-librespot/proto/spotify/metadata" "github.com/devgianlu/go-librespot/spclient" @@ -289,3 +290,18 @@ func TestStateRetryDelayBacksOff(t *testing.T) { require.GreaterOrEqual(t, prev, statePutMaxBackoff*4/5, "backoff should reach the cap") } + +// Under external volume the first state carries the level the app holds, not a +// placeholder a controller would step its buttons from. +func TestInitStateVolume(t *testing.T) { + held := uint32(player.MaxStateVolume * 37 / 100) + + p := &AppPlayer{app: &App{cfg: &Config{ExternalVolume: true, VolumeSteps: 100}, volume: &held}} + p.initState() + require.Equal(t, held, p.state.device.Volume) + + p = &AppPlayer{app: &App{cfg: &Config{VolumeSteps: 100}, volume: &held}} + p.initState() + require.Equal(t, uint32(player.MaxStateVolume), p.state.device.Volume, + "without external volume the session sets its own level once the dealer connects") +} From fb46a5dbc81e0b9bd7525e149ca028173f11a79f Mon Sep 17 00:00:00 2001 From: devgianlu Date: Fri, 16 Oct 2026 10:45:43 +0200 Subject: [PATCH 2/2] feat: answer GET /player/volume without a session Now that the app holds a volume across sessions and POST /player/volume is accepted without one, GET answers with the volume the next session starts from instead of 204. The conversion to the API's steps is shared by every path that reports or reads a volume in steps. Co-Authored-By: Claude Opus 5.5 (1M context) --- api-spec.yml | 4 +++- daemon/api_server_test.go | 9 ++++++++- daemon/app.go | 4 ++++ daemon/controls.go | 19 +++++++++++++++---- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/api-spec.yml b/api-spec.yml index c741a361..9c5b9c6f 100644 --- a/api-spec.yml +++ b/api-spec.yml @@ -1008,7 +1008,9 @@ paths: /player/volume: get: operationId: playerGetVolume - description: Get player volume + description: | + Get player volume. Without a session, the volume the next session starts from + when the volume is external. responses: 200: description: Returns the player volume settings diff --git a/daemon/api_server_test.go b/daemon/api_server_test.go index 3e63f8c5..9749fe25 100644 --- a/daemon/api_server_test.go +++ b/daemon/api_server_test.go @@ -916,7 +916,11 @@ func TestAnswerWithoutSessionTakesVolume(t *testing.T) { stop := app.answerWithoutSession(context.Background()) t.Cleanup(stop) - resp := ts.do(http.MethodPost, "/player/volume", map[string]any{"volume": 37}) + resp := ts.do(http.MethodGet, "/player/volume", nil) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.JSONEq(t, `{"value": 100, "max": 100}`, body(t, resp), "nothing set yet: initial_volume") + + resp = ts.do(http.MethodPost, "/player/volume", map[string]any{"volume": 37}) require.Equal(t, http.StatusOK, resp.StatusCode) requireSaved(t, saves, player.MaxStateVolume*37/100) @@ -925,6 +929,9 @@ func TestAnswerWithoutSessionTakesVolume(t *testing.T) { requireSaved(t, saves, player.MaxStateVolume*32/100) require.Equal(t, uint32(player.MaxStateVolume*32/100), app.sessionVolume()) + resp = ts.do(http.MethodGet, "/player/volume", nil) + require.Equal(t, http.StatusOK, resp.StatusCode) + require.JSONEq(t, `{"value": 32, "max": 100}`, body(t, resp)) } func TestApiUnknownPathIsNotFound(t *testing.T) { diff --git a/daemon/app.go b/daemon/app.go index b8b61094..6376f594 100644 --- a/daemon/app.go +++ b/daemon/app.go @@ -484,6 +484,8 @@ func (app *App) answerWithoutSession(ctx context.Context) (stop func()) { switch req.Type { case ApiRequestTypeRoot: req.Reply(&ApiRoot{}, nil) + case ApiRequestTypeGetVolume: + req.Reply(app.volumeWithoutSession(), nil) case ApiRequestTypeSetVolume: app.setVolumeWithoutSession(req.Data.(ApiSetVolume)) req.Reply(nil, nil) @@ -642,6 +644,8 @@ func (app *App) withAppPlayer(ctx context.Context, appPlayerFunc func(context.Co switch req.Type { case ApiRequestTypeRoot: req.Reply(&ApiRoot{}, nil) + case ApiRequestTypeGetVolume: + req.Reply(app.volumeWithoutSession(), nil) case ApiRequestTypeSetVolume: app.setVolumeWithoutSession(req.Data.(ApiSetVolume)) req.Reply(nil, nil) diff --git a/daemon/controls.go b/daemon/controls.go index cd654b18..cfe85409 100644 --- a/daemon/controls.go +++ b/daemon/controls.go @@ -1612,10 +1612,15 @@ func (p *AppPlayer) startAutoplay(prevTrackUris []string, drop bool, then func(b }) } -// Return the volume as an integer in the range 0..player.MaxStateVolume, as -// used in the API. +// apiVolume is the session's volume in the API's range, 0..volume_steps. func (p *AppPlayer) apiVolume() uint32 { - return uint32(math.Round(float64(p.state.device.Volume*p.app.cfg.VolumeSteps) / player.MaxStateVolume)) + return p.app.volumeSteps(p.state.device.Volume) +} + +// volumeSteps converts a volume in the range 0..player.MaxStateVolume to the +// API's range, 0..volume_steps. +func (app *App) volumeSteps(volume uint32) uint32 { + return uint32(math.Round(float64(volume*app.cfg.VolumeSteps) / player.MaxStateVolume)) } // Set the player volume to the new volume, also notifies about the change. @@ -1662,6 +1667,12 @@ func (app *App) setVolumeWithoutSession(data ApiSetVolume) { app.setVolume(newVal) } +// volumeWithoutSession answers a volume request while there is no session with +// the volume the next one starts from. +func (app *App) volumeWithoutSession() *ApiVolume { + return &ApiVolume{Max: app.cfg.VolumeSteps, Value: app.volumeSteps(app.sessionVolume())} +} + // sessionVolume is the volume a session starts at when the volume is external: // the last one set while this process runs, else the one saved by an earlier // run unless ignore_last_volume is set, else initial_volume. @@ -1685,7 +1696,7 @@ func (app *App) volumeFromApi(data ApiSetVolume, current uint32) uint32 { steps := int32(app.cfg.VolumeSteps) target := data.Volume if data.Relative { - target += int32(math.Round(float64(current) * float64(steps) / player.MaxStateVolume)) + target += int32(app.volumeSteps(current)) } target = max(min(target, steps), 0) return uint32(target) * player.MaxStateVolume / uint32(steps)