Artwork: deliver the protocol's per-channel clear - #105
Merged
Conversation
An artwork binary message carrying only the type byte and timestamp is the protocol's per-channel clear: the artwork on that channel is no longer valid. Until now process_notification() dropped it silently on its data_length == 0 check, so a consumer had no way to tell "this item has no artwork" from "artwork unchanged, nothing sent" -- the previous track's image just stayed on screen. Route the empty payload through the same path a frame takes, so it inherits the ordering and scheduling the frame path already has: - handle_binary() skips the buffer staging block entirely (there are no bytes to stage) and queues a notification with data_length == 0; buffer_idx/generation are unused and left at 0. - process_notification() treats data_length == 0 as is_clear: it skips the generation/null-buffer validation, the drain_active bookkeeping, and the decode callback, but keeps the stream-epoch staleness check, the ack gate, and the timestamp-scheduled hand-off. A clear is a delivery like any frame and owes exactly one frame_done(). - ArtworkDisplayUpdate gains clear_mask, mirrored by the main-thread hold held_display_clear, so the deadline loop fires on_image_clear() instead of on_image_display(). Both are assigned per bit rather than OR-ed, because latest-wins replaces a slot's pending entry wholesale: a frame landing on an undrained clear must reset the bit just as a clear landing on an undrained frame sets it. Because it rides the same deadline as a display, display_offset_ms applies, so a consumer can fade out on the lead it would have faded in on. It still goes through the decode thread rather than straight to the main loop, which keeps it ordered behind any image already queued for that slot. Tests: eight new ArtworkChannelClear cases covering the ungated path, slot isolation, parking behind an un-acked frame, latest-wins between two parked clears, the one-ack contract, a stream end superseding an un-acked clear, and the stream_active guard. These negative assertions drive drain_events() across the window via a new poll_drain_never() helper rather than using never_within(). on_image_clear() fires only from drain_events() and handle_stream_ring_event(), both on the main loop thread, so a window that parks the test thread freezes the counter it is watching and can never fail. Verified by mutation: deleting the held_display_mask bit clear in the deadline loop makes four of these tests fail, and left all eight passing under the never_within() form.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements timestamp-scheduled, per-channel artwork clears through the existing decode, ordering, and acknowledgment pipeline.
Changes:
- Routes empty artwork payloads to
on_image_clear(slot). - Tracks clear versus display deliveries through scheduling state.
- Expands API documentation, integration guidance, and tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/artwork_role.cpp |
Implements clear processing and delivery. |
src/artwork_role_impl.h |
Adds clear-state tracking. |
include/sendspin/artwork_role.h |
Documents the public clear contract. |
docs/integration-guide.md |
Explains consumer integration behavior. |
tests/test_artwork_role.cpp |
Adds per-channel clear tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The ack-gate documentation described one supersession rule for both clear mechanisms, but they behave oppositely at the gate. A per-channel clear is a payload: process_notification() parks it when the slot gate is occupied, so it is delivered after the outstanding delivery's frame_done() and then owes its own. A stream end or stream clear is a lifecycle event: handle_stream_ring_event() drops anything parked, forces ack_state to PRESENTED, and fires on_image_clear() immediately, so exactly one frame_done() is owed whatever was in flight. The header and the integration guide each stated one of those as the rule for both, and in the direction that hurts: a consumer implementing "a clear supersedes my outstanding delivery, so I owe one ack" wedges the slot permanently the first time a per-channel clear parks behind an un-acked frame, since there is no timeout. Extra frame_done() calls are safe no-ops (the IDLE early return), so the asymmetry is worth stating outright. Split both descriptions along the payload/lifecycle line and lead with the rule that is safe to follow without knowing which mechanism fired: call frame_done() exactly once per on_image_display()/on_image_clear() received. No behavior change. The guide's example listener already acked every clear, so only the prose was wrong.
merge_artwork_display_update() assigns clear_mask per bit rather than OR-ing it, because latest-wins replaces a slot's pending entry wholesale: a frame landing on an undrained clear must reset the bit just as a clear landing on an undrained frame sets it. Nothing tested that. Exercising it end to end needs two same-slot deliveries of different kinds to accumulate before the main loop takes the slot, and no test produced that shape -- each either drained between the two, sent them to different slots, or parked the second on the ack gate, where it replaces SlotBuffer::parked instead of reaching the merge. So both reset branches ran constantly and only ever as no-ops. The uncovered case is reachable: an item with no artwork is cleared and the next item's frame is decoded before the main loop ticks, which is routine on ESP where one loop iteration can straddle both. Under OR semantics the deadline fires on_image_clear() for a decoded frame, blanking the display and dropping the image. Promote the function to a pure static member of Impl and test it directly, matching display_overdue_us()/display_lateness_ms(), which are static for the same reason. Four cases: both cross-kind transitions, same-kind replacement in both directions, and cross-slot preservation. Verified by mutation: collapsing the assignment to `current.clear_mask |= (delta.clear_mask & bit)` fails ArtworkDisplayMerge.FrameAfterUndrainedClearResetsKind and leaves the rest of the suite green, which is what it did before these tests existed. The mirrored fold-in in drain_events() stays untested: a held entry that is not yet due requires a real deadline, and the harness builds a never-started client whose get_client_time() returns 0, so every folded-in entry fires in the same call that folds it in. Cross-referenced the two sites instead; covering the fold-in needs an injectable clock.
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
An artwork binary message carrying only the type byte and timestamp is the protocol's per-channel clear: the artwork on that channel is no longer valid. This delivers it to the consumer as
on_image_clear(slot).Why
process_notification()dropped the empty payload silently on itsdata_length == 0check, so nothing reached the listener. Combined with the fact that artwork stays valid until replaced (the server does not resend an unchanged image on every track, and the artwork role is independent of the metadata role), a consumer had no way to tell "this item has no artwork" from "artwork unchanged, nothing sent" and the previous track's image just stayed on screen.on_image_clear(slot)for that sloton_image_clear(slot)for every configured slotHow
The empty payload rides the same path a frame takes, so it inherits the ordering and scheduling already in place:
handle_binary()skips the buffer staging block entirely (there are no bytes to stage) and queues a notification withdata_length == 0.buffer_idx/generationare unused and left at 0.process_notification()treatsdata_length == 0asis_clear: it skips the generation/null-buffer validation, thedrain_activebookkeeping, and the decode callback, but keeps the stream-epoch staleness check, the ack gate, and the timestamp-scheduled hand-off. A clear is a delivery like any frame and owes exactly oneframe_done().ArtworkDisplayUpdategainsclear_mask, mirrored by the main-thread holdheld_display_clear, so the deadline loop fireson_image_clear()instead ofon_image_display().Because the clear rides the same deadline as a display,
display_offset_msapplies and a consumer can fade out on the lead it would have faded in on. It still goes through the decode thread rather than straight to the main loop, which keeps it ordered behind any image already queued for that slot.Notes for reviewers
clear_maskis assigned per bit, not OR-ed, in both the cross-thread merge and the main-loop fold-in. Latest-wins replaces a slot's pending entry wholesale, so a frame landing on an undrained clear must reset the bit just as a clear landing on an undrained frame sets it.held_display_clearis set and cleared in lockstep withheld_display_maskat all five of its mutation sites.A stream end or stream clear on top of an un-acked per-channel clear fires
on_image_clear()again and still owes exactly one ack. It supersedes the earlier clear the same way it supersedes an un-acked frame.handle_stream_ring_event()forces every ack-enabled slot toPRESENTEDregardless of prior state, so this is idempotent.Tests
Eight new
ArtworkChannelClearcases: the ungated path, slot isolation, parking behind an un-acked frame, latest-wins between two parked clears, the one-ack contract, a stream end superseding an un-acked clear, and thestream_activeguard.Their negative assertions drive
drain_events()across the window via a newpoll_drain_never()helper rather than usingnever_within().on_image_clear()fires only fromdrain_events()andhandle_stream_ring_event(), both on the main loop thread, so a window that parks the test thread freezes the very counter it is watching and can never fail.Verified by mutation: deleting the
held_display_mask &= ~bitin the deadline loop, which makes the role re-fire a clear every tick:never_within()formpoll_drain_never()form