Skip to content

Artwork: deliver the protocol's per-channel clear #105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kahrendt merged 3 commits into from
Aug 11, 2026
Merged

Artwork: deliver the protocol's per-channel clear #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d668d1f
Artwork: deliver the protocol's per-channel clear
kahrendt Aug 11, 2026
fc2e9ba
Artwork: split the ack contract by clear kind
kahrendt Aug 11, 2026
a012eba
Artwork: cover the clear_mask latest-wins merge
kahrendt Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions docs/integration-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,28 @@ struct MyArtworkListener : ArtworkRoleListener {
display.show_image(slot, decoded_images[slot]);
}

// Called from the main loop thread when artwork should be cleared.
// Called from the main loop thread when artwork should be cleared, either for this slot
// alone or for every slot at the end of a stream.
void on_image_clear(uint8_t slot) override {
display.clear_slot(slot);
}
};
```

**Cross-fades with back-pressure (opt-in).** By default the role decodes and displays every frame as it arrives. A slot can instead opt into a back-pressure gate by setting `ImageSlotPreference::require_frame_done`. With the gate on, the role keeps at most one un-acked *delivery* (a frame or a clear) in flight for that slot; any newer payload that arrives is buffered latest-wins and delivered only after the consumer calls `ArtworkRole::frame_done(slot)` from the main loop -- e.g. once a cross-fade animation finishes. A clear is itself a delivery and supersedes any un-acked frame, so exactly one `frame_done()` is owed after it. There is no timeout: the acknowledgment is the contract.
**Knowing when there is no artwork.** Artwork stays valid until the server replaces or clears it, and the artwork role is independent of the metadata role, so a track change alone sends nothing: the next track of the same album keeps showing the image already delivered. When an item genuinely has no artwork, the server clears that channel and `on_image_clear()` fires for that slot alone, scheduled to its server timestamp like a display (`display_offset_ms` included) so it lands on the item boundary. `on_image_clear()` also fires for every configured slot on stream end, stream clear, and disconnect.

| What happened | What the listener sees |
| --- | --- |
| Artwork unchanged (e.g. next track of the same album) | nothing; the current image stays valid |
| Item has no artwork | `on_image_clear(slot)` for that slot |
| Stream ended, cleared, or connection lost | `on_image_clear(slot)` for every configured slot |

**Cross-fades with back-pressure (opt-in).** By default the role decodes and displays every frame as it arrives. A slot can instead opt into a back-pressure gate by setting `ImageSlotPreference::require_frame_done`. With the gate on, the role keeps at most one un-acked *delivery* (a frame or a clear) in flight for that slot. Call `ArtworkRole::frame_done(slot)` from the main loop exactly once for every `on_image_display()` and `on_image_clear()` that slot receives -- e.g. once a cross-fade animation finishes. An extra call is a harmless no-op, but a missed one wedges the slot: there is no timeout, the acknowledgment is the contract.

Payloads and stream-level clears reach the gate differently:

- A **frame or per-channel clear** arriving while a delivery is un-acked is buffered latest-wins and delivered only after `frame_done(slot)`, and then owes its own `frame_done()`. It waits behind the outstanding delivery rather than replacing it, so a consumer is never interrupted mid-fade.
- A **stream end or stream clear** is a lifecycle event, not a payload, so it is never buffered: it fires `on_image_clear()` immediately for every configured slot, discards anything buffered, and replaces whatever delivery was outstanding. Exactly one `frame_done()` is owed afterward whatever was in flight.

Pair the gate with `ImageSlotPreference::display_offset_ms` to start a fade before the track boundary (positive fires the display early, mirroring `PlayerRoleConfig::fixed_delay_us`), and use `lateness_ms` to shorten the fade so it still ends on schedule:

Expand Down
36 changes: 28 additions & 8 deletions include/sendspin/artwork_role.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,25 @@ class SendspinClient;
///
/// ACK GATE (opt-in per slot via ImageSlotPreference::require_frame_done): a "delivery" is
/// either a frame (on_image_decode() followed later by on_image_display()) or a clear
/// (on_image_clear()). For an ack-enabled slot, at most one un-acked delivery is ever in flight;
/// the newest payload that arrives while a delivery is un-acked is buffered latest-wins and
/// delivered only after the consumer calls ArtworkRole::frame_done(slot). A clear supersedes any
/// un-acked frame for that slot -- exactly one ack is owed, and it is for the clear. A stream
/// restart automatically releases a frame that was decoded but never displayed (its display can
/// no longer fire), but a delivery that already reached on_image_display()/on_image_clear() stays
/// gated until frame_done() is called; there is no timeout.
/// (on_image_clear()). Call ArtworkRole::frame_done(slot) exactly once for every
/// on_image_display() and on_image_clear() that slot receives. An extra call is a harmless no-op,
/// but a missed one wedges the slot forever: there is no timeout.
///
/// For an ack-enabled slot, at most one un-acked delivery is ever in flight. The two ways a clear
/// reaches the gate differ, so they are worth keeping apart:
/// - A payload -- a frame, or the server's per-channel clear for that slot -- arriving while a
/// delivery is un-acked is buffered latest-wins and delivered only after frame_done(slot), and
/// then owes its own frame_done(). It waits behind the outstanding delivery rather than
/// replacing it, so a consumer is never interrupted mid-presentation.
/// - A stream end or stream clear is a lifecycle event, not a payload, so it is never buffered:
/// it fires on_image_clear() immediately for every configured slot, discards anything buffered,
/// and replaces whatever delivery was outstanding. Exactly one frame_done() is owed afterward
/// whatever was in flight -- including when it lands on an un-acked per-channel clear, which
/// fires on_image_clear() again and still owes exactly one ack.
///
/// A stream restart automatically releases a frame that was decoded but never displayed (its
/// display can no longer fire), but a delivery that already reached on_image_display()/
/// on_image_clear() stays gated until frame_done() is called.
class ArtworkRoleListener {
public:
virtual ~ArtworkRoleListener() = default;
Expand Down Expand Up @@ -77,7 +89,15 @@ class ArtworkRoleListener {

/// @brief Called on the main loop thread when artwork should be cleared for a slot
///
/// Fires on stream end or stream clear for each configured slot.
/// Fires on stream end or stream clear for each configured slot, and for a single slot when
/// the server clears that channel (the artwork for the current item is gone, e.g. a track
/// with no album art). A per-channel clear is scheduled to its server timestamp exactly like
/// on_image_display(), ImageSlotPreference::display_offset_ms included, so it lands on the
/// item boundary rather than as soon as it arrives.
///
/// Artwork stays valid until it is replaced or cleared, so the server does not resend an
/// unchanged image on every track: no callback at a track boundary means the image already
/// delivered still applies.
/// @param slot The artwork slot index to clear.
virtual void on_image_clear(uint8_t /*slot*/) {}
};
Expand Down
135 changes: 89 additions & 46 deletions src/artwork_role.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,6 @@ static int64_t be64_to_host(const uint8_t* bytes) {

namespace sendspin {

namespace {

/// @brief Merges a single-slot display delta into the accumulated cross-thread update
///
/// Called under the Inbox mutex via InboxSlot::merge() (see Impl::drain_thread_func), so it must
/// stay a pure data operation with no callbacks into application code. `delta` carries exactly
/// one slot's bit (set by the decode thread after a single image finishes decoding); OR-ing
/// valid_mask and overwriting only the masked timestamps entries preserves latest-wins per slot
/// while leaving any other slot's already-accumulated (not yet drained) timestamp untouched.
void merge_artwork_display_update(ArtworkDisplayUpdate& current, ArtworkDisplayUpdate&& delta) {
current.valid_mask |= delta.valid_mask;
for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) {
if (delta.valid_mask & (1U << slot)) {
current.timestamps[slot] = delta.timestamps[slot];
current.epochs[slot] = delta.epochs[slot];
}
}
}

} // namespace

// ============================================================================
// ArtworkRole::Impl lifecycle
// ============================================================================
Expand Down Expand Up @@ -148,6 +127,26 @@ void ArtworkRole::Impl::build_hello_fields(ClientHelloMessage& msg) const {
// Display-deadline and ack-gate helpers (used from network, decode, and main threads)
// ============================================================================

void ArtworkRole::Impl::merge_artwork_display_update(ArtworkDisplayUpdate& current,
ArtworkDisplayUpdate&& delta) {
current.valid_mask |= delta.valid_mask;
for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) {
const uint8_t bit = static_cast<uint8_t>(1U << slot);
if (delta.valid_mask & bit) {
current.timestamps[slot] = delta.timestamps[slot];
current.epochs[slot] = delta.epochs[slot];
// clear_mask is assigned, not OR-ed: it says what kind of delivery this slot's
// (latest-wins) pending entry is, so a frame arriving after an undrained clear must
// reset the bit just as a clear after an undrained frame sets it.
if (delta.clear_mask & bit) {
current.clear_mask |= bit;
} else {
current.clear_mask &= static_cast<uint8_t>(~bit);
}
}
}
}

int64_t ArtworkRole::Impl::display_overdue_us(int64_t client_ts, int32_t display_offset_ms,
int64_t now) {
// get_client_time returns 0 when there is no current connection. Without a connection we
Expand Down Expand Up @@ -218,9 +217,16 @@ void ArtworkRole::Impl::handle_binary(uint8_t slot, const uint8_t* data, size_t
image_format = this->config.preferred_formats[slot].format;
}

// An empty payload -- a binary message carrying only the type byte and timestamp -- is the
// protocol's per-channel clear: the artwork on this channel is no longer valid, as distinct
// from the server simply not resending an image that still is. There are no bytes to stage, so
// the buffer machinery below is skipped entirely and the notification travels with
// data_length == 0 (see ArtworkNotification). It still goes through the decode thread rather
// than straight to the main loop, so it stays ordered behind any image already queued for this
// slot and takes the same ack-gate path a frame does.
uint32_t generation = 0;
uint8_t write_idx = 0;
{
if (image_len > 0) {
// Hold the slot mutex across the read-modify-write of write_idx/drain_active/
// write_generation and the memcpy itself, so the decode thread can never observe a
// buffer mid-write (torn image) and can never have a buffer stolen out from under it
Expand Down Expand Up @@ -260,7 +266,8 @@ void ArtworkRole::Impl::handle_binary(uint8_t slot, const uint8_t* data, size_t
ArtworkNotification notif{slot, write_idx, image_len, timestamp,
image_format, generation, epoch};
if (!this->drain_task->notify_queue.send(notif, 0)) {
SS_LOGW(TAG, "Artwork notify queue full; dropping image for slot %u", slot);
SS_LOGW(TAG, "Artwork notify queue full; dropping %s for slot %u",
image_len > 0 ? "image" : "clear", slot);
}
}

Expand Down Expand Up @@ -363,6 +370,7 @@ void ArtworkRole::Impl::handle_stream_ring_event(ArtworkEventType event) {
case ArtworkEventType::STREAM_END:
case ArtworkEventType::STREAM_CLEAR:
this->held_display_mask = 0;
this->held_display_clear = 0;
this->event_state->display_slot.reset();
{
// A clear is itself a delivery that must be acked: it may drive a fade-out, and
Expand Down Expand Up @@ -404,10 +412,21 @@ void ArtworkRole::Impl::drain_events() {
ArtworkDisplayUpdate update{};
if (this->event_state->display_slot.take(update)) {
for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) {
if (update.valid_mask & (1U << slot)) {
const uint8_t bit = static_cast<uint8_t>(1U << slot);
if (update.valid_mask & bit) {
this->held_display_ts[slot] = update.timestamps[slot];
this->held_display_epoch[slot] = update.epochs[slot];
this->held_display_mask |= static_cast<uint8_t>(1U << slot);
this->held_display_mask |= bit;
// Assigned rather than OR-ed, for the same latest-wins reason as the cross-thread
// merge: this slot's held entry has just been replaced wholesale, so the kind of
// delivery it is must be replaced too. This mirrors
// merge_artwork_display_update(), which is unit-tested directly
// (ArtworkDisplayMerge); the two must stay in agreement.
if (update.clear_mask & bit) {
this->held_display_clear |= bit;
} else {
this->held_display_clear &= static_cast<uint8_t>(~bit);
}
}
}
}
Expand All @@ -425,13 +444,15 @@ void ArtworkRole::Impl::drain_events() {
const int64_t now = platform_time_us();
const uint32_t current_epoch = this->stream_epoch.load(std::memory_order_relaxed);
for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) {
if (!(this->held_display_mask & (1U << slot))) {
const uint8_t bit = static_cast<uint8_t>(1U << slot);
if (!(this->held_display_mask & bit)) {
continue;
}
// Drop a display decoded under a since-replaced stream (restart with no intervening
// end/clear bumps the epoch but cannot reach these main-thread holds to cancel it).
if (this->held_display_epoch[slot] != current_epoch) {
this->held_display_mask &= static_cast<uint8_t>(~(1U << slot));
this->held_display_mask &= static_cast<uint8_t>(~bit);
this->held_display_clear &= static_cast<uint8_t>(~bit);
if (this->ack_enabled(slot)) {
bool should_wake = false;
{
Expand Down Expand Up @@ -460,16 +481,25 @@ void ArtworkRole::Impl::drain_events() {
if (overdue_us < 0) {
continue;
}
this->held_display_mask &= static_cast<uint8_t>(~(1U << slot));
this->held_display_mask &= static_cast<uint8_t>(~bit);
// A per-channel clear is scheduled exactly like a frame, offset shift included, so a
// consumer can fade out on the same lead it would have faded in on.
const bool is_clear = (this->held_display_clear & bit) != 0;
this->held_display_clear &= static_cast<uint8_t>(~bit);
if (this->ack_enabled(slot)) {
// Arm the "awaiting frame_done()" state before the callback fires and release the
// mutex before invoking it: frame_done() may be called synchronously from inside
// on_image_display(), which would deadlock if this mutex were still held.
// on_image_display()/on_image_clear(), which would deadlock if this mutex were still
// held.
std::lock_guard<std::mutex> lock(this->drain_task->slot_mutex);
this->drain_task->slot_buffers[slot].ack_state = SlotAckState::PRESENTED;
}
if (this->listener) {
this->listener->on_image_display(slot, display_lateness_ms(client_ts, overdue_us));
if (is_clear) {
this->listener->on_image_clear(slot);
} else {
this->listener->on_image_display(slot, display_lateness_ms(client_ts, overdue_us));
}
}
}
}
Expand All @@ -487,6 +517,7 @@ void ArtworkRole::Impl::cleanup() {
// call, which runs before any role's cleanup() -- so there is no per-event ring reset to do
// here.
this->held_display_mask = 0;
this->held_display_clear = 0;
this->event_state->display_slot.reset();

// Enqueue a clean STREAM_END - handle_stream_ring_event() will fire the on_image_clear()
Expand Down Expand Up @@ -529,6 +560,13 @@ void ArtworkRole::Impl::process_notification(const ArtworkNotification& notif) {
uint8_t slot = notif.slot;
uint8_t buf_idx = notif.buffer_idx;

// A per-channel clear (see handle_binary) names no buffer, so it skips the buffer validation
// and the decode callback below. Everything else is deliberately shared with a frame: the same
// stream-epoch staleness check, the same ack gate (a clear is a delivery owing exactly one
// frame_done()), and the same timestamp-scheduled hand-off to the main loop, which fires
// on_image_clear() rather than on_image_display() when the deadline is reached.
const bool is_clear = notif.data_length == 0;

uint8_t* decode_data = nullptr;
size_t decode_length = 0;
{
Expand All @@ -544,13 +582,13 @@ void ArtworkRole::Impl::process_notification(const ArtworkNotification& notif) {
if (notif.stream_epoch != this->stream_epoch.load(std::memory_order_relaxed)) {
return;
}
if (notif.generation != sb.write_generation[buf_idx]) {
return;
}

auto& buf = sb.buffers[buf_idx];
if (notif.data_length == 0 || buf.data() == nullptr) {
return;
if (!is_clear) {
if (notif.generation != sb.write_generation[buf_idx]) {
return;
}
if (sb.buffers[buf_idx].data() == nullptr) {
return;
}
}

// Ack gate: a slot with require_frame_done set allows only one un-acked delivery in
Expand All @@ -570,18 +608,20 @@ void ArtworkRole::Impl::process_notification(const ArtworkNotification& notif) {
sb.ack_state = SlotAckState::DECODE_DELIVERED;
}

// Mark this buffer as in-use so the network thread avoids it while we decode.
sb.drain_buf_idx = buf_idx;
sb.drain_active = true;
decode_data = buf.data();
decode_length = notif.data_length;
if (!is_clear) {
// Mark this buffer as in-use so the network thread avoids it while we decode.
sb.drain_buf_idx = buf_idx;
sb.drain_active = true;
decode_data = sb.buffers[buf_idx].data();
decode_length = notif.data_length;
}
}

if (this->listener) {
this->listener->on_image_decode(slot, decode_data, decode_length, notif.format);
}
if (!is_clear) {
if (this->listener) {
this->listener->on_image_decode(slot, decode_data, decode_length, notif.format);
}

{
std::lock_guard<std::mutex> lock(this->drain_task->slot_mutex);
this->drain_task->slot_buffers[slot].drain_active = false;
}
Expand All @@ -597,6 +637,9 @@ void ArtworkRole::Impl::process_notification(const ArtworkNotification& notif) {
// the display if the stream is replaced after this hand-off (see held_display_epoch).
delta.epochs[slot] = notif.stream_epoch;
delta.valid_mask = static_cast<uint8_t>(1U << slot);
if (is_clear) {
delta.clear_mask = static_cast<uint8_t>(1U << slot);
}
this->event_state->display_slot.merge(merge_artwork_display_update, delta);
}
}
Expand Down
Loading
Loading