mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-15 21:27:14 +00:00
feat(song-info): publish the playable stem list so stems can preload (fixes the 698ms freeze) (#972)
* 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.
This commit is contained in:
@@ -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.
|
carry their gig log; instruments their gig count.
|
||||||
|
|
||||||
### Changed
|
### 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
|
- **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 `<div>`
|
render *every* song it held. On a flat 50,944-song library that was one `<div>`
|
||||||
with 50,938 children and ~1.3 **million** DOM nodes (~4.2 GB of renderer memory),
|
with 50,938 children and ~1.3 **million** DOM nodes (~4.2 GB of renderer memory),
|
||||||
|
|||||||
+69
-5
@@ -829,9 +829,60 @@ def post_song_gap_fill(filename: str, data: dict):
|
|||||||
return {"ok": True, "written": additions, "skipped": skipped}
|
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}")
|
@router.get("/api/song/{filename:path}")
|
||||||
async def get_song_info(filename: str):
|
async def get_song_info(filename: str, stems: int = 0):
|
||||||
"""Return song metadata, from cache or by extracting it from the song source."""
|
"""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
|
import asyncio
|
||||||
dlc = _get_dlc_dir()
|
dlc = _get_dlc_dir()
|
||||||
if not dlc:
|
if not dlc:
|
||||||
@@ -854,8 +905,21 @@ async def get_song_info(filename: str):
|
|||||||
|
|
||||||
mtime, size = appstate.stat_for_cache(song_path)
|
mtime, size = appstate.stat_for_cache(song_path)
|
||||||
cached = appstate.meta_db.get(cache_key, mtime, size)
|
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:
|
if cached:
|
||||||
return cached
|
return await _with_stems(cached)
|
||||||
|
|
||||||
# Extract in thread pool
|
# Extract in thread pool
|
||||||
def _extract():
|
def _extract():
|
||||||
@@ -863,5 +927,5 @@ async def get_song_info(filename: str):
|
|||||||
appstate.meta_db.put(cache_key, mtime, size, meta)
|
appstate.meta_db.put(cache_key, mtime, size, meta)
|
||||||
return meta
|
return meta
|
||||||
|
|
||||||
meta = await asyncio.get_event_loop().run_in_executor(None, _extract)
|
meta = await loop.run_in_executor(None, _extract)
|
||||||
return meta
|
return await _with_stems(meta)
|
||||||
|
|||||||
+19
-6
@@ -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]]:
|
def partition_stems(stems: list[dict]) -> tuple[dict | None, list[dict]]:
|
||||||
"""Split stem descriptors into (mixdown, instrument_stems) for PLAYBACK.
|
"""Split stem descriptors into (mixdown, instrument_stems) for PLAYBACK.
|
||||||
|
|
||||||
@@ -1100,12 +1114,11 @@ def load_song(
|
|||||||
sfile = str(s.get("file", ""))
|
sfile = str(s.get("file", ""))
|
||||||
if not sid or not sfile:
|
if not sid or not sfile:
|
||||||
continue
|
continue
|
||||||
default_val = s.get("default", True)
|
stems.append({
|
||||||
if isinstance(default_val, str):
|
"id": sid,
|
||||||
default_on = default_val.lower() not in ("off", "false", "0", "no")
|
"file": sfile,
|
||||||
else:
|
"default": stem_default_on(s.get("default", True)),
|
||||||
default_on = bool(default_val)
|
})
|
||||||
stems.append({"id": sid, "file": sfile, "default": default_on})
|
|
||||||
|
|
||||||
# The complete mixdown is a stem (spec §5.3), but it is not a *layer*: lift
|
# 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
|
# it out so that no consumer of `stems` — the mixer, the library's stem
|
||||||
|
|||||||
@@ -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}
|
||||||
Reference in New Issue
Block a user