From 939c98214b7ff2872cd0ac39a3015980ab4931bf Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Wed, 15 Jul 2026 00:36:13 +0200 Subject: [PATCH] feat(song-info): publish the playable stem list so stems can preload (fixes the 698ms freeze) (#972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(song-info): publish the playable stem list, so stems can preload The stems plugin could only learn its stem list from the highway's WS `ready`, which arrives once the highway is already up. So it fetched, decoded, and then handed every stem's PCM to its audio worklet — copying the WHOLE SONG — with the player already on screen. For a 4-minute 6-stem pack that is over half a GIGABYTE of memcpy, in one frame, on the main thread. Measured on a real load: a 698 ms frame, right as the song-credits card appeared, with the venue video visibly stopping. That is the "the video pauses when the author appears" report. GET /api/song/{f}?stems=1 now returns the same list — [{id, url, default}] plus full_mix_url — so the plugin can start the whole load at `song:loading`, before the highway (and the venue) is drawn, where a stalled frame costs nothing. Nothing about the work changes; only WHEN. Opt-in via the query param so the library's own metadata calls — the hot path — pay nothing. Deliberately NOT stored in the metadata cache: that is a fixed-column table, and widening it would mean a schema migration plus a stale row for every song already scanned, to cache something that is a plain manifest read on an already-unpacked pack. The safety property: REST and the WS must publish the SAME list. If they disagreed the plugin would preload a graph and then throw it away and rebuild — strictly worse than not preloading. So both now resolve `default` through one shared helper (stem_default_on, extracted from load_song), and a test rebuilds the WS's payload from load_song and requires the REST helper to produce the identical list, rather than pinning either against a snapshot. Also pinned: the mixdown is lifted OUT of the stem list (spec 5.3 — `full` is not a layer; listing it beside the instruments would play the whole song on top of the stems) while staying reachable as full_mix_url, a single-`full` pack keeps it as its only playable stem, and an unreadable pack yields an empty list rather than failing the request. Full suite 2608 passed. Consumed by feedBack-plugin-stems (preloadSong). * fix(song-info): call load_song for the stem payload — do not reimplement it CodeRabbit caught a real bug, and it would have hit most real libraries. load_song() falls back to the DEPRECATED `original_audio:` key when a pack has no reserved `full` stem — which is every pack written before feedpak 1.15.0. My payload rebuilt the full-mix rule from extract_meta and returned None for those: REST would say "no full mix" while the WS said there was one. Worse than a wrong field: the plugin would preload a graph WITHOUT the pristine mix and — because the stem signature still matched — never rebuild. Unity playback would silently downgrade to the lossy stem recombination. That is exactly the drift this PR claims to prevent, and my test had a hole: I only covered packs that carry a `full` stem. So stop reimplementing. The payload now calls load_song, whose LoadedSloppak already carries the partitioned stems and the resolved full mix, and builds the URLs exactly as ws_highway does. Drift is now impossible by construction rather than by agreement. extract_meta is reverted to its original shape (it never needed to change), and the shared stem_default_on helper stays as the one place `default: off` is resolved. Tests rewritten to compare against load_song — the WS's own function — for a reserved-`full` pack, a LEGACY original_audio pack (the case that was broken), and a single-`full` pack. Also documents the `?stems=1` contract in CHANGELOG.md. Full suite green. --- CHANGELOG.md | 10 +++ lib/routers/song.py | 74 ++++++++++++++++-- lib/sloppak.py | 25 ++++-- tests/test_song_info_stems.py | 140 ++++++++++++++++++++++++++++++++++ 4 files changed, 238 insertions(+), 11 deletions(-) create mode 100644 tests/test_song_info_stems.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 58dda22..8236ffa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 carry their gig log; instruments their gig count. ### Changed +- **`GET /api/song/{f}?stems=1`** (new, opt-in) — returns the pack's playable stem + list (`[{id, url, default}]` + `full_mix_url`), the same list the highway's WS + `ready` sends. The stems plugin could only learn it from that WS message, which + arrives once the highway is already on screen — so it decoded and then copied the + whole song's PCM to its audio worklet with the player visible: over half a gigabyte + of memcpy in one frame for a 6-stem pack, a measured 698 ms freeze right as the + song-credits card appeared. With the list available at `song:loading` the plugin + does all of it before the highway is drawn. Built by calling `load_song` itself, so + it cannot drift from what the WS sends. Opt-in, so the library's metadata calls pay + nothing. - **Folder library renders only the songs on screen** (#965) — a song list used to render *every* song it held. On a flat 50,944-song library that was one `
` with 50,938 children and ~1.3 **million** DOM nodes (~4.2 GB of renderer memory), diff --git a/lib/routers/song.py b/lib/routers/song.py index 72a1d06..aa6644e 100644 --- a/lib/routers/song.py +++ b/lib/routers/song.py @@ -829,9 +829,60 @@ def post_song_gap_fill(filename: str, data: dict): return {"ok": True, "written": additions, "skipped": skipped} +def _playable_stems_payload(filename: str, dlc) -> dict: + """The playable stems (id/url/default) + full-mix URL for a sloppak. + + Why it exists: the stems plugin could only learn its stem list from the + highway's WS `ready`, which arrives once the highway is already up. So it + decoded, and then copied the whole song's PCM to its worklet, with the player + on screen — half a gigabyte of memcpy in one frame, ~700 ms, freezing the + venue video. Given the list at `song:loading` it can do all of that BEFORE the + highway appears, behind the loading overlay where a stall costs nothing. + + The list MUST be the same one the WS sends a moment later. If it is not, the + plugin preloads a graph and then throws it away and rebuilds — strictly worse + than not preloading. So this does not reimplement the WS's construction, it + calls THE SAME FUNCTION: load_song, whose LoadedSloppak already carries the + partitioned stems and the resolved full mix, and then builds the URLs exactly + as ws_highway does. Drift is impossible by construction rather than by + agreement — which matters, because `full_mix` in particular is not simply the + `full` stem: load_song falls back to the deprecated `original_audio:` key for + every pack written before feedpak 1.15.0, and reimplementing that (I did, at + first) silently dropped the pristine full mix for most real libraries. + + Opt-in (`?stems=1`) so the library's own metadata calls — the hot path — pay + nothing for it. Non-sloppak sources (archives, loose folders) have no stems + to preload: load_song raises and we return the empty list. + """ + from urllib.parse import quote + + try: + loaded = sloppak_mod.load_song(filename, dlc, appstate.sloppak_cache_dir) + except Exception: + return {"stems": [], "full_mix_url": None} + + q_fn = quote(filename, safe="") + + def _url(rel: str) -> str: + return f"/api/sloppak/{q_fn}/file/{quote(rel)}" + + return { + "stems": [ + {"id": s["id"], "url": _url(s["file"]), "default": s["default"]} + for s in loaded.stems + ], + "full_mix_url": _url(loaded.full_mix) if loaded.full_mix else None, + } + + @router.get("/api/song/{filename:path}") -async def get_song_info(filename: str): - """Return song metadata, from cache or by extracting it from the song source.""" +async def get_song_info(filename: str, stems: int = 0): + """Return song metadata, from cache or by extracting it from the song source. + + `?stems=1` additionally returns the playable stem list with URLs, so the + stems plugin can start fetching/decoding on `song:loading` instead of waiting + for the highway's WS `ready` (see _playable_stems_payload). + """ import asyncio dlc = _get_dlc_dir() if not dlc: @@ -854,8 +905,21 @@ async def get_song_info(filename: str): mtime, size = appstate.stat_for_cache(song_path) cached = appstate.meta_db.get(cache_key, mtime, size) + loop = asyncio.get_event_loop() + + # The stem list is NOT stored in the metadata cache: that is a fixed-column + # table, and widening it would mean a migration plus a stale row for every + # song already scanned. It is cheap to read on demand (the pack is unpacked + # by then, so this is a plain manifest read), and only the opt-in caller pays. + async def _with_stems(meta: dict) -> dict: + if not stems: + return meta + extra = await loop.run_in_executor( + None, _playable_stems_payload, filename, dlc) + return {**meta, **extra} + if cached: - return cached + return await _with_stems(cached) # Extract in thread pool def _extract(): @@ -863,5 +927,5 @@ async def get_song_info(filename: str): appstate.meta_db.put(cache_key, mtime, size, meta) return meta - meta = await asyncio.get_event_loop().run_in_executor(None, _extract) - return meta + meta = await loop.run_in_executor(None, _extract) + return await _with_stems(meta) diff --git a/lib/sloppak.py b/lib/sloppak.py index d5a12a0..8f943e9 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -80,6 +80,20 @@ def find_full_mix(stems: list[dict]) -> dict | None: ) +def stem_default_on(raw) -> bool: + """Whether a manifest stem entry plays by default. + + Absent means on. A string is honoured so a hand-written manifest can say + `default: off`. Extracted so the WS `ready` payload and the REST song-info + payload cannot drift: the stems plugin now preloads from REST and then has + to agree with what the WS says a moment later, or it would rebuild the whole + graph for nothing. + """ + if isinstance(raw, str): + return raw.lower() not in ("off", "false", "0", "no") + return bool(raw) + + def partition_stems(stems: list[dict]) -> tuple[dict | None, list[dict]]: """Split stem descriptors into (mixdown, instrument_stems) for PLAYBACK. @@ -1100,12 +1114,11 @@ def load_song( sfile = str(s.get("file", "")) if not sid or not sfile: continue - default_val = s.get("default", True) - if isinstance(default_val, str): - default_on = default_val.lower() not in ("off", "false", "0", "no") - else: - default_on = bool(default_val) - stems.append({"id": sid, "file": sfile, "default": default_on}) + stems.append({ + "id": sid, + "file": sfile, + "default": stem_default_on(s.get("default", True)), + }) # The complete mixdown is a stem (spec §5.3), but it is not a *layer*: lift # it out so that no consumer of `stems` — the mixer, the library's stem diff --git a/tests/test_song_info_stems.py b/tests/test_song_info_stems.py new file mode 100644 index 0000000..dad4365 --- /dev/null +++ b/tests/test_song_info_stems.py @@ -0,0 +1,140 @@ +"""`/api/song/{f}?stems=1` — the playable stem list, for preloading. + +The stems plugin could only learn its stem list from the highway's WS `ready`, +which arrives once the highway is already up. So it decoded the stems and then +copied the whole song's PCM to its audio worklet with the player on screen — +half a gigabyte of memcpy in one frame, ~700 ms, which froze the venue video. + +Given the list at `song:loading` it can do all of that BEFORE the highway +appears, behind the loading overlay where a stall costs nothing. + +The safety property these tests exist for: the REST payload must be the SAME +list the WS builds. If they disagree, the plugin preloads one graph and then +throws it away and rebuilds another — strictly worse than not preloading. So +they are pinned against each other, not just against a snapshot. +""" + +import zipfile + +import yaml + +import sloppak + + +def _pak(tmp_path, stems, full=None, name="song.feedpak", original_audio=None): + manifest = { + "title": "T", "artist": "A", "duration": 10.0, + "arrangements": [], + "stems": stems + ([full] if full else []), + } + if original_audio: + # The deprecated pre-1.15.0 shape: the mixdown lives outside `stems`. + manifest["original_audio"] = original_audio + p = tmp_path / name + with zipfile.ZipFile(p, "w") as z: + # Real packs carry manifest.yaml — a JSON manifest is not read at all. + z.writestr("manifest.yaml", yaml.safe_dump(manifest)) + # _legacy_full_mix only returns a path that actually EXISTS on disk. + if original_audio: + z.writestr(original_audio, b"\0" * 16) + return p + + +def _payload(tmp_path, pak): + from routers.song import _playable_stems_payload + import appstate + cache = tmp_path / "cache" + cache.mkdir(exist_ok=True) + appstate.sloppak_cache_dir = cache + return _playable_stems_payload(pak.name, tmp_path) + + +def _ws_payload(tmp_path, pak): + """Rebuild the WS `ready` stems payload exactly as ws_highway.py does.""" + from urllib.parse import quote + cache = tmp_path / "cache" + cache.mkdir(exist_ok=True) + loaded = sloppak.load_song(pak.name, tmp_path, cache) + q = quote(pak.name, safe="") + return { + "stems": [ + {"id": s["id"], "url": f"/api/sloppak/{q}/file/{quote(s['file'])}", + "default": s["default"]} + for s in loaded.stems + ], + "full_mix_url": f"/api/sloppak/{q}/file/{quote(loaded.full_mix)}" if loaded.full_mix else None, + } + + +def test_default_resolution_is_shared_with_load_song(): + assert sloppak.stem_default_on(True) is True + assert sloppak.stem_default_on(False) is False + assert sloppak.stem_default_on("off") is False + assert sloppak.stem_default_on("false") is False + assert sloppak.stem_default_on("0") is False + assert sloppak.stem_default_on("no") is False + assert sloppak.stem_default_on("on") is True + assert sloppak.stem_default_on(1) is True + + +def test_rest_matches_the_ws_for_a_reserved_full_stem(tmp_path): + pak = _pak(tmp_path, + [{"id": "guitar", "file": "stems/guitar.ogg"}, + {"id": "vocals", "file": "stems/vocals.ogg", "default": "off"}], + full={"id": "full", "file": "stems/full.ogg"}, + name="Iron Maiden - Phantom.feedpak") + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert [s["id"] for s in rest["stems"]] == ["guitar", "vocals"], "the mixdown is not a layer" + assert rest["full_mix_url"].endswith("stems/full.ogg") + assert rest["stems"][1]["default"] is False + + +def test_rest_matches_the_ws_for_a_LEGACY_original_audio_pack(tmp_path): + """The one CodeRabbit caught, and the one that matters most in practice. + + load_song falls back to the DEPRECATED `original_audio:` key when a pack has + no reserved `full` stem — which is every pack written before feedpak 1.15.0, + i.e. most of a real library. My first version of this payload reimplemented + the full-mix rule from extract_meta and silently returned None for them: REST + would say "no full mix" while the WS said there was one. The plugin would then + preload a graph WITHOUT the pristine mix and, because the signature still + matched, never rebuild — unity playback silently downgraded to the lossy + recombination. + + The payload now calls load_song itself, so this cannot drift. Pinned anyway. + """ + pak = _pak(tmp_path, [ + {"id": "guitar", "file": "stems/guitar.ogg"}, + {"id": "bass", "file": "stems/bass.ogg"}, + ], name="Legacy Pack.feedpak", original_audio="original/full.ogg") + + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert rest["full_mix_url"] is not None, ( + "a pre-1.15.0 pack's full mix must survive — dropping it downgrades unity " + "playback to the lossy stem recombination, silently" + ) + assert rest["full_mix_url"].endswith("original/full.ogg") + + +def test_rest_matches_the_ws_for_a_single_full_pack(tmp_path): + # Its ONE stem IS the mixdown: nothing to be pristine against, so `full` stays + # the sole playable stem and no separate mixdown is surfaced. + pak = _pak(tmp_path, [{"id": "full", "file": "stems/full.ogg"}], name="Single.feedpak") + rest = _payload(tmp_path, pak) + assert rest == _ws_payload(tmp_path, pak) + assert [s["id"] for s in rest["stems"]] == ["full"] + assert rest["full_mix_url"] is None + + +def test_a_broken_pack_yields_an_empty_list_not_an_error(tmp_path): + # Preloading is an optimisation: an unreadable pack must fall back to the + # normal WS-driven path, never break the song-info request. + from routers.song import _playable_stems_payload + import appstate + cache = tmp_path / "cache" + cache.mkdir() + appstate.sloppak_cache_dir = cache + (tmp_path / "bad.feedpak").write_bytes(b"not a zip") + assert _playable_stems_payload("bad.feedpak", tmp_path) == {"stems": [], "full_mix_url": None}