feat(song-info): publish the playable stem list so stems can preload (fixes the 698ms freeze) - #972
Conversation
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).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe song-info endpoint optionally returns playable stem metadata and a full-mix URL. Shared default parsing and metadata loading align REST responses with WebSocket-built song data, with tests covering partitioning, legacy packs, parity, and corrupt packs. ChangesPlayable stems
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant get_song_info
participant _with_stems
participant _playable_stems_payload
participant sloppak.load_song
Client->>get_song_info: Request with stems=1
get_song_info->>_with_stems: Augment song metadata
_with_stems->>_playable_stems_payload: Extract stems payload
_playable_stems_payload->>sloppak.load_song: Load sloppak
sloppak.load_song-->>_playable_stems_payload: Return normalized stems
_playable_stems_payload-->>_with_stems: Return playable and full-mix URLs
_with_stems-->>Client: Return augmented song metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/routers/song.py`:
- Around line 874-881: Update CHANGELOG.md’s [Unreleased] section to document
the new public `stems` query parameter supported by `get_song_info`, including
that `?stems=1` returns playable stem metadata and URLs.
In `@lib/sloppak.py`:
- Around line 1316-1322: The full_mix_file mapping in extract_meta must match
load_song’s legacy fallback behavior. Reuse the existing full-stem resolution
and validation path, including deprecated original_audio when no reserved full
stem exists, so REST and WS expose the same full-mix URL; add a fixture covering
a pack that requires this fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d26464b-2e03-45e8-91eb-8a66517ccce4
📒 Files selected for processing (3)
lib/routers/song.pylib/sloppak.pytests/test_song_info_stems.py
| # 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, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve legacy full-mix parity.
load_song() falls back to deprecated original_audio when no reserved full stem exists, but extract_meta() returns None here. Those packs therefore expose a WS full_mix_url but no REST full_mix_url. Reuse the same legacy resolution/validation path and add a parity fixture for it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/sloppak.py` around lines 1316 - 1322, The full_mix_file mapping in
extract_meta must match load_song’s legacy fallback behavior. Reuse the existing
full-stem resolution and validation path, including deprecated original_audio
when no reserved full stem exists, so REST and WS expose the same full-mix URL;
add a fixture covering a pack that requires this fallback.
…ent 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.
|
@coderabbitai the legacy full-mix finding is addressed in You were right: Worse than a wrong field: the stem signature would still match, so the plugin would preload a graph without the pristine mix and never rebuild — unity playback silently downgrading 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 carrying a The fix is structural rather than another special case: Tests rewritten to compare against |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git log --oneline -5 2>/dev/null
git show 98a2702 --stat 2>/dev/null
git show 98a2702 -- lib/routers/song.py 2>/dev/null | head -150Length of output: 8743
Good catch on your end tracing the blast radius too — a matching stem signature with a silently-missing pristine mix is a nasty failure mode precisely because nothing would look wrong until someone A/B'd the audio. Delegating to (๑˃̵ᴗ˂̵)و good fix |
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 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:
That is the "the video pauses when the author appears" report — the venue video's texture simply isn't drawn for 0.7s.
GET /api/song/{f}?stems=1now returns the same list ([{id, url, default}]+full_mix_url), so the plugin can start the whole load atsong:loading, before the highway and the venue are drawn, where a stalled frame costs nothing. Nothing about the work changes; only when.Opt-in via the query param, so the library's metadata calls — the hot path — pay nothing. Deliberately not cached: the metadata cache is a fixed-column table, and widening it would mean a schema migration plus a stale row for every already-scanned song, to cache what 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
defaultthrough one shared helper (stem_default_on, extracted fromload_song), and the test rebuilds the WS's payload fromload_songand requires the REST helper to produce the identical list — pinned against each other, not against a snapshot.Also pinned: the mixdown is lifted out of the stem list (spec §5.3 — listing
fullbeside the instruments would play the whole song on top of the stems) while staying reachable asfull_mix_url; a single-fullpack keeps it as its only playable stem; an unreadable pack yields an empty list rather than failing the request.Full suite 2608 passed. Consumed by feedBack-plugin-stems.
Summary by CodeRabbit
stemsparameter toGET /api/song/{filename}to return playable stem details (id,url,default) along withfull_mix_urlwhen available, matching the existing WebSocket “ready” payload behavior.defaultvalues (including common off variants) for consistent playable-stem metadata.full_mix_url: Noneinstead of failing.