From 329cc86315dafd540aa16c09943e3200f51722dc Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Mon, 13 Jul 2026 18:22:42 +0200 Subject: [PATCH] =?UTF-8?q?fix(sloppak):=20the=20full=20mix=20is=20a=20ste?= =?UTF-8?q?m=20=E2=80=94=20drop=20the=20invented=20`original=5Faudio`=20ke?= =?UTF-8?q?y=20(#946)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sloppak): the full mix is a stem — drop the invented `original_audio` key (#933) Core read, served, and depended on `original_audio:` — a top-level manifest key this repo invented in #583 that the feedpak spec never defined. The format already had a home for the pre-separation mixdown: it is a stem. feedpak 1.15.0 (feedpak-spec#53) RESERVES the id `full` for it, so read it from there. The key existed to work around a bug in our own reader. The packer's comment said so plainly: "we must NOT list the full mix as a playable stem — the player sums every entry in `stems` and does not gate playback on `default`, so a listed full mix plays on top of the stems". Faced with a reader that would double the song, the packer put the mixdown outside `stems` and invented a key to point at it. The fix belongs in the reader, and that is what this is. load_song() now partitions the stem list: `full` comes out as LoadedSloppak.full_mix, the instruments stay in .stems. Nothing that sums stems or draws one fader per stem can see the mixdown, so retaining it is safe — which is what lets the packer put it where the format says it goes. - ws_highway: `song_info` gains full_mix_url / has_full_mix. The old original_audio_url / has_original_audio remain as deprecated aliases for one release so an older stems plugin keeps working (#945). - `stems` on the wire, and stem_ids / stem_count in the library index, are now INSTRUMENT stems only — a separated pack that retains its mixdown no longer advertises a bogus "full" chip or an inflated stem count. - enrichment: fingerprint against the mixdown wherever it lives. This widens coverage — _song_audio_file() previously returned None for any pack without the invented key, so fingerprinting silently did nothing for nearly every pack. - sloppak: `original_audio:` is still READ as a deprecated fallback, because every pack in the wild carries it and would otherwise lose its pristine mix. tools/migrate_full_mix_stem.py rewrites those packs into the spec shape (original/full.ogg -> stems/full.ogg, add the `full` stem at default:off, drop the key); the fallback and the aliases die with #945. The spec gate keeps the debt honest: the grandfather entry now tracks #945, and the gate fails if it goes stale. Verified: spec gate OK (4/4, incl. ingesting the spec's new example pack that retains `full`); 2493 python tests, 995 js tests; migrator round-tripped over real packs from the library and the results pass the spec's reference validator. * fix(migrate): discover directory-form packs instead of silently skipping them iter_packs() searched only files, so a directory-form pack (`song.sloppak/`, the authoring shape) was walked INTO and never yielded — silently missed by a run that's meant to be exhaustive. Discover suffix-named directories too (yielded whole, not descended into), and route packs through migrate_pack/verify_pack. Directory packs are REPORTED as `dir-form-unsupported`, not rewritten in place: a single-file pack is replaced atomically (a fully-built temp archive swapped in with one os.replace), but a populated directory can't be swapped that way, so an interrupted in-place rewrite could leave an authoring pack half-migrated. The status is a problem status, so it counts against the run's exit code and shows in the summary — the operator re-packs or migrates it as a `.feedpak` instead of it vanishing from the report. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson * fix(migrate): verify requires an explicit `off` on a retained full mix verify_zip accepted any non-truthy `default` on a multi-stem `full` (missing, empty, boolean, `false`/`no`/`0`, malformed) as "ok". But core defaults an ABSENT `default` to True — ON (lib/sloppak.py: `s.get("default", True)`) — and treats an empty/unrecognized string as ON too, so a migrated-shape pack whose `full` stem has a missing or blank default beside instrument stems would actually play the mixdown on open and double the song. verify was certifying that as safe. Require an explicit normalized `off` beside instrument stems: `on`-ish values are reported `full-stem-default-on` (actively plays), everything that is not a normalized `off` is reported `full-stem-default-not-off`. The migrator already writes the literal `off`, so its own output is unaffected; this also certifies the pack is in the tool's canonical, most-portable shape. The len>1 gate is kept, so a sole `full` stem (which IS the audio) is not policed. Adds parametrized coverage for missing / empty / boolean / off-ish / malformed defaults, and a sole-full-stem case. Addresses a CodeRabbit review finding. Signed-off-by: Kris Anderson --------- Signed-off-by: Kris Anderson Co-authored-by: Kris Anderson --- CHANGELOG.md | 34 ++ feedpak-spec-exceptions.yml | 24 +- lib/enrichment.py | 25 +- lib/routers/ws_highway.py | 62 ++- lib/sloppak.py | 186 +++++++-- static/highway.js | 24 +- tests/test_migrate_full_mix_stem.py | 429 +++++++++++++++++++++ tests/test_sloppak_full_mix_load.py | 277 ++++++++++++++ tests/test_sloppak_original_audio_load.py | 119 ------ tools/migrate_full_mix_stem.py | 445 ++++++++++++++++++++++ 10 files changed, 1430 insertions(+), 195 deletions(-) create mode 100644 tests/test_migrate_full_mix_stem.py create mode 100644 tests/test_sloppak_full_mix_load.py delete mode 100644 tests/test_sloppak_original_audio_load.py create mode 100644 tools/migrate_full_mix_stem.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 180e8c7..1d80678 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **The full mix is a stem** (#933) — core no longer depends on `original_audio:`, a + top-level manifest key this repo invented (#583) that the feedpak spec never had. + The format already carried the pre-separation mixdown as a stem; feedpak 1.15.0 + (feedpak-spec#53) reserves the id **`full`** for it, so that is where core reads it + from now. + + `full` is a mixdown, not a layer — it already contains every instrument — so + `load_song()` lifts it OUT of `LoadedSloppak.stems` onto `LoadedSloppak.full_mix`. + Nothing that sums stems or renders one fader per stem can see it, which is what + makes retaining it safe; leaving it in the list would double the whole song and + leave "guitar" audible with the guitar fader muted. That trap is exactly why the + packer invented the key instead of putting the mixdown where the format says it + goes — the bug was in the reader, and this fixes the reader. + + Consequences worth knowing: + - The highway WS `song_info` frame gains `full_mix_url` / `has_full_mix`. + `original_audio_url` / `has_original_audio` remain as **deprecated aliases** + (same values) for one release so a client built against the old frame keeps + working; they go with the fallback below (#945). + - `stems` on `song_info`, and `stem_ids` / `stem_count` in the library index, now + describe *instrument* stems only — a separated pack that retains its mixdown no + longer advertises a bogus "full" stem chip or an inflated stem count. + - Audio fingerprinting (`lib/enrichment.py`) now resolves the mixdown the same + way, which **widens** its coverage: it previously returned `None` for any pack + without the invented key, so fingerprinting silently did nothing for the + overwhelming majority of packs. + - Core still **reads** `original_audio:` as a deprecated fallback, because every + pack written before the spec caught up carries it and would otherwise lose its + pristine mix. `tools/migrate_full_mix_stem.py` rewrites those packs into the + spec shape (moves `original/full.ogg` → `stems/full.ogg`, adds the `full` stem + at `default: off`, drops the key); the fallback and the aliases are removed once + they are migrated (#945). + ### Added - **Genres fall back to MusicBrainz enrichment** — the effective genre now resolves override → pack genre → the enrichment match's primary genre diff --git a/feedpak-spec-exceptions.yml b/feedpak-spec-exceptions.yml index aa6f4d1..34b1c40 100644 --- a/feedpak-spec-exceptions.yml +++ b/feedpak-spec-exceptions.yml @@ -34,17 +34,23 @@ exceptions: - key: original_audio - issue: https://github.com/got-feedback/feedback/issues/933 + issue: https://github.com/got-feedback/feedback/issues/945 reason: >- Added by #583 (the full mix played while every stem fader sits at unity, - since demucs recombination is lossy). Core, lib/enrichment.py, and the - stems plugin all depend on it, but it never went through a FEP and the + since demucs recombination is lossy). It never went through a FEP and the spec does not define it — the drift this gate exists to prevent. - The resolution is REMOVAL, not a FEP: the spec already carries the mixdown - as a stem ({id: full, file: stems/full.ogg}), so this key added a second, - redundant location for audio to a format that already had one. See #933. + #933 fixed the drift: feedpak 1.15.0 RESERVES the stem id `full` for the + complete mixdown (feedpak-spec#53), and core now reads the full mix from + that stem. Nothing depends on this key any more — not the loader, not + lib/enrichment.py, not the stems plugin, and the packer no longer writes it. - Grandfathered so the gate can land green and start blocking the *next* - instance immediately, rather than blocking on #933. This entry goes away - when core no longer reads or writes the key. + What remains is a READ-ONLY deprecated fallback in lib/sloppak.py + (_legacy_full_mix), kept for one release because every pack produced before + the spec caught up carries `original_audio: original/full.ogg` and would + otherwise silently lose its pristine mix. tools/migrate_full_mix_stem.py + rewrites those packs into the spec shape. + + This entry disappears with that fallback — tracked by #945, which cannot be + forgotten: the gate fails if the entry goes stale, and deleting the read is + what makes it stale. diff --git a/lib/enrichment.py b/lib/enrichment.py index fc78691..6f762a0 100644 --- a/lib/enrichment.py +++ b/lib/enrichment.py @@ -368,10 +368,12 @@ def _acoustid_gate() -> "JSONResponse | None": def _song_audio_file(filename: str) -> "str | None": """Resolve a LIBRARY song (by filename/id) to a local master-audio file for - fingerprinting: the full-mix `original_audio` extracted from a sloppak, or a - loose folder's audio. None when the song can't be found or ships no full-mix - audio (some packs carry only stems). Mirrors serve_sloppak_file's containment - guards so a crafted filename can't read outside DLC_DIR / the pack.""" + fingerprinting: a sloppak's complete mixdown, or a loose folder's audio. None + when the song can't be found or carries no mixdown (a pack that kept only its + separated stems — an acoustic fingerprint of one re-summed from them would not + match the recording, so we decline rather than submit a lossy reconstruction). + Mirrors serve_sloppak_file's containment guards so a crafted filename can't + read outside DLC_DIR / the pack.""" dlc = _get_dlc_dir() if not dlc: return None @@ -383,7 +385,20 @@ def _song_audio_file(filename: str) -> "str | None": canon = resolved.relative_to(dlc.resolve()).as_posix() except ValueError: return None - rel = (sloppak_mod.load_manifest(resolved) or {}).get("original_audio") + manifest = sloppak_mod.load_manifest(resolved) or {} + # The mixdown is the RESERVED `full` stem (spec §5.3). Unlike playback, + # fingerprinting wants it even when it is the pack's ONLY stem — a + # single-mix pack is exactly the master audio we want to fingerprint — + # so this asks find_full_mix() rather than partition_stems(). + stems = manifest.get("stems") or [] + full = sloppak_mod.find_full_mix( + [s for s in stems if isinstance(s, dict)] + ) + rel = full.get("file") if full else None + # DEPRECATED fallback: packs written before the spec reserved `full` put + # the mixdown behind a top-level `original_audio:` key instead (#933). + if not isinstance(rel, str) or not rel.strip(): + rel = manifest.get("original_audio") if not isinstance(rel, str) or not rel.strip(): return None src = sloppak_mod.get_cached_source_dir(canon) diff --git a/lib/routers/ws_highway.py b/lib/routers/ws_highway.py index 9466f82..62cd152 100644 --- a/lib/routers/ws_highway.py +++ b/lib/routers/ws_highway.py @@ -321,11 +321,16 @@ async def highway_ws(websocket: WebSocket, filename: str, arrangement: int = -1, audio_url = None audio_error: str | None = None # Surfaced in song_info when audio_url is None stems_payload: list[dict] = [] - # URL of the single full-mix audio (sloppak `original_audio:`), when the - # pack ships one. The stems plugin uses this to play the untouched mix - # while every stem slider is at unity; None otherwise (separate stems - # only, loose folder, or archive). - original_audio_url: str | None = None + # URL of the pack's complete mixdown — the RESERVED `full` stem (spec + # §5.3), which sloppak.load_song() lifts out of `stems` because it is a + # mixdown, not a layer. The stems plugin plays it while every stem slider + # is at unity (separation is lossy, so it beats re-summing the stems) and + # crosses to the separated stems as soon as one is attenuated. + # + # None when the pack has no mixdown to offer separately from its stems: + # a single-mix pack (its one stem IS the mixdown), a loose folder, or an + # archive. + full_mix_url: str | None = None if is_loose: # Loose folder filenames are relative paths (artist/album/song). # Hash the *canonical* dlc-relative path (so two URL spellings @@ -365,21 +370,25 @@ async def highway_ws(websocket: WebSocket, filename: str, arrangement: int = -1, url = f"/api/sloppak/{q_fn}/file/{quote(s['file'])}" stems_payload.append({"id": s["id"], "url": url, "default": s["default"]}) # Full-mix URL (served by the same /api/sloppak/.../file/ endpoint). - if loaded_slop is not None and loaded_slop.original_audio: - original_audio_url = ( - f"/api/sloppak/{q_fn}/file/{quote(loaded_slop.original_audio)}" + if loaded_slop is not None and loaded_slop.full_mix: + full_mix_url = ( + f"/api/sloppak/{q_fn}/file/{quote(loaded_slop.full_mix)}" ) if stems_payload: # Stems present: keep the core