Name cached artwork by its bytes so art that arrives before its metadata still refreshes

docs/ARCHITECTURE.md

@@ -543,8 +543,8 @@ Collected because each one is silent when wrong.
 `xesam:artist` is `as` not `s`; `mpris:trackid` is an **object path** unique per track and outside the
 reserved `/org/mpris` namespace; `mpris:length` is `x` in **microseconds**. Album art must be
 `file://` — KDE's lock screen blocks `http` and `data:`, and GNOME has no `data:` backend — written to
-a **unique filename per track**, because GNOME's texture cache is keyed on the icon string for the
-life of the shell. Media keys need **nothing beyond MPRIS**: GNOME removed its `SettingsDaemon.MediaKeys`
+a **unique filename per picture** — named by a hash of the bytes, see the Flatpak note below for why
+not per track — because GNOME's texture cache is keyed on the icon string for the life of the shell. Media keys need **nothing beyond MPRIS**: GNOME removed its `SettingsDaemon.MediaKeys`
 API in 2021 with the message "superseded by MPRIS".
 
 **Linux tray.** Avalonia's `SetTitleAndTooltip` early-outs on a null tooltip and ships a
@@ -578,7 +578,11 @@ output is swallowed inside a macios-hosted `.app`; log to a file during bring-up
 `<app_id>.*` and declaring it is a Flathub linter error. Prefer `--filesystem=xdg-run/pipewire-0`
 over `--socket=pulseaudio`, which forces `enable-shm=no` and pushes every buffer through the socket.
 Album art must go to `$XDG_RUNTIME_DIR/app/$FLATPAK_ID/`, not `/tmp` — the sandbox's `/tmp` is not
-the host's and the shell cannot follow a path into it.
+the host's and the shell cannot follow a path into it. Artwork files are named by a hash of the
+image bytes, not by the track's metadata: with a queue the server can deliver the next track's
+picture before the next track's metadata, and a metadata-derived name would overwrite the current
+track's file in place — under a path the window dedupes on and the Plasma and GNOME applets cache
+by URL, so nothing would refresh.
 
 ## UI shell
 

docs/NEXT_STEPS.md

@@ -239,6 +239,22 @@ 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.
+
+---
+
 ## Things that are done and should not be reopened
 
 Recorded so the reasoning is not relitigated from scratch:

src/Sendspin.Core/MediaSession/MediaSessionMapper.cs

@@ -1,4 +1,3 @@
-using System.Globalization;
 using System.Security.Cryptography;
 using System.Text;
 using Sendspin.SDK.Models;
@@ -166,27 +165,35 @@ public static string ToMprisTrackId(string? trackIdentity)
     }
 
     /// <summary>
-    /// Builds a filename for this track's artwork.
+    /// Builds a filename for a piece of artwork from the image bytes themselves.
     /// </summary>
     /// <remarks>
-    /// Unique per track, and that is a requirement rather than tidiness: GNOME's texture cache
-    /// is keyed on the icon string for the lifetime of the shell, so reusing one filename
-    /// leaves the first track's art on screen forever.
+    /// <para>
+    /// Unique per picture, and that is a requirement rather than tidiness. Every consumer of
+    /// the path dedupes by it: the window and each platform's media session reload only when
+    /// the path changes, and GNOME's texture cache and Plasma's applet cache cover art by URL
+    /// for the life of the shell. So a new picture must always land at a new path, and the same
+    /// picture re-sent may land at the same one.
+    /// </para>
+    /// <para>
+    /// Hashing the bytes rather than the track's metadata is what makes that hold: with a queue
+    /// the server can deliver the next track's picture before the next track's metadata, and a
+    /// name taken from the metadata would then overwrite the current track's file in place,
+    /// under a path nobody rereads. Everything else that touches the file names points here
+    /// for the reason rather than repeating it.
+    /// </para>
     /// </remarks>
