Artwork role: back-pressure, timing offset, and lateness reporting#97
Merged
Conversation
Adds a back-pressure gate so a consumer can hold the next artwork delivery until it finishes presenting the current one (e.g. a cross-fade that needs both decoded frames alive). Opt-in per slot via ImageSlotPreference::require_frame_done; acknowledged with the new ArtworkRole::frame_done(slot). While a delivery is un-acked, the decode thread parks the newest notification (latest-wins) instead of decoding it and replays it once the gate reopens, reusing the existing generation/epoch validation. A clear counts as a delivery owing its own ack, since it may drive a fade-out; a stream restart auto-releases a frame that was decoded but never displayed so the slot cannot wedge. frame_done wakes the decode thread via a sentinel notification, with the 100ms receive timeout as fallback. Default behavior (require_frame_done false) is unchanged.
Lets a consumer fire on_image_display() earlier (or later) than the server's display timestamp, so a cross-fade can straddle the track boundary: with a 2 s fade, an offset of 1000 starts the fade 1 s before the boundary. Positive-equals-earlier mirrors PlayerRoleConfig::fixed_delay_us. The offset is applied at the drain_events() deadline check via a pure display_deadline_reached() helper, keeping the no-connection sentinel (fire immediately) ahead of the shift and the held timestamps in true server time. Best-effort: an image that arrives or decodes after the shifted deadline fires as soon as it is ready, matching existing past-timestamp handling.
Displays are best-effort: an image that arrives or decodes after its (offset-shifted) deadline fires as soon as it is ready. Passing the slip as a lateness_ms parameter lets a consumer compensate, e.g. shorten a cross-fade by the lateness so it still ends on schedule, or snap instantly on a huge value such as joining mid-track where the artwork timestamp is long past. Replaces the display_deadline_reached() helper with display_overdue_us(), whose >= 0 result doubles as the reported lateness (0 for the no-connection fire-immediately path, where no deadline exists). Breaking listener signature change, made before the v0.7.0 tag freezes the API: on_image_display(uint8_t) -> on_image_display(uint8_t, uint32_t).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances ArtworkRole (a main-loop scheduled display role with a decode thread) with three opt-in features aimed at smoother UI cross-fades: per-slot back-pressure via an explicit frame_done() acknowledgment, a per-slot display timing offset, and lateness reporting on display callbacks. It also introduces a breaking API change to ArtworkRoleListener::on_image_display() to carry lateness_ms.
Changes:
- Add an opt-in per-slot ack gate (
ImageSlotPreference::require_frame_done) andArtworkRole::frame_done(slot)to enforce “one un-acked delivery at a time” with latest-wins buffering. - Add per-slot
display_offset_msand compute/reportlateness_mstoon_image_display(slot, lateness_ms). - Add a new
test_artwork_role.cppsuite covering the ack gate state machine and display-deadline/lateness arithmetic.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_artwork_role.cpp | New unit tests validating ack gate behavior, restart/clear interactions, and offset/lateness pure helpers. |
| tests/CMakeLists.txt | Adds the new artwork role test target source. |
| src/artwork_role.cpp | Implements ack gate, parked-notification replay, deadline offsetting, and lateness reporting; adds frame_done(). |
| src/artwork_role_impl.h | Adds ack gate state, parked notification storage, sentinel wake slot, and new helper declarations. |
| include/sendspin/config.h | Extends ImageSlotPreference with require_frame_done and display_offset_ms. |
| include/sendspin/artwork_role.h | Updates listener API to on_image_display(slot, lateness_ms) and documents the ack-gate contract; adds frame_done(). |
| docs/internals.md | Updates internals docs for offset/lateness computation and documents the ack-gate state machine. |
| docs/integration-guide.md | Updates integration docs/examples for new on_image_display signature, lateness semantics, and back-pressure usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Adds three opt-in artwork role features aimed at smooth cross-fades on ESPHome displays, all backward-compatible (defaults preserve today's decode-and-display-every-frame behavior) beyond a function signature breaking change.
Changes
Per-slot
frame_doneacknowledgment (opt-in back-pressure): SetImageSlotPreference::require_frame_doneand the role delivers at most one un-acked "delivery" (a frame or a clear) at a time for that slot; newer payloads are buffered latest-wins until the consumer callsArtworkRole::frame_done(slot). A clear supersedes any un-acked frame. Lets a consumer hold the gate open for the duration of a cross-fade animation instead of having buffers reused mid-fade. No timeout by design — the ack is the contract.Per-slot
display_offset_ms: Shifts a slot's display deadline (positive fires early, mirroringPlayerRoleConfig::fixed_delay_us) so a cross-fade can straddle the track boundary.Display lateness reporting:
on_image_display()now takes alateness_msargument reporting how far past the deadline it fired, so a consumer can shorten a fade to still end on schedule, or snap instantly when joining mid-track.lateness_ms == 0is reserved for the no-connection case; a connected on-time display never reports 0.New
tests/test_artwork_role.cppcovers the gate state machine (hold/supersede/clear/restart/reentrant-ack), the deadline/offset arithmetic, and the lateness mapping.Breaking change
on_image_display(uint8_t)signature is nowon_image_display(uint8_t, uint32_t lateness_ms).