Allow players to change supported_commands mid-session - #177
Merged
Conversation
arturpragacz
left a comment
Contributor
There was a problem hiding this comment.
Weren't we supposed to align all dynamically changing values and move them all to client/state? So including for example visualizer's and artwork's stream/request-format fields.
Member
Author
Thats for a different PR though since it touches multiple roles, I'll write that once #175 is merged. |
maximmaxim345
force-pushed
the
refactor/player-supported-commands
branch
from
August 28, 2026 10:00
269e670 to
b9b2ba3
Compare
The reading rules excluded players without volume support while the write path averaged over all players and sent a command to each, so a player that does not list 'volume' received a command `server/command` forbids.
maximmaxim345
force-pushed
the
refactor/player-supported-commands
branch
from
August 31, 2026 06:27
b9b2ba3 to
634dacb
Compare
kahrendt
reviewed
Sep 2, 2026
kahrendt
pushed a commit
that referenced
this pull request
Sep 2, 2026
`stream/request-format` was a request in name only: a decline is invisible (the stream continues and the request MUST NOT be treated as an error), so a client only ever learns the operative configuration from `stream/start`, and with no stream active the server already had to remember the requested format as de facto state. Per the CONTRIBUTING rule that fields expected to change during a connection belong in `client/state`, the message is dropped and its fields move there. # Changes `player` gains an optional `format` preference in its state object; absent means the server selects per the `supported_formats` priority order, a present preference must be one of the listed entries (which also removes the previously undefined partial-request case), and the server may still deviate, for example to match a track's native sample rate or to apply an operator-configured format. `artwork` moves the per-channel `source`/`format`/`width`/`height` into a state `channels` array and `artwork@v1_support` is removed entirely: the array is positional from channel 0, an uncovered channel is `source: 'none'` so clients may truncate after their last active channel, an array longer than 4 is a protocol error, and `format`/`width`/`height` are required only when `source` is not `'none'`. `visualizer` moves `types`, `rate_max` and `spectrum` into state; its hello object keeps only `buffer_capacity`. `stream/start` now derives each role's configuration from the hello constants plus the role's current state, answers a state change on an active stream with a new `stream/start` when the configuration changed, never starts a stream in response, and should wait for the state update a role (re-)activation requires before starting that role's stream. # Breaking changes The `stream/request-format` message is removed; clients that send it break, and servers drop the handler. Artwork clients no longer send `artwork@v1_support` and instead carry their channel configuration in `client/state`; visualizer clients keep only `buffer_capacity` in their support object and carry the rest in `client/state`. Players are affected only if they used `stream/request-format`; the new `format` state field is optional. Conflicts with #177 in the player state object; whichever lands second needs a rebase. Closes #190
…rted-commands # Conflicts: # README.md # roles/player/v1.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
supported_commandswas split across two messages,'volume'and'mute'in theclient/hellosupport object and'set_output_delay'inclient/state, so a player could not change which commands it accepts without reconnecting andserver/commandhad to name both locations as sources of truth. The whole list moves into theclient/stateplayer object.The split also failed silently: an SDK reading only
client/helloignoresset_output_delay, one reading onlyclient/stateignoresvolumeandmute, and neither errors. Consolidating matches the CONTRIBUTING rule that a field expected to change during a connection belongs inclient/state.This breaks
player@v1on the wire, and a server can no longer see command capabilities atserver/activatetime, which is exotic but worth stating.#175 already made
supported_commandsnon-optional and defined an empty list as accepting no commands, so the field line itself only gains'volume'and'mute', and theclient/hellocopy is deleted.Rules the move requires
A server MUST NOT send a player command before the client has sent a
client/statecontaining theplayerobject, mirroring the existing rule for binary data. The move opens that window becauseserver/activateprecedes the first state message, and the player object is the unit that carriessupported_commands.supported_commandsadvertises settability, not reportability. A player MAY reportvolumeormutedwithout accepting the matching command, an amplifier with a physical volume knob being the real case, and capability cannot be inferred from field presence becauseoutput_delay_msis never optional either way. A server MUST NOT treat a reportedvolumeormutedas settable while the matching command is absent, though it MAY still surface the value read-only.Group volume averages over volume-capable players, so when the last capable player withdraws the reported value jumps to 100 and the control disappears with nobody having touched a volume. Both reading rules now say so and require a recompute when a player's support changes.
Group volume writes
The group volume algorithm already disagreed with itself before this change: it computed its delta against the average of all player volumes and sent a command to every player, while the reading rules excluded players without volume support. The move makes the mismatch reachable mid-session, so both paths now filter on capability, and setting group mute gets the same fix. This part is separable if you would rather it went on its own.
Editorial
The "Server behavior" bullets sat under the
client/hellosupport-object heading while describingrequired_lead_time_msandmin_buffer_ms, which live inclient/state, and two cross-references pointed at that mismatched anchor. The text merges into "Server Audio Send Constraints", which already carries the overlapping lead and send-ahead rules.Closes #142