Repository navigation
Conversation
Two contained player-role conformance gaps against roles/player/v1.md. supported_formats non-empty (spec #257): a player's supported_formats must list at least one entry. Add ClientCapabilities.ValidateAudioFormats, gated on the player role like the visualizer/pairing-code checks, and call it at both construction sites (SendSpinClient, SendSpinHostService) so an empty AudioFormats throws before the first client/hello. Discard audio while unavailable (spec #270): while the client reports available: false its pipeline is not consuming, so drop inbound player audio in the binary dispatch rather than decode it, keeping the connection open (discard, MUST NOT close). Logged once per unavailable period, not per chunk, so a live stream does not flood the log. The clock-sync startup window is already handled by the #204 role-binary gate, so this only affects the external-source / pipeline-error cases. A shared stream-start test fixture assumed the default unconverged clock (available: false); give it a converged clock so it reflects a client that is actually receiving audio, which is what queues the chunks those tests drain.
Item 1 of #290, plus the three accepted Copilot findings from round 1. Output delay clamp (spec #176): the applied output delay is now clamped to 0-5000 at a single site, the KalmanClockSynchronizer.OutputDelayMs setter, which is the one owner of the applied value. The duplicate clamps on the command path and the wire projection are removed and rely on it; MinOutputDelayMs drops from -5000 to 0 (the persisted-store load included); and the "reported value differs from applied" log goes, since the two can no longer differ beyond rounding. SendPlayerStateAsync and the persisted-store load persist the applied (clamped) value. A negative no longer schedules audio later. The IOutputDelayStore contract and MIGRATION-10.0.0.md are updated to match. Review findings: - ValidateAudioFormats matches the player role by family (player@), like the hello builder, so player@v2 with empty formats is rejected too. - The "first drop while unavailable" log latch resets on the availability transition rather than on an available audio frame, so a false-true-false sequence with no audio in the available window still logs once per period. - A host-construction test asserts SendspinHostService throws on empty AudioFormats before any connection. Updated the tests that pinned the old negative-applied-delay design (KalmanClockSynchronizerTests, OutputDelayWireProjectionTests, PlayerStateDelayReportingTests): the wire-clamp and scheduler-keeps-negative cases become setter-clamp and rounding cases. net8.0: 1176 passed. net10.0: 1178 passed.
- Reject a visualizer configuration that requests a periodic type (loudness, f_peak, spectrum) with a non-positive rate_max, at both capability validation and the runtime setter (spec #257). - On an artwork stream/end (playback termination) clear the display: raise ArtworkCleared for every channel still showing an image, after discarding pending updates. A stream/clear seek keeps the image and drops only what is pending (spec #266). - Drop artwork and visualizer binary that arrives while the client is unavailable rather than scheduling it; the connection stays open and one debug line is logged per unavailable period (spec #266/#271 SHOULD). - Doc fixes: artwork image bytes are JPEG/PNG; rate_max is the per-periodic-type ceiling, not the display refresh rate; buffer_capacity counts each reassembled message's bytes, not the full wire size.
…izer-artwork-small-items-positive-rate-max
…-small-items-one-output-delay-ms-clamp-non
roles/visualizer/v1.md defines rate_max as a positive integer in every visualizer state, including empty and event-only type lists (Sendspin/spec#257). The capability validation and SetVisualizerConfigurationAsync now reject a non-positive value unconditionally, and the periodic-type helpers that conditioned the check are gone.
The latch was cleared only when a display frame arrived while available, so an unavailable, available (no display frame), unavailable sequence never logged the second period's first drop. It now clears when the client becomes available again and on the per-connection reset.
The scheduler loop takes due artwork out of its slot and raises it outside the lock, so a stream/end landing in between raised ArtworkCleared and the taken image was still delivered afterwards. FlushArtwork now bumps a generation the loop checks immediately before each raise, and the channel is recorded as shown at that point rather than when the image is taken.
ArtworkCleared is also raised for a stream/end, which has no clear message of its own: the event then carries the timestamp of the image it clears. The packaged README example listed bmp among the image formats; artwork is jpeg or png.
The activate after an in-band re-handshake is a subsequent one on the same connection, but the re-key handler emptied the active-role mirror first: an activate omitting active_roles left the client with none, and one dropping a role saw nothing to clear. The roles are now set aside on re-key and the next activate persists or removes them, re-evaluating playback-capability under the newly matched PSK. A role removed by server/activate also stops its stream output and clears its buffers without a preceding stream/end.
chrisuthe
marked this pull request as ready for review
October 6, 2026 16:59
Member
Author
|
Merged the #297 branch in (which carries #293). #297 changed the stream/end teardown that this PR extracts into |
…end clear through StopStreamRolesAsync
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Audio recovery, display teardown ordering, and custom-synchronizer delay safeguards have unresolved regressions.
Review effort: Balanced
Findings: 1
Open (6)
What changed in this PR
Updates SDK role persistence across re-handshakes and teardown when roles are removed, alongside protocol validation and media-handling changes.
Changes:
- Preserves or discards roles and state according to subsequent activations.
- Stops removed stream roles and updates artwork clearing and availability handling.
- Tightens delay and capability validation, with regression tests and documentation.
| File | Description |
|---|---|
| tests/Sendspin.SDK.Tests/Synchronization/KalmanClockSynchronizerTests.cs | Tests output-delay clamping. |
| tests/Sendspin.SDK.Tests/Client/SendspinClientServiceVisualizerTests.cs | Tests positive-rate validation. |
| tests/Sendspin.SDK.Tests/Client/SendspinClientServiceStreamStartTests.cs | Uses a converged test clock. |
| tests/Sendspin.SDK.Tests/Client/SendspinClientServiceArtworkTests.cs | Keeps artwork fixtures available. |
| tests/Sendspin.SDK.Tests/Client/RoleStateAcrossActivationsTests.cs | Tests role persistence and removal. |
| tests/Sendspin.SDK.Tests/Client/PlayerSupportedFormatsValidationTests.cs | Tests required player formats. |
| tests/Sendspin.SDK.Tests/Client/PlayerStateDelayReportingTests.cs | Tests fractional-delay persistence. |
| tests/Sendspin.SDK.Tests/Client/PlayerAudioDispatchTests.cs | Tests unavailable-audio dropping. |
| tests/Sendspin.SDK.Tests/Client/OutputDelayWireProjectionTests.cs | Updates delay-projection coverage. |
| tests/Sendspin.SDK.Tests/Client/MediaDisplaySchedulingTests.cs | Tests display dropping and teardown. |
| tests/Sendspin.SDK.Tests/Client/ClientHelloSupportRoleGatingTests.cs | Supplies a valid visualizer rate. |
| src/Sendspin.SDK/Synchronization/KalmanClockSynchronizer.cs | Clamps applied output delays. |
| src/Sendspin.SDK/README.md | Documents artwork clearing. |
| src/Sendspin.SDK/Protocol/Messages/VisualizerMessages.cs | Clarifies buffer-size documentation. |
| src/Sendspin.SDK/MIGRATION-10.0.0.md | Documents delay behavior changes. |
| src/Sendspin.SDK/Client/VisualizerRoleSupport.cs | Validates positive rates. |
| src/Sendspin.SDK/Client/SendSpinHostService.cs | Validates player formats at construction. |
| src/Sendspin.SDK/Client/SendSpinClient.cs | Updates activation, teardown, availability, and delays. |
| src/Sendspin.SDK/Client/MediaDisplayScheduler.cs | Tracks displayed artwork and flush generations. |
| src/Sendspin.SDK/Client/IOutputDelayStore.cs | Documents bounded persisted delays. |
| src/Sendspin.SDK/Client/ClientCapabilities.cs | Rejects empty player format lists. |
| src/Sendspin.SDK/Client/ArtworkEventArgs.cs | Documents teardown clear events. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+5421
to
+5423
| if (!CurrentAvailability) | ||
| { | ||
| if (!_audioDroppedWhileUnavailable) |
Member
Author
Comment on lines
+772
to
+773
| _shownArtwork[artwork.Channel] = artwork.ImageData.Length == 0 ? null : artwork.ServerTimestamp; | ||
| } |
Member
Author
There was a problem hiding this comment.
Declined: what remains is the few microseconds between releasing the lock and calling the handler, and closing it means holding a lock across application callbacks. Worst case is a stale image until the next one arrives.
Comment on lines
+2128
to
+2129
| private static int ToWireOutputDelayMs(double outputDelayMs) | ||
| => (int)Math.Round(outputDelayMs, MidpointRounding.AwayFromZero); |
Member
Author
There was a problem hiding this comment.
Declined: one clamp, in the synchronizer, is the design. #293 now states in the IClockSynchronizer contract that implementations must keep the value in 0-5000.
| .ToList(); | ||
| if (removedStreamRoles.Count > 0) | ||
| { | ||
| DispatchStreamLifecycle(() => StopStreamRolesAsync(removedStreamRoles)); |
Member
Author
There was a problem hiding this comment.
Declined: display cleanup waits at most for an in-flight audio start or stop; keeping the teardown on one queue is deliberate.
|
|
||
| _clockSynchronizer.OutputDelayMs = clamped; | ||
| TrySaveOutputDelay(clamped); | ||
| _clockSynchronizer.OutputDelayMs = requestedDelayMs.Value; |
Member
Author
There was a problem hiding this comment.
Declined: one clamp, in the synchronizer, is the design. #293 now states in the IClockSynchronizer contract that implementations must keep the value in 0-5000.
| // Media held for a display time that belongs to the stream just ended must not | ||
| // surface after it, and the artwork on display is cleared: both a stream/end and a | ||
| // role's removal are playback termination (spec #266), unlike a stream/clear seek. | ||
| FlushDisplayRoles(roles, endingStream: true); |
Member
Author
There was a problem hiding this comment.
Declined: it needs the application to remove the visualizer role from inside its own frame handler mid-batch; worst case is one more frame.
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.
What changed
Role state now survives, and is discarded, correctly around an in-band re-handshake and on role removal. All three audit rows were real gaps on main (a5a2701) and share one code path in
SendspinClientService.DetectSessionRekeyemptied the active-role mirror, andHandleServerActivateonly wrote roles when the message carried them, so an activate omittingactive_rolesafter a re-handshake left the client with none. The re-key handler now sets the roles aside and the activate that follows persists them. The mirror still reads empty between the re-key and that activate, so nothing is granted from a retired session's roles before an activate has been admitted under the new PSK.server/stateobject and pending scheduled update discarded and the cleared events raised, as on any other activation.stream/end. Removingplayer,artworkorvisualizerfromactive_rolesnow stops playback, clears the audio buffers and drops pending visualizer frames and artwork. The teardown is the onestream/endalready ran, extracted intoStopStreamRolesAsync;StreamEndReceivedis not raised, since nostream/endarrived.Two consequences of applying "the activation rules ... under the newly matched PSK" that go past the brief's letter:
active_roles, not only the one after a re-handshake. (protocol: admit the rc1 activity sets and keep playback running during pairing #299 listed this rule as not implemented.)source@v1persisted onto a session that is not long-term closesunauthorized, exactly as an explicit one does.Spec (1.0.0-rc1, 671a34d)
connection.md, "Re-handshake" (Sendspin/spec#287):
messaging.md,
server/activate:Breaking behaviour
active_roleskeeps the previous roles; it used to leave none.playerstops playback and sets the group's playback state to idle even when nostream/endpreceded it.active_roleson a connection that is not playback-capable now clears the roles and their state. Until protocol: admit the rc1 activity sets and keep playback running during pairing #299 lands, main's activity table makes['pairing']on an unpaired session with unpaired access enabled such a case.Not in this PR
v1, so no replacement can occur on this wire.Tests
RoleStateAcrossActivationsTests(new): roles and state kept across a re-handshake whenactive_rolesis omitted; metadata cleared and announced when the post-re-handshake activate drops it; persisted roles treated as empty on a session that is not playback-capable; persistedsource@v1on an unpaired session closesunauthorized; droppingplayerstops the pipeline with nostream/end; dropping another role leaves playback running.MediaDisplaySchedulingTests: droppingvisualizerdiscards its pending frames.StreamingPermittedBeforeAnInBandRekey_IsNotHonouredAfterItWithNoActivatestill passes unchanged.Interop (
tools/interop, aiosendspin 10.0.0):unpairedPASS,pairingPASS (this one re-handshakes onto the new long-term PSK),static-pinPASS.