From 1cd6f2dd65587c35c00a3b5946930f070bfd1905 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sun, 19 Jul 2026 01:19:59 -0500 Subject: [PATCH] fix(gp8): AssetId is a key into , not a filename stem (#1007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(gp8): AssetId is a key into , not a filename stem GPIF declares the backing track's audio as: 0 Content/Assets/.mp3 so AssetId indexes the registry, which names the exact path in the ZIP. `_resolve_audio_asset` instead compared it against each audio file's FILENAME STEM. GP8 names embedded files by hash while ids are small integers, so that match essentially never hit: every such file logged "declared AssetId not found" and fell through to "first audio asset". Silently correct while a file carries exactly ONE audio asset — but with two, a backing track declaring id 1 resolved to asset 0, i.e. the wrong recording, for both extract_sync and extract_audio. Found while verifying embedded-audio extraction for a reported GP8 import; that file logged the warning on the normal path. - `_asset_path_from_registry()` reads , normalising separators (a writer may emit backslashes). It never decides a path exists — the caller verifies membership in the archive, since the value comes out of the file and a stale entry must fall through rather than resolve to nothing. - Resolution is now a ladder: registry → legacy stem match → first audio asset. Steps 2 and 3 are the previous behaviour, kept so existing files and odd shapes are unaffected. Same-stem OGG preference is preserved on the registry path too, so quality behaviour is unchanged. Tests: registry resolution on the real-world shape (integer id, hashed filename), the second asset finally being reachable (the actual bug), a registry entry pointing at a missing file falling through, backslash normalisation, OGG preference among same-stem duplicates, malformed and absent registries degrading, and the legacy stem match still working. Suite 1725 passed vs 1720 on main, same 99 pre-existing env failures. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01929LgKdJMyPGLf8N1WpEVW Signed-off-by: ChrisBeWithYou * docs(changelog): record the GP8 AssetId resolution fix Every other change in this release notes itself; this one shipped without an entry, and the GP import path has had three fixes in two days — the history is worth being able to read later. Signed-off-by: ChrisBeWithYou --------- Signed-off-by: ChrisBeWithYou Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 11 +++++ lib/gp8_audio_sync.py | 74 +++++++++++++++++++++++++--- tests/test_gp_audio_sync.py | 97 +++++++++++++++++++++++++++++++++++-- 3 files changed, 173 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e84c53..637c7cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 engine (`app.js`, `highway.js`, `playSong`, `showScreen`, the capability registry). ### Fixed +- **Guitar Pro 8: the right backing track is extracted when a file carries more than one.** + `BackingTrack/AssetId` is a key into the GPIF's `` registry — `` names the exact path inside the archive — but it + was being matched against embedded *filename stems*. GP8 names embedded audio + by hash while ids are small integers, so that match essentially never hit: every + such file warned and fell through to "first audio asset". That was silently + correct while a file carried exactly one recording — with two, a backing track + declaring id 1 resolved to asset 0, i.e. the wrong take. Resolution now reads + the registry first (verifying the path is really in the archive, so a stale + entry falls through rather than resolving to nothing), then the legacy stem + match, then the first asset. - **Guitar Pro import no longer fails on non-ASCII song metadata (Windows).** The GP→arrangement-XML writers wrote their output with `Path.write_text()` and no explicit encoding, so on Windows (cp1252 default) a metadata diff --git a/lib/gp8_audio_sync.py b/lib/gp8_audio_sync.py index f39c1f3..5a9aeb1 100644 --- a/lib/gp8_audio_sync.py +++ b/lib/gp8_audio_sync.py @@ -72,15 +72,59 @@ def _parse_gpif(data: bytes): return ET.fromstring(data) +def _asset_path_from_registry(root, asset_id: str) -> str | None: + """The ZIP path an ```` declares, or None. + + GPIF shape:: + + + + Content/Assets/<hash>.mp3 + + Separators are normalised (a writer may emit backslashes) and the + result is returned as-is for the caller to verify against the + archive — this function never decides that a path exists. + """ + if root is None or not asset_id: + return None + try: + for asset in root.iter('Asset'): + if (asset.get('id') or '').strip() != asset_id: + continue + node = asset.find('EmbeddedFilePath') + path = (node.text or '').strip() if node is not None else '' + if not path: + return None + return path.replace('\\', '/').lstrip('./') + except Exception: + # A malformed registry is not fatal — the caller has two more + # resolution steps behind this one. + return None + return None + + def _resolve_audio_asset(zf, root=None) -> tuple[str, str | None]: """Resolve the embedded backing-track audio asset inside a .gp ZIP. - Matches ``BackingTrack/AssetId`` against the audio files under - ``Content/Assets/`` (OGG, MP3, M4A, …) and falls back to the first - audio asset when the declared id is missing or unmatched. Returns - ``(asset_stem, audio_zip_path)``, or ``('', None)`` when the archive - has no audio asset. Shared by ``extract_sync`` and ``extract_audio`` - so the matching logic can't drift between them. + ``BackingTrack/AssetId`` is a key into the GPIF's ```` + registry — ```` names the exact path + inside the ZIP — NOT a filename stem. Resolution order: + + 1. the registry entry for the declared id (authoritative); + 2. a filename-stem match (files whose stem IS the id); + 3. the archive's first audio asset. + + Step 2 was previously the only lookup, which mattered because GP8 + names embedded files by hash while ids are small integers, so the + stem match essentially never hit: every such file logged a warning + and fell through to step 3. That was silently correct only because a + file almost always carries exactly ONE audio asset — with two, a + backing track declaring id 1 resolved to asset 0, i.e. the wrong + recording. + + Returns ``(asset_stem, audio_zip_path)``, or ``('', None)`` when the + archive has no audio asset. Shared by ``extract_sync`` and + ``extract_audio`` so the matching logic can't drift between them. """ audio_files = [ n for n in zf.namelist() @@ -115,6 +159,24 @@ def _resolve_audio_asset(zf, root=None) -> tuple[str, str | None]: declared = (aid.text or '').strip() if aid is not None else '' if declared: + # 1. The registry is authoritative: it maps the id to the + # embedded path directly. Membership in the archive is verified + # rather than trusted — the path comes out of the file, and a + # stale/edited entry must fall through, not resolve to nothing. + registry_path = _asset_path_from_registry(root, declared) + if registry_path: + same_stem = [ + n for n in audio_files + if Path(n).stem == Path(registry_path).stem + ] + if same_stem: + return Path(registry_path).stem, _prefer_ogg(same_stem) + _log.warning( + 'gp8_audio_sync: AssetId %r maps to %r, which is not an audio ' + 'asset in the archive; falling back', + declared, registry_path, + ) + # 2. Legacy shape: files whose stem IS the declared id. matched = [n for n in audio_files if Path(n).stem == declared] if matched: return declared, _prefer_ogg(matched) diff --git a/tests/test_gp_audio_sync.py b/tests/test_gp_audio_sync.py index d1d09a1..20c1c01 100644 --- a/tests/test_gp_audio_sync.py +++ b/tests/test_gp_audio_sync.py @@ -38,18 +38,25 @@ from gp_autosync import ( # ── helpers ─────────────────────────────────────────────────────────────────── -def _gpif_bytes(asset_id: str = "abc-123") -> bytes: +def _gpif_bytes(asset_id: str = "abc-123", registry=None) -> bytes: + """`registry` maps Asset id -> EmbeddedFilePath, mirroring real GP8 files.""" root = ET.Element("GPIF") bt = ET.SubElement(root, "BackingTrack") ET.SubElement(bt, "AssetId").text = asset_id + if registry: + assets = ET.SubElement(root, "Assets") + for aid, path in registry.items(): + a = ET.SubElement(assets, "Asset") + a.set("id", aid) + ET.SubElement(a, "EmbeddedFilePath").text = path return ET.tostring(root) def _make_gp_zip(asset_id="abc-123", ogg_stems=("abc-123",), - asset_ext=".ogg") -> zipfile.ZipFile: + asset_ext=".ogg", registry=None) -> zipfile.ZipFile: buf = io.BytesIO() with zipfile.ZipFile(buf, "w") as zf: - zf.writestr("Content/score.gpif", _gpif_bytes(asset_id)) + zf.writestr("Content/score.gpif", _gpif_bytes(asset_id, registry)) for stem in ogg_stems: zf.writestr(f"Content/Assets/{stem}{asset_ext}", b"fake-audio") buf.seek(0) @@ -284,3 +291,87 @@ def test_extract_sync_points_empty_when_no_bars(): root = ET.Element("GPIF") # no MasterBars _, wp, times, sr, hop = _identity_setup() assert _extract_sync_points(wp, root, times, times, sr, hop, 4) == [] + + +# ── AssetId is a key into , not a filename stem ────────────────────── +# Real GP8 files name embedded audio by hash while AssetId is a small +# integer, so the stem match never hit: every such file warned and fell +# through to "first audio asset". Silently correct with ONE asset; with two, +# a backing track declaring id 1 resolved to asset 0 — the wrong recording. + +_REAL_SHAPE = {"0": "Content/Assets/1312f2aa-10ee-5f35-a4d5-e999eee1d9d0.mp3"} + + +def test_asset_id_resolves_through_the_registry_not_the_stem(): + zf = _make_gp_zip( + asset_id="0", + ogg_stems=("1312f2aa-10ee-5f35-a4d5-e999eee1d9d0",), + asset_ext=".mp3", + registry=_REAL_SHAPE, + ) + stem, path = _resolve_audio_asset(zf) + assert path == "Content/Assets/1312f2aa-10ee-5f35-a4d5-e999eee1d9d0.mp3" + assert stem == "1312f2aa-10ee-5f35-a4d5-e999eee1d9d0" + + +def test_the_second_asset_is_reachable(): + """The actual bug: id 1 used to resolve to asset 0.""" + zf = _make_gp_zip( + asset_id="1", + ogg_stems=("first-track", "second-track"), + registry={ + "0": "Content/Assets/first-track.ogg", + "1": "Content/Assets/second-track.ogg", + }, + ) + stem, path = _resolve_audio_asset(zf) + assert path == "Content/Assets/second-track.ogg", "declared id 1 must win" + assert stem == "second-track" + + +def test_registry_entry_pointing_at_a_missing_file_falls_through(): + zf = _make_gp_zip( + asset_id="0", + ogg_stems=("real-track",), + registry={"0": "Content/Assets/deleted-track.ogg"}, + ) + stem, path = _resolve_audio_asset(zf) + assert path == "Content/Assets/real-track.ogg" + + +def test_backslash_separators_in_the_registry_are_normalised(): + zf = _make_gp_zip( + asset_id="0", + ogg_stems=("winpath",), + registry={"0": r"Content\Assets\winpath.ogg"}, + ) + _, path = _resolve_audio_asset(zf) + assert path == "Content/Assets/winpath.ogg" + + +def test_registry_prefers_ogg_among_same_stem_duplicates(): + """Quality behaviour is preserved: OGG is copied out, others transcoded.""" + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr("Content/score.gpif", _gpif_bytes( + "0", {"0": "Content/Assets/dual.mp3"})) + zf.writestr("Content/Assets/dual.mp3", b"fake") + zf.writestr("Content/Assets/dual.ogg", b"fake") + buf.seek(0) + _, path = _resolve_audio_asset(zipfile.ZipFile(buf)) + assert path.endswith(".ogg") + + +def test_a_malformed_registry_does_not_break_resolution(): + for reg in ({"0": ""}, {"9": "Content/Assets/other.ogg"}, {}): + zf = _make_gp_zip(asset_id="0", ogg_stems=("fallback",), registry=reg) + _, path = _resolve_audio_asset(zf) + assert path == "Content/Assets/fallback.ogg" + + +def test_legacy_stem_match_still_works_without_a_registry(): + """Files whose stem IS the id keep resolving — step 2 of the ladder.""" + zf = _make_gp_zip(asset_id="abc-123", ogg_stems=("zzz", "abc-123")) + stem, path = _resolve_audio_asset(zf) + assert stem == "abc-123" + assert path == "Content/Assets/abc-123.ogg"