diff --git a/api-spec.yml b/api-spec.yml index 74749cc2..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 @@ -1018,7 +1020,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..9749fe25 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,34 @@ 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.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) + + 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()) + 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) { ts := newTestServer(t, okReply) diff --git a/daemon/app.go b/daemon/app.go index 32bb42f7..6376f594 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,11 @@ 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) default: // Renames included, unlike the zeroconf branch: the login // running meanwhile reads the device name. @@ -634,6 +644,11 @@ 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) 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..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. @@ -1628,12 +1633,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 +1649,59 @@ 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) +} + +// 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. +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(app.volumeSteps(current)) + } + 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") +}