-    public static string ArtworkFileName(string? trackIdentity, string extension = "jpg")
-    {
-        var token = string.IsNullOrEmpty(trackIdentity) ? "notrack" : ToHexToken(trackIdentity);
-        return $"artwork-{token}.{extension.TrimStart('.')}";
-    }
+    public static string ArtworkFileName(ReadOnlySpan<byte> imageData, string extension = "jpg") =>
+        $"artwork-{ToHexToken(imageData)}.{extension.TrimStart('.')}";
 
     /// <summary>
     /// Derives a stable identity for a track from its metadata.
     /// </summary>
     /// <remarks>
     /// Hashed from artist, album and title rather than taken from a server-supplied id,
     /// because the protocol's metadata carries no track identifier. Position is deliberately
-    /// excluded so that the identity, and therefore the artwork filename and the MPRIS track
-    /// id, stay put as a track plays.
+    /// excluded so that the identity, and therefore the MPRIS track id, stays put as a track
+    /// plays.
     /// </remarks>
     public static string? BuildTrackIdentity(TrackMetadata? metadata)
     {
@@ -231,12 +238,17 @@ private static bool Supports(IEnumerable<string> commands, string command) =>
     /// filename.
     /// </summary>
     /// <remarks>
-    /// SHA-256 truncated to 16 bytes. Not a security boundary — it exists so that a title
-    /// containing a slash, a colon or a non-ASCII character cannot produce an invalid path.
+    /// Not a security boundary — it exists so that a title containing a slash, a colon or a
+    /// non-ASCII character cannot produce an invalid path.
     /// </remarks>
-    private static string ToHexToken(string value)
+    private static string ToHexToken(string value) => ToHexToken(Encoding.UTF8.GetBytes(value));
+
+    /// <summary>
+    /// Reduces arbitrary bytes to a short hex token: SHA-256 truncated to 16 bytes.
+    /// </summary>
+    private static string ToHexToken(ReadOnlySpan<byte> value)
     {
-        var hash = SHA256.HashData(Encoding.UTF8.GetBytes(value));
-        return Convert.ToHexString(hash.AsSpan(0, 16)).ToLower(CultureInfo.InvariantCulture);
+        var hash = SHA256.HashData(value);
+        return Convert.ToHexStringLower(hash.AsSpan(0, 16));
     }
 }

src/Sendspin.Platform.Shared/Client/SendspinPlayerService.cs

@@ -1057,16 +1057,11 @@ private void PersistVolumeIfChanged(int volume, bool muted)
         });
     }
 
+    // Deliberately not keyed on the group's current metadata, which can lag the picture: see
+    // MediaSessionMapper.ArtworkFileName.
     private void OnArtworkReceived(object? sender, ArtworkReceivedEventArgs e)
     {
-        GroupState? group;
-        lock (_sessionGate)
-        {
-            group = _group;
-        }
-
-        var identity = MediaSessionMapper.BuildTrackIdentity(group?.Metadata);
-        var path = _artworkCache.Write(identity, e.ImageData);
+        var path = _artworkCache.Write(e.ImageData);
 
         lock (_sessionGate)
         {

src/Sendspin.Platform.Shared/Media/ArtworkCache.cs

@@ -16,11 +16,11 @@ namespace Sendspin.Platform.Shared.Media;
 /// discover that separately, every platform gets a file.
 /// </para>
 /// <para>
-/// <strong>One file per track, never one reused file.</strong> GNOME's texture cache is keyed
+/// <strong>One file per picture, never one reused file.</strong> GNOME's texture cache is keyed
 /// on the icon string for the lifetime of the shell, so writing every track's art to
 /// <c>artwork.jpg</c> leaves the first track's picture on screen for the rest of the session.
-/// Names come from <see cref="MediaSessionMapper.ArtworkFileName"/>, which derives them from
-/// track identity.
+/// Names come from <see cref="MediaSessionMapper.ArtworkFileName"/>, which hashes the image
+/// bytes rather than the track's metadata; its remarks say why.
 /// </para>
 /// <para>
 /// Files land in <see cref="IPlatformPaths.AlbumArtCacheDirectory"/>. Under Flatpak that
@@ -53,24 +53,22 @@ public ArtworkCache(IPlatformPaths paths, ILogger<ArtworkCache> logger)
     }
 
     /// <summary>
-    /// Writes artwork for a track and returns its absolute path, or null when it could not be
-    /// written.
+    /// Writes a picture and returns its absolute path, or null when it could not be written.
     /// </summary>
     /// <remarks>
     /// Returns null rather than throwing: artwork is decoration, and failing to cache a
     /// picture must not interrupt playback. The reason is logged.
     /// </remarks>
-    /// <param name="trackIdentity">Track identity, from <see cref="MediaSessionMapper"/>.</param>
     /// <param name="imageData">Encoded image bytes as received from the server.</param>
-    public string? Write(string? trackIdentity, ReadOnlySpan<byte> imageData)
+    public string? Write(ReadOnlySpan<byte> imageData)
     {
         if (imageData.IsEmpty)
         {
             return null;
         }
 
         var extension = DetectExtension(imageData);
-        var fileName = MediaSessionMapper.ArtworkFileName(trackIdentity, extension);
+        var fileName = MediaSessionMapper.ArtworkFileName(imageData, extension);
 
         try
         {

src/Sendspin.Player/ViewModels/MainViewModel.cs

@@ -884,6 +884,8 @@ private void OnProgressTick(object? sender, EventArgs e)
     /// </summary>
     private void LoadArtwork(string? path)
     {
+        // The path is the whole test: the cache names files by content, so a new picture is
+        // always a new path (MediaSessionMapper.ArtworkFileName).
         if (path == _artworkPath)
         {
             return;

src/Sendspin.Tests/ArtworkCacheTests.cs

@@ -0,0 +1,82 @@
+using Microsoft.Extensions.Logging.Abstractions;
+using Sendspin.Platform.Shared.Media;
+using Xunit;
+
+namespace Sendspin.Tests;
+
+/// <summary>
+/// Retention in <see cref="ArtworkCache"/>: a bounded set of files, keyed by content.
+/// </summary>
+/// <remarks>
+/// The cache lives in a runtime directory that is commonly a size-limited tmpfs, so it must prune;
+/// and because files are named by their bytes, a picture sent twice must occupy one slot, not two.
+/// </remarks>
+public sealed class ArtworkCacheTests
+{
+    [Fact]
+    public void Write_KeepsTheNewestEightPictures()
+    {
+        using var paths = new TemporaryPaths();
+        var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
+
+        var written = Enumerable.Range(0, 9).Select(i => cache.Write(Jpeg((byte)i))).ToList();
+
+        Assert.All(written, Assert.NotNull);
+        Assert.False(File.Exists(written[0]));
+        Assert.All(written.Skip(1), path => Assert.True(File.Exists(path)));
+        Assert.Equal(8, Directory.GetFiles(paths.AlbumArtCacheDirectory).Length);
+    }
+
+    [Fact]
+    public void Write_ThenTheSamePictureAgain_IsOneFileAtOnePath()
+    {
+        using var paths = new TemporaryPaths();
+        var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
+
+        var first = cache.Write(Jpeg(0xA1));
+        var again = cache.Write(Jpeg(0xA1));
+
+        Assert.Equal(first, again);
+        Assert.Single(Directory.GetFiles(paths.AlbumArtCacheDirectory));
+    }
+
+    /// <summary>
+    /// Re-sending a picture refreshes its place in the retention order rather than adding to it,
+    /// so it is the untouched oldest picture that goes when the cache is full.
+    /// </summary>
+    [Fact]
+    public void Write_ResendingAPicture_MovesItToNewest()
+    {
+        using var paths = new TemporaryPaths();
+        var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
+
+        var oldest = cache.Write(Jpeg(0));
+        var second = cache.Write(Jpeg(1));
+        for (byte i = 2; i < 8; i++)
+        {
+            cache.Write(Jpeg(i));
+        }
+
+        cache.Write(Jpeg(0));
+        cache.Write(Jpeg(8));
+
+        Assert.True(File.Exists(oldest));
+        Assert.False(File.Exists(second));
+        Assert.Equal(8, Directory.GetFiles(paths.AlbumArtCacheDirectory).Length);
+    }
+
+    [Fact]
+    public void Clear_DeletesEverythingWritten()
+    {
+        using var paths = new TemporaryPaths();
+        var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
+        var path = cache.Write(Jpeg(0xA1));
+
+        cache.Clear();
+
+        Assert.False(File.Exists(path));
+    }
+
+    /// <summary>A JPEG signature followed by one byte that makes this picture distinct.</summary>
+    private static byte[] Jpeg(byte marker) => [0xFF, 0xD8, 0xFF, marker];
+}

src/Sendspin.Tests/ClientAdvertisementTests.cs

@@ -316,7 +316,7 @@ public async Task Artwork_InTheAdvertisedFormatIsStoredUnderThatFormatsExtension
         using var paths = new TemporaryPaths();
         var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
 
-        var path = cache.Write("track-1", JpegBytes(width: 512, height: 512));
+        var path = cache.Write(JpegBytes(width: 512, height: 512));
 
         Assert.NotNull(path);
         Assert.Equal(ExtensionFor(format), Path.GetExtension(path));
@@ -354,7 +354,7 @@ public async Task Artwork_LargerThanTheAdvertisedDimensionsIsStoredUnchanged()
         using var paths = new TemporaryPaths();
         var cache = new ArtworkCache(paths, NullLogger<ArtworkCache>.Instance);
 
-        var path = cache.Write("track-1", oversized);
+        var path = cache.Write(oversized);
 
         Assert.NotNull(path);
         Assert.Equal(oversized, await File.ReadAllBytesAsync(path));

src/Sendspin.Tests/MediaSessionMapperTests.cs

@@ -209,23 +209,37 @@ public void ToMprisTrackId_SurvivesHostileMetadata()
     }
 
     /// <summary>
-    /// Artwork filenames must be unique per track.
+    /// Artwork filenames must be unique per picture, and stable for one.
     /// </summary>
     /// <remarks>
-    /// GNOME's texture cache is keyed on the icon string for the lifetime of the shell, so reusing
-    /// one filename leaves the first track's picture on screen for the rest of the session.
+    /// Every consumer dedupes by path, so a new picture must be a new path; the name comes from the
+    /// bytes rather than the track for the reason <see cref="MediaSessionMapper.ArtworkFileName"/>
+    /// gives.
     /// </remarks>
     [Fact]
-    public void ArtworkFileName_DiffersPerTrackAndIsStableForOne()
+    public void ArtworkFileName_DiffersPerPictureAndIsStableForOne()
     {
-        var first = MediaSessionMapper.BuildTrackIdentity(
-            new TrackMetadata { Title = "One", Artist = "A", Album = "X" });
-        var second = MediaSessionMapper.BuildTrackIdentity(
-            new TrackMetadata { Title = "Two", Artist = "A", Album = "X" });
+        byte[] first = [0xFF, 0xD8, 0xFF, 0x01];
+        byte[] second = [0xFF, 0xD8, 0xFF, 0x02];
 
         Assert.NotEqual(MediaSessionMapper.ArtworkFileName(first), MediaSessionMapper.ArtworkFileName(second));
-        Assert.Equal(MediaSessionMapper.ArtworkFileName(first), MediaSessionMapper.ArtworkFileName(first));
+        Assert.Equal(MediaSessionMapper.ArtworkFileName(first), MediaSessionMapper.ArtworkFileName([.. first]));
         Assert.DoesNotContain(Path.DirectorySeparatorChar, MediaSessionMapper.ArtworkFileName(first));
+        Assert.DoesNotContain(Path.AltDirectorySeparatorChar, MediaSessionMapper.ArtworkFileName(first));
+    }
+
+    /// <summary>
+    /// The extension is the caller's, and the name does not depend on it being dotted.
+    /// </summary>
+    [Fact]
+    public void ArtworkFileName_TakesTheExtensionEitherWay()
+    {
+        byte[] bytes = [1, 2, 3];
+
+        Assert.EndsWith(".png", MediaSessionMapper.ArtworkFileName(bytes, "png"));
+        Assert.Equal(
+            MediaSessionMapper.ArtworkFileName(bytes, "png"),
+            MediaSessionMapper.ArtworkFileName(bytes, ".png"));
     }
 
     /// <summary>