Merge pull request #23 from chrisuthe/chrisuthe/task/hold-scheduled-artwork-and-metadata-until-their

Hold scheduled artwork and metadata until their timestamps

Author: chrisuthe

docs/ARCHITECTURE.md

@@ -230,6 +230,37 @@ Still true on 9.3.2, checked rather than assumed: 9.3.0 reworked the filter's pr
 that stays noisy now falls back to the steady-state interval and withholds `IsClockSynced` — but the
 timeout string above is byte-identical in both assemblies.
 
+### The SDK surfaces the artwork and metadata timestamps but holds nothing pending
+
+Both roles carry a server-clock timestamp saying *when* the value takes effect, and the spec has a
+client keep a **current** value plus at most one **pending** one per channel: a future timestamp is
+held and applied when the local clock reaches it, a past one applies at once and discards the pending
+one, a new future one replaces it (`roles/artwork/v1.md`, `roles/metadata/v1.md`). The SDK exposes
+the timestamps — `ArtworkReceivedEventArgs.Timestamp`, `ArtworkClearedEventArgs.Timestamp`,
+`TrackMetadata.Timestamp` — and then applies everything on arrival: `GroupState.Metadata` is
+whatever came last, and there is no pending slot anywhere. With a queue that put the next track's
+cover and title on screen seconds before the audible change.
+
+The pending model therefore lives here, in `Sendspin.Core.MediaSession.ScheduledValue<T>`: one per
+artwork channel and one for the track metadata, owned by `SendspinPlayerService`, which converts each
+timestamp with `IClockSynchronizer.ServerToClientTime` (the current best estimate — the spec says
+not to wait for convergence — and inclusive of this player's static delay, so the display changes
+when *this* player's audio does) and promotes on a `System.Threading.Timer`. Playback state, volume
+and the command set apply immediately; only the track metadata is scheduled. Position is the spec's
+formula run from the *current* metadata's timestamp (`MediaSessionMapper.ProjectPosition`), never
+from arrival time and never from a pending update. `PlayerServiceEventTests` drives the whole path
+with a known clock offset and a hand-cranked clock.
+
+Two SDK facts that shape it. The SDK builds a **new `TrackMetadata` instance** for every
+`server/state` that carries a metadata object and leaves the instance alone for one that does not
+(it mutates the one `GroupState` in place), so reference identity is what distinguishes a metadata
+update from a volume change. And the SDK raises **no event for `stream/end`**, although the spec
+discards pending values there; they are discarded on disconnect, and otherwise survive a stream end
+until their timestamp — at most the 20 s the spec lets a server schedule ahead. The spec also changed
+artwork transfers on 2026-09-02 to an announce-and-parts sequence with an explicit cancel; 9.3.2
+still raises one event per complete image, and the scheduler is built on that event rather than on
+the wire shape, so the coming SDK bump changes only where "transfer complete" comes from.
+
 ### Other SDK API facts worth not rediscovering
 
 - `IStaticDelayStore` is the SDK's persistence seam for `static_delay_ms`. It must be backed by the

docs/COMPLIANCE.md

@@ -16,8 +16,8 @@ contradict what the plan originally assumed.
 |---|---|
 | `player@v1` | Implemented |
 | `controller@v1` | Implemented (transport commands, volume, mute, `switch`) |
-| `metadata@v1` | Implemented (title, artist, album, album artist, progress) |
-| `artwork@v1` | Implemented (one album channel, JPEG, 512×512) |
+| `metadata@v1` | Implemented (title, artist, album, album artist, progress; scheduled updates held until their timestamp and position projected from the current metadata's timestamp — `PlayerServiceEventTests`, `ScheduledValueTests`, `MediaSessionMapperTests.ProjectPosition_*`) |
+| `artwork@v1` | Implemented (one album channel, JPEG, 512×512; scheduled images and clears held until their timestamp per channel — `PlayerServiceEventTests.ScheduledArtworkAndMetadata_*`; the announce-and-parts transfer the spec adopted on 2026-09-02 awaits an SDK bump, see `docs/ARCHITECTURE.md`) |
 | `visualizer@v1` | Implemented (loudness and beat at up to 30 frames a second, 4 096-byte buffer; no spectrum) — drives the living backdrop |
 | `color@v1` | Implemented (the server's palette, picked per theme variant) — drives the living backdrop |
 | `management@v1` | **Not implemented** — see "Blocked on the SDK" below |

docs/NEXT_STEPS.md

@@ -239,19 +239,26 @@ dotnet run --project scripts/spike/ShellSpike -- clock
 
 ---
 
-## 9. The artwork handler ignores the frame's channel and display timestamp
-
-`SendspinPlayerService.OnArtworkReceived` and `OnArtworkCleared` read neither `Channel` nor
-`Timestamp` off the artwork frame. Harmless today: `client/hello` advertises exactly one artwork
-channel, so every frame is channel 0, and the server-clock timestamp — when the picture *should be
-shown* — is ignored in favour of showing it on arrival. Naming the cached file by its bytes fixed
-the stale-art race that ordering produced, so the residue is at worst a sub-second early cover
-when the next track's picture lands ahead of its metadata.
-
-**First action:** none until a second channel (artist art) is advertised; a clear on channel 1 would
-then blank the album art. Honouring the timestamp means holding the publish until clock sync says
-server time has reached it, and is its own change — it does not replace per-picture paths, which are
-still needed at the boundary.
+## 9. Scheduled artwork and metadata are honoured; `stream/end` is not yet seen
+
+`SendspinPlayerService` now holds a picture, a clear or a metadata update stamped for the future
+until its timestamp, per channel for artwork, and computes position from the current metadata's
+timestamp (`docs/ARCHITECTURE.md`, "The SDK surfaces the artwork and metadata timestamps but holds
+nothing pending"). What remains is upstream: SDK 9.3.2 raises no event for `stream/end`, where the
+spec discards pending values, so a pending value outlives a stream end until its own timestamp; and
+the announce-and-parts artwork transfer the spec adopted on 2026-09-02 is not in 9.3.2 at all.
+
+Measured live on 2026-09-02 against Music Assistant (the Debug lines the service now writes per
+update): at a gapless queue boundary MA stamped the next picture 109 ms *in the past* and the next
+metadata 3 ms in the past, and sent the picture about 270 ms before the metadata. Past-stamped
+values apply at once by the spec's own rule, so with today's MA the cover still leads the title by
+that quarter second and both lead the audible change by the buffer. The fix for that is on the
+server: stamping both for the audible boundary, which this player now honours.
+
+**First action:** none until the SDK bump that adds both. When it lands, discard pending values on
+the stream-end event and take "transfer complete" from the SDK — the scheduler is built on the
+complete-image event, so nothing else moves. The cross-fade the spec permits around a picture's
+timestamp is a UI follow-up that can be picked up independently.
 
 ---
 

src/Sendspin.Core/MediaSession/MediaSessionMapper.cs

@@ -26,21 +26,53 @@ public static class MediaSessionMapper
     /// </remarks>
     public const string TrackIdRoot = "/io/sendspin/client/track/";
 
+    /// <summary>The protocol's <c>playback_speed</c> for normal speed: a ×1000 integer.</summary>
+    private const double NormalPlaybackSpeed = 1000;
+
+    /// <summary>
+    /// Projects the server's authoritative group state into a media-session snapshot, taking the
+    /// track metadata as it stands at its own timestamp.
+    /// </summary>
+    /// <remarks>
+    /// The group's own <see cref="GroupState.Metadata"/> is whatever the server sent last, which
+    /// may be a scheduled update that is not in effect yet. The service therefore does not call
+    /// this; it schedules the metadata and calls
+    /// <see cref="FromGroupState(GroupState?, TrackMetadata?, string?, long)"/> with the current
+    /// one. This overload is the plain projection for callers with no clock in hand.
+    /// </remarks>
+    /// <param name="group">The group state, or null when disconnected.</param>
+    /// <param name="artworkFilePath">
+    /// Path to the artwork file for the current track, or null when there is none.
+    /// </param>
+    public static MediaSessionState FromGroupState(GroupState? group, string? artworkFilePath = null) =>
+        FromGroupState(group, group?.Metadata, artworkFilePath, elapsedMicrosSinceMetadata: 0);
+
     /// <summary>
     /// Projects the server's authoritative group state into a media-session snapshot.
     /// </summary>
     /// <param name="group">The group state, or null when disconnected.</param>
+    /// <param name="metadata">
+    /// The track metadata currently in effect — the scheduler's current value, not necessarily the
+    /// group's latest — or null when there is none.
+    /// </param>
     /// <param name="artworkFilePath">
     /// Path to the artwork file for the current track, or null when there is none.
     /// </param>
-    public static MediaSessionState FromGroupState(GroupState? group, string? artworkFilePath = null)
+    /// <param name="elapsedMicrosSinceMetadata">
+    /// How long ago, on the local clock, <paramref name="metadata"/> took effect. The position is
+    /// projected forward by this, per <see cref="ProjectPosition"/>.
+    /// </param>
+    public static MediaSessionState FromGroupState(
+        GroupState? group,
+        TrackMetadata? metadata,
+        string? artworkFilePath,
+        long elapsedMicrosSinceMetadata)
     {
         if (group is null)
         {
             return MediaSessionState.Idle;
         }
 
-        var metadata = group.Metadata;
         var duration = ToTimeSpan(metadata?.Duration);
         var identity = BuildTrackIdentity(metadata);
         var commands = group.SupportedCommands ?? [];
@@ -54,7 +86,7 @@ public static MediaSessionState FromGroupState(GroupState? group, string? artwor
             AlbumArtist = NullIfBlank(metadata?.AlbumArtist),
             ArtworkFilePath = artworkFilePath,
             Duration = duration,
-            Position = ToTimeSpan(metadata?.Position) ?? TimeSpan.Zero,
+            Position = ProjectPosition(metadata?.Progress, elapsedMicrosSinceMetadata),
             CanGoNext = Supports(commands, Commands.Next),
             CanGoPrevious = Supports(commands, Commands.Previous),
             // Always false. The player role has no seek command — position belongs to the server,
@@ -71,6 +103,48 @@ public static MediaSessionState FromGroupState(GroupState? group, string? artwor
         };
     }
 
+    /// <summary>
+    /// The spec's track-position formula (<c>roles/metadata/v1.md</c>, "Calculating current track
+    /// position"): where the track is <paramref name="elapsedMicros"/> after the metadata took
+    /// effect.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// <c>progress = track_progress + elapsed × playback_speed / 1000</c>, in milliseconds, with
+    /// <c>playback_speed</c> the spec's ×1000 integer (1000 is normal, 0 is paused). Clamped to the
+    /// duration when one is known and never negative, exactly as the spec writes it.
+    /// </para>
+    /// <para>
+    /// <paramref name="elapsedMicros"/> is measured from the metadata's own timestamp, converted to
+    /// the local clock, never from when the message arrived: a server sends the next track's
+    /// metadata ahead of the audible change, so arrival time would credit the new track with the
+    /// seconds it had not yet played.
+    /// </para>
+    /// </remarks>
+    /// <param name="progress">The metadata's progress object, or null when it has none.</param>
+    /// <param name="elapsedMicros">
+    /// Local microseconds since the metadata's timestamp. A negative value, which only a caller
+    /// projecting a not-yet-current update could produce, counts as zero.
+    /// </param>
+    public static TimeSpan ProjectPosition(PlaybackProgress? progress, long elapsedMicros)
+    {
+        if (progress is null || progress.TrackProgress is not { } start || double.IsNaN(start))
+        {
+            return TimeSpan.Zero;
+        }
+
+        var speed = progress.PlaybackSpeed ?? NormalPlaybackSpeed;
+        var elapsedMilliseconds = Math.Max(elapsedMicros, 0) / 1000.0;
+        var projected = start + elapsedMilliseconds * speed / NormalPlaybackSpeed;
+
+        if (progress.TrackDuration is > 0 and var duration)
+        {
+            projected = Math.Min(projected, duration);
+        }
+
+        return TimeSpan.FromMilliseconds(Math.Max(projected, 0));
+    }
+
     /// <summary>
     /// Maps the SDK's playback state onto what a shell can display.
     /// </summary>

src/Sendspin.Core/MediaSession/ScheduledValue.cs

@@ -0,0 +1,137 @@
+namespace Sendspin.Core.MediaSession;
+
+/// <summary>
+/// What <see cref="ScheduledValue{T}.Offer"/> did with a value.
+/// </summary>
+public enum ScheduledOffer
+{
+    /// <summary>The value was due, so it became current at once.</summary>
+    Applied,
+
+    /// <summary>The value is not due yet and is being held as the pending one.</summary>
+    Held
+}
+
+/// <summary>
+/// A value that takes effect at a scheduled time: what is current now, plus at most one pending
+/// value waiting for its moment.
+/// </summary>
+/// <remarks>
+/// <para>
+/// This is the spec's model for both scheduled artwork (<c>roles/artwork/v1.md</c>) and scheduled
+/// metadata (<c>roles/metadata/v1.md</c>), which state the same rules in different words. A server
+/// with a queue sends the next track's picture and metadata a few seconds before the audible track
+/// change, stamped with the server-clock time they should take effect; applying them on arrival is
+/// what made the cover flip early. The rules, with times already converted to this machine's clock:
+/// </para>
+/// <list type="bullet">
+/// <item>A value whose time is past or present applies at once and discards any pending value.</item>
+/// <item>A value whose time is in the future becomes the pending value, replacing the one held.</item>
+/// <item>The pending value becomes current once its time is reached. It is never dropped for
+/// lateness: a promotion that runs late still promotes.</item>
+/// <item>A cancel discards the pending value and leaves the current one alone.</item>
+/// <item>A clear is not a separate operation: the spec expresses it as an empty value that follows
+/// the same timing, so a future-stamped <see cref="Offer"/> of null schedules the clear.</item>
+/// </list>
+/// <para>
+/// Pure and clock-free on purpose: every method takes the local time as an argument, so the rules
+/// are testable without a timer and the service owning an instance decides when to ask. It is not
+/// thread-safe; the owner serialises access.
+/// </para>
+/// </remarks>
+/// <typeparam name="T">The scheduled value. Null is the empty value.</typeparam>
+public sealed class ScheduledValue<T>
+    where T : class
+{
+    private T? _pending;
+    private long _pendingDue;
+    private bool _hasPending;
+
+    /// <summary>Gets the value in effect now, or null when there is none.</summary>
+    public T? Current { get; private set; }
+
+    /// <summary>
+    /// Gets the local time, in microseconds, at which <see cref="Current"/> took effect. Zero until
+    /// something has been applied.
+    /// </summary>
+    /// <remarks>
+    /// For metadata this is the origin the spec's progress formula runs from; the position is
+    /// projected from here, not from when the message arrived.
+    /// </remarks>
+    public long CurrentSince { get; private set; }
+
+    /// <summary>Gets whether a value is being held for a future time.</summary>
+    public bool HasPending => _hasPending;
+
+    /// <summary>Gets the held value, or null when nothing is pending.</summary>
+    public T? Pending => _hasPending ? _pending : null;
+
+    /// <summary>
+    /// Gets the local time, in microseconds, at which the pending value is due, or null when nothing
+    /// is pending. The owner arms its timer on this.
+    /// </summary>
+    public long? NextDue => _hasPending ? _pendingDue : null;
+
+    /// <summary>
+    /// Offers a value that should take effect at <paramref name="dueLocalMicros"/>.
+    /// </summary>
+    /// <param name="value">The value, or null for the empty value (a scheduled clear).</param>
+    /// <param name="dueLocalMicros">When it takes effect, on the local clock.</param>
+    /// <param name="nowLocalMicros">The local clock now.</param>
+    /// <returns>Whether the value was applied at once or is being held.</returns>
+    public ScheduledOffer Offer(T? value, long dueLocalMicros, long nowLocalMicros)
+    {
+        if (dueLocalMicros <= nowLocalMicros)
+        {
+            // Past or present: applies now and, per the spec, cancels whatever was scheduled. This
+            // is also how a server cancels a scheduled update — by re-sending the current state.
+            Apply(value, dueLocalMicros);
+            return ScheduledOffer.Applied;
+        }
+
+        _pending = value;
+        _pendingDue = dueLocalMicros;
+        _hasPending = true;
+        return ScheduledOffer.Held;
+    }
+
+    /// <summary>
+    /// Makes the pending value current if its time has come.
+    /// </summary>
+    /// <returns>True when something was promoted.</returns>
+    public bool Promote(long nowLocalMicros)
+    {
+        if (!_hasPending || _pendingDue > nowLocalMicros)
+        {
+            return false;
+        }
+
+        Apply(_pending, _pendingDue);
+        return true;
+    }
+
+    /// <summary>Discards the pending value. The current value is unaffected.</summary>
+    public void Cancel()
+    {
+        _pending = null;
+        _hasPending = false;
+    }
+
+    /// <summary>
+    /// Forgets both the current and the pending value, as on a stream end or a disconnect: there is
+    /// no longer a stream for either to belong to.
+    /// </summary>
+    public void Reset()
+    {
+        Cancel();
+        Current = null;
+        CurrentSince = 0;
+    }
+
+    private void Apply(T? value, long since)
+    {
+        Current = value;
+        CurrentSince = since;
+        Cancel();
+    }
+}