Replace client error state with not_synchronized#102
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
maximmaxim345
left a comment
maximmaxim345
left a comment
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 |
There was a problem hiding this comment.
Hmm, the edge case where the state changes from synchronized to not_synchronized mid stream is a bit loosely defined.
Specifically the interpretation of 'MUST NOT send binary messages to a client unless it is in the synchronized state' I'm worried about is: Lets just drop all those messages.
There was a problem hiding this comment.
I was thinking a client can never go from synchronized to not_synchronized, but I realize I didn't spell that out. This was mainly to just guard against getting overwhelmed on initial startup.
| - `'synchronized'` - client is operational and synchronized with server timestamps | ||
| - `'error'` - client has a problem preventing normal operation (unable to keep up, clock sync issues, etc.) | ||
| - `state`: 'synchronized' | 'not_synchronized' | 'external_source' - operational state of the client | ||
| - `'synchronized'` - client's clock is synchronized with the server; ready to receive timestamped binary data |
There was a problem hiding this comment.
Ok, thats a bit of a stretch since Music Assistant/aiosendspin doesn't send them in advance, but what about other timestamped messages (like metadata)?
I mean artwork also is about as time critical as metadata, why handle them differently?
There was a problem hiding this comment.
Metadata messages are relatively small and are only sent once per song. Visualizer/audio send a lot of data frequently, and artwork sends a very large chunk of data all at once. That was the intention behind just blocking binary messages. Otherwise it would be a bit more convoluted to say the server can't send messages that have a timestamp to them, but I guess that's doable.
| - 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 |
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).
| ### Server → Client: Audio Chunks (Binary) | ||
|
|
||
| Binary messages should be rejected if there is no active stream. | ||
| Binary messages SHOULD be rejected if there is no active stream or the client is not in the `synchronized` [state](#client--server-clientstate). |
There was a problem hiding this comment.
Same edge case is also difficult on the client side.
Let's assume a scenario where the client just switched from synchronized to not_synchronized. But Just at this moment the server already sent the buffer for the next 30 seconds.
Even if the client switched back to synchronized immediately after that, it's already too late and those 30s are lost, resulting in silence.
Ok, those are a couple of tight requirements, but on the other side I think the most likely point in time where it switches to not_synchronized is at the very beginning of the connection, just after it became synchronized (when the time filter is just on the edge between synced and not synced).
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 <teancom@users.noreply.github.com>
…#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
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
2 participants
Reworks the client state model so a client reports synchronization status rather than errors.
errorstate tonot_synchronized: the client's default state on connect, held until clock synchronization with the server is established.Updates the
client/stateenum, the Playback Synchronization rules, and the handshake sequence diagram (initial state:not_synchronized, transitioning tosynchronizedafter clock sync).Ref #88 (addresses point 3 by dropping the
errorstate on underflow)