diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index cbe41e9..a707902 100644 --- a/docs/ARCHITECTURE.md +++ b/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 `.*` 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 diff --git a/docs/NEXT_STEPS.md b/docs/NEXT_STEPS.md index a0f484a..0749005 100644 --- a/docs/NEXT_STEPS.md +++ b/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: diff --git a/src/Sendspin.Core/MediaSession/MediaSessionMapper.cs b/src/Sendspin.Core/MediaSession/MediaSessionMapper.cs index f318441..9452bf1 100644 --- a/src/Sendspin.Core/MediaSession/MediaSessionMapper.cs +++ b/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,18 +165,26 @@ public static string ToMprisTrackId(string? trackIdentity) } /// - /// Builds a filename for this track's artwork. + /// Builds a filename for a piece of artwork from the image bytes themselves. /// /// - /// 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. + /// + /// 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. + /// + /// + /// 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. + /// /// - 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 imageData, string extension = "jpg") => + $"artwork-{ToHexToken(imageData)}.{extension.TrimStart('.')}"; /// /// Derives a stable identity for a track from its metadata. @@ -185,8 +192,8 @@ public static string ArtworkFileName(string? trackIdentity, string extension = " /// /// 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. /// public static string? BuildTrackIdentity(TrackMetadata? metadata) { @@ -231,12 +238,17 @@ private static bool Supports(IEnumerable commands, string command) => /// filename. /// /// - /// 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. /// - private static string ToHexToken(string value) + private static string ToHexToken(string value) => ToHexToken(Encoding.UTF8.GetBytes(value)); + + /// + /// Reduces arbitrary bytes to a short hex token: SHA-256 truncated to 16 bytes. + /// + private static string ToHexToken(ReadOnlySpan 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)); } } diff --git a/src/Sendspin.Platform.Shared/Client/SendspinPlayerService.cs b/src/Sendspin.Platform.Shared/Client/SendspinPlayerService.cs index 1af28b5..79f3928 100644 --- a/src/Sendspin.Platform.Shared/Client/SendspinPlayerService.cs +++ b/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) { diff --git a/src/Sendspin.Platform.Shared/Media/ArtworkCache.cs b/src/Sendspin.Platform.Shared/Media/ArtworkCache.cs index 455fd71..9cced2a 100644 --- a/src/Sendspin.Platform.Shared/Media/ArtworkCache.cs +++ b/src/Sendspin.Platform.Shared/Media/ArtworkCache.cs @@ -16,11 +16,11 @@ namespace Sendspin.Platform.Shared.Media; /// discover that separately, every platform gets a file. /// /// -/// One file per track, never one reused file. GNOME's texture cache is keyed +/// One file per picture, never one reused file. GNOME's texture cache is keyed /// on the icon string for the lifetime of the shell, so writing every track's art to /// artwork.jpg leaves the first track's picture on screen for the rest of the session. -/// Names come from , which derives them from -/// track identity. +/// Names come from , which hashes the image +/// bytes rather than the track's metadata; its remarks say why. /// /// /// Files land in . Under Flatpak that @@ -53,16 +53,14 @@ public ArtworkCache(IPlatformPaths paths, ILogger logger) } /// - /// 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. /// /// /// Returns null rather than throwing: artwork is decoration, and failing to cache a /// picture must not interrupt playback. The reason is logged. /// - /// Track identity, from . /// Encoded image bytes as received from the server. - public string? Write(string? trackIdentity, ReadOnlySpan imageData) + public string? Write(ReadOnlySpan imageData) { if (imageData.IsEmpty) { @@ -70,7 +68,7 @@ public ArtworkCache(IPlatformPaths paths, ILogger logger) } var extension = DetectExtension(imageData); - var fileName = MediaSessionMapper.ArtworkFileName(trackIdentity, extension); + var fileName = MediaSessionMapper.ArtworkFileName(imageData, extension); try { diff --git a/src/Sendspin.Player/ViewModels/MainViewModel.cs b/src/Sendspin.Player/ViewModels/MainViewModel.cs index e41f4e3..cb32c39 100644 --- a/src/Sendspin.Player/ViewModels/MainViewModel.cs +++ b/src/Sendspin.Player/ViewModels/MainViewModel.cs @@ -884,6 +884,8 @@ private void OnProgressTick(object? sender, EventArgs e) /// 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; diff --git a/src/Sendspin.Tests/ArtworkCacheTests.cs b/src/Sendspin.Tests/ArtworkCacheTests.cs new file mode 100644 index 0000000..43bc51c --- /dev/null +++ b/src/Sendspin.Tests/ArtworkCacheTests.cs @@ -0,0 +1,82 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Sendspin.Platform.Shared.Media; +using Xunit; + +namespace Sendspin.Tests; + +/// +/// Retention in : a bounded set of files, keyed by content. +/// +/// +/// 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. +/// +public sealed class ArtworkCacheTests +{ + [Fact] + public void Write_KeepsTheNewestEightPictures() + { + using var paths = new TemporaryPaths(); + var cache = new ArtworkCache(paths, NullLogger.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.Instance); + + var first = cache.Write(Jpeg(0xA1)); + var again = cache.Write(Jpeg(0xA1)); + + Assert.Equal(first, again); + Assert.Single(Directory.GetFiles(paths.AlbumArtCacheDirectory)); + } + + /// + /// 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. + /// + [Fact] + public void Write_ResendingAPicture_MovesItToNewest() + { + using var paths = new TemporaryPaths(); + var cache = new ArtworkCache(paths, NullLogger.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.Instance); + var path = cache.Write(Jpeg(0xA1)); + + cache.Clear(); + + Assert.False(File.Exists(path)); + } + + /// A JPEG signature followed by one byte that makes this picture distinct. + private static byte[] Jpeg(byte marker) => [0xFF, 0xD8, 0xFF, marker]; +} diff --git a/src/Sendspin.Tests/ClientAdvertisementTests.cs b/src/Sendspin.Tests/ClientAdvertisementTests.cs index ed78220..bb3256c 100644 --- a/src/Sendspin.Tests/ClientAdvertisementTests.cs +++ b/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.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.Instance); - var path = cache.Write("track-1", oversized); + var path = cache.Write(oversized); Assert.NotNull(path); Assert.Equal(oversized, await File.ReadAllBytesAsync(path)); diff --git a/src/Sendspin.Tests/MediaSessionMapperTests.cs b/src/Sendspin.Tests/MediaSessionMapperTests.cs index f02a323..c9959d3 100644 --- a/src/Sendspin.Tests/MediaSessionMapperTests.cs +++ b/src/Sendspin.Tests/MediaSessionMapperTests.cs @@ -209,23 +209,37 @@ public void ToMprisTrackId_SurvivesHostileMetadata() } /// - /// Artwork filenames must be unique per track. + /// Artwork filenames must be unique per picture, and stable for one. /// /// - /// 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 + /// gives. /// [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)); + } + + /// + /// The extension is the caller's, and the name does not depend on it being dotted. + /// + [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")); } /// diff --git a/src/Sendspin.Tests/PlayerServiceEventTests.cs b/src/Sendspin.Tests/PlayerServiceEventTests.cs index 0d3502d..3b352e0 100644 --- a/src/Sendspin.Tests/PlayerServiceEventTests.cs +++ b/src/Sendspin.Tests/PlayerServiceEventTests.cs @@ -13,7 +13,7 @@ namespace Sendspin.Tests; /// /// Tests that the player service forwards the palette and the visualizer frames a session -/// delivers, as it does the artwork. +/// delivers, and that the artwork it publishes survives the order a queue delivers it in. /// /// /// @@ -86,6 +86,87 @@ public async Task TheDialledSession_ForwardsLoudnessAndBeatFrames() Assert.Null(frames[1].Loudness); } + /// + /// A picture that arrives before its track's metadata must still be published as a change. + /// + /// + /// + /// The order a queue produces: track 1's metadata, its artwork A, then artwork B before + /// track 2's metadata. Every consumer of the published path dedupes by it, so if B were written + /// under the name the current metadata implies it would overwrite A's file in place and nothing + /// would notice. The path has to change the moment B arrives, and the two paths have to differ. + /// + /// + /// The same picture re-sent must land at the same path: that is the dedupe working as intended, + /// not a regression. + /// + /// + [Fact] + public async Task ArtworkArrivingBeforeItsMetadata_IsPublishedUnderANewPath() + { + using var paths = new TemporaryPaths(); + var (service, connection) = await ConnectAsync(paths); + await using var _ = service; + + var published = new List(); + service.StateChanged += (_, state) => published.Add(state.ArtworkFilePath); + + connection.Receive(""" + {"type":"server/state","payload":{"metadata":{"title":"One","artist":"A","album":"X"}}} + """); + connection.ReceiveBinary(ArtworkFrame(timestamp: 1_000, Jpeg(marker: 0xA1))); + var pathA = published[^1]; + + connection.ReceiveBinary(ArtworkFrame(timestamp: 2_000, Jpeg(marker: 0xB2))); + var pathB = published[^1]; + + connection.Receive(""" + {"type":"server/state","payload":{"metadata":{"title":"Two","artist":"A","album":"X"}}} + """); + + Assert.NotNull(pathA); + Assert.NotNull(pathB); + Assert.NotEqual(pathA, pathB); + Assert.Equal(pathB, published[^1]); + Assert.True(File.Exists(pathA)); + Assert.True(File.Exists(pathB)); + + connection.ReceiveBinary(ArtworkFrame(timestamp: 3_000, Jpeg(marker: 0xB2))); + + Assert.Equal(pathB, published[^1]); + } + + /// + /// The first track after connect is the same race with no metadata at all: two pictures before + /// any server/state must be two files, with the newer one published once there is a + /// state to publish it in. + /// + /// + /// Nothing is published until the first group state arrives — the session is idle — so the + /// evidence is the cache directory, not the event stream. + /// + [Fact] + public async Task TwoPicturesBeforeAnyMetadata_AreTwoFiles() + { + using var paths = new TemporaryPaths(); + var (service, connection) = await ConnectAsync(paths); + await using var _ = service; + + var published = new List(); + service.StateChanged += (_, state) => published.Add(state.ArtworkFilePath); + + connection.ReceiveBinary(ArtworkFrame(timestamp: 1_000, Jpeg(marker: 0xA1))); + connection.ReceiveBinary(ArtworkFrame(timestamp: 2_000, Jpeg(marker: 0xB2))); + connection.Receive(""" + {"type":"server/state","payload":{"metadata":{"title":"One","artist":"A","album":"X"}}} + """); + + var path = published[^1]; + Assert.NotNull(path); + Assert.Equal(Jpeg(marker: 0xB2), await File.ReadAllBytesAsync(path)); + Assert.Equal(2, Directory.GetFiles(paths.AlbumArtCacheDirectory).Length); + } + /// /// The unsubscribe on the client path: a palette that arrives on a transport the service has /// let go must not reach anyone, or a stale session could recolour the window. @@ -171,6 +252,12 @@ private static byte[] LoudnessFrame(long timestamp, ushort loudness) private static byte[] BeatFrame(long timestamp, bool downbeat) => Frame(BinaryMessageTypes.VisualizerBeat, timestamp, [(byte)(downbeat ? 1 : 0)]); + private static byte[] ArtworkFrame(long timestamp, byte[] image) => + Frame(BinaryMessageTypes.Artwork0, timestamp, image); + + /// A JPEG signature followed by one byte that makes this picture distinct. + private static byte[] Jpeg(byte marker) => [0xFF, 0xD8, 0xFF, marker]; + private sealed class NoAudioDevices : IAudioDeviceEnumerator { public IReadOnlyList GetDevices() => [];