fix: start an external-volume session from the real volume

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:

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)
 

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

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.

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))
+		})
+	}
+}

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)

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,

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")
+}