Simplify client/state status#102
Open
kahrendt wants to merge 5 commits into
Open
Conversation
- `not_synchronized` is the default startup state - Servers MUST NOT send binary messages to a client unless it is in the `synchronized` state - Drops the `error` state; clients no longer report transient audible syncing issues or visual delays
This was referenced Jun 16, 2026
maximmaxim345
left a comment
maximmaxim345
left a comment
Member
There was a problem hiding this comment.
Two ideas I have for solving this edge case are:
- only allow the
not_synchronizedstate at the very start - or loosen the requirements 'The server MUST NOT send binary messages ...' and 'Binary messages SHOULD be rejected if ....' if the clock isn't synced yet the client can still buffer the data.
| - Each client is responsible for maintaining synchronization with the server's timestamps | ||
| - Clients maintain accurate sync by adding or removing samples using interpolation to compensate for clock drift | ||
| - When a client cannot maintain sync (e.g., buffer underrun), it should send `state: 'error'` via [`client/state`](#client--server-clientstate), mute its audio output, and continue buffering until it can resume synchronized playback, at which point it should send `state: 'synchronized'` | ||
| - On connect a client reports `state: 'not_synchronized'` via [`client/state`](#client--server-clientstate), then `state: 'synchronized'` once clock synchronization is established. The server MUST NOT send binary messages to a client unless it is in the `synchronized` state |
Member
There was a problem hiding this comment.
Should we define what it means to have a 'synchronized clock`? I mean we already require the exact clock filter implementation (and with #104 also we recommend/require how to feed the time filter).
Contributor
Author
There was a problem hiding this comment.
This opens a can of worms. Do we just define it as it can translate server time to client time? Or do we specify it's accurate enough that we can meet the playback syncing standards recently added to the spec (well that specifically says we need to be accurate to within the time filter, not to absolute accuracy, so it's not quite applicable).
teancom
added a commit
to Sendspin/sendspin-rs
that referenced
this pull request
Jun 17, 2026
Per PR review and Sendspin/spec#102, the client/state.state enum now reports clock-synchronization status rather than transient playback hiccups: a buffer underrun no longer changes state. Drop the now-obsolete protocol reporting path: - PlaybackRecoveryMonitor / PlaybackRecoveryState and the pending- transition bit-field + take_pending_transition - the playback_recovery_monitor_task and ConnectionGuard::monitor_playback_recovery (plus the underrun_handle) - SyncedPlayer::recovery_monitor The audio-side underrun/mute/rebuild behavior is unchanged: PlaybackRecoveryCoordinator keeps report_underrun / is_recovering / observe_buffered_duration / set_player_timing, and the timing config (required_lead_time_ms / min_buffer_ms) is untouched. Rename ClientSyncState::Error -> NotSynchronized (serde "not_synchronized") and refocus the enum docs on sync status. Co-authored-by: David Bishop <[email protected]>
teancom
added a commit
to Sendspin/sendspin-rs
that referenced
this pull request
Jun 17, 2026
…#102 Add a playback module with a split-handle recovery primitive so an underrunning player mutes output while it rebuilds buffer, then resumes once enough continuous audio is available: - PlaybackRecoveryCoordinator (audio-side): non-blocking, allocation-free methods (report_underrun / is_recovering / observe_buffered_duration / set_player_timing) safe for the real-time audio callback. - SyncedPlayer wires it in: on underrun the callback emits silence and gates playback; while recovering it watches continuous buffered duration and releases once max(required_lead_time_ms, min_buffer_ms) is met. - Add required_lead_time_ms / min_buffer_ms to PlayerState; export the new public types. Align ClientSyncState with Sendspin/spec#102: the enum reports clock-synchronization status, not transient playback hiccups. Rename Error -> NotSynchronized (serde "not_synchronized"); keep Synchronized and ExternalSource. BREAKING CHANGE: SyncedPlayer::new / with_process_callback now take a SyncedPlayerConfig instead of the individual device/volume/muted/buffer_size arguments. Closes #51
maximmaxim345
pushed a commit
to DanielHabenicht/fork.sendspin-rs
that referenced
this pull request
Jun 25, 2026
I went to implement Sendspin#51 and discovered that while we did in fact define the ClientSyncState::Error, we were doing basically nothing else in terms of actually detecting and recovering from buffer underruns, at least properly. *Actually* implementing it turned into a whole thing. I attempted to keep the protocol and audio sides of the house as clean as possible. I ended up with a small 'playback' module to store the stuff needed to keep the two sides in sync. I'm guessing we'll need to do some more of that in the future. And it may turn into a facade that combines SyncedPlayer and ProtocolClient into one simple API for apps who want to just easily use sendspin-rs with less boilerplate setup. But this is already a *quite* large PR and I wasn't going to go all the way down that road. Commit message: Add a playback module with a split-handle recovery primitive so an underrunning player mutes output while it rebuilds buffer, then resumes once enough continuous audio is available: - PlaybackRecoveryCoordinator (audio-side): non-blocking, allocation-free methods (report_underrun / is_recovering / observe_buffered_duration / set_player_timing) safe for the real-time audio callback. - SyncedPlayer wires it in: on underrun the callback emits silence and gates playback; while recovering it watches continuous buffered duration and releases once max(required_lead_time_ms, min_buffer_ms) is met. - Add required_lead_time_ms / min_buffer_ms to PlayerState; export the new public types. Align ClientSyncState with Sendspin/spec#102: the enum reports clock-synchronization status, not transient playback hiccups. Rename Error -> NotSynchronized (serde "not_synchronized"); keep Synchronized and ExternalSource. BREAKING CHANGE: SyncedPlayer::new / with_process_callback now take a SyncedPlayerConfig instead of the individual device/volume/muted/buffer_size arguments. Closes Sendspin#51
kahrendt
changed the title
…ts its synchronized state
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
2 participants
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.
Reworks the client state model so a client reports synchronization status rather than errors.
errorstateclient/statemessage when time is synchronizedUpdates the
client/stateenum, the Playback Synchronization rules, and the handshake sequence diagram (send firstclient/stateonly after time synchronization is established).Ref #88 (addresses point 3 by dropping the
errorstate on underflow)