Skip to content

Commit a72ccb4

Browse files
chrisutheclaude
andcommitted
Tell the server the volume we are actually applying
Reading Sendspin/spec settled the question I had parked for item 13, and not in the direction I expected: this was never a behaviour change to weigh up, it was a spec violation with a visible symptom we had already been staring at. - client/state's player `volume` MUST be included when a player advertises the `volume` command, which this one does. - "Group volume is the average of the volumes of players in the group that support the `volume` command." Group volume is *derived from us*. So the `group volume: 0` that confused this entire field test was our own misreport averaged straight back to us -- not a server oddity, ours. - Setting group volume works off "delta = requested_volume - current_group_volume", so a wrong figure mis-applies every later group volume change by exactly the error. A player claiming 0 while playing at full hears a request for 30 as a cut from full rather than a rise from silence, which is the symptom I had predicted without knowing the mechanism. - "A server MUST NOT assume these values are unchanged after a reconnect" -- the player is the authority and the server depends on being told. So main.cpp reports the sink's real gain before anything can connect. update_volume() rather than reaching for the sink, because it does not invoke on_volume_changed() -- that fires only for server-initiated changes -- so status still tells a default apart from a volume a server chose. Confirmed against the live server: group volume now reads 100 for a single player at 100 where it read 0 before, and a following `vol 30` lands at a real 30 with the default qualifier correctly gone. Two deviations found in the same reading are left for item 13, now known to be deviations rather than open questions: persisting volume and muted across restarts is the spec's RECOMMENDED and needs item 8's store, and the software taper is (volume/100)^2 where the spec says (volume/100)^1.5 -- roughly 3 dB quiet at volume 50, 6 dB at 25, inherited from upstream and shared by both sinks. The spec also says volume changes SHOULD be ramped, and nothing here ramps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 094cb44 commit a72ccb4

3 files changed

Lines changed: 59 additions & 11 deletions

File tree

README.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -433,10 +433,16 @@ server does not re-report, the estimate drifts by however far you jumped. Paused
433433
it is the server's own snapshot and carries no marker.
434434

435435
`player volume` is the gain **this box's output** is applying, and it says
436-
`(default; no server has set it)` until a server sends a volume command — because
437-
until one does, the sink runs at full while the library's own stored volume reads
438-
0. Those disagree, and the qualifier is how you tell "nobody has set this" from a
439-
server that deliberately chose full output.
436+
`(default; no server has set it)` until a server sends a volume command. The
437+
qualifier is how you tell "nobody has set this" from a server that deliberately
438+
chose full output.
439+
440+
That figure is also what the server is told, from the first message — which the
441+
spec requires and which matters more than it looks: **group volume is the average
442+
of the players' volumes**, and setting group volume applies a *delta* against it.
443+
A player that reported a volume it was not applying would skew the group reading
444+
for every controller, and make the next group volume change land wrong by exactly
445+
that error.
440446

441447
Two `status` lines are worth reading together. `state` is the **group's** transport
442448
state, from the metadata `playback_speed`, and reads `unknown` rather than guessing

docs/ROADMAP.md

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -740,13 +740,35 @@ caller of `set_volume()`, so the only thing that knows what the sink was told
740740
distinguishable from one that never spoke. `DEFAULT_SINK_VOLUME` replaces the bare `{100}` the
741741
three sinks each spelled separately, with the disagreement written down beside it.
742742

743-
**The underlying incoherence is left for item 13**, which already owns advertised state that does
744-
not match reality: we tell the server 0 while playing at full, so the first volume command a
745-
server sends is heard as a *cut* from full rather than a rise from silence. Fixing it means
746-
either seeding the role from the sink at startup (`update_volume(DEFAULT_SINK_VOLUME)`, which
747-
changes what we advertise) or seeding the sink from the role (which makes a fresh player silent
748-
until a server speaks). Both are behaviour changes wider than a control channel, and neither is
749-
this item's to make.
743+
**And the same disagreement was a spec violation on the wire, so it is fixed here too.** Reading
744+
`Sendspin/spec` settled which direction it had to go, and it is not a matter of taste:
745+
746+
- `client/state`'s player `volume` **MUST** be included when a player advertises the `volume`
747+
command, which this one does.
748+
- *"Group volume is the average of the volumes of players in the group that support the `volume`
749+
command"* -- so group volume is **derived from us**, and a player reporting a figure it is not
750+
applying corrupts the group reading for every controller in the group. The `group volume: 0` that
751+
confused this whole field test was our own misreport averaged back to us, not a server oddity.
752+
- Setting group volume works off *"delta = requested_volume - current_group_volume"*, so that wrong
753+
figure then mis-applies every later group volume change by exactly the error: a player claiming 0
754+
while playing at full hears a request for 30 as a cut from full rather than a rise from silence.
755+
- *"A server MUST NOT assume these values are unchanged after a reconnect"* -- the player is the
756+
authority and the server depends on being told.
757+
758+
So `main.cpp` reports the sink's real gain with `player.update_volume(DEFAULT_SINK_VOLUME)` before
759+
anything can connect. `update_volume()` rather than reaching for the sink because it does not
760+
invoke `on_volume_changed()`, which fires only for server-initiated changes -- so `status` still
761+
tells a default apart from a volume a server chose. Confirmed against the live server: it now
762+
reports `group volume: 100` for a single player at 100, where it reported 0 before, and a
763+
subsequent `vol 30` lands at a real 30.
764+
765+
**Two things still left for item 13**, both now known to be spec deviations rather than open
766+
questions. Persisting `volume` and `muted` across restarts is the spec's RECOMMENDED and needs a
767+
store, which item 8 owns. And the software taper is `(volume/100)^2` where the spec says
768+
*"`amplitude = (volume / 100)^1.5`"* -- about 3 dB quiet at volume 50 and 6 dB at 25, inherited
769+
from upstream's `PortAudioSink::apply_volume_()` and shared by both sinks through
770+
`src/pcm_volume.cpp`. The spec also says volume changes SHOULD be ramped to avoid clicks, and
771+
nothing here ramps.
750772

751773
**Every command works, and finding that out took listening rather than reading.** An earlier pass
752774
concluded that `seek-rel`, `repeat` and `shuffle` were advertised and unimplemented at the server's

src/main.cpp

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,26 @@ int main(int argc, char* argv[]) {
638638
player_config.audio_formats = advertised_formats(*sink);
639639
sendspin::PlayerRole& player = client.add_player(std::move(player_config));
640640
player.set_static_delay_adjustable(true);
641+
// Report the gain the sink is really applying, before anything can connect.
642+
//
643+
// Not cosmetic, and not a preference -- three spec rules make it necessary. `client/state`'s
644+
// `volume` MUST be included when a player advertises the `volume` command, which this one
645+
// does. Group volume is *derived* from us: "Group volume is the average of the volumes of
646+
// players in the group that support the `volume` command", so a player reporting a figure it
647+
// is not applying corrupts the group reading for every controller in the group. And setting
648+
// group volume works off "delta = requested_volume - current_group_volume", so that wrong
649+
// figure then mis-applies every later group volume change by exactly the error -- a player
650+
// claiming 0 while playing at full hears a request for 30 as a cut from full, not a rise.
651+
//
652+
// The library's own default is 0 while every AudioSink starts at DEFAULT_SINK_VOLUME, so
653+
// without this the two disagree from the first message. update_volume() is the right call
654+
// rather than reaching for the sink: it does not invoke on_volume_changed(), which fires only
655+
// for server-initiated changes, so `status` still distinguishes this from a volume a server
656+
// chose.
657+
//
658+
// Persisting volume and mute across restarts is the spec's RECOMMENDED and is deliberately
659+
// not done here -- that needs a store, which docs/ROADMAP.md item 8 owns.
660+
player.update_volume(DEFAULT_SINK_VOLUME);
641661
sendspin::MetadataRole& metadata = client.add_metadata();
642662
// Added unconditionally, so `client/hello` always carries `controller@v1` -- including under
643663
// --no-control, and including on a host with no $XDG_RUNTIME_DIR to put a socket in. That is

0 commit comments

Comments
 (0)