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 50ed815..aa6644e 100644 --- a/lib/routers/song.py +++ b/lib/routers/song.py @@ -829,45 +829,49 @@ def post_song_gap_fill(filename: str, data: dict): return {"ok": True, "written": additions, "skipped": skipped} -def _playable_stems_payload(song_path, filename: str) -> dict: +def _playable_stems_payload(filename: str, dlc) -> dict: """The playable stems (id/url/default) + full-mix URL for a sloppak. - Byte-for-byte the same shape the highway's WS `ready` builds — same - partition (the mixdown lifted out), same default resolution, same URL form — - because the stems plugin now preloads from THIS and then has to agree with - what the WS says a moment later, or it rebuilds the graph for nothing. + 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. - Why it exists: the stems plugin could only learn its stem list from the WS - `ready`, which arrives once the highway is up. So it decoded, and then copied - the whole song's PCM to its worklet, with the player already 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. + 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: - meta = sloppak_mod.extract_meta(song_path) + loaded = sloppak_mod.load_song(filename, dlc, appstate.sloppak_cache_dir) except Exception: return {"stems": [], "full_mix_url": None} q_fn = quote(filename, safe="") - stems = [ - { - "id": s["id"], - "url": f"/api/sloppak/{q_fn}/file/{quote(s['file'])}", - "default": bool(s.get("default", True)), - } - for s in (meta.get("stems") or []) - if s.get("id") and s.get("file") - ] - full_file = meta.get("full_mix_file") + + def _url(rel: str) -> str: + return f"/api/sloppak/{q_fn}/file/{quote(rel)}" + return { - "stems": stems, - "full_mix_url": f"/api/sloppak/{q_fn}/file/{quote(full_file)}" if full_file else None, + "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, } @@ -911,7 +915,7 @@ async def get_song_info(filename: str, stems: int = 0): if not stems: return meta extra = await loop.run_in_executor( - None, _playable_stems_payload, song_path, filename) + None, _playable_stems_payload, filename, dlc) return {**meta, **extra} if cached: diff --git a/lib/sloppak.py b/lib/sloppak.py index d02d5e7..8f943e9 100644 --- a/lib/sloppak.py +++ b/lib/sloppak.py @@ -1278,15 +1278,7 @@ def extract_meta(path: Path) -> dict: isinstance(sid, str) and sid and isinstance(sfile, str) and sfile ): - # `file` and `default` ride along so the REST song-info payload can - # publish the same playable-stem list the WS `ready` message does — - # the stems plugin needs it BEFORE the highway connects (see - # get_song_info). Same helper as load_song, so they cannot disagree. - valid_stems.append({ - "id": sid, - "file": sfile, - "default": stem_default_on(s.get("default", True)), - }) + valid_stems.append({"id": sid, "file": sfile}) # Partition exactly as load_song() does, for the same reason the library # filter must not lie: `full` is the mixdown, not an instrument (spec §5.3). # A separated pack that retains it would otherwise offer the user a "full" @@ -1313,11 +1305,4 @@ def extract_meta(path: Path) -> dict: "stem_count": stem_count, # feedBack#129: per-stem filter needs the id list, not just count. "stem_ids": stem_ids, - # The PLAYABLE stems (id/file/default), partitioned exactly as load_song - # does — the mixdown lifted out, never a layer. get_song_info turns these - # into URLs so the stems plugin can start fetching and decoding on - # `song:loading`, instead of waiting for the highway's WS `ready`. - "stems": instrument_stems, - # The mixdown, when the pack carries one (spec §5.3). Same reason. - "full_mix_file": (_full or {}).get("file") or None, } diff --git a/tests/test_song_info_stems.py b/tests/test_song_info_stems.py index d785902..dad4365 100644 --- a/tests/test_song_info_stems.py +++ b/tests/test_song_info_stems.py @@ -21,60 +21,52 @@ import yaml import sloppak -def _pak(tmp_path, stems, full=None, name="song.feedpak"): +def _pak(tmp_path, stems, full=None, name="song.feedpak", original_audio=None): manifest = { "title": "T", "artist": "A", "duration": 10.0, "arrangements": [], - "stems": stems, + "stems": stems + ([full] if full else []), } - if full: - manifest["stems"] = stems + [full] + 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 test_extract_meta_carries_file_and_default(tmp_path): - p = _pak(tmp_path, [ - {"id": "guitar", "file": "stems/guitar.ogg"}, # absent => on - {"id": "vocals", "file": "stems/vocals.ogg", "default": False}, - {"id": "drums", "file": "stems/drums.ogg", "default": "off"}, # string form - ]) - meta = sloppak.extract_meta(p) - by_id = {s["id"]: s for s in meta["stems"]} - assert by_id["guitar"]["default"] is True, "absent default means ON" - assert by_id["vocals"]["default"] is False - assert by_id["drums"]["default"] is False, "'off' must be honoured" - assert by_id["guitar"]["file"] == "stems/guitar.ogg" +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 test_the_mixdown_is_lifted_out_of_the_stem_list(tmp_path): - # `full` is the mixdown, not a layer (spec 5.3). Listing it beside the - # instruments would make the plugin play the whole song ON TOP of the stems. - p = _pak(tmp_path, - [{"id": "guitar", "file": "stems/guitar.ogg"}, - {"id": "bass", "file": "stems/bass.ogg"}], - full={"id": "full", "file": "stems/full.ogg"}) - meta = sloppak.extract_meta(p) - ids = [s["id"] for s in meta["stems"]] - assert ids == ["guitar", "bass"], "the mixdown must not be a layer" - assert meta["full_mix_file"] == "stems/full.ogg", "...but it must still be reachable" +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_a_single_full_pack_keeps_full_as_its_only_stem(tmp_path): - # A pack whose ONLY stem is `full` is a single-mix pack: there is nothing to - # be pristine against, so `full` stays playable and no mixdown is surfaced. - p = _pak(tmp_path, [{"id": "full", "file": "stems/full.ogg"}]) - meta = sloppak.extract_meta(p) - assert [s["id"] for s in meta["stems"]] == ["full"] - assert meta["full_mix_file"] is None - - -def test_default_resolution_is_shared_with_load_song(tmp_path): - # The whole point: REST and the WS must not drift. Both go through - # stem_default_on, so pin the helper's contract directly. +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 @@ -85,47 +77,64 @@ def test_default_resolution_is_shared_with_load_song(tmp_path): assert sloppak.stem_default_on(1) is True -def test_rest_payload_matches_what_the_ws_would_build(tmp_path): - """The safety property, pinned end to end. +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 - Rebuild the WS's stems_payload from load_song exactly as ws_highway does, - and require the REST helper to produce the identical list. + +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. """ - from urllib.parse import quote - from routers.song import _playable_stems_payload + 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") - p = _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") - - cache = tmp_path / "cache" - cache.mkdir() - loaded = sloppak.load_song(p.name, tmp_path, cache) - - q_fn = quote(p.name, safe="") - ws_stems = [ - {"id": s["id"], "url": f"/api/sloppak/{q_fn}/file/{quote(s['file'])}", - "default": s["default"]} - for s in loaded.stems - ] - ws_full = f"/api/sloppak/{q_fn}/file/{quote(loaded.full_mix)}" if loaded.full_mix else None - - rest = _playable_stems_payload(p, p.name) - - assert rest["stems"] == ws_stems, ( - "REST and the WS must publish the SAME stem list — a mismatch means the " - "plugin preloads a graph and then rebuilds it, which is worse than not " - "preloading at all" + 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"] == ws_full + 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. A pack we cannot read must fall back to the + # 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 - bad = tmp_path / "bad.feedpak" - bad.write_bytes(b"not a zip") - assert _playable_stems_payload(bad, "bad.feedpak") == {"stems": [], "full_mix_url": None} + 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}