From 2c1c6f7eac7531993a3432605b18082da7a9c8a2 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Sun, 5 Jul 2026 00:15:29 +0200 Subject: [PATCH 1/8] fix(starter): sync _BUILTIN_STARTER_SOURCES with content/starter on disk (#775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two commits (delete beethoven-ode_to_joy, re-add The Adicts' Ode to Joy) never updated _BUILTIN_STARTER_SOURCES: it still listed the deleted pack and omitted the added one. The listed-but-missing file made the all-present gate never fire, so NO starter content seeded on first run — and the on-disk-but-unlisted pack would bundle as dead weight. Both starter-seed guard tests were red on main, reddening ci/test on every core PR. Sync the manifest to disk. Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + server.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f20cda0..4b44814 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Player frame-time hotspots removed (trace-backed) + weak-hardware hardening.** A Chrome performance trace of a 3D-highway session surfaced two core per-frame layout-thrash sources, now fixed: the highway's visibility check read `canvas.offsetParent` every rAF frame (forces style/layout recalc — now sampled every 10th frame with a cached value, force-refreshed on init/canvas-replace/resize/override-clear), and the v3 player chrome loop called `matches(':hover')` per frame and unconditionally rewrote the Up-Next pill's `textContent`/bar width at 6 Hz (now hover-tracked via mouseenter/mouseleave, DOM writes only on value change, progress bar moved from `width` to compositor-only `scaleX`). The 3D highway pre-warms shader programs (`ren.compile`) and deterministic label textures at init — and chart-dependent chord/section label textures on first draw — so first-appearance shader-compile/texture-upload frame spikes move into the load spinner. For weaker hardware: the per-frame renderer bundle is now a single reused object instead of a fresh ~35-field allocation per frame (object identity is stable and meaningless; array fields still swap reference on chart changes), custom viz get `bundle.lowerBoundT`/`bundle.lowerBoundTime` binary-search helpers for visible-window culling, the default 2D highway's beat lines no longer scan every beat in the song per frame, and the 3D highway stops reading `localStorage` per frame (1 Hz poll) and caches its lyrics text-measurement layout per displayed line instead of re-measuring every syllable every frame. A second, throttled-CPU trace pass additionally removed: shader-program re-resolution churn from label texture swaps (`material.needsUpdate` is now only set on a null↔texture transition — swapping between two cached label textures never changes the compiled program), the 3D highway's per-frame `getBoundingClientRect` layout read in its canvas-size self-check (now every 10th frame, still immediate on backing-store change), and the core 60 Hz HUD clock rewriting `textContent` on every tick (now write-on-change, ~1/s). The dominant residual — steady `getParameters` shader-program re-resolution (~4% of throttled main thread) — turned out to be Three r158+'s transparent-DoubleSide two-pass rendering, which sets `material.needsUpdate` twice per object per frame; all 18 of the 3D highway's transparent DoubleSide materials are flat unlit quads (labels, rails, chord frames, lanes), so they now declare `forceSinglePass: true`, eliminating the recompile churn and halving those objects' draw calls. ### Fixed +- **Starter content seeds again (and now ships The Adicts' "Ode to Joy").** `_BUILTIN_STARTER_SOURCES` still listed `beethoven-ode_to_joy.feedpak` after that pack was deleted, and never wired up its replacement `the_adicts-ode-to-joy_vst_cover.feedpak` that landed on disk. The listed-but-missing file made the all-present gate never fire, so **no** starter songs seeded on first run. Synced the manifest to what's on disk (Für Elise, Star Spangled Banner, The Adicts' Ode to Joy). Tests: `tests/test_builtin_starter_seed.py` (the present/unlisted guards were red on `main`). - **Edit Metadata now writes into `.feedpak` files, not just legacy `.sloppak` ones.** `lib/songmeta.py`'s suffix gate predated the format rename — core reads both suffixes everywhere else (`sloppak.SONG_EXTS`), but the metadata writer only dispatched on `.sloppak`, so editing a zip-form `.feedpak`'s title/artist/album/year silently fell back to a DB-only update. That looked fine until the next **full library rescan** re-derived metadata from the file and reverted the edit (directory-form packages were unaffected — they dispatch on manifest presence, not suffix). The gate now accepts both package suffixes. Tests: `tests/test_songmeta.py` `TestWriteSongMetadata` (both zip suffixes, mixed-case suffix, directory form, unknown-suffix fallback). - **3D Drum & Keys highways now re-frame on fullscreen/layout drift under splitscreen.** The guitar/bass `highway_3d` self-detects when its panel canvas changes size and re-runs `applySize()` every frame, because the splitscreen host overrides `hw.resize` and never calls `renderer.resize()`. The drum and keys highways lacked that fallback — they only re-framed when the host explicitly called `resize(w, h)` — so their panels stayed framed for the pre-fullscreen size while the guitar/bass panels adapted (visible as a too-small, off-center highway after maximizing a split-screen session). Both draw loops now port `highway_3d`'s per-frame drift check: they re-apply on backing-store change (`canvas.width/height`) AND on CSS-box drift (`clientWidth/clientHeight` vs the last applied logical size, throttled to every 10th frame), and reset the tracking in `destroy()` so a reused instance re-frames on the next song. `plugins/drum_highway_3d` → 0.3.1, `plugins/keys_highway_3d` → 0.1.1. Tests: `tests/js/drum_keys_highway_3d_resize_reframe.test.js`. - **Tuner: finished the "remove unused settings" cleanup and fixed the sidebar panel position.** The Floating Button and Tuning Visibility settings sections were removed, but their config was still live: `disabledTunings` still filtered the tuner menu (with no UI left to re-enable a hidden tuning — a one-way trap) and `showFloatingButton` still gated the floating launcher. Both are now fully retired — the enforcement paths in `plugins/tuner/screen.js`/`utils/ui.js` and the persistence in `plugins/tuner/routes.py` are gone (and `routes.py` strips the retired keys on write, so stale values are purged). The tuner panel opened from the v3 sidebar Plugins rail popover now anchors beside it via the host's stable plugin-control slot API (falling back to the popover id), is **clamped to the viewport** so it can't open off the right/bottom edge on narrow/short windows, and re-anchors on window resize. `plugins/tuner` → 1.3.3. diff --git a/server.py b/server.py index 94f282f..5a1513a 100644 --- a/server.py +++ b/server.py @@ -5822,8 +5822,8 @@ _BUILTIN_STARTER_SOURCES: list[tuple[str, str]] = [ "content/starter/star_spangled_banner.feedpak", ), ( - "beethoven-ode_to_joy.feedpak", - "content/starter/beethoven-ode_to_joy.feedpak", + "the_adicts-ode-to-joy_vst_cover.feedpak", + "content/starter/the_adicts-ode-to-joy_vst_cover.feedpak", ), ] _STARTER_SEED_MARKER = ".starter-content-seeded" From 41e907fa52e846eb4121760b18722b67c2491987 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:15:50 -0500 Subject: [PATCH 2/8] fix(library): serve art/load for songs mounted through a library junction (#766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(library): serve art/load songs mounted through a library junction A song library mounted through a directory JUNCTION/symlink subfolder (a library shared across app installs; the desktop app's own mounts) had broken album art and couldn't load: the scanner's rglob follows the junction and indexes the songs, but _resolve_dlc_path (via safe_join's .resolve()) followed the junction to its real target, saw it outside DLC_DIR, and rejected every song reached through it → 403 on /art, 404 on /art/candidates, broken covers. - _resolve_dlc_path now uses LEXICAL containment (os.path.normpath, no symlink following) so an in-library junction is allowed, while `..` traversal and absolute paths are still rejected (the traversal tests pin this). - safe_join is left STRICT (.resolve()-based) — it is the zip-slip / plugin- asset / avatar guard, where following a symlink out IS the defense — but gains an explicit NUL guard (on Python 3.13/Windows resolve() no longer raises on an embedded NUL, so the byte was leaking through; strictly-more- rejection, no effect on the zip-slip contract). Tests: test_dlc_junction (junction allowed; `..`/absolute/NUL rejected; the safe_join-stays-strict contrast). Existing traversal/safepath/art-candidates suites stay green. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(library): reject Windows drive-letter paths in _resolve_dlc_path The new test_absolute_path_rejected pins 'C:/Windows/system32/x' → None, but on POSIX a drive-letter path isn't absolute, so Path(dlc)/'C:/…' becomes the contained relative dir '/C:/…' and slipped through the lexical containment check (red on the Linux CI). Not an escape, but the traversal contract should hold cross-platform (a shared library is reached from either OS). Reject a path that is absolute or drive-qualified in either POSIX or Windows semantics before the containment check. Legitimate relative/junction paths are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- lib/safepath.py | 7 +++ server.py | 48 ++++++++++++++++++-- tests/test_dlc_junction.py | 92 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 tests/test_dlc_junction.py diff --git a/lib/safepath.py b/lib/safepath.py index 1eebbdc..4442e11 100644 --- a/lib/safepath.py +++ b/lib/safepath.py @@ -26,6 +26,13 @@ def safe_join(root: Path, name: str) -> Path | None: """ if not name: return None + # Reject embedded NULs explicitly. This used to ride on `.resolve()` + # raising ValueError, but on Python 3.13 (Windows) resolve() no longer + # raises for an embedded NUL, so the byte would otherwise leak through + # containment. An explicit guard is strictly-more-rejection (no effect on + # the zip-slip / traversal contract). + if "\x00" in name: + return None safe = name.replace("\\", "/") try: root_resolved = root.resolve() diff --git a/server.py b/server.py index 5a1513a..c78355c 100644 --- a/server.py +++ b/server.py @@ -5229,10 +5229,52 @@ def _resolve_dlc_path(dlc: Path, filename: str) -> Path | None: check so every filename-bound handler validates before touching the filesystem. - Returns the validated resolved Path, or None if the path is empty - or escapes the DLC root. + Containment here is LEXICAL (normalize `.`/`..` WITHOUT following + symlinks), not `safe_join`'s `.resolve()`-based check — because users + commonly mount their song library through a directory JUNCTION/symlink + (a library shared across app installs; the desktop app's own mounts). + `.resolve()` follows that junction to its real target, sees it sits + outside DLC_DIR, and wrongly rejects every song reached through it — the + scanner's `rglob` indexes those songs, but art/load then 403/404s (broken + covers, unplayable songs). Lexical normalization still rejects the only + escapes a `:path` filename can express — `..` traversal and absolute + paths — which the traversal tests pin. `safe_join` stays strict (it is + the zip-slip / plugin-asset guard, where following a symlink out IS the + defense); the loose-folder art handler keeps its own per-file symlink + re-check for defence-in-depth. + + Returns the validated Path (not necessarily link-resolved), or None if + the filename is empty, contains a NUL, or escapes the DLC root. """ - return safe_join(dlc, filename) + if not filename: + return None + # Backslashes → forward slashes so a Windows-style `..\\x` traversal is + # rejected identically on POSIX (mirrors safe_join's normalisation). + safe = filename.replace("\\", "/") + if "\x00" in safe: + return None + # Reject drive-letter / absolute paths in BOTH conventions. A POSIX "/x" is + # caught by the containment check below (the `/` operator discards `root`), + # but a Windows drive-absolute "C:/x" is treated as a relative "C:" dir on + # POSIX and would otherwise slip in as `/C:/x` — so the contract must + # hold cross-platform (a shared library is reached from either OS). + from pathlib import PurePosixPath, PureWindowsPath + if (PurePosixPath(safe).is_absolute() + or PureWindowsPath(safe).is_absolute() + or PureWindowsPath(safe).drive): + return None + try: + root = dlc.resolve() + # normpath collapses `.`/`..`/duplicate separators purely lexically — + # it never touches the filesystem, so an in-library junction component + # is preserved (allowed) while `..`/absolute segments still escape and + # get caught by the containment check below. + candidate = Path(os.path.normpath(root / safe)) + if not candidate.is_relative_to(root): + return None + except (ValueError, OSError): + return None + return candidate _SMART_TYPE_BASE: dict[str, int] = {"Lead": 0, "Rhythm": 10, "Bass": 20} diff --git a/tests/test_dlc_junction.py b/tests/test_dlc_junction.py new file mode 100644 index 0000000..5b87fa9 --- /dev/null +++ b/tests/test_dlc_junction.py @@ -0,0 +1,92 @@ +"""Unit tests for ``server._resolve_dlc_path`` — the DLC-library containment +guard. + +It must (1) allow a library mounted through a directory JUNCTION/symlink (the +shared-library-across-installs / desktop-app case that a ``.resolve()``-based +check wrongly rejected, breaking album art + song load), while (2) still +rejecting ``..`` traversal and absolute paths — the only escapes a ``:path`` +filename can express. ``safe_join`` stays strict on purpose (zip-slip guard), +so the contrast is pinned here too. +""" + +import importlib +import os +import sys + +import pytest + + +@pytest.fixture() +def server(tmp_path, monkeypatch): + (tmp_path / "cfg").mkdir() + monkeypatch.setenv("CONFIG_DIR", str(tmp_path / "cfg")) + monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") + sys.modules.pop("server", None) + srv = importlib.import_module("server") + try: + yield srv + finally: + conn = getattr(getattr(srv, "meta_db", None), "conn", None) + if conn is not None: + getattr(sys.modules.get("server"), "_join_background_db_threads", lambda: None)() + conn.close() + sys.modules.pop("server", None) + + +def _dlc(tmp_path): + d = tmp_path / "dlc" + d.mkdir() + return d + + +# ── still-rejected escapes (the security contract) ──────────────────────────── + +def test_dotdot_traversal_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "../../etc/passwd") is None + # a Windows-style backslash traversal is normalised + rejected identically + assert server._resolve_dlc_path(dlc, "..\\..\\secret") is None + assert server._resolve_dlc_path(dlc, "a/../../b") is None + + +def test_absolute_path_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "/etc/passwd") is None + assert server._resolve_dlc_path(dlc, "C:/Windows/system32/x") is None + + +def test_empty_and_nul_rejected(server, tmp_path): + dlc = _dlc(tmp_path) + assert server._resolve_dlc_path(dlc, "") is None + assert server._resolve_dlc_path(dlc, "a\x00b") is None + + +# ── allowed: legitimate in-library paths ────────────────────────────────────── + +def test_safe_relative_allowed(server, tmp_path): + dlc = _dlc(tmp_path) + p = server._resolve_dlc_path(dlc, "CDLC/City Pop/song.feedpak") + assert p is not None + assert p.is_relative_to(dlc.resolve()) + + +def test_junction_subfolder_allowed(server, tmp_path): + """A library mounted through a directory junction/symlink must resolve — + the case that broke album art for Christian's shared city-pop library.""" + dlc = _dlc(tmp_path) + real = tmp_path / "real_library" + real.mkdir() + (real / "song.feedpak").write_bytes(b"pack") + link = dlc / "CDLC" + try: + os.symlink(real, link, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlink/junction creation not permitted on this host") + + p = server._resolve_dlc_path(dlc, "CDLC/song.feedpak") + assert p is not None, "a junctioned library subfolder was wrongly rejected" + assert p.exists(), "the resolved path should reach the file through the junction" + # Contrast: safe_join stays strict (it .resolve()s and follows the junction + # to its real target outside the root), which is correct for its zip-slip + # callers but is exactly why _resolve_dlc_path can't reuse it here. + assert server.safe_join(dlc, "CDLC/song.feedpak") is None From a86abadb144e6cc510bf8aca4f95bd6ba751b1e0 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:16:42 -0500 Subject: [PATCH 3/8] settings: add host instrument profiles (#753) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * settings: add host instrument profiles Signed-off-by: ChrisBeWithYou * settings: add instrument pathway selection Signed-off-by: ChrisBeWithYou * fix(settings): profile-aware saves/resets/switch, provider tunings, bass-5 Five regressions from the instrument-profiles rework: 1. save_settings canonicalized profiles on EVERY save -> empty/unrelated POST froze default profiles into config.json (broke test_empty_post_preserves_all_existing_keys). Gate on the save touching instrument settings; GET already virtualizes profiles. 2. pathway is profile-mirrored, so the Gameplay reset (flat-key delete) was a no-op. reset_settings now resets pathway inside the persisted profiles too. 3. Per-profile tuning validation rejected provider/custom tunings (tuner plugin, /api/tunings). _valid_tuning_for_key now accepts a name unknown to every built-in table while still rejecting a built-in misapplied to the wrong key. 4. First-migration overwrote an explicit active_instrument_profile with the legacy-inferred one, so a fresh-config switch to 'bass' was lost. Use setdefault so an explicit request wins. 5. Pre-existing test_instrument_fields_persist used bass-5 + 'Drop D' (a 4-string tuning). Updated to the valid 'Drop A'. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(settings): partial-merge instrument_profiles; clamp tuning on string-count switch Two partial-update follow-ups: - save_settings normalized a POSTed instrument_profiles by FILLING every omitted profile with defaults and replacing wholesale, so a one-profile update reset the others. Validate each PROVIDED profile individually and merge the partial over the persisted set inside the lock — /api/settings is partial-merge. - the string-count picker posted only string_count, so the backend silently reset a now-invalid tuning to Standard while the UI kept the old value (settings/tuner desync). Clamp + post the valid tuning too, mirroring the instrument-switch path. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Signed-off-by: ChrisBeWithYou Co-authored-by: Claude Opus 4.8 (1M context) --- lib/tunings.py | 397 +++++++++++++++++++++++--- server.py | 92 +++++- static/app.js | 20 ++ static/capabilities/working-tuning.js | 2 +- static/v3/badges.js | 58 +++- static/v3/index.html | 17 ++ static/v3/settings.js | 2 +- tests/js/working_tuning.test.js | 9 +- tests/test_settings_api.py | 117 +++++++- tests/test_settings_instrument.py | 6 +- tests/test_tunings.py | 119 +++++++- 11 files changed, 786 insertions(+), 53 deletions(-) diff --git a/lib/tunings.py b/lib/tunings.py index ff248fa..2b9bede 100644 --- a/lib/tunings.py +++ b/lib/tunings.py @@ -4,51 +4,132 @@ Kept separate from server.py so tests can import it without triggering FastAPI / SQLite module-level side effects. """ +from __future__ import annotations + +import math + DEFAULT_REFERENCE_PITCH = 440.0 -# Canonical tuning frequencies at 440 Hz reference, keyed by instrument then -# tuning name. This is the authoritative source; tuner/routes.py previously -# held a copy — it was removed in favour of this one. -DEFAULT_TUNINGS: dict[str, dict[str, list[float]]] = { +# Canonical open strings, low to high, as MIDI notes. This is the host-level +# source of truth for guitar/bass tuning profiles; UI surfaces derive names, +# frequencies, and semitone offsets from these absolute pitches. +STANDARD_OPEN_MIDIS: dict[str, list[int]] = { + "guitar-6": [40, 45, 50, 55, 59, 64], + "guitar-7": [35, 40, 45, 50, 55, 59, 64], + "guitar-8": [30, 35, 40, 45, 50, 55, 59, 64], + "bass-4": [28, 33, 38, 43], + "bass-5": [23, 28, 33, 38, 43], + "bass-6": [23, 28, 33, 38, 43, 48], +} + +# Curated built-in profiles. This intentionally starts by absorbing the useful +# Virtuoso guitar/bass coverage into host-owned data so the host selector, +# tuner, practice tools, and plugins can converge on one profile model. +TUNING_PRESET_MIDIS: dict[str, dict[str, list[int]]] = { "guitar-6": { - "Standard": [82.41, 110.00, 146.83, 196.00, 246.94, 329.63], - "Eb Standard": [77.78, 103.83, 138.59, 185.00, 233.08, 311.13], - "Drop D": [73.42, 110.00, 146.83, 196.00, 246.94, 329.63], - "D Standard": [73.42, 98.00, 130.81, 174.61, 220.00, 293.66], - "Drop C": [65.41, 98.00, 130.81, 174.61, 220.00, 293.66], - "Open G": [73.42, 98.00, 146.83, 196.00, 246.94, 293.66], - "Open D": [73.42, 110.00, 146.83, 185.00, 220.00, 293.66], - "DADGAD": [73.42, 110.00, 146.83, 196.00, 220.00, 293.66], - "Open E": [82.41, 123.47, 164.81, 207.65, 246.94, 329.63], + "Standard": [40, 45, 50, 55, 59, 64], + "Eb Standard": [39, 44, 49, 54, 58, 63], + "D Standard": [38, 43, 48, 53, 57, 62], + "C# Standard": [37, 42, 47, 52, 56, 61], + "C Standard": [36, 41, 46, 51, 55, 60], + "Drop D": [38, 45, 50, 55, 59, 64], + "Drop C": [36, 43, 48, 53, 57, 62], + "Drop B": [35, 42, 47, 52, 56, 61], + "Drop A": [33, 40, 45, 50, 54, 59], + "Drop Ab": [32, 39, 44, 49, 53, 58], + "Open G": [38, 43, 50, 55, 59, 62], + "Open D": [38, 45, 50, 54, 57, 62], + "DADGAD": [38, 45, 50, 55, 57, 62], + "Open E": [40, 47, 52, 56, 59, 64], }, "guitar-7": { - "Standard": [61.74, 82.41, 110.00, 146.83, 196.00, 246.94, 329.63], - "Drop A": [55.00, 82.41, 110.00, 146.83, 196.00, 246.94, 329.63], - "A Standard": [55.00, 73.42, 98.00, 130.81, 174.61, 220.00, 293.66], - "Drop G": [49.00, 73.42, 110.00, 146.83, 196.00, 246.94, 329.63], - "Bb Standard": [58.27, 77.78, 103.83, 138.59, 185.00, 233.08, 311.13], + "Standard": [35, 40, 45, 50, 55, 59, 64], + "Bb Standard": [34, 39, 44, 49, 54, 58, 63], + "A Standard": [33, 38, 43, 48, 53, 57, 62], + "G Standard": [31, 36, 41, 46, 51, 55, 60], + "Drop A": [33, 40, 45, 50, 55, 59, 64], + "Drop G": [31, 38, 43, 48, 53, 57, 62], + "Drop F#": [30, 37, 42, 47, 52, 56, 61], }, "guitar-8": { - "Standard": [46.25, 61.74, 82.41, 110.00, 146.83, 196.00, 246.94, 329.63], - "Drop E": [41.20, 61.74, 82.41, 110.00, 146.83, 196.00, 246.94, 329.63], - "E Standard": [41.20, 55.00, 73.42, 98.00, 130.81, 174.61, 220.00, 293.66], - "Drop D": [36.71, 55.00, 73.42, 98.00, 130.81, 174.61, 220.00, 293.66], - "Eb Standard": [38.89, 51.91, 69.30, 92.50, 123.47, 164.81, 207.65, 277.18], + "Standard": [30, 35, 40, 45, 50, 55, 59, 64], + "Drop E": [28, 35, 40, 45, 50, 55, 59, 64], + "Drop A + Drop E": [28, 33, 40, 45, 50, 55, 59, 64], + "E Standard": [28, 33, 38, 43, 48, 53, 57, 62], + "Eb Standard": [27, 32, 37, 42, 47, 52, 56, 61], + "Drop D": [26, 33, 38, 43, 48, 53, 57, 62], }, "bass-4": { - "Standard": [41.20, 55.00, 73.42, 98.00], - "Eb Standard": [38.89, 51.91, 69.30, 92.50], - "Drop D": [36.71, 55.00, 73.42, 98.00], - "D Standard": [36.71, 48.99, 65.41, 87.31], - "Drop C": [32.70, 48.99, 65.41, 87.31], + "Standard": [28, 33, 38, 43], + "Eb Standard": [27, 32, 37, 42], + "D Standard": [26, 31, 36, 41], + "C# Standard": [25, 30, 35, 40], + "C Standard": [24, 29, 34, 39], + "Drop D": [26, 33, 38, 43], + "Drop C": [24, 31, 36, 41], + "BEAD": [23, 28, 33, 38], }, "bass-5": { - "Standard": [30.87, 41.20, 55.00, 73.42, 98.00], - "Eb Standard": [29.14, 38.89, 51.91, 69.30, 92.50], - "Drop D": [30.87, 36.71, 55.00, 73.42, 98.00], - "D Standard": [27.50, 36.71, 48.99, 65.41, 87.31], - "Drop C": [27.50, 32.70, 48.99, 65.41, 87.31], + "Standard": [23, 28, 33, 38, 43], + "High C": [28, 33, 38, 43, 48], + "Eb Standard": [22, 27, 32, 37, 42], + "D Standard": [21, 26, 31, 36, 41], + "C# Standard": [20, 25, 30, 35, 40], + "C Standard": [19, 24, 29, 34, 39], + "Drop A": [21, 28, 33, 38, 43], }, + "bass-6": { + "Standard": [23, 28, 33, 38, 43, 48], + "Eb Standard": [22, 27, 32, 37, 42, 47], + "D Standard": [21, 26, 31, 36, 41, 46], + "C# Standard": [20, 25, 30, 35, 40, 45], + "C Standard": [19, 24, 29, 34, 39, 44], + }, +} + + +def midi_to_freq(midi: int, reference_pitch: float = DEFAULT_REFERENCE_PITCH) -> float: + """Return the frequency for a MIDI note at the supplied A4 reference.""" + return reference_pitch * math.pow(2, (midi - 69) / 12) + + +def open_midis_to_freqs(midis: list[int], reference_pitch: float = DEFAULT_REFERENCE_PITCH) -> list[float]: + """Return rounded frequencies for low-to-high MIDI open strings.""" + return [round(midi_to_freq(m, reference_pitch), 2) for m in midis] + + +def tuning_offsets_from_midis(instrument_key: str, midis: list[int]) -> list[int] | None: + """Return semitone offsets from the instrument's standard open strings.""" + standard = STANDARD_OPEN_MIDIS.get(instrument_key) + if not standard or len(standard) != len(midis): + return None + return [int(m - s) for m, s in zip(midis, standard)] + + +def tuning_midis_from_offsets(instrument_key: str, offsets: list[int]) -> list[int] | None: + """Return absolute open-string MIDI notes for host semitone offsets.""" + standard = STANDARD_OPEN_MIDIS.get(instrument_key) + if not standard or len(standard) != len(offsets): + return None + return [int(s + o) for s, o in zip(standard, offsets)] + + +def tuning_preset_offsets(instrument_key: str, name: str) -> list[int] | None: + """Return host semitone offsets for a named preset.""" + midis = TUNING_PRESET_MIDIS.get(instrument_key, {}).get(name) + if not midis: + return None + return tuning_offsets_from_midis(instrument_key, midis) + + +# Canonical tuning frequencies at 440 Hz reference, keyed by instrument then +# tuning name. Kept for the existing /api/tunings contract. +DEFAULT_TUNINGS: dict[str, dict[str, list[float]]] = { + instrument: { + name: open_midis_to_freqs(midis) + for name, midis in presets.items() + } + for instrument, presets in TUNING_PRESET_MIDIS.items() } @@ -67,6 +148,256 @@ def apply_reference_pitch( } +PROFILE_IDS = ("guitar-lead", "guitar-rhythm", "bass") +PROFILE_PATHWAYS = ("songs", "practice", "learn", "studio") +DEFAULT_ACTIVE_INSTRUMENT_PROFILE = "guitar-lead" +PROFILE_DEFAULTS: dict[str, dict] = { + "guitar-lead": { + "id": "guitar-lead", + "label": "Lead Guitar", + "instrument": "guitar", + "role": "lead", + "string_count": 6, + "tuning": "Standard", + "reference_pitch": DEFAULT_REFERENCE_PITCH, + "pathway": "songs", + }, + "guitar-rhythm": { + "id": "guitar-rhythm", + "label": "Rhythm Guitar", + "instrument": "guitar", + "role": "rhythm", + "string_count": 6, + "tuning": "Standard", + "reference_pitch": DEFAULT_REFERENCE_PITCH, + "pathway": "songs", + }, + "bass": { + "id": "bass", + "label": "Bass", + "instrument": "bass", + "role": "bass", + "string_count": 4, + "tuning": "Standard", + "reference_pitch": DEFAULT_REFERENCE_PITCH, + "pathway": "songs", + }, +} + + +def instrument_key(instrument: str, string_count: int) -> str: + return f"{instrument}-{string_count}" + + +def default_instrument_profiles() -> dict[str, dict]: + return {profile_id: dict(profile) for profile_id, profile in PROFILE_DEFAULTS.items()} + + +def _valid_reference_pitch(value) -> float | None: + if isinstance(value, bool): + return None + try: + ref = float(value) + except (TypeError, ValueError, OverflowError): + return None + if not math.isfinite(ref) or ref < 430.0 or ref > 450.0: + return None + return ref + + +def _valid_tuning_for_key(key: str, tuning): + if isinstance(tuning, str): + if len(tuning) > 64: + return None + if tuning in TUNING_PRESET_MIDIS.get(key, {}): + return tuning + # A name that IS a built-in preset for a different key is a misapplied + # built-in (e.g. "Drop D" on a 5-string bass, whose low string is B) — + # reject it. A name unknown to every built-in table is a provider/custom + # tuning (the tuner plugin's, exposed via /api/tunings) that this pure + # layer can't resolve — accept it so settings round-trip; the provider + # owns its validity. + if any(tuning in names for names in TUNING_PRESET_MIDIS.values()): + return None + return tuning + if isinstance(tuning, list): + expected = len(STANDARD_OPEN_MIDIS.get(key, [])) + if len(tuning) != expected: + return None + if any(isinstance(o, bool) or not isinstance(o, int) or o < -12 or o > 12 for o in tuning): + return None + return list(tuning) + return None + + +def normalize_instrument_profile(profile_id: str, raw) -> tuple[dict | None, str | None]: + """Validate one persisted host instrument profile.""" + base = dict(PROFILE_DEFAULTS.get(profile_id, {})) + if not base: + return None, f"unknown instrument profile: {profile_id}" + if raw is None: + return base, None + if not isinstance(raw, dict): + return None, f"instrument_profiles.{profile_id} must be an object" + + instrument = raw.get("instrument", base["instrument"]) + if instrument not in ("guitar", "bass"): + return None, f"instrument_profiles.{profile_id}.instrument must be 'guitar' or 'bass'" + + try: + string_count = int(raw.get("string_count", base["string_count"])) + except (TypeError, ValueError, OverflowError): + return None, f"instrument_profiles.{profile_id}.string_count must be valid for the instrument" + key = instrument_key(instrument, string_count) + if key not in STANDARD_OPEN_MIDIS: + return None, f"instrument_profiles.{profile_id}.string_count must be valid for the instrument" + + tuning = _valid_tuning_for_key(key, raw.get("tuning", base["tuning"])) + if tuning is None: + return None, f"instrument_profiles.{profile_id}.tuning must match {key}" + + ref = _valid_reference_pitch(raw.get("reference_pitch", base["reference_pitch"])) + if ref is None: + return None, f"instrument_profiles.{profile_id}.reference_pitch must be a number between 430 and 450" + + label = raw.get("label", base["label"]) + if not isinstance(label, str) or len(label) > 64: + return None, f"instrument_profiles.{profile_id}.label must be a short string" + role = raw.get("role", base["role"]) + if not isinstance(role, str) or len(role) > 32: + return None, f"instrument_profiles.{profile_id}.role must be a short string" + pathway = raw.get("pathway", base["pathway"]) + if not isinstance(pathway, str) or pathway not in PROFILE_PATHWAYS: + return None, f"instrument_profiles.{profile_id}.pathway must be one of songs, practice, learn, studio" + + out = dict(base) + out.update({ + "id": profile_id, + "label": label, + "instrument": instrument, + "role": role, + "string_count": string_count, + "tuning": tuning, + "reference_pitch": ref, + "pathway": pathway, + }) + return out, None + + +def normalize_instrument_profiles(raw_profiles=None) -> tuple[dict[str, dict] | None, str | None]: + """Validate persisted host profiles, filling omitted built-ins with defaults.""" + if raw_profiles is None: + return default_instrument_profiles(), None + if not isinstance(raw_profiles, dict): + return None, "instrument_profiles must be an object" + profiles = {} + for profile_id in PROFILE_IDS: + profile, error = normalize_instrument_profile(profile_id, raw_profiles.get(profile_id)) + if error: + return None, error + profiles[profile_id] = profile + return profiles, None + + +def active_profile_id(raw) -> str: + return raw if raw in PROFILE_DEFAULTS else DEFAULT_ACTIVE_INSTRUMENT_PROFILE + + +def profile_from_legacy_settings(cfg: dict) -> dict: + """Build an active profile from the old flat settings keys.""" + instrument = cfg.get("instrument") if cfg.get("instrument") in ("guitar", "bass") else "guitar" + fallback_sc = 4 if instrument == "bass" else 6 + try: + sc = int(cfg.get("string_count", fallback_sc)) + except (TypeError, ValueError, OverflowError): + sc = fallback_sc + key = instrument_key(instrument, sc) + if key not in STANDARD_OPEN_MIDIS: + sc = fallback_sc + key = instrument_key(instrument, sc) + tuning = _valid_tuning_for_key(key, cfg.get("tuning", "Standard")) or "Standard" + ref = _valid_reference_pitch(cfg.get("reference_pitch", DEFAULT_REFERENCE_PITCH)) or DEFAULT_REFERENCE_PITCH + pathway = cfg.get("pathway") if cfg.get("pathway") in PROFILE_PATHWAYS else "songs" + profile_id = "bass" if instrument == "bass" else DEFAULT_ACTIVE_INSTRUMENT_PROFILE + profile = dict(PROFILE_DEFAULTS[profile_id]) + profile.update({ + "instrument": instrument, + "string_count": sc, + "tuning": tuning, + "reference_pitch": ref, + "pathway": pathway, + }) + return profile + + +def settings_with_instrument_profiles(cfg: dict) -> dict: + """Return settings with canonical host profiles and mirrored flat keys.""" + out = dict(cfg) + profiles, _error = normalize_instrument_profiles(out.get("instrument_profiles")) + if profiles is None: + profiles = default_instrument_profiles() + if "instrument_profiles" not in out: + legacy = profile_from_legacy_settings(out) + profiles[legacy["id"]] = legacy + # Default the active profile to the one migrated from the legacy flat + # fields, but DON'T clobber an explicit request — a fresh-config + # `POST {"active_instrument_profile": "bass"}` must switch, not be + # overwritten by the guitar-lead inferred from defaults. active_profile_id + # below normalizes an invalid value. + out.setdefault("active_instrument_profile", legacy["id"]) + active = active_profile_id(out.get("active_instrument_profile")) + selected = profiles[active] + out["instrument_profiles"] = profiles + out["active_instrument_profile"] = active + out["instrument"] = selected["instrument"] + out["string_count"] = selected["string_count"] + out["tuning"] = selected["tuning"] + out["reference_pitch"] = selected["reference_pitch"] + out["pathway"] = selected["pathway"] + return out + + +def apply_flat_instrument_patch_to_profiles(cfg: dict, updates: dict) -> dict: + """Mirror legacy flat instrument updates into the active host profile.""" + out = settings_with_instrument_profiles(cfg) + if not any(k in updates for k in ("instrument", "string_count", "tuning", "reference_pitch", "pathway")): + return out + active = active_profile_id(out.get("active_instrument_profile")) + if "instrument" in updates: + active = "bass" if updates["instrument"] == "bass" else "guitar-lead" + out["active_instrument_profile"] = active + current = dict(out["instrument_profiles"][active]) + + if "instrument" in updates: + current["instrument"] = updates["instrument"] + if "string_count" not in updates: + current["string_count"] = 4 if updates["instrument"] == "bass" else 6 + if "string_count" in updates: + current["string_count"] = updates["string_count"] + if "reference_pitch" in updates: + current["reference_pitch"] = updates["reference_pitch"] + if "pathway" in updates: + current["pathway"] = updates["pathway"] + if "tuning" in updates: + current["tuning"] = updates["tuning"] + else: + key = instrument_key(current["instrument"], current["string_count"]) + if _valid_tuning_for_key(key, current.get("tuning")) is None: + current["tuning"] = "Standard" + + profile, error = normalize_instrument_profile(active, current) + if error: + raise ValueError(error) + out["instrument_profiles"][active] = profile + out.update({ + "instrument": profile["instrument"], + "string_count": profile["string_count"], + "tuning": profile["tuning"], + "reference_pitch": profile["reference_pitch"], + "pathway": profile["pathway"], + }) + return out + def tuning_name(offsets: list[int]) -> str: # All three pattern checks below are gated on `len(offsets) == 6`. The # naming conventions here are 6-string-specific — e.g. a 7-string all-zeros diff --git a/server.py b/server.py index c78355c..9ed3b7e 100644 --- a/server.py +++ b/server.py @@ -43,7 +43,12 @@ from song import ( scale_degree_for_pitch, ) from audio import find_wem_files, convert_wem -from tunings import tuning_name, DEFAULT_TUNINGS, DEFAULT_REFERENCE_PITCH, apply_reference_pitch +from tunings import ( + DEFAULT_REFERENCE_PITCH, DEFAULT_TUNINGS, PROFILE_IDS, PROFILE_PATHWAYS, + apply_flat_instrument_patch_to_profiles, apply_reference_pitch, + normalize_instrument_profile, normalize_instrument_profiles, + settings_with_instrument_profiles, tuning_name, +) import sloppak as sloppak_mod import drums as drums_mod import notation as notation_mod @@ -9862,7 +9867,7 @@ def get_tunings(): @app.get("/api/settings") def get_settings(): cfg = _load_config(CONFIG_DIR / "config.json") - return cfg if cfg is not None else _default_settings() + return settings_with_instrument_profiles(cfg if cfg is not None else _default_settings()) @app.post("/api/settings") @@ -10073,6 +10078,38 @@ def save_settings(data: dict): else: return {"error": "tuning must be a name (string) or a list of semitone offsets"} + if "pathway" in data: + raw = data["pathway"] + if raw is not None: + if not isinstance(raw, str) or raw not in PROFILE_PATHWAYS: + return {"error": "pathway must be one of songs, practice, learn, studio"} + updates["pathway"] = raw + + _profile_patch = None + if "instrument_profiles" in data: + raw = data["instrument_profiles"] + if raw is not None: + if not isinstance(raw, dict): + return {"error": "instrument_profiles must be an object"} + # Validate each PROVIDED profile individually and keep the patch + # PARTIAL — /api/settings is a partial-merge endpoint, so updating one + # profile must NOT reset the others to defaults. Merged over the + # persisted profiles inside the lock below (not via the wholesale + # `updates` merge, which would clobber the unspecified ones). + _profile_patch = {} + for _pid, _praw in raw.items(): + if _pid not in PROFILE_IDS: + return {"error": f"unknown instrument profile: {_pid}"} + _prof, _perr = normalize_instrument_profile(_pid, _praw) + if _perr: + return {"error": _perr} + _profile_patch[_pid] = _prof + if "active_instrument_profile" in data: + raw = data["active_instrument_profile"] + if raw is not None: + if not isinstance(raw, str) or raw not in PROFILE_IDS: + return {"error": "active_instrument_profile must be one of guitar-lead, guitar-rhythm, bass"} + updates["active_instrument_profile"] = raw CONFIG_DIR.mkdir(parents=True, exist_ok=True) # Critical section — the read-merge-write must be atomic. FastAPI runs # sync handlers in a threadpool, so two concurrent partial POSTs (e.g. @@ -10089,6 +10126,29 @@ def save_settings(data: dict): if cfg is None: cfg = _default_settings() cfg.update(updates) + if _profile_patch is not None: + # Merge the validated partial over the persisted profiles so a + # single-profile update leaves the others intact (a fresh config + # falls back to the built-in defaults for the unspecified ones). + _existing, _ = normalize_instrument_profiles(cfg.get("instrument_profiles")) + if _existing is None: + _existing = {} + _existing.update(_profile_patch) + cfg["instrument_profiles"] = _existing + # Only canonicalize/persist the instrument profiles when this save + # actually touches them (or the config already carries them). GET always + # virtualizes profiles via settings_with_instrument_profiles, so a save + # that doesn't touch instrument settings must stay a plain partial merge + # — otherwise an empty (or unrelated) POST would freeze the default + # profiles into the on-disk config. + _profile_keys = ("instrument", "string_count", "tuning", "reference_pitch", + "pathway", "instrument_profiles", "active_instrument_profile") + if "instrument_profiles" in cfg or any(k in updates for k in _profile_keys): + try: + cfg = apply_flat_instrument_patch_to_profiles(cfg, updates) + except ValueError as exc: + return {"error": str(exc)} + cfg = settings_with_instrument_profiles(cfg) _atomic_write_file(config_file, json.dumps(cfg, indent=2).encode("utf-8")) return {"message": ". ".join(messages) if messages else "Settings saved"} @@ -10100,7 +10160,8 @@ def save_settings(data: dict): _RESETTABLE_SETTINGS_KEYS = frozenset({ "default_arrangement", "demucs_server_url", "master_difficulty", "av_offset_ms", "countdown_before_song", "miss_penalty", "fail_behavior", - "reference_pitch", "instrument", "string_count", "tuning", + "reference_pitch", "instrument", "string_count", "tuning", "pathway", + "instrument_profiles", "active_instrument_profile", "achievements_enabled", "use_amp_sims", }) @@ -10125,6 +10186,16 @@ def reset_settings(data: dict): removed = [k for k in keys if k in cfg] for k in removed: del cfg[k] + # `pathway` is mirrored into every instrument profile, so deleting the + # flat key alone doesn't reset it — GET re-derives the value from the + # active profile. Reset it inside the persisted profiles too (back to the + # "songs" default), without disturbing the rest of the instrument config. + if "pathway" in keys and isinstance(cfg.get("instrument_profiles"), dict): + for prof in cfg["instrument_profiles"].values(): + if isinstance(prof, dict): + prof["pathway"] = "songs" + if "pathway" not in removed: + removed.append("pathway") _atomic_write_file(config_file, json.dumps(cfg, indent=2).encode("utf-8")) return {"message": "Settings reset", "reset": removed} @@ -10216,6 +10287,18 @@ def _validate_server_config_types(cfg: dict) -> str | None: return "server_config.tuning offsets must be ≤8 integers between -12 and 12" else: return "server_config.tuning must be a name (string) or a list of semitone offsets" + if "pathway" in cfg: + v = cfg["pathway"] + if v is not None and (not isinstance(v, str) or v not in PROFILE_PATHWAYS): + return "server_config.pathway must be one of songs, practice, learn, studio" + if "instrument_profiles" in cfg: + profiles, error = normalize_instrument_profiles(cfg["instrument_profiles"]) + if error: + return f"server_config.{error}" + if "active_instrument_profile" in cfg: + v = cfg["active_instrument_profile"] + if v is not None and (not isinstance(v, str) or v not in PROFILE_IDS): + return "server_config.active_instrument_profile must be one of guitar-lead, guitar-rhythm, bass" return None @@ -10585,6 +10668,7 @@ def export_settings(): server_config = _load_config(config_file) if server_config is None: server_config = _default_settings() + server_config = settings_with_instrument_profiles(server_config) # Snapshot the library DB + custom art FIRST: if the irreplaceable state # can't be captured, abort with an error rather than hand back a bundle @@ -10827,7 +10911,7 @@ def import_settings(bundle: dict): with _settings_lock: _atomic_write_file( CONFIG_DIR / "config.json", - json.dumps(server_config, indent=2).encode("utf-8"), + json.dumps(settings_with_instrument_profiles(server_config), indent=2).encode("utf-8"), ) except OSError as e: # Phase-1 validation should have caught all foreseeable diff --git a/static/app.js b/static/app.js index 50a6181..d04826f 100644 --- a/static/app.js +++ b/static/app.js @@ -2758,6 +2758,12 @@ function goFavTreePage(p) { // ── Settings ───────────────────────────────────────────────────────────── let _defaultArrangement = ''; +const INSTRUMENT_PATHWAYS = ['songs', 'practice', 'learn', 'studio']; + +function _normalizeInstrumentPathway(value) { + return INSTRUMENT_PATHWAYS.includes(value) ? value : 'songs'; +} + function _syncDefaultArrangementSelect(value) { const sel = document.getElementById('default-arrangement'); if (!sel) return; @@ -3410,6 +3416,8 @@ async function loadSettings() { if (dlcEl) dlcEl.value = data.dlc_dir || ''; _defaultArrangement = data.default_arrangement || ''; _syncDefaultArrangementSelect(_defaultArrangement); + const pathwayEl = document.getElementById('setting-instrument-pathway'); + if (pathwayEl) pathwayEl.value = _normalizeInstrumentPathway(data.pathway); const demucsEl = document.getElementById('demucs-server-url'); if (demucsEl) demucsEl.value = data.demucs_server_url || ''; const leftyEl = document.getElementById('setting-lefty'); @@ -3901,6 +3909,18 @@ function persistSetting(key, value) { _settingSaveChain = next.catch(() => {}); return next; } +function setInstrumentPathway(value) { + const pathway = _normalizeInstrumentPathway(value); + const el = document.getElementById('setting-instrument-pathway'); + if (el) el.value = pathway; + persistSetting('pathway', pathway).then(() => { + if (window.v3Badges && typeof window.v3Badges.reload === 'function') { + try { window.v3Badges.reload(); } catch (_) { /* noop */ } + } + }); +} + + async function _postSetting(key, value) { const status = document.getElementById('settings-status'); try { diff --git a/static/capabilities/working-tuning.js b/static/capabilities/working-tuning.js index e000692..133447c 100644 --- a/static/capabilities/working-tuning.js +++ b/static/capabilities/working-tuning.js @@ -305,7 +305,7 @@ return fetch('/api/tunings') .then(function (r) { return r && r.ok ? r.json() : null; }) .then(function (t) { - const byName = t && t[key]; + const byName = t && ((t.tunings && t.tunings[key]) || t[key]); commit(byName ? _offsetsFromFreqs(byName[s.tuning], byName.Standard) : null); }) .catch(function () { commit(null); }); diff --git a/static/v3/badges.js b/static/v3/badges.js index 0157cd7..c67b3e1 100644 --- a/static/v3/badges.js +++ b/static/v3/badges.js @@ -21,7 +21,13 @@ const esc = (s) => String(s == null ? '' : s).replace(/[&<>"']/g, (c) => ( { '&': '&', '<': '<', '>': '>', '"': '"', "'": ''' }[c])); - const STRING_COUNTS = { guitar: [6, 7, 8], bass: [4, 5] }; + const STRING_COUNTS = { guitar: [6, 7, 8], bass: [4, 5, 6] }; + const PATHWAY_OPTIONS = [ + { id: 'songs', label: 'Songs' }, + { id: 'practice', label: 'Practice' }, + { id: 'learn', label: 'Learn' }, + { id: 'studio', label: 'Studio' }, + ]; // Tuning names per instrument key (e.g. 'guitar-6', 'bass-4'), loaded from // GET /api/tunings. Falls back to empty arrays until the fetch resolves. let _tuningsByKey = {}; @@ -106,7 +112,7 @@ } } - let settings = { instrument: 'guitar', string_count: 6, tuning: 'Standard', reference_pitch: 440 }; + let settings = { instrument: 'guitar', string_count: 6, tuning: 'Standard', reference_pitch: 440, pathway: 'songs', instrument_profiles: {}, active_instrument_profile: 'guitar-lead' }; async function loadTunings() { try { @@ -126,6 +132,15 @@ } catch (_) { /* non-fatal — TUNINGS falls back to empty, dropdown shows nothing */ } } + function pathwayForProfile(profiles, profileId, fallback) { + const p = profiles && profiles[profileId]; + return p && PATHWAY_OPTIONS.some((o) => o.id === p.pathway) ? p.pathway : (fallback || 'songs'); + } + + function profileIdForInstrument(inst) { + return inst === 'bass' ? 'bass' : 'guitar-lead'; + } + async function loadSettings() { try { const r = await fetch('/api/settings'); @@ -150,16 +165,34 @@ if (typeof s.tuning === 'string') tuning = tunings.includes(s.tuning) ? s.tuning : (tunings[0] || 'Standard'); else if (Array.isArray(s.tuning)) tuning = s.tuning; else tuning = tunings[0] || 'Standard'; + const profiles = s.instrument_profiles && typeof s.instrument_profiles === 'object' ? s.instrument_profiles : {}; + const pathway = PATHWAY_OPTIONS.some((o) => o.id === s.pathway) ? s.pathway : 'songs'; settings = { instrument: instrument, string_count: scValid, tuning: tuning, reference_pitch: Math.min(450, Math.max(430, ref)), + pathway: pathway, + instrument_profiles: profiles, + active_instrument_profile: typeof s.active_instrument_profile === 'string' ? s.active_instrument_profile : profileIdForInstrument(instrument), }; } } catch (e) { /* settings endpoint always present */ } } + function syncLocalProfilePatch(patch) { + const profileId = profileIdForInstrument(patch.instrument || settings.instrument); + if (!settings.instrument_profiles || typeof settings.instrument_profiles !== 'object') settings.instrument_profiles = {}; + if (patch.instrument) settings.active_instrument_profile = profileId; + const profile = Object.assign({}, settings.instrument_profiles[profileId] || {}); + let changed = false; + if (patch.instrument) { profile.instrument = patch.instrument; changed = true; } + if (patch.string_count != null) { profile.string_count = patch.string_count; changed = true; } + if (patch.tuning != null) { profile.tuning = patch.tuning; changed = true; } + if (patch.reference_pitch != null) { profile.reference_pitch = patch.reference_pitch; changed = true; } + if (patch.pathway != null) { profile.pathway = patch.pathway; changed = true; } + if (changed) settings.instrument_profiles[profileId] = profile; + } async function saveSettings(patch) { // Only adopt the patch once the server accepts it. /api/settings returns // {error: ...} with HTTP 200 on a validation failure, so a rejected @@ -177,8 +210,9 @@ } catch (e) { /* non-fatal — leave settings unchanged */ } if (!accepted) return false; Object.assign(settings, patch); + syncLocalProfilePatch(patch); if (sm && sm.emit) sm.emit('instrument:changed', { - instrument: settings.instrument, stringCount: settings.string_count, tuning: settings.tuning, + instrument: settings.instrument, stringCount: settings.string_count, tuning: settings.tuning, pathway: settings.pathway, }); pushToTuner(); renderTuner(); // reflect new tuning on the tuner card @@ -424,6 +458,9 @@ // (picking a named tuning still works and replaces the custom one). (typeof settings.tuning === 'string' ? '' : '') + _tuningsForInstrument(settings.instrument, settings.string_count).map((t) => '' + esc(t) + '').join('') + '' + + '
Pathway
' + + '
' + '
Reference pitch' + settings.reference_pitch + ' Hz
' + '
' + ''; @@ -454,6 +491,7 @@ instrument: v, string_count: newSc, tuning: tunings.includes(settings.tuning) ? settings.tuning : (tunings[0] || settings.tuning), + pathway: pathwayForProfile(settings.instrument_profiles, profileIdForInstrument(v), settings.pathway), }); // Only move the working-tuning context once the switch was actually persisted — // otherwise the selector stays on the old instrument while the card shows the @@ -462,11 +500,21 @@ renderInstrument(); keepOpen(); })); menu.querySelectorAll('[data-pill="strings"]').forEach((b) => b.addEventListener('click', async () => { - await saveSettings({ string_count: Number(b.getAttribute('data-val')) }); - setWorkingInstrument(settings.instrument, settings.string_count); + const newSc = Number(b.getAttribute('data-val')); + // Clamp the tuning to one valid for the new string count and post it + // alongside string_count — otherwise the backend silently resets a + // now-invalid tuning to Standard while this UI keeps showing the old + // one (settings/tuner desync). Mirrors the instrument-switch clamp. + const tunings = _tuningsForInstrument(settings.instrument, newSc); + await saveSettings({ + string_count: newSc, + tuning: tunings.includes(settings.tuning) ? settings.tuning : (tunings[0] || settings.tuning), + }); + setWorkingInstrument(settings.instrument, newSc); renderInstrument(); keepOpen(); })); menu.querySelector('[data-inst-tuning]').addEventListener('change', (e) => saveSettings({ tuning: e.target.value })); + menu.querySelector('[data-inst-pathway]').addEventListener('change', (e) => saveSettings({ pathway: e.target.value })); const ref = menu.querySelector('[data-inst-ref]'); ref.addEventListener('input', (e) => { menu.querySelector('[data-ref-val]').textContent = e.target.value + ' Hz'; }); ref.addEventListener('change', (e) => saveSettings({ reference_pitch: Number(e.target.value) })); diff --git a/static/v3/index.html b/static/v3/index.html index 41f9e76..2196535 100644 --- a/static/v3/index.html +++ b/static/v3/index.html @@ -429,6 +429,23 @@ + +
+ +
+
Instrument pathway
+
Preferred path for the selected instrument. This is remembered per instrument profile.
+
+
+ +
+
diff --git a/static/v3/settings.js b/static/v3/settings.js index 31e83fb..b961b7c 100644 --- a/static/v3/settings.js +++ b/static/v3/settings.js @@ -28,7 +28,7 @@ var RESET_MAP = { gameplay: { server: ['master_difficulty', 'av_offset_ms', 'miss_penalty', - 'fail_behavior', 'countdown_before_song', 'default_arrangement'], + 'fail_behavior', 'countdown_before_song', 'default_arrangement', 'pathway'], local: ['lefty', 'autoplayExit', 'showUpNext', 'confirmExitSong', 'arrangementNamingMode', 'countdownBeforeSong'], after: function () { // Left-handed is held on the highway object, not re-derived diff --git a/tests/js/working_tuning.test.js b/tests/js/working_tuning.test.js index c31bcc9..3a94b3a 100644 --- a/tests/js/working_tuning.test.js +++ b/tests/js/working_tuning.test.js @@ -16,8 +16,8 @@ const { createWindow, ROOT } = require('./capabilities_test_harness'); const CAPABILITIES_JS = path.join(ROOT, 'static', 'capabilities.js'); const WORKING_TUNING_JS = path.join(ROOT, 'static', 'capabilities', 'working-tuning.js'); -// A /api/tunings-shaped fixture (frequencies at 440), enough to resolve names to offsets. -const TUNINGS = { +// Tuning frequency fixture at 440 Hz, enough to resolve names to offsets. +const TUNING_TABLE = { 'guitar-6': { Standard: [82.41, 110.00, 146.83, 196.00, 246.94, 329.63], 'Drop D': [73.42, 110.00, 146.83, 196.00, 246.94, 329.63], @@ -26,6 +26,7 @@ const TUNINGS = { Standard: [30.87, 41.20, 55.00, 73.42, 98.00], }, }; +const API_TUNINGS = { referencePitch: 440, tunings: TUNING_TABLE }; function deferred() { let resolve; @@ -159,7 +160,7 @@ test('bare-instrument writes target the current selection, not a hard-coded defa test('seed resolves a NAMED tuning to offsets via /api/tunings', async () => { const { wt, changes } = loadWorkingTuning({ '/api/settings': { instrument: 'guitar', string_count: 6, tuning: 'Drop D', reference_pitch: 440 }, - '/api/tunings': TUNINGS, + '/api/tunings': API_TUNINGS, }); await flush(); const s = wt.get('guitar-6'); @@ -183,7 +184,7 @@ test('boot race: an explicit set() before settings resolve is not clobbered by t const settings = deferred(); const { wt } = loadWorkingTuning({ '/api/settings': settings.promise, // held open - '/api/tunings': TUNINGS, + '/api/tunings': API_TUNINGS, }); // A consumer writes before the seed lands. wt.set({ offsets: [-5, -5, -5, -5, -5, -5] }, { instrument: 'guitar-6' }); diff --git a/tests/test_settings_api.py b/tests/test_settings_api.py index e79b230..aae4a11 100644 --- a/tests/test_settings_api.py +++ b/tests/test_settings_api.py @@ -753,29 +753,142 @@ def test_defaults_include_gameplay_keys(client, tmp_path): assert data["fail_behavior"] == "continue" + +def test_get_settings_exposes_default_instrument_profiles(client, tmp_path): + data = client.get("/api/settings").json() + assert data["active_instrument_profile"] == "guitar-lead" + assert set(data["instrument_profiles"]) == {"guitar-lead", "guitar-rhythm", "bass"} + assert data["instrument"] == "guitar" + assert data["string_count"] == 6 + assert data["tuning"] == "Standard" + assert data["pathway"] == "songs" + + +def test_post_flat_instrument_updates_active_profile(client, tmp_path): + r = client.post("/api/settings", json={"instrument": "bass", "pathway": "practice"}) + assert r.status_code == 200 + cfg = _read_cfg(tmp_path) + assert cfg["active_instrument_profile"] == "bass" + assert cfg["instrument"] == "bass" + assert cfg["string_count"] == 4 + assert cfg["tuning"] == "Standard" + assert cfg["pathway"] == "practice" + assert cfg["instrument_profiles"]["bass"]["string_count"] == 4 + assert cfg["instrument_profiles"]["bass"]["pathway"] == "practice" + + +def test_post_instrument_profiles_mirrors_active_profile(client, tmp_path): + r = client.post("/api/settings", json={ + "active_instrument_profile": "guitar-rhythm", + "instrument_profiles": { + "guitar-rhythm": { + "string_count": 7, + "tuning": "Drop A", + "reference_pitch": 432, + "pathway": "studio", + }, + "bass": { + "string_count": 6, + "tuning": "C Standard", + }, + }, + }) + assert r.status_code == 200 + cfg = _read_cfg(tmp_path) + assert cfg["active_instrument_profile"] == "guitar-rhythm" + assert cfg["instrument"] == "guitar" + assert cfg["string_count"] == 7 + assert cfg["tuning"] == "Drop A" + assert cfg["reference_pitch"] == 432 + assert cfg["pathway"] == "studio" + + +def test_post_pathway_rejects_bad_value(client, tmp_path): + (tmp_path / "config.json").write_text(json.dumps({"pathway": "songs"})) + r = client.post("/api/settings", json={"pathway": "invalid"}) + assert "error" in r.json() + assert _read_cfg(tmp_path)["pathway"] == "songs" + + +def test_post_instrument_profiles_rejects_bad_custom_string_count(client, tmp_path): + r = client.post("/api/settings", json={ + "instrument_profiles": { + "bass": {"string_count": 6, "tuning": [0, 0, 0, 0]}, + }, + }) + assert "error" in r.json() + # ── /api/settings/reset ───────────────────────────────────────────────────── def test_reset_clears_requested_keys(client, tmp_path): (tmp_path / "config.json").write_text(json.dumps({ "master_difficulty": 40, "countdown_before_song": True, + "pathway": "studio", "default_arrangement": "Lead", "demucs_server_url": "http://demucs.example:9000", })) r = client.post("/api/settings/reset", - json={"keys": ["master_difficulty", "countdown_before_song"]}) + json={"keys": ["master_difficulty", "countdown_before_song", "pathway"]}) assert r.status_code == 200 body = r.json() - assert set(body["reset"]) == {"master_difficulty", "countdown_before_song"} + assert set(body["reset"]) == {"master_difficulty", "countdown_before_song", "pathway"} cfg = _read_cfg(tmp_path) # Reset removes the key so GET falls back to the default. assert "master_difficulty" not in cfg assert "countdown_before_song" not in cfg + assert "pathway" not in cfg # Unlisted keys are untouched. assert cfg["default_arrangement"] == "Lead" assert cfg["demucs_server_url"] == "http://demucs.example:9000" +def test_partial_instrument_profiles_update_preserves_others(client, tmp_path): + # /api/settings is a partial-merge endpoint, so a POST that carries only ONE + # instrument profile must not reset the others to defaults. + gl = client.get("/api/settings").json()["instrument_profiles"]["guitar-lead"] + gl = dict(gl); gl["tuning"] = "Drop D" + client.post("/api/settings", json={"instrument_profiles": {"guitar-lead": gl}}) + assert (client.get("/api/settings").json()["instrument_profiles"] + ["guitar-lead"]["tuning"] == "Drop D") + # Now update ONLY bass (Drop D is valid for a 4-string bass). + bass = client.get("/api/settings").json()["instrument_profiles"]["bass"] + bass = dict(bass); bass["tuning"] = "Drop D" + client.post("/api/settings", json={"instrument_profiles": {"bass": bass}}) + out = client.get("/api/settings").json()["instrument_profiles"] + assert out["guitar-lead"]["tuning"] == "Drop D", "the untouched profile survived" + assert out["bass"]["tuning"] == "Drop D" + + +def test_active_profile_switch_on_fresh_config(client, tmp_path): + # A fresh config has no instrument_profiles; an explicit active-profile + # switch must be honored, not overwritten by the profile inferred from the + # legacy flat defaults (guitar-lead). + r = client.post("/api/settings", json={"active_instrument_profile": "bass"}) + assert r.status_code == 200 and "error" not in r.json() + got = client.get("/api/settings").json() + assert got["active_instrument_profile"] == "bass" + assert got["instrument"] == "bass" + + +def test_reset_pathway_reaches_into_instrument_profiles(client, tmp_path): + # pathway is mirrored into every instrument profile, so a Gameplay reset + # that only deleted the flat key would leave GET re-deriving the old value + # from the profile. The reset must reach into the persisted profiles too. + client.post("/api/settings", json={"pathway": "studio"}) + assert client.get("/api/settings").json()["pathway"] == "studio" + profiles = _read_cfg(tmp_path)["instrument_profiles"] + assert any(p["pathway"] == "studio" for p in profiles.values()) + + r = client.post("/api/settings/reset", json={"keys": ["pathway"]}) + assert r.status_code == 200 + assert "pathway" in r.json()["reset"] + # GET re-derives from the profile — which must now be back to the default. + assert client.get("/api/settings").json()["pathway"] == "songs" + for prof in _read_cfg(tmp_path)["instrument_profiles"].values(): + assert prof["pathway"] == "songs" + + def test_reset_ignores_unknown_keys(client, tmp_path): (tmp_path / "config.json").write_text(json.dumps({"master_difficulty": 40})) # Unknown / non-resettable keys are silently ignored, not an error, and diff --git a/tests/test_settings_instrument.py b/tests/test_settings_instrument.py index ecbf707..d45724f 100644 --- a/tests/test_settings_instrument.py +++ b/tests/test_settings_instrument.py @@ -31,12 +31,14 @@ def _cfg(tmp_path): def test_instrument_fields_persist(env): srv, tmp = env c = TestClient(srv.app) + # "Drop A" is the 5-string bass drop tuning (its low string is B, not E, so + # "Drop D" is a 4-string tuning — now correctly rejected per-profile). r = c.post("/api/settings", json={"instrument": "bass", "string_count": 5, - "tuning": "Drop D", "reference_pitch": 442}) + "tuning": "Drop A", "reference_pitch": 442}) assert r.status_code == 200 cfg = _cfg(tmp) assert cfg["instrument"] == "bass" and cfg["string_count"] == 5 - assert cfg["tuning"] == "Drop D" and cfg["reference_pitch"] == 442.0 + assert cfg["tuning"] == "Drop A" and cfg["reference_pitch"] == 442.0 # Reflected back through GET. got = c.get("/api/settings").json() assert got["instrument"] == "bass" and got["reference_pitch"] == 442.0 diff --git a/tests/test_tunings.py b/tests/test_tunings.py index 8f67037..b2f67e2 100644 --- a/tests/test_tunings.py +++ b/tests/test_tunings.py @@ -2,7 +2,31 @@ import pytest -from tunings import tuning_name +from tunings import ( + DEFAULT_TUNINGS, + TUNING_PRESET_MIDIS, + _valid_tuning_for_key, + apply_flat_instrument_patch_to_profiles, + open_midis_to_freqs, + settings_with_instrument_profiles, + tuning_midis_from_offsets, + tuning_name, + tuning_offsets_from_midis, + tuning_preset_offsets, +) + + +def test_valid_tuning_for_key_builtin_and_provider_names(): + # A built-in valid for the key is accepted; a built-in valid only for a + # DIFFERENT key (misapplied, e.g. "Drop D" on a 5-string bass) is rejected. + assert _valid_tuning_for_key("bass-5", "Drop A") == "Drop A" + assert _valid_tuning_for_key("bass-5", "Drop D") is None + assert _valid_tuning_for_key("guitar-6", "Standard") == "Standard" + # A name unknown to every built-in table is a provider/custom tuning (tuner + # plugin, /api/tunings) the pure layer can't resolve — accept it so settings + # round-trip rather than normalizing it away to Standard. + assert _valid_tuning_for_key("bass-5", "My Custom DADGAD") == "My Custom DADGAD" + assert _valid_tuning_for_key("guitar-6", "x" * 65) is None # length cap kept # ── Standard tunings (all six strings share the same offset) ───────────────── @@ -132,3 +156,96 @@ def test_drop_pattern_takes_precedence_over_named_dict(): # auto-generator fires first and produces the same string. The named dict entry # is effectively dead code for this case — this test documents the behavior. assert tuning_name([-2, 0, 0, 0, 0, 0]) == "Drop D" + + +# ── Host tuning profile catalogue ------------------------------------------- + +def test_default_tunings_include_extended_host_profiles(): + assert "bass-6" in DEFAULT_TUNINGS + assert "C Standard" in DEFAULT_TUNINGS["guitar-6"] + assert "C# Standard" in DEFAULT_TUNINGS["guitar-6"] + assert "Drop Ab" in DEFAULT_TUNINGS["guitar-6"] + assert "BEAD" in DEFAULT_TUNINGS["bass-4"] + assert "High C" in DEFAULT_TUNINGS["bass-5"] + assert "Drop A + Drop E" in DEFAULT_TUNINGS["guitar-8"] + + +def test_default_tuning_frequencies_are_derived_from_midis(): + assert DEFAULT_TUNINGS["guitar-6"]["Standard"] == open_midis_to_freqs([40, 45, 50, 55, 59, 64]) + assert DEFAULT_TUNINGS["bass-6"]["Standard"] == open_midis_to_freqs([23, 28, 33, 38, 43, 48]) + + +def test_tuning_offsets_from_named_presets(): + assert tuning_preset_offsets("guitar-6", "Drop D") == [-2, 0, 0, 0, 0, 0] + assert tuning_preset_offsets("guitar-6", "C Standard") == [-4, -4, -4, -4, -4, -4] + assert tuning_preset_offsets("bass-4", "BEAD") == [-5, -5, -5, -5] + assert tuning_preset_offsets("bass-5", "High C") == [5, 5, 5, 5, 5] + + +def test_tuning_midis_round_trip_offsets(): + offsets = [-2, 0, 0, 0, 0, 0] + midis = tuning_midis_from_offsets("guitar-6", offsets) + assert midis == TUNING_PRESET_MIDIS["guitar-6"]["Drop D"] + assert tuning_offsets_from_midis("guitar-6", midis) == offsets + + +def test_tuning_conversion_rejects_wrong_string_count(): + assert tuning_offsets_from_midis("guitar-6", [40, 45, 50, 55]) is None + assert tuning_midis_from_offsets("bass-4", [0, 0, 0, 0, 0]) is None + +def test_settings_profiles_default_to_lead_rhythm_and_bass(): + settings = settings_with_instrument_profiles({}) + assert settings["active_instrument_profile"] == "guitar-lead" + assert set(settings["instrument_profiles"]) == {"guitar-lead", "guitar-rhythm", "bass"} + assert settings["instrument"] == "guitar" + assert settings["string_count"] == 6 + assert settings["tuning"] == "Standard" + assert settings["pathway"] == "songs" + assert settings["instrument_profiles"]["guitar-lead"]["pathway"] == "songs" + + +def test_settings_profiles_migrate_legacy_flat_bass_selection(): + settings = settings_with_instrument_profiles({ + "instrument": "bass", + "string_count": 6, + "tuning": "C Standard", + "reference_pitch": 432, + "pathway": "practice", + }) + assert settings["active_instrument_profile"] == "bass" + assert settings["instrument_profiles"]["bass"]["string_count"] == 6 + assert settings["instrument_profiles"]["bass"]["tuning"] == "C Standard" + assert settings["reference_pitch"] == 432 + assert settings["pathway"] == "practice" + assert settings["instrument_profiles"]["bass"]["pathway"] == "practice" + + +def test_flat_patch_updates_active_profile_and_mirrors_legacy_keys(): + settings = settings_with_instrument_profiles({}) + patched = apply_flat_instrument_patch_to_profiles(settings, {"tuning": "Drop D"}) + assert patched["tuning"] == "Drop D" + assert patched["instrument_profiles"]["guitar-lead"]["tuning"] == "Drop D" + + +def test_flat_pathway_patch_updates_active_profile_and_mirrors_legacy_key(): + settings = settings_with_instrument_profiles({}) + patched = apply_flat_instrument_patch_to_profiles(settings, {"pathway": "studio"}) + assert patched["pathway"] == "studio" + assert patched["instrument_profiles"]["guitar-lead"]["pathway"] == "studio" + + +def test_flat_instrument_patch_defaults_to_target_string_count(): + settings = settings_with_instrument_profiles({"instrument": "guitar", "string_count": 6, "tuning": "Drop D"}) + patched = apply_flat_instrument_patch_to_profiles(settings, {"instrument": "bass"}) + assert patched["instrument"] == "bass" + assert patched["string_count"] == 4 + assert patched["tuning"] == "Standard" + assert patched["active_instrument_profile"] == "bass" + assert patched["instrument_profiles"]["bass"]["string_count"] == 4 + + +def test_flat_string_count_patch_resets_incompatible_named_tuning(): + settings = settings_with_instrument_profiles({"instrument": "guitar", "string_count": 6, "tuning": "DADGAD"}) + patched = apply_flat_instrument_patch_to_profiles(settings, {"string_count": 7}) + assert patched["string_count"] == 7 + assert patched["tuning"] == "Standard" From a65d8cfa131d7c6349242c026ef0ad05acc8a4f3 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:17:05 -0500 Subject: [PATCH 4/8] fix(enrichment): rank the canonical studio take over live/comp versions (#758) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(enrichment): rank the canonical studio take over live/comp versions A flat MusicBrainz /recording text search ties every take of a song at the same score, so "AC/DC — Highway to Hell" returns a wall of live bootlegs and compilations with the 1979 studio version buried (or below the fetch limit). - build_recording_query: drop live-ONLY recordings (`-secondarytype:Live`). Compilations are deliberately kept — they REUSE the studio recording, so filtering them cuts the very recording we want (verified against MB). - _best_release / parse_recording_doc: pick the canonical studio album (primary Album, no Live/Compilation/Remix/... secondary type) for the displayed album/year, and expose a `studio` flag. - rank_candidates: since the combined score caps at 1.0 (perfect text match ties), break ties on the studio flag and — when the caller knows the audio length — on duration proximity, so the studio take wins over live/extended cuts. The studio distinction is intentionally NOT scored (a live take is still the right SONG), only re-ordered. - /api/enrichment/search: accept an optional `duration` param so a caller that has the audio but no library row (the editor's create modal) can pass the master-track length for the duration tiebreak. Verified end-to-end against live MusicBrainz: AC/DC "Highway to Hell" now returns the 1979 studio recording at #1 with the correct album + year. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(enrichment): official releases outrank unofficial studio albums _best_release sorted (clean, status_ok, date), so an UNofficial bootleg Album outranked an official Single/EP/comp — regressing canonical album/year and seeding cover-art from a bootleg for single-only songs. Order status_ok before clean: official first, then prefer a clean studio album among the official releases (still surfaces the studio album over an official live/comp album). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(enrichment): keep live recordings for genuinely-live charts build_recording_query unconditionally added -secondarytype:Live, but denoise() strips a '(Live at …)' qualifier from the query — so a chart that IS a live take had its only correct recording filtered out (both background enrichment and manual search). Skip the live filter when the source title carries a parenthetical live marker; a bare title word ('Live and Let Die') still filters. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(enrichment): drop the studio tiebreak when the chart is a live take Follow-through on keeping live recordings for live charts: rank_candidates still ranked the studio take ahead of a tied live one, so a live chart would auto-match the studio recording. Skip the studio tiebreak when the source title has a live marker — duration proximity + score then pick the right live version. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- lib/mb_match.py | 87 +++++++++++++++++++++++++++++++++++++----- server.py | 20 +++++++--- tests/test_mb_match.py | 54 +++++++++++++++++++++++++- 3 files changed, 145 insertions(+), 16 deletions(-) diff --git a/lib/mb_match.py b/lib/mb_match.py index 2efb233..9230961 100644 --- a/lib/mb_match.py +++ b/lib/mb_match.py @@ -39,6 +39,14 @@ DURATION_BONUS_LOOSE = 0.025 # …within 15s _DURATION_TIGHT = 5 _DURATION_LOOSE = 15 +# Release-group secondary types that mark a NON-canonical release (a live album, +# a greatest-hits comp, a remix/DJ set, …). Used both to pick the canonical +# studio album for display and to reward studio recordings in ranking. +_SECONDARY_SKIP = { + "live", "compilation", "remix", "dj-mix", "mixtape/street", + "demo", "interview", "audiobook", "spokenword", +} + # ── Denoise ─────────────────────────────────────────────────────────────────── # A parenthetical/bracketed group is dropped when it contains any of these # noise terms as a whole word (chart-variant markers, tuning/pitch notes, @@ -154,6 +162,10 @@ def score_candidate(song: dict, cand: dict) -> float: score += DURATION_BONUS elif diff <= _DURATION_LOOSE: score += DURATION_BONUS_LOOSE + # NB: the studio-vs-live distinction is deliberately NOT scored here — a live + # take is still the RIGHT SONG (same title/artist), so it must not change the + # auto/review confidence. Canonical-version preference lives in the RANK sort + # (rank_candidates) instead, where it only reorders same-song candidates. return min(score, 1.0) @@ -179,15 +191,34 @@ def classify(song: dict, cand: dict, score: float, auto_min: float | None = None def rank_candidates(song: dict, candidates: list[dict]) -> list[dict]: - """Score every candidate against the song and return them sorted by our - score (MusicBrainz's own search score is only a tiebreak). Each returned - dict is a copy carrying `score` (rounded — it's displayed and stored).""" + """Score every candidate against the song and return them sorted best-first. + The combined `score` caps at 1.0, so a perfect-text-match query (every "AC/DC + Highway to Hell" recording) ties at the top — there the studio flag and, when + the caller knows the audio length, the duration match break the tie so the + canonical studio take wins over live/promo/extended cuts. Each returned dict + is a copy carrying `score` (rounded — it's displayed and stored).""" + sd = _duration_int(song.get("duration")) + # For a chart that IS a live take (build_recording_query keeps live + # recordings for these) the studio take is the WRONG recording, so drop the + # studio tiebreak — duration proximity + text/mb score then pick the right + # live version instead of auto-matching the studio one. + prefer_studio = not _LIVE_GROUP_RE.search(str(song.get("title") or "")) + + def _dur_diff(c): + cd = _duration_int(c.get("duration")) + return abs(sd - cd) if (sd and cd) else 10 ** 6 + ranked = [] for cand in candidates or []: c = dict(cand) c["score"] = round(score_candidate(song, cand), 4) ranked.append(c) - ranked.sort(key=lambda c: (c["score"], c.get("mb_score") or 0), reverse=True) + ranked.sort( + key=lambda c: (c["score"], + (1 if c.get("studio") else 0) if prefer_studio else 0, + -_dur_diff(c), # closest to the audio length + c.get("mb_score") or 0), + reverse=True) return ranked @@ -198,6 +229,11 @@ def _lucene_escape_phrase(s: str) -> str: return s.replace("\\", "\\\\").replace('"', '\\"') +# A parenthetical/bracketed "(Live …)" marker — the live signal denoise() strips +# from the title. Mirrors _NOISE_GROUP_RE but for the `live` term only. +_LIVE_GROUP_RE = re.compile(r"[(\[][^)\]]*\blive\b[^)\]]*[)\]]", re.IGNORECASE) + + def build_recording_query(artist, title) -> str: """Lucene query for /ws/2/recording. Built from the DENOISED fields — the noise we strip (author credits, "(Live)", "(v2)") would otherwise @@ -209,7 +245,22 @@ def build_recording_query(artist, title) -> str: parts.append('recording:"%s"' % _lucene_escape_phrase(t)) if a: parts.append('artist:"%s"' % _lucene_escape_phrase(a)) - return " AND ".join(parts) + q = " AND ".join(parts) + # Drop live-ONLY recordings (bootlegs, live albums) — the canonical studio + # take is never tagged Live, and this is the single biggest source of junk in + # a flat recording search. Compilations are deliberately NOT excluded: they + # REUSE the studio recording, so filtering them would drop the very recording + # we want (verified against MusicBrainz — `-secondarytype:Compilation` cut the + # AC/DC studio "Highway to Hell" recording entirely). + # + # EXCEPT when the source chart is itself a live take: denoise() strips the + # "(Live at …)" qualifier from the query, so filtering Live would leave the + # genuinely-live chart with NO correct recording. Only a parenthetical marker + # counts — a bare title word ("Live and Let Die") is a real word, not a live + # tag — mirroring what denoise removes. + if q and not _LIVE_GROUP_RE.search(str(title or "")): + q += " AND -secondarytype:Live" + return q def _artist_credit(doc: dict) -> tuple[str, str, str]: @@ -226,19 +277,33 @@ def _artist_credit(doc: dict) -> tuple[str, str, str]: return name, str(artist.get("id", "") or ""), str(artist.get("sort-name", "") or "") +def _is_clean_studio_album(rg: dict) -> bool: + """A release-group that is a primary-type Album with NO non-canonical + secondary type (Live / Compilation / Remix / …) — i.e. a studio album.""" + if str(rg.get("primary-type", "")).lower() != "album": + return False + secs = {str(s).lower() for s in (rg.get("secondary-types") or [])} + return not (secs & _SECONDARY_SKIP) + + def _best_release(doc: dict) -> dict: - """Pick the release used for canon album/year: prefer Official status and - an Album release-group, then the earliest date. Returns {} if none.""" + """Pick the release used for canon album/year: prefer an OFFICIAL studio + Album (primary Album with no Live/Compilation/… secondary type), then the + earliest date. Falls back to any release when none is clean. {} if none.""" releases = [r for r in (doc.get("releases") or []) if isinstance(r, dict)] if not releases: return {} def sort_key(r): - status_ok = 0 if str(r.get("status", "")).lower() == "official" else 1 rg = r.get("release-group") or {} - album_ok = 0 if str(rg.get("primary-type", "")).lower() == "album" else 1 + clean = 0 if _is_clean_studio_album(rg) else 1 + status_ok = 0 if str(r.get("status", "")).lower() == "official" else 1 date = str(r.get("date", "") or "9999") - return (status_ok, album_ok, date) + # Official FIRST, then prefer a clean studio album: this still surfaces + # the studio album over an (official) live/comp album for the display + # album/year, but never lets an UNofficial bootleg album outrank an + # official single/EP/comp — which `(clean, status_ok, …)` would. + return (status_ok, clean, date) return sorted(releases, key=sort_key)[0] @@ -261,6 +326,7 @@ def parse_recording_doc(doc: dict) -> dict | None: return None artist_name, artist_id, artist_sort = _artist_credit(doc) release = _best_release(doc) + studio = _is_clean_studio_album(release.get("release-group") or {}) length = doc.get("length") try: duration = int(round(float(length) / 1000.0)) if length else None @@ -281,6 +347,7 @@ def parse_recording_doc(doc: dict) -> dict | None: "isrc": isrcs[0] if isrcs else "", "genres": _genres(doc), "mb_score": int(doc.get("score") or 0), + "studio": studio, } diff --git a/server.py b/server.py index 9ed3b7e..c3d4bd3 100644 --- a/server.py +++ b/server.py @@ -6282,8 +6282,11 @@ def _mb_http_get(path: str, params: dict) -> dict | None: raise EnrichTransportError("bad JSON from musicbrainz") from e -def _mb_search_recordings(artist, title, limit: int = 8) -> list[dict]: - """Text search (tier 2–4): denoised Lucene query over /recording.""" +def _mb_search_recordings(artist, title, limit: int = 12) -> list[dict]: + """Text search (tier 2–4): denoised Lucene query over /recording. The query + now drops live-only recordings and our ranker rewards the studio take, so a + slightly larger default result set gives the re-ranker room to surface the + canonical version (one request per song regardless of limit).""" query = mb_match.build_recording_query(artist, title) if not query: return [] @@ -7460,13 +7463,16 @@ def api_enrichment_pick(filename: str, data: dict = Body(...)): @app.get("/api/enrichment/search") def api_enrichment_search(artist: str = "", title: str = "", limit: int = 8, - filename: str = ""): + filename: str = "", duration: float = 0.0): """Manual-search proxy to MusicBrainz (throttled + identified like the background matcher — a user typing in the drawer must not sidestep the rate limit). `filename` optionally scores results against that song's stored identity (year/duration corroboration) instead of just the typed - text. Sync route on purpose: FastAPI runs it in the threadpool, so the - throttle's sleep never blocks the event loop.""" + text. `duration` (seconds) lets a caller that HAS the audio but no library + row — e.g. the editor's create modal, which holds the master track — pass + its length so the studio take ranks above live/extended cuts. Sync route on + purpose: FastAPI runs it in the threadpool, so the throttle's sleep never + blocks the event loop.""" if not (artist.strip() or title.strip()): raise HTTPException(status_code=400, detail="artist or title required") limit = max(1, min(int(limit), 25)) @@ -7480,6 +7486,10 @@ def api_enrichment_search(artist: str = "", title: str = "", limit: int = 8, ref = meta_db.enrichment_song_row(filename) if ref is None: ref = {"artist": artist, "title": title} + # A caller-supplied duration corroborates the take even without a library row. + if duration and duration > 0 and not ref.get("duration"): + ref = dict(ref) + ref["duration"] = duration return {"candidates": mb_match.rank_candidates(ref, cands)} diff --git a/tests/test_mb_match.py b/tests/test_mb_match.py index b2912c9..7f0da98 100644 --- a/tests/test_mb_match.py +++ b/tests/test_mb_match.py @@ -145,11 +145,40 @@ def test_rank_candidates_orders_by_our_score(): assert all("score" in c for c in ranked) +def test_rank_candidates_studio_preference_is_dropped_for_live_charts(): + """Tied-score candidates: a studio chart prefers the studio take, but a + LIVE chart must NOT be forced to the studio recording.""" + studio = {"recording_id": "studio", "artist": "AC/DC", "title": "Highway to Hell", + "studio": True, "mb_score": 90} + live = {"recording_id": "live", "artist": "AC/DC", "title": "Highway to Hell", + "studio": False, "mb_score": 95} + # Studio chart -> studio take wins the tie (studio flag), despite lower mb_score. + studio_song = {"artist": "AC/DC", "title": "Highway to Hell"} + assert m.rank_candidates(studio_song, [live, studio])[0]["recording_id"] == "studio" + # Live chart -> studio preference dropped, so the higher-mb_score live take wins. + live_song = {"artist": "AC/DC", "title": "Highway to Hell (Live at Donington)"} + assert m.rank_candidates(live_song, [studio, live])[0]["recording_id"] == "live" + + # ── query building ──────────────────────────────────────────────────────────── def test_build_recording_query_denoises_and_quotes(): q = m.build_recording_query("ACDC", 'Thunderstruck (v2)') - assert q == 'recording:"thunderstruck" AND artist:"acdc"' + # Live-only recordings are excluded — the studio take is never tagged Live, + # and it's the biggest source of junk in a flat recording search. + assert q == 'recording:"thunderstruck" AND artist:"acdc" AND -secondarytype:Live' + + +def test_build_recording_query_keeps_live_for_live_charts(): + """A chart that IS a live take must NOT get the live filter, or its only + correct recording is excluded. A bare title word ("Live and Let Die") is a + real word, not a marker, so it still filters.""" + live = m.build_recording_query("AC/DC", "Highway to Hell (Live at Donington)") + assert "-secondarytype:Live" not in live + assert 'recording:"highway to hell"' in live + # A real word "live" in the title is not a live marker → still filtered. + bare = m.build_recording_query("Wings", "Live and Let Die") + assert "-secondarytype:Live" in bare def test_build_recording_query_escapes_and_handles_missing_artist(): @@ -200,6 +229,29 @@ def test_parse_recording_doc_normalizes(): assert c["mb_score"] == 98 +def test_best_release_prefers_official_single_over_unofficial_album(): + """An OFFICIAL single/EP must outrank an UNofficial bootleg album for the + canonical album/year: official comes before the studio-album preference, so + a single-only song is never seeded from a bootleg. (`(clean, status_ok, …)` + would wrongly pick the bootleg.)""" + doc = { + "id": "rec-x", "title": "One-Off", "score": 90, + "artist-credit": [ + {"name": "A", "joinphrase": "", + "artist": {"id": "a", "name": "A", "sort-name": "A"}}], + "releases": [ + {"id": "rel-boot", "title": "Boot LP", "status": "Bootleg", + "date": "1990-01-01", "release-group": {"primary-type": "Album"}}, + {"id": "rel-single", "title": "The Single", "status": "Official", + "date": "1988-01-01", "release-group": {"primary-type": "Single"}}, + ], + } + c = m.parse_recording_doc(doc) + assert c["release_id"] == "rel-single" + assert c["album"] == "The Single" + assert c["studio"] is False # a Single isn't a clean studio ALBUM + + def test_parse_recording_doc_joined_artist_credit(): doc = dict(MB_DOC) doc["artist-credit"] = [ From 73c5ab149e970f31d675bcc0bebc268b5250f93d Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:17:43 -0500 Subject: [PATCH 5/8] feat(enrichment): AcoustID audio-fingerprint identification (opt-in) (#759) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(enrichment): AcoustID audio-fingerprint identification (opt-in) Text search can only guess the version; the definitive fix is content-based — fingerprint the actual audio with Chromaprint (fpcalc) and look it up on AcoustID, which maps the fingerprint to the EXACT MusicBrainz recording (the approach Lidarr uses). Sidesteps the studio-vs-live ambiguity entirely. - lib/acoustid_match.py: pure response parsing + config gating (unit-tested); normalizes AcoustID hits into the same candidate shape as mb_match so the review UI + editor Match popup render fingerprint and text hits identically. - server.py: _fpcalc (Chromaprint subprocess), _acoustid_lookup (throttled, offline-guarded HTTP), _identify_by_fingerprint (also available to the library-enrichment pipeline), and POST /api/enrichment/identify (upload the master audio → candidates). - Fully OPT-IN and graceful: absent the fpcalc binary or an ACOUSTID_API_KEY the whole path is a no-op / 503 and the text matcher runs unchanged. Requires (both optional): the `fpcalc` (Chromaprint) binary on PATH/$FPCALC, and a free AcoustID application key in $ACOUSTID_API_KEY. Pure parsing/gating is unit-tested; the fpcalc + live-lookup path needs those two to exercise. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(enrichment): make AcoustID self-serve — opt-in toggle + API key in settings Fingerprinting was env-var only (ACOUSTID_API_KEY), so only an operator could enable it. Add two core settings so a user can turn it on themselves: - acoustid_enabled (bool, default OFF — opt-in) - acoustid_api_key (string, ≤128 chars, trimmed; env var stays a fallback) _acoustid_available()/_acoustid_lookup() now resolve (enabled, key) from settings via _acoustid_settings(). /api/enrichment/identify distinguishes "not set up" (412 needs_setup — the UI nudges the user to enable it) from "set up but fpcalc/network missing" (503) so the client never fakes a match. Verified: default off; POST round-trips + trims; 412 vs 503 gating; bad types/over-length rejected. acoustid_match unit tests green (8/8). * fix(enrichment): POST the AcoustID lookup instead of GET A Chromaprint fingerprint is multi-KB (a 3.5-min track ≈ 3.5k chars), so sending it as a GET query param overflows the request URL for longer songs and fails spuriously. AcoustID accepts the same params form-encoded — POST them. * fix(acoustid): space-separate the lookup `meta` (was silently dropping metadata) The meta value was `+`-joined ("recordings+releasegroups+compress"). Sent over the wire the literal `+` percent-encodes to %2B, which AcoustID does NOT split into flags — so every hit came back with an empty `recordings` array and the parser produced zero candidates (a fingerprint match that resolved to nothing). AcoustID wants the flags space-separated. Verified against real fingerprints: `+`-joined → 0 recordings; space-joined → 28, resolving Highway to Hell and Living After Midnight to their canonical studio albums as the top hit. * feat(acoustid): resolve the canonical original album + year from the fingerprint AcoustID hits resolved the right recording but a weak album/blank year: the album picker took the first studio-typed group (a later comp/soundtrack typed "Album" could win) and the year took an arbitrary release (often a reissue). Request the `releases` meta (which carries per-release dates) and use them to (1) pick the EARLIEST original studio album among the groups and (2) fill the year from that album's earliest release. Verified against real fingerprints: Smoke on the Water → Machine Head (1972) not a later comp; Highway to Hell → 1979; Living After Midnight → British Steel (1980). +2 unit tests. * feat(acoustid): per-song "Identify by audio" for the library metadata tooling Add POST /api/enrichment/identify/{filename} — fingerprints an EXISTING library song's own master audio (resolves the sloppak's original_audio or a loose folder's audio), the library counterpart to the upload-based /identify used by the editor. Wire an "Identify by audio" action into the match-review / Fix-match modal: it renders fingerprint hits in the same candidate list and pins the pick via the existing /review/{f}/pick. Shared _acoustid_gate() (412 needs_setup / 503) for both endpoints; 404 when a pack has no full mix. Both identify routes added to the demo-mode block list (they spend fpcalc + the AcoustID budget) — fixes a pre-existing miss on the upload route. * fix(acoustid): regenerate stale tailwind CSS + cap identify upload - static/tailwind.min.css was stale vs a fresh rebuild (ci/tailwind-fresh red); regenerated with the pinned tailwindcss@3.4.19 (byte-stable). - /api/enrichment/identify read the whole multipart upload into memory before writing it; stream it to the temp file with a 256 MB cap (413 over) so an oversized upload can't balloon RAM. fpcalc reads from the temp file anyway. Co-Authored-By: Claude Opus 4.8 (1M context) * feat(acoustid): pre-parse upload guard + settings UI to enable it - /api/enrichment/identify is now async: a pre-parse Content-Length check + request.form(max_part_size=…) reject an oversized body BEFORE Starlette spools the multipart to temp disk (mirrors the song-upload endpoint), and the blocking fpcalc subprocess + AcoustID HTTP run off the event loop via run_in_executor. - The v3 Metadata-matching settings card gains an 'Identify by audio' opt-in toggle (acoustid_enabled, default OFF) + an AcoustID key input (acoustid_api_key), wired in match-review.js — so the advertised feature is reachable from the UI instead of only via a manual settings POST. Reuses existing classes only; committed tailwind.min.css stays fresh. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- lib/acoustid_match.py | 152 ++++++++++++++++++ server.py | 290 ++++++++++++++++++++++++++++++++++- static/v3/index.html | 7 + static/v3/match-review.js | 55 ++++++- tests/test_acoustid_match.py | 120 +++++++++++++++ 5 files changed, 622 insertions(+), 2 deletions(-) create mode 100644 lib/acoustid_match.py create mode 100644 tests/test_acoustid_match.py diff --git a/lib/acoustid_match.py b/lib/acoustid_match.py new file mode 100644 index 0000000..f5027b6 --- /dev/null +++ b/lib/acoustid_match.py @@ -0,0 +1,152 @@ +"""AcoustID audio-fingerprint identification for MusicBrainz enrichment. + +A flat MusicBrainz *text* search ties every take of a song at the same score — +studio, a dozen live bootlegs, and every compilation — so "AC/DC — Highway to +Hell" returns junk (see lib/mb_match.py's canonical re-ranking, which mitigates +it). The definitive fix is content-based: fingerprint the actual audio with +Chromaprint (`fpcalc`) and look it up on AcoustID, which maps the fingerprint +straight to the *exact* MusicBrainz recording — the same approach Lidarr uses. + +This module is the PURE half (no network, no subprocess): response parsing + +config gating, so it is unit-testable in isolation. server.py owns the `fpcalc` +subprocess and the throttled HTTP GET to api.acoustid.org. + +Operational requirements (both optional — absent ⇒ this path is a graceful +no-op and the text matcher still runs): + * `fpcalc` (Chromaprint) on PATH or at $FPCALC — generates the fingerprint. + * an AcoustID application API key in $ACOUSTID_API_KEY — free from + https://acoustid.org/new-application ; AcoustID etiquette limits to ~3 req/s. +""" + +import os + +ACOUSTID_API_ROOT = "https://api.acoustid.org/v2" + +# The `meta` fields we ask AcoustID to return so a hit resolves to displayable +# metadata without a second MusicBrainz round-trip. SPACE-separated, not +# `+`-joined: a literal `+` in the value gets percent-encoded to %2B, which +# AcoustID does NOT split into flags — it then attaches no recording metadata +# and every hit comes back empty (verified: `+` → 0 recordings, space → 28). +# `releases` is what carries the per-release DATE (nested under each +# releasegroup), which we need to pick the earliest original album + fill year. +LOOKUP_META = "recordings releasegroups releases compress" + +# Mirror mb_match._SECONDARY_SKIP: release-group secondary types that mark a +# non-canonical (live/comp/remix) release, so we can flag the studio take. +_SECONDARY_SKIP = { + "live", "compilation", "remix", "dj-mix", "mixtape/street", + "demo", "interview", "audiobook", "spokenword", +} + + +def api_key(explicit: str | None = None) -> str: + """The AcoustID application API key: an explicit value (e.g. a host setting) + wins, else $ACOUSTID_API_KEY, else "" (⇒ fingerprinting disabled).""" + return (explicit or os.environ.get("ACOUSTID_API_KEY") or "").strip() + + +def is_configured(explicit_key: str | None = None) -> bool: + """True when an API key is available. `fpcalc` presence is checked by + server.py (it owns the binary lookup); both are required to actually run.""" + return bool(api_key(explicit_key)) + + +def _rg_is_studio(rg: dict) -> bool: + if str(rg.get("type", "")).lower() != "album": + return False + secs = {str(s).lower() for s in (rg.get("secondarytypes") or [])} + return not (secs & _SECONDARY_SKIP) + + +def _rg_earliest_year(rg: dict) -> "int | None": + """Earliest release YEAR in a release-group (min over its nested releases' + dates). None when no release carries a date. This is what separates the + original pressing from later reissues/comps sharing the same group.""" + years = [] + for rel in (rg.get("releases") or []): + d = (rel or {}).get("date") + if isinstance(d, dict) and d.get("year"): + try: + years.append(int(d["year"])) + except (TypeError, ValueError): + pass + return min(years) if years else None + + +def _best_group(recording: dict) -> dict: + """Pick the display album: a clean studio Album first, and among those the + EARLIEST-released one — the original, not a later reissue or a compilation + that happens to be typed 'Album' (e.g. a soundtrack). This is what pulls + "Machine Head" ahead of a later comp for "Smoke on the Water". Falls back to + the first group when nothing is a studio album or nothing carries a date.""" + groups = [g for g in (recording.get("releasegroups") or []) if isinstance(g, dict)] + if not groups: + return {} + + def sort_key(g): + yr = _rg_earliest_year(g) + # studio (0) before non-studio (1); then earliest year (undated last). + return (0 if _rg_is_studio(g) else 1, yr if yr is not None else 9999) + + return sorted(groups, key=sort_key)[0] + + +def _first_artist(recording: dict) -> str: + for a in (recording.get("artists") or []): + if isinstance(a, dict) and a.get("name"): + return str(a["name"]) + return "" + + +def parse_lookup_response(body: dict) -> list[dict]: + """Normalize an AcoustID /v2/lookup response into the same flat candidate + shape as mb_match (recording_id / title / artist / album / year / duration / + studio / mb_score / score), so the review UI and the editor's Match popup + render fingerprint hits and text hits identically. `mb_score` carries the + AcoustID confidence (0-100) — a fingerprint hit is high-signal by nature.""" + if not isinstance(body, dict) or body.get("status") != "ok": + return [] + out: list[dict] = [] + seen: set[str] = set() + for result in (body.get("results") or []): + if not isinstance(result, dict): + continue + try: + score = float(result.get("score") or 0.0) + except (TypeError, ValueError): + score = 0.0 + for rec in (result.get("recordings") or []): + if not isinstance(rec, dict) or not rec.get("id"): + continue + rid = str(rec["id"]) + if rid in seen: + continue + seen.add(rid) + rg = _best_group(rec) + _yr = _rg_earliest_year(rg) + year = str(_yr) if _yr else "" + dur = rec.get("duration") + try: + duration = int(round(float(dur))) if dur else None + except (TypeError, ValueError): + duration = None + out.append({ + "recording_id": rid, + "title": str(rec.get("title", "") or ""), + "artist": _first_artist(rec), + "album": str(rg.get("title", "") or ""), + "year": year, + "duration": duration, + "isrc": "", + "genres": [], + "studio": _rg_is_studio(rg), + "acoustid_score": round(score, 4), + # Fingerprint hits are content-verified, not text-guessed — carry + # the AcoustID confidence as the display score band. + "mb_score": int(round(score * 100)), + "score": round(score, 4), + "source": "acoustid", + }) + # Best AcoustID confidence first; studio take breaks ties. + out.sort(key=lambda c: (c["acoustid_score"], 1 if c["studio"] else 0), reverse=True) + return out diff --git a/server.py b/server.py index c3d4bd3..3118317 100644 --- a/server.py +++ b/server.py @@ -57,6 +57,7 @@ import loosefolder as loosefolder_mod # tier classification + response parsing. No network/DB in there — the # throttled transport and the song_enrichment writes live in this module. import mb_match +import acoustid_match # Metadata extraction lives in a side-effect-free module so ProcessPool # scan workers can import + unpickle _scan_one without re-running this # module's import-time side effects (see lib/scan_worker.py). @@ -246,6 +247,11 @@ _DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [ ("POST", re.compile(r"^/api/enrichment/review/.+$")), ("POST", re.compile(r"^/api/enrichment/kick$")), ("GET", re.compile(r"^/api/enrichment/search$")), + # AcoustID audio fingerprinting: both identify endpoints run fpcalc (CPU) + # and spend the shared AcoustID rate budget on the caller's behalf — same + # rule as the search/kick relays above; not for anonymous demo visitors. + ("POST", re.compile(r"^/api/enrichment/identify$")), + ("POST", re.compile(r"^/api/enrichment/identify/.+$")), # Context menus (R2): the per-song re-match mutates the cache + spends # rate limit; Get-info exposes filesystem paths. ("POST", re.compile(r"^/api/enrichment/refresh/.+$")), @@ -6294,6 +6300,179 @@ def _mb_search_recordings(artist, title, limit: int = 12) -> list[dict]: return mb_match.parse_search_response(body or {}) +# ── AcoustID audio fingerprinting (content-based identification) ────────────── +# Optional path: requires the Chromaprint `fpcalc` binary AND an AcoustID API +# key ($ACOUSTID_API_KEY). Both absent ⇒ graceful no-op; the text matcher runs. + +_ACOUSTID_MAX_UPLOAD_BYTES = 256 * 1024 * 1024 # 256 MB — an uncompressed master + + +def _fpcalc_bin() -> str | None: + """Locate the Chromaprint `fpcalc` binary: $FPCALC override, else PATH.""" + import shutil + cand = os.environ.get("FPCALC") + if cand and Path(cand).exists(): + return cand + return shutil.which("fpcalc") + + +def _acoustid_settings() -> "tuple[bool, str]": + """(enabled, api_key) for AcoustID, resolved from settings with an env-var + fallback for the key. Opt-in: `acoustid_enabled` defaults off. The key lives + in settings so a user can set it themselves in the UI; $ACOUSTID_API_KEY is a + server-wide fallback for a headless deploy.""" + cfg = _load_config(CONFIG_DIR / "config.json") or {} + enabled = cfg.get("acoustid_enabled", False) is True + key = cfg.get("acoustid_api_key") + if not isinstance(key, str) or not key.strip(): + key = os.environ.get("ACOUSTID_API_KEY", "") + return enabled, (key or "").strip() + + +def _acoustid_available() -> bool: + """True only when the user opted in, a key is set (settings or env), the + network is on, AND fpcalc exists.""" + enabled, key = _acoustid_settings() + return (enabled + and _enrich_network_enabled() + and acoustid_match.is_configured(key) + and _fpcalc_bin() is not None) + + +def _fpcalc(path: str) -> "tuple[int, str] | None": + """Fingerprint a local audio file → (duration_seconds, fingerprint). None on + any failure (missing binary/file, decode error, timeout).""" + binp = _fpcalc_bin() + if not binp or not Path(path).exists(): + return None + import subprocess + import json as _json + try: + pr = subprocess.run([binp, "-json", str(path)], + capture_output=True, timeout=30) + except Exception: + return None + if pr.returncode != 0: + return None + try: + data = _json.loads(pr.stdout.decode("utf-8", "replace")) + dur = int(round(float(data.get("duration")))) + fp = str(data.get("fingerprint") or "") + except Exception: + return None + if not fp or dur <= 0: + return None + return dur, fp + + +def _acoustid_lookup(duration: int, fingerprint: str) -> list[dict]: + """Look a fingerprint up on AcoustID → candidate dicts (mb_match shape). + Throttled + offline-guarded like the MusicBrainz path. [] when unavailable + or no hit; raises EnrichTransportError for network-shaped failures.""" + _, key = _acoustid_settings() + if not key or not _enrich_network_enabled(): + return [] + import requests + _enrich_throttle() + try: + # POST, not GET: a fingerprint is multi-KB (a 3.5-min track is ~3.5k + # chars), so a GET crams it into the URL and a long song overflows the + # server's URL limit → a spurious failure. AcoustID accepts the same + # params form-encoded in the body. + resp = requests.post( + f"{acoustid_match.ACOUSTID_API_ROOT}/lookup", + data={ + "client": key, "format": "json", + "meta": acoustid_match.LOOKUP_META, + "duration": duration, "fingerprint": fingerprint, + }, + headers={"User-Agent": _enrich_user_agent()}, + timeout=10, + ) + except requests.RequestException as e: + raise EnrichTransportError(str(e)) from e + if resp.status_code == 429: + raise EnrichTransportError("acoustid 429 (rate limited)") + if resp.status_code != 200: + raise EnrichTransportError(f"acoustid HTTP {resp.status_code}") + try: + body = resp.json() + except ValueError as e: + raise EnrichTransportError("bad JSON from acoustid") from e + return acoustid_match.parse_lookup_response(body) + + +def _identify_by_fingerprint(path: str) -> list[dict]: + """fpcalc + AcoustID lookup for a local audio file. [] if fingerprinting is + unavailable, the file can't be read, or nothing matched. Available to the + library-enrichment pipeline as well as the /identify endpoint.""" + if not _acoustid_available(): + return [] + fp = _fpcalc(path) + if not fp: + return [] + return _acoustid_lookup(fp[0], fp[1]) + + +def _acoustid_gate() -> "JSONResponse | None": + """Shared availability gate for the identify endpoints: None when ready, + else a 412 needs_setup (opt-in off / no key → the UI re-prompts) or a 503 + (set up but fpcalc/network missing). Never lets a caller pretend a + fingerprint ran.""" + if _acoustid_available(): + return None + enabled, key = _acoustid_settings() + if not enabled or not key: + return JSONResponse( + {"error": "audio fingerprinting not set up", "needs_setup": True, + "detail": "Turn on AcoustID and add a free API key to identify by audio — " + "it reads the recording itself, far more reliable than text search."}, + status_code=412) + return JSONResponse( + {"error": "audio fingerprinting unavailable", "needs_setup": False, + "detail": "the fpcalc (Chromaprint) binary was not found on the server"}, + status_code=503) + + +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.""" + dlc = _get_dlc_dir() + if not dlc: + return None + resolved = _resolve_dlc_path(dlc, filename) + if resolved is None or not resolved.exists(): + return None + if sloppak_mod.is_sloppak(resolved): + try: + canon = resolved.relative_to(dlc.resolve()).as_posix() + except ValueError: + return None + rel = (sloppak_mod.load_manifest(resolved) or {}).get("original_audio") + if not isinstance(rel, str) or not rel.strip(): + return None + src = sloppak_mod.get_cached_source_dir(canon) + if src is None: + try: + src = sloppak_mod.resolve_source_dir(canon, dlc, SLOPPAK_CACHE_DIR) + except Exception: + return None + target = (src / rel.strip()).resolve() + try: + target.relative_to(src.resolve()) + except ValueError: + return None + return str(target) if target.is_file() else None + try: + audio = loosefolder_mod.find_audio(resolved) + except Exception: + audio = None + return str(audio) if audio and Path(str(audio)).is_file() else None + + def _mb_lookup_recording(mbid: str) -> dict | None: """Direct lookup for a manifest-carried recording MBID (tier 0).""" body = _mb_http_get( @@ -7493,6 +7672,93 @@ def api_enrichment_search(artist: str = "", title: str = "", limit: int = 8, return {"candidates": mb_match.rank_candidates(ref, cands)} +@app.post("/api/enrichment/identify") +async def api_enrichment_identify(request: Request): + """Identify a song by AUDIO FINGERPRINT (AcoustID) rather than text — the + reliable way to get the EXACT recording/version (the studio take, not a live + bootleg or an extended cut). Upload the master audio; returns candidates in + the same shape as /search, so the review UI and the editor's Match popup can + render fingerprint hits identically. 412 `needs_setup` when the user hasn't + opted in / has no key (the UI nudges them to Settings); 503 when it's set up + but the fpcalc Chromaprint binary is missing or the network is off. Async so + the multipart is size-capped BEFORE spooling; the blocking fpcalc subprocess + + AcoustID HTTP run in the threadpool via run_in_executor.""" + gate = _acoustid_gate() + if gate is not None: + return gate + # Pre-parse Content-Length guard — reject an oversized body before Starlette + # spools the multipart to temp disk (mirrors the song-upload endpoint). The + # per-part cap below is the authoritative limit; this is the fast up-front no. + cl = request.headers.get("content-length") + if cl is not None: + try: + cl_int = int(cl) + except ValueError: + return JSONResponse({"error": "Invalid Content-Length header"}, status_code=400) + if cl_int > _ACOUSTID_MAX_UPLOAD_BYTES + _MULTIPART_OVERHEAD_SLACK: + return JSONResponse({"error": "audio upload too large (256 MB max)"}, status_code=413) + try: + form = await request.form(max_part_size=_ACOUSTID_MAX_UPLOAD_BYTES) + except Exception: + return JSONResponse({"error": "audio upload too large (256 MB max)"}, status_code=413) + file = form.get("file") + if not isinstance(file, UploadFile): + raise HTTPException(status_code=400, detail="missing file upload") + import tempfile + ext = (Path(file.filename or "").suffix or ".bin").lower() + tmpdir = tempfile.mkdtemp(prefix="feedback_acoustid_") + tmp = os.path.join(tmpdir, "audio" + ext) + try: + total = 0 + with open(tmp, "wb") as fh: + while True: + chunk = await file.read(1024 * 1024) + if not chunk: + break + total += len(chunk) + if total > _ACOUSTID_MAX_UPLOAD_BYTES: + return JSONResponse( + {"error": "audio upload too large (256 MB max)"}, status_code=413) + fh.write(chunk) + if total == 0: + raise HTTPException(status_code=400, detail="empty upload") + # fpcalc subprocess + AcoustID HTTP are blocking — off the event loop. + cands = await asyncio.get_event_loop().run_in_executor( + None, _identify_by_fingerprint, tmp) + except EnrichTransportError as e: + return JSONResponse({"error": "acoustid unavailable", "detail": str(e)}, + status_code=503) + finally: + shutil.rmtree(tmpdir, ignore_errors=True) + return {"candidates": cands} + + +@app.post("/api/enrichment/identify/{filename:path}") +def api_enrichment_identify_song(filename: str): + """Identify an EXISTING library song by AUDIO FINGERPRINT — the library-side + counterpart to /api/enrichment/identify (which takes an upload). Fingerprints + the song's own master audio on disk (the manual "Identify by audio" action in + the Fix-metadata / match-review flow). Same candidate shape as /search, so the + review UI renders fingerprint hits like text hits. Same 412/503 gating; 404 + when the song has no full-mix audio to fingerprint.""" + gate = _acoustid_gate() + if gate is not None: + return gate + audio = _song_audio_file(filename) + if not audio: + return JSONResponse( + {"error": "no audio", + "detail": "couldn't find this song's master audio to fingerprint " + "(a stems-only pack has no full mix to identify)."}, + status_code=404) + try: + cands = _identify_by_fingerprint(audio) + except EnrichTransportError as e: + return JSONResponse({"error": "acoustid unavailable", "detail": str(e)}, + status_code=503) + return {"candidates": cands} + + @app.get("/api/startup-status") def startup_status(): return _get_startup_status() @@ -9839,6 +10105,17 @@ def _default_settings(): # the external browser, never media delivered in-app. "artist_pages_enabled": True, "artist_external_links": False, + # Audio fingerprinting (AcoustID + Chromaprint). OPT-IN, default OFF. + # Text matching (MusicBrainz) can't reliably pick the exact recording + # for a song with many comp/live/reissue takes (especially a + # non-title-track — the title can't find the album); fingerprinting + # reads the audio itself and resolves the EXACT recording. Needs the + # user's own free AcoustID application key + # (https://acoustid.org/new-application) plus the `fpcalc` binary. The + # key lives here (settings) — not only an env var — so a user can set it + # themselves in the UI; $ACOUSTID_API_KEY stays a server-wide fallback. + "acoustid_enabled": False, + "acoustid_api_key": "", } @@ -10013,13 +10290,24 @@ def save_settings(data: dict): "enrich_apply_names", "enrich_apply_year", "enrich_apply_genres", "enrich_apply_art", # Artist pages (PR-B): page on/off + external-links opt-in. - "artist_pages_enabled", "artist_external_links"): + "artist_pages_enabled", "artist_external_links", + # AcoustID audio-fingerprinting opt-in (default off). + "acoustid_enabled"): if _bool_key in data: raw = data[_bool_key] if raw is not None: if not isinstance(raw, bool): return {"error": f"{_bool_key} must be a boolean"} updates[_bool_key] = raw + if "acoustid_api_key" in data: + # Free AcoustID application key (opaque token). null is a no-op, empty + # string clears; length-capped so a bad POST can't bloat config.json. + # Never logged. The matcher trims + validates presence at read time. + raw = data["acoustid_api_key"] + if raw is not None: + if not isinstance(raw, str) or len(raw) > 128: + return {"error": "acoustid_api_key must be a string (at most 128 chars)"} + updates["acoustid_api_key"] = raw.strip() if "enrich_review_order" in data: raw = data["enrich_review_order"] if raw is not None: diff --git a/static/v3/index.html b/static/v3/index.html index 2196535..32aee83 100644 --- a/static/v3/index.html +++ b/static/v3/index.html @@ -788,6 +788,13 @@
What a confident match may fill in on its own — matches you confirm in the review queue always apply in full.
+ +
+
Audio fingerprint (AcoustID)
+ + +
Opt-in. Get a free key at acoustid.org/new-application; the fpcalc (Chromaprint) binary must be on the server's PATH.
+
' + // Practice-aware library home: a repertoire progress meter + a @@ -3450,6 +3482,7 @@ state.artist = ''; state.album = ''; try { sm.libraryProviders && await sm.libraryProviders.select(state.provider); } catch (err) { /* */ } + _updateMetaBtnVisibility(); // enrichment is local-only await loadArtistCatalog(); refreshArtistAlbumSelects(); reload(); @@ -3479,6 +3512,10 @@ }); byId('v3-songs-select').addEventListener('click', () => setSelectMode(!state.selectMode)); byId('v3-songs-refresh')?.addEventListener('click', refreshLibrary); + // Refresh Metadata: local-only, so hide it for remote providers. The + // button doubles as its own Stop while a pass runs (see onMetaBtnClick). + byId('v3-songs-refresh-meta')?.addEventListener('click', onMetaBtnClick); + _updateMetaBtnVisibility(); // Reflect a scan already in progress (Settings button or a background // pass) on the Refresh button, so its state isn't just tied to clicks here. (async () => { @@ -3488,6 +3525,15 @@ if (sd && sd.running) { _setRefreshState(sd); _watchScan({ announce: false }); } } catch (e) { /* */ } })(); + // Reflect an enrichment pass already running (Settings "Match now" or a + // post-scan background pass) on the Metadata button + bar. + (async () => { + try { + const r = await fetch('/api/enrichment/status'); + const es = r.ok ? await r.json() : null; + if (es && es.running) { _setMetaState(es); _watchEnrich({ announce: false }); } + } catch (e) { /* */ } + })(); // Capture-phase select-mode guard on each persistent list host. Without // it, clicking a card/row (or its arrangement chip) in select mode falls @@ -3706,6 +3752,187 @@ }, 1000); } + // ── Refresh Metadata (batch enrichment) from the Songs toolbar ───────────── + // The metadata counterpart to ⟳ Refresh (which scans FILES): matches + // titles/artist/album/artwork against MusicBrainz for the songs that still + // need it — the ambient background matcher, run on demand (a media-server's + // "Refresh Metadata" vs "Scan Files"). Mirrors the scan machinery: a 1 Hz + // poll of /api/enrichment/status drives the button + batch bar, while + // /api/enrichment/states drives per-tile badges on the visible window. + // Enrichment is local-only, so the button hides for remote providers. + let _metaPoll = null; + let _metaRunning = false; + + function _updateMetaBtnVisibility() { + const btn = document.getElementById('v3-songs-refresh-meta'); + if (btn) btn.style.display = (state.provider === 'local') ? '' : 'none'; + } + + // The local filenames the grid is currently SHOWING (data-fn is the local + // filename the enrichment cache keys on). The grid is windowed, so this is + // the visible slice only — exactly what the per-tile poll should cover. + function _visibleLocalFilenames() { + const grid = document.getElementById('v3-songs-grid'); + if (!grid) return []; + return [...grid.querySelectorAll('[data-fn]')] + .map((el) => el.getAttribute('data-fn')).filter(Boolean); + } + + // Set/clear one card's live badge (recycled cards re-derive from _metaTile on + // the next paint, so update the map too — mirrors _patchCardFav). + function _patchCardEnrich(fn, st) { + if (st) _metaTile[fn] = st; else delete _metaTile[fn]; + const sel = (window.CSS && CSS.escape) ? CSS.escape(fn) : fn; + document.querySelectorAll('[data-fn="' + sel + '"] [data-v3-play]').forEach((play) => { + const el = play.querySelector('.v3-meta-tile'); + const html = enrichBadge(fn); + if (!html) { if (el) el.remove(); return; } + if (el) el.outerHTML = html; else play.insertAdjacentHTML('beforeend', html); + }); + } + + function _clearMetaTiles() { + Object.keys(_metaTile).forEach((fn) => { delete _metaTile[fn]; }); + document.querySelectorAll('.v3-meta-tile').forEach((el) => el.remove()); + } + + + // Drive the button (which doubles as Stop) + the batch bar from a status body. + function _setMetaState(es) { + const btn = document.getElementById('v3-songs-refresh-meta'); + const prog = document.getElementById('v3-meta-progress'); + const fill = document.getElementById('v3-meta-progress-fill'); + const label = document.getElementById('v3-meta-progress-label'); + if (!btn) return; + const running = !!(es && es.running); + _metaRunning = running; + if (running) { + const total = (es && es.total) || 0, done = (es && es.matched) || 0; + const cancelling = !!(es && es.cancelling); + btn.textContent = cancelling ? 'Stopping…' : ('⏹ Stop' + (total ? ' · ' + done + '/' + total : '')); + btn.disabled = cancelling; + btn.classList.toggle('opacity-70', cancelling); + btn.title = cancelling ? 'Stopping after the current song…' : 'Stop refreshing metadata'; + if (prog) { + prog.classList.remove('hidden'); prog.classList.add('flex'); + if (label) label.textContent = total ? ('Matching metadata ' + done + '/' + total) : 'Matching metadata…'; + // Real songs-processed ratio; a tiny sliver while the queue size + // is still being computed (phase 1) so the bar isn't dead-empty. + if (fill) fill.style.width = (total ? Math.round((done / total) * 100) : 6) + '%'; + } + } else { + btn.textContent = '🏷 Metadata'; + btn.disabled = false; + btn.classList.remove('opacity-70'); + btn.title = 'Refresh metadata for the songs shown (re-match titles, artwork & more)'; + if (prog) { prog.classList.add('hidden'); prog.classList.remove('flex'); } + } + } + + // Completion toast — reuse the shared fbNotify surface (visual-only, so + // hearing-safe for free). Honest + never-punishing copy, in-game suppressed. + function _metaCompleteToast(es) { + const active = document.querySelector('.screen.active'); + if (active && active.id === 'player') return; + if (!window.fbNotify) return; + const matched = (es && es.matched) || 0; + const msg = matched + ? (matched + ' song' + (matched === 1 ? '' : 's') + ' matched') + : 'Your library metadata is up to date'; + try { window.fbNotify.show({ title: 'Metadata refresh complete', message: msg, icon: '🏷️', accent: '#22C55E' }); } catch (e) { /* */ } + } + + // Poll enrichment status (button + bar) AND the visible window's per-song + // states (tile badges) until the pass finishes. announce:false = we only + // attached to a pass we didn't start (no toast unless it actually changed + // something). + function _watchEnrich(opts) { + if (_metaPoll) return; + const announce = !opts || opts.announce !== false; + let sawRunning = false, ticks = 0, lastStatus = null; + _metaPoll = setInterval(async () => { + ticks++; + let es = null; + try { const r = await fetch('/api/enrichment/status'); if (r.ok) es = await r.json(); } catch (e) { /* */ } + if (es) { lastStatus = es; _setMetaState(es); if (es.running) sawRunning = true; } + // Per-tile badges: only songs we're tracking (seeded 'queued'). A + // tile flips to 'working' when it's the current song, then to + // 'done' (matched) / 'nochange' (failed) once it leaves unscanned. + if (Object.keys(_metaTile).length) { + const fns = _visibleLocalFilenames(); + try { + const r = await fetch('/api/enrichment/states', { + method: 'POST', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ filenames: fns }), + }); + if (r.ok) { + const j = await r.json(); + const states = j.states || {}, current = j.current; + fns.forEach((fn) => { + if (!(fn in _metaTile)) return; + if (fn === current) { _patchCardEnrich(fn, 'working'); return; } + const s = states[fn]; + if (s && s !== 'unscanned' && s !== 'pending') { + _patchCardEnrich(fn, s === 'failed' ? 'nochange' : 'done'); + } + }); + } + } catch (e) { /* */ } + } + // Cap at 20 min (a ~1000-song trickle at ≤1/s is ~17 min); a + // user-initiated no-op that never saw a running pass ends quickly. + const noopDone = announce && !sawRunning && ticks >= 3; + if ((sawRunning && es && !es.running) || noopDone || ticks >= 1200) { + clearInterval(_metaPoll); _metaPoll = null; + _setMetaState(null); + const changed = sawRunning && lastStatus && (lastStatus.matched || 0) > 0; + if (announce || changed) _metaCompleteToast(lastStatus); + // Let the final 'done' badges register, then clear + (if anything + // matched) reload so new canonical titles/art show. + setTimeout(() => { + _clearMetaTiles(); + if (changed && window.feedBack) { try { window.feedBack.emit('library:changed', { reason: 'enrich', matched: lastStatus.matched }); } catch (e) { /* */ } } + }, 1600); + } + }, 1000); + } + + // Force a fresh re-match of the songs currently SHOWN (the visible grid + // window) — a media-server-style per-view "Refresh Metadata". Resets those + // songs and re-fetches, so it's visible even on an already-matched library. + // Manual pins are skipped server-side; scoped to the visible set so it's + // fast + can't blow the whole rate budget. + async function refreshMetadata() { + if (_metaRunning || _metaPoll) return; // already running + const fns = _visibleLocalFilenames(); + _clearMetaTiles(); + if (!fns.length) { _metaCompleteToast({ matched: 0 }); return; } + let queued = []; + try { + const r = await fetch('/api/enrichment/rematch', { + method: 'POST', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ filenames: fns }), + }); + if (r.ok) queued = (await r.json()).queued || []; + } catch (e) { /* offline → nothing queued */ } + // Badge exactly what the server queued (everything visible except your + // manual pins). Nothing queued = all visible songs are pinned/unknown. + queued.forEach((fn) => _patchCardEnrich(fn, 'queued')); + if (!queued.length) { _metaCompleteToast({ matched: 0 }); return; } + _watchEnrich({ announce: true }); + } + + async function stopMetadata() { + try { await fetch('/api/enrichment/cancel', { method: 'POST' }); } catch (e) { /* */ } + _setMetaState({ running: true, cancelling: true }); // optimistic; the poll confirms + } + + // The Metadata button toggles role: kick a refresh when idle, Stop when a + // pass is running. + function onMetaBtnClick() { + if (_metaRunning) stopMetadata(); else refreshMetadata(); + } + // Topbar search drives this screen. async function search(q) { state.q = q || ''; diff --git a/tests/test_enrichment_plumbing.py b/tests/test_enrichment_plumbing.py index 6c1a290..eb88f77 100644 --- a/tests/test_enrichment_plumbing.py +++ b/tests/test_enrichment_plumbing.py @@ -150,3 +150,152 @@ def test_art_cache_dir_created(server): d = server._enrichment_art_dir() assert d.is_dir() assert d.name == "art_cache" + + +# ── Refresh Metadata batch: per-tile states, progress, Stop ─────────────────── + +def test_states_for_returns_only_known_filenames(server): + _put(server, "a.archive") + server._background_enrich() + got = server.meta_db.enrichment_states_for(["a.archive", "nope.archive"]) + assert got == {"a.archive": "unscanned"} # unknown filename absent + assert server.meta_db.enrichment_states_for([]) == {} + + +def test_states_endpoint(client, server): + _put(server, "a.archive") + _put(server, "b.archive", title="Other") + server._background_enrich() + body = client.post("/api/enrichment/states", + json={"filenames": ["a.archive", "zzz.missing"]}).json() + assert body["states"] == {"a.archive": "unscanned"} + assert body["running"] is False + assert body["current"] is None + + +def test_status_exposes_progress_fields(client, server): + _put(server, "a.archive") + server._background_enrich() + body = client.get("/api/enrichment/status").json() + for k in ("total", "matched", "current", "cancelling"): + assert k in body + assert body["cancelling"] is False + + +def test_cancel_is_noop_when_idle(client, server): + body = client.post("/api/enrichment/cancel").json() + assert body == {"ok": True, "was_running": False} + # A no-op must not arm the flag (which would then poison the next pass). + assert server._enrich_cancel.is_set() is False + + +def test_cancel_flag_halts_matching_loop_between_songs(server, monkeypatch): + for i in range(4): + _put(server, f"s{i}.archive", title=f"Song {i}") + # Force the matcher path on (the test env is offline by default) and stub the + # per-song matcher so nothing touches the network — it just trips Stop after + # the first song, exactly as the /cancel route would mid-pass. + monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True) + calls = [] + + def fake_enrich_one(row, **_kw): + calls.append(row["filename"]) + server._enrich_cancel.set() + + monkeypatch.setattr(server, "_enrich_one", fake_enrich_one) + server._enrich_cancel.clear() + server._background_enrich() + # The loop checks cancel BEFORE each song, so exactly one is processed before + # it breaks — not the whole 4-row queue. + assert calls == ["s0.archive"] + assert server._enrich_status["total"] == 4 + assert server._enrich_status["matched"] == 1 + + +def test_rematch_requeues_visible_but_skips_manual(server, client): + _put(server, "a.archive") # will be 'matched' + _put(server, "b.archive", title="Other") # will be 'failed' + _put(server, "c.archive", title="Pinned") # will be 'manual' — untouchable + server._background_enrich() + with server.meta_db._lock: + server.meta_db.conn.execute( + "UPDATE song_enrichment SET match_state='matched' WHERE filename='a.archive'") + server.meta_db.conn.execute( + "UPDATE song_enrichment SET match_state='failed' WHERE filename='b.archive'") + server.meta_db.conn.execute( + "UPDATE song_enrichment SET match_state='manual' WHERE filename='c.archive'") + server.meta_db.conn.commit() + body = client.post("/api/enrichment/rematch", json={ + "filenames": ["a.archive", "b.archive", "c.archive", "nope.archive"]}).json() + # A per-view refresh re-runs everything shown EXCEPT the manual pin (and an + # unknown filename); matched + failed are both re-queued. + assert set(body["queued"]) == {"a.archive", "b.archive"} + assert body["count"] == 2 + server._join_background_db_threads() + assert server.meta_db.get_enrichment("a.archive")["match_state"] == "unscanned" + assert server.meta_db.get_enrichment("b.archive")["match_state"] == "unscanned" + assert server.meta_db.get_enrichment("c.archive")["match_state"] == "manual" + + +# ── filename-derived artist/title fallback (blank-artist packs) ─────────────── + +def test_filename_artist_title_parse(server): + f = server._artist_title_from_filename + assert f("CDLC/0 - City Pop/Tatsuro-Yamashita_Ride-On-Time_v1_p.feedpak") == \ + {"artist": "Tatsuro Yamashita", "title": "Ride On Time"} + assert f("Anri_Windy-Summer_v1_p.feedpak") == {"artist": "Anri", "title": "Windy Summer"} + # a trailing "(440Hz)" retune tag is stripped before parsing + assert f("Cindy_Watashitachi-o-Shinjite-Ite_v1_p (440Hz).feedpak") == \ + {"artist": "Cindy", "title": "Watashitachi o Shinjite Ite"} + # doesn't fit the convention → no guess + assert f("nounderscore.feedpak") is None + + +def test_blank_artist_seeds_match_from_filename(server, monkeypatch): + server.meta_db.put("Tatsuro-Yamashita_Ride-On-Time_v1_p.feedpak", 0, 0, { + "title": "Tatsuro-Yamashita_Ride-On-Time_v1_p", "artist": "", "album": "", + "duration": 240, "arrangements": [{"name": "Bass", "index": 0}]}) + monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True) + monkeypatch.setattr(server, "_manifest_exact_ids", lambda fn: {}) + seen = {} + + def fake_search(artist, title, limit=8): + seen["artist"], seen["title"] = artist, title + return [] + + monkeypatch.setattr(server, "_mb_search_recordings", fake_search) + row = next(r for r in server.meta_db.enrichment_pending() + if r["filename"].startswith("Tatsuro")) + server._enrich_one(row) + # the blank pack artist was replaced by the filename-derived identity for + # the search (this is exactly what rescues the 'failed' pile) + assert seen == {"artist": "Tatsuro Yamashita", "title": "Ride On Time"} + + +def test_present_artist_is_not_overridden_by_filename(server, monkeypatch): + server.meta_db.put("Weird-Filename_x_y.feedpak", 0, 0, { + "title": "Real Title", "artist": "Real Artist", "album": "", "duration": 100, + "arrangements": [{"name": "Lead", "index": 0}]}) + monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True) + monkeypatch.setattr(server, "_manifest_exact_ids", lambda fn: {}) + seen = {} + + def fake_search(artist, title, limit=8): + seen["artist"], seen["title"] = artist, title + return [] + + monkeypatch.setattr(server, "_mb_search_recordings", fake_search) + row = next(r for r in server.meta_db.enrichment_pending() + if r["filename"].startswith("Weird")) + server._enrich_one(row) + # a pack that DOES carry an artist keeps it — the filename is never consulted + assert seen == {"artist": "Real Artist", "title": "Real Title"} + + +def test_kick_clears_a_stale_cancel(server): + # A cancelled-then-rekicked pass must start clean: _kick_enrich clears the + # flag so the fresh pass isn't aborted the instant it checks. + server._enrich_cancel.set() + server._kick_enrich() + server._join_background_db_threads() + assert server._enrich_cancel.is_set() is False From c7aa5a10b049fe3a9259bf9efbc28853960e20e3 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:19:46 -0500 Subject: [PATCH 7/8] fix(v3): recycle library grid cards on scroll instead of rebuilding the window (#742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The virtualized v3 Songs grid rebuilt its entire visible window (grid.innerHTML = _renderCardsRange(...) + a full wireCards pass) every time it slid by one row. Each row-boundary crossing was therefore a heavy synchronous frame — reparse ~60 cards, re-attach hundreds of listeners, reflow — that stalled the main thread and buffered held-arrow key-repeats, flushing them in a burst. Testers saw the library "go super fast for a second then slow down," skipping "every so many scrolls," up or down, at the same spots each time. It hitched scrolling back up over already-loaded songs too, because the cost was DOM teardown, not fetching. renderWindow() now reconciles the window in place: it reuses the card nodes that stay on-screen and builds only the row that enters/leaves (~6 nodes per slide instead of ~60). Nodes are keyed by absolute index (data-idx) with a real-vs-skeleton + select-mode signature (data-sig) so hole-fills after a page fetch and select-mode toggles still rebuild exactly the nodes that changed. wireCards()'s existing data-wired guard then wires only the freshly-built nodes, so per-slide listener churn drops with it. Everything keyed off data-fn (favorites, ⋮ menu, right-click, selection, accuracy badges, A–Z rail) is unaffected. Follow-up to the stage-2 virtualized grid (#636 item 3). Frontend-only. Tests: tests/js/v3_songs_window_recycle.test.js — window stays [start,end) contiguous and in-window node identity is reused across a down-then-up scroll; select-mode toggle and a rail-seek jump rebuild correctly. Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: byrongamatos --- CHANGELOG.md | 1 + static/v3/songs.js | 56 ++++++++- tests/js/v3_songs_window_recycle.test.js | 143 +++++++++++++++++++++++ 3 files changed, 194 insertions(+), 6 deletions(-) create mode 100644 tests/js/v3_songs_window_recycle.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b44814..2cd1009 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Player frame-time hotspots removed (trace-backed) + weak-hardware hardening.** A Chrome performance trace of a 3D-highway session surfaced two core per-frame layout-thrash sources, now fixed: the highway's visibility check read `canvas.offsetParent` every rAF frame (forces style/layout recalc — now sampled every 10th frame with a cached value, force-refreshed on init/canvas-replace/resize/override-clear), and the v3 player chrome loop called `matches(':hover')` per frame and unconditionally rewrote the Up-Next pill's `textContent`/bar width at 6 Hz (now hover-tracked via mouseenter/mouseleave, DOM writes only on value change, progress bar moved from `width` to compositor-only `scaleX`). The 3D highway pre-warms shader programs (`ren.compile`) and deterministic label textures at init — and chart-dependent chord/section label textures on first draw — so first-appearance shader-compile/texture-upload frame spikes move into the load spinner. For weaker hardware: the per-frame renderer bundle is now a single reused object instead of a fresh ~35-field allocation per frame (object identity is stable and meaningless; array fields still swap reference on chart changes), custom viz get `bundle.lowerBoundT`/`bundle.lowerBoundTime` binary-search helpers for visible-window culling, the default 2D highway's beat lines no longer scan every beat in the song per frame, and the 3D highway stops reading `localStorage` per frame (1 Hz poll) and caches its lyrics text-measurement layout per displayed line instead of re-measuring every syllable every frame. A second, throttled-CPU trace pass additionally removed: shader-program re-resolution churn from label texture swaps (`material.needsUpdate` is now only set on a null↔texture transition — swapping between two cached label textures never changes the compiled program), the 3D highway's per-frame `getBoundingClientRect` layout read in its canvas-size self-check (now every 10th frame, still immediate on backing-store change), and the core 60 Hz HUD clock rewriting `textContent` on every tick (now write-on-change, ~1/s). The dominant residual — steady `getParameters` shader-program re-resolution (~4% of throttled main thread) — turned out to be Three r158+'s transparent-DoubleSide two-pass rendering, which sets `material.needsUpdate` twice per object per frame; all 18 of the 3D highway's transparent DoubleSide materials are flat unlit quads (labels, rails, chord frames, lanes), so they now declare `forceSinglePass: true`, eliminating the recompile churn and halving those objects' draw calls. ### Fixed +- **v3 Songs grid: fixed the scroll stutter that "skips every so many scrolls," up or down.** The virtualized grid rebuilt its **entire** visible window (`grid.innerHTML = …` + a full `wireCards` pass) every time it slid by one row, so each row-boundary crossing was a heavy synchronous frame that stalled the main thread and buffered held-arrow key-repeats into a visible lurch (a tester's "super fast for a second then slowed down") at fixed scroll offsets — in **both directions and regardless of whether the page was already loaded** (the cost was DOM teardown, not fetching, which is why scrolling back up over cached songs hitched too). `renderWindow()` now **reconciles the window in place**: it reuses the card nodes that stay on-screen and builds only the row that enters/leaves (~6 nodes per slide instead of ~60), keyed by absolute index with a real-vs-skeleton + select-mode signature so hole-fills (after a page fetch) and select-mode toggles still rebuild exactly the nodes that changed. `wireCards`'s `data-wired` guard then wires only the freshly-built nodes, so per-slide listener churn drops with it. Follow-up to the stage-2 virtualized grid (got-feedback/feedBack#636 item 3). Frontend-only: `static/v3/songs.js`. Tests: `tests/js/v3_songs_window_recycle.test.js` (window stays `[start,end)` contiguous + in-window node identity reused across a down-then-up scroll; select-mode toggle and rail-seek jump rebuild correctly). - **Starter content seeds again (and now ships The Adicts' "Ode to Joy").** `_BUILTIN_STARTER_SOURCES` still listed `beethoven-ode_to_joy.feedpak` after that pack was deleted, and never wired up its replacement `the_adicts-ode-to-joy_vst_cover.feedpak` that landed on disk. The listed-but-missing file made the all-present gate never fire, so **no** starter songs seeded on first run. Synced the manifest to what's on disk (Für Elise, Star Spangled Banner, The Adicts' Ode to Joy). Tests: `tests/test_builtin_starter_seed.py` (the present/unlisted guards were red on `main`). - **Edit Metadata now writes into `.feedpak` files, not just legacy `.sloppak` ones.** `lib/songmeta.py`'s suffix gate predated the format rename — core reads both suffixes everywhere else (`sloppak.SONG_EXTS`), but the metadata writer only dispatched on `.sloppak`, so editing a zip-form `.feedpak`'s title/artist/album/year silently fell back to a DB-only update. That looked fine until the next **full library rescan** re-derived metadata from the file and reverted the edit (directory-form packages were unaffected — they dispatch on manifest presence, not suffix). The gate now accepts both package suffixes. Tests: `tests/test_songmeta.py` `TestWriteSongMetadata` (both zip suffixes, mixed-case suffix, directory form, unknown-suffix fallback). - **3D Drum & Keys highways now re-frame on fullscreen/layout drift under splitscreen.** The guitar/bass `highway_3d` self-detects when its panel canvas changes size and re-runs `applySize()` every frame, because the splitscreen host overrides `hw.resize` and never calls `renderer.resize()`. The drum and keys highways lacked that fallback — they only re-framed when the host explicitly called `resize(w, h)` — so their panels stayed framed for the pre-fullscreen size while the guitar/bass panels adapted (visible as a too-small, off-center highway after maximizing a split-screen session). Both draw loops now port `highway_3d`'s per-frame drift check: they re-apply on backing-store change (`canvas.width/height`) AND on CSS-box drift (`clientWidth/clientHeight` vs the last applied logical size, throttled to every 10th frame), and reset the tracking in `destroy()` so a reused instance re-frames on the next song. `plugins/drum_highway_3d` → 0.3.1, `plugins/keys_highway_3d` → 0.1.1. Tests: `tests/js/drum_keys_highway_3d_resize_reframe.test.js`. diff --git a/static/v3/songs.js b/static/v3/songs.js index 845c954..9d6fe9d 100644 --- a/static/v3/songs.js +++ b/static/v3/songs.js @@ -1859,13 +1859,57 @@ ''; } - function _renderCardsRange(start, end) { - let html = ''; + // Signature of the card at absolute index i: real-card vs skeleton, plus the + // select-mode it was built under. A change here is the ONLY reason a recycled + // node must be rebuilt (a hole filled after a fetch, or select mode toggled) — + // otherwise the node is reused as-is across window slides. + function _cardSig(i) { + return (state.songs[i] ? 'r' : 's') + (state.selectMode ? '1' : '0'); + } + + function _buildCardNode(i) { + const s = state.songs[i]; + const tmp = document.createElement('div'); + tmp.innerHTML = s ? songCard(s) : _skeletonCard(); + const node = tmp.firstElementChild; + node.setAttribute('data-idx', String(i)); + node.setAttribute('data-sig', _cardSig(i)); + return node; + } + + // Reconcile the grid's children to exactly cover [start, end) in ascending + // index order, REUSING the card nodes that stay in-window. Sliding the window + // one row now mutates only the row that entered/left instead of tearing down + + // rebuilding (+ re-wiring) the whole ~60-card window every frame — that + // per-slide teardown was the main-thread stall behind the "library skips every + // so many scrolls, up or down" report (the stall buffers held-arrow key-repeats + // that then flush in a burst). wireCards()'s data-wired guard wires only the + // freshly-built nodes. + function _syncWindow(grid, start, end) { + // Pass 1: drop nodes that left the window, are untagged, or whose content + // signature is stale (skeleton→real, or select-mode toggled). What remains + // is a reusable, correctly-rendered subset in ascending DOM order. + for (const el of Array.from(grid.children)) { + const a = el.getAttribute('data-idx'); + const idx = a == null ? NaN : Number(a); + if (!(idx >= start && idx < end) || el.getAttribute('data-sig') !== _cardSig(idx)) { + el.remove(); + } + } + // Pass 2: walk [start, end) in order, reusing survivors and inserting new + // nodes into their correct slot; `ref` tracks the child expected next. + const existing = new Map(); + for (const el of grid.children) existing.set(Number(el.getAttribute('data-idx')), el); + let ref = grid.firstChild; for (let i = start; i < end; i++) { - const s = state.songs[i]; - html += s ? songCard(s) : _skeletonCard(); + let node = existing.get(i); + if (!node) node = _buildCardNode(i); + if (node === ref) { + ref = ref.nextSibling; + } else { + grid.insertBefore(node, ref); + } } - return html; } // Fetch a single OFFSET page into the sparse store. Uses the stage-1 keyset @@ -1983,7 +2027,7 @@ } if (_closeCardMenu) _closeCardMenu(); // its DOM is about to be replaced grid.style.top = (firstRow * rowH) + 'px'; - grid.innerHTML = _renderCardsRange(start, end); + _syncWindow(grid, start, end); // recycle in-window nodes; only the entering/leaving row rebuilds wireCards(grid); decorateTuningChips(grid); // colour tuning chips by working-tuning match (async, feature-detected) state.winRange = { start, end }; diff --git a/tests/js/v3_songs_window_recycle.test.js b/tests/js/v3_songs_window_recycle.test.js new file mode 100644 index 0000000..47639c2 --- /dev/null +++ b/tests/js/v3_songs_window_recycle.test.js @@ -0,0 +1,143 @@ +'use strict'; +const { test } = require('node:test'); +const assert = require('node:assert'); + +// Mirror of static/v3/songs.js _cardSig / _buildCardNode / _syncWindow (the +// windowed-grid recycle path, #636 item 3 follow-up) — keep in sync. Exercised +// against a minimal DOM shim so the reconcile invariants are covered off-browser: +// (1) after every slide the grid's children are exactly [start,end) ascending, +// (2) card nodes for indices that stay in-window are REUSED (identity kept) — +// i.e. sliding one row never tears down + rebuilds the whole window (the +// per-slide stall behind the "skips every so many scrolls" report), and +// (3) a select-mode toggle rebuilds the visible window (checkbox/ring change). + +let NODE_SEQ = 0; +function makeNode() { + const attrs = {}; + return { + _uid: ++NODE_SEQ, + parent: null, + getAttribute(k) { return k in attrs ? attrs[k] : null; }, + setAttribute(k, v) { attrs[k] = String(v); }, + get nextSibling() { + const p = this.parent; if (!p) return null; + const i = p._kids.indexOf(this); + return i >= 0 && i + 1 < p._kids.length ? p._kids[i + 1] : null; + }, + remove() { + const p = this.parent; if (!p) return; + const i = p._kids.indexOf(this); + if (i >= 0) p._kids.splice(i, 1); + this.parent = null; + }, + }; +} +function makeGrid() { + return { + _kids: [], + get children() { return this._kids.slice(); }, + get firstChild() { return this._kids[0] || null; }, + insertBefore(node, ref) { + if (node.parent) node.remove(); + if (ref == null) this._kids.push(node); + else { const i = this._kids.indexOf(ref); this._kids.splice(i < 0 ? this._kids.length : i, 0, node); } + node.parent = this; + return node; + }, + }; +} + +// --- state + the three helpers, mirrored from songs.js --- +const state = { songs: [], selectMode: false }; +for (let i = 0; i < 5000; i++) state.songs[i] = { filename: 'song' + i }; + +function _cardSig(i) { return (state.songs[i] ? 'r' : 's') + (state.selectMode ? '1' : '0'); } +function _buildCardNode(i) { + const node = makeNode(); + node.setAttribute('data-idx', String(i)); + node.setAttribute('data-sig', _cardSig(i)); + return node; +} +function _syncWindow(grid, start, end) { + for (const el of Array.from(grid.children)) { + const a = el.getAttribute('data-idx'); + const idx = a == null ? NaN : Number(a); + if (!(idx >= start && idx < end) || el.getAttribute('data-sig') !== _cardSig(idx)) el.remove(); + } + const existing = new Map(); + for (const el of grid.children) existing.set(Number(el.getAttribute('data-idx')), el); + let ref = grid.firstChild; + for (let i = start; i < end; i++) { + let node = existing.get(i); + if (!node) node = _buildCardNode(i); + if (node === ref) ref = ref.nextSibling; + else grid.insertBefore(node, ref); + } +} + +const idxOf = (g) => g._kids.map((n) => Number(n.getAttribute('data-idx'))); +const uidOf = (g) => { const m = new Map(); for (const n of g._kids) m.set(Number(n.getAttribute('data-idx')), n._uid); return m; }; +function assertContig(g, start, end) { + const a = idxOf(g); + assert.strictEqual(a.length, end - start, `len == ${end - start}`); + for (let k = 0; k < a.length; k++) assert.strictEqual(a[k], start + k, `child ${k} == ${start + k}`); +} + +const COLS = 6, WIN = 12 * COLS; // 12 rows visible + +test('window stays [start,end) contiguous scrolling down, one row at a time', () => { + const grid = makeGrid(); + for (let row = 0; row < 40; row++) { + const start = row * COLS; + _syncWindow(grid, start, start + WIN); + assertContig(grid, start, start + WIN); + } +}); + +test('in-window card nodes are reused across a slide (no whole-window teardown)', () => { + const grid = makeGrid(); + _syncWindow(grid, 0, WIN); + const before = uidOf(grid); + _syncWindow(grid, COLS, COLS + WIN); // slide down one row + const after = uidOf(grid); + let reused = 0, built = 0; + for (const [i, uid] of after) (before.get(i) === uid ? reused++ : built++); + assert.strictEqual(built, COLS, `only the entering row is built (${COLS}), got ${built}`); + assert.strictEqual(reused, WIN - COLS, 'every overlapping card node is reused'); +}); + +test('scrolling back UP reuses nodes too and keeps order', () => { + const grid = makeGrid(); + for (let row = 0; row < 30; row++) _syncWindow(grid, row * COLS, row * COLS + WIN); + let prev = uidOf(grid); + for (let row = 29; row >= 0; row--) { + const start = row * COLS; + _syncWindow(grid, start, start + WIN); + assertContig(grid, start, start + WIN); + const now = uidOf(grid); + for (const [i, uid] of prev) if (i >= start && i < start + WIN) assert.strictEqual(now.get(i), uid, `idx ${i} reused going up`); + prev = now; + } +}); + +test('a select-mode toggle rebuilds the visible window', () => { + const grid = makeGrid(); + const start = 6 * COLS; + _syncWindow(grid, start, start + WIN); + const before = uidOf(grid); + state.selectMode = true; + _syncWindow(grid, start, start + WIN); + const after = uidOf(grid); + let rebuilt = 0; + for (const [i, uid] of before) if (after.get(i) !== uid) rebuilt++; + assert.strictEqual(rebuilt, WIN, 'select-mode change rebuilds every visible card'); + assertContig(grid, start, start + WIN); + state.selectMode = false; +}); + +test('a large jump (rail seek) rebuilds cleanly with no stale survivors', () => { + const grid = makeGrid(); + _syncWindow(grid, 0, WIN); + _syncWindow(grid, 1000 * COLS, 1000 * COLS + WIN); // non-overlapping jump + assertContig(grid, 1000 * COLS, 1000 * COLS + WIN); +}); From bde25c0bc88fd9179813dc3a6731210c5516f95f Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 17:20:31 -0500 Subject: [PATCH 8/8] fix(gpx): clamp partial final BCFS sector so GP6 .gpx import works (#749) Every real Guitar Pro 6 (.gpx) file failed to import with "GPX BCFS sector pointer out of range (malformed file)". A real .gpx's BCFZ-declared decompressed size isn't 0x1000-aligned, so its last (small) container file lands in a partial trailing sector. _parse_bcfs raised whenever a sector read would run past the buffer end, rejecting the whole container before score.gpif could be extracted -- so no GP6 file could be charted in the song editor. (GP7/GP8 .gp files take the ZIP path, not BCFS, which is why this wasn't caught earlier.) Clamp the final sector read to the buffer end (the per-file size field trims the padding anyway), matching canonical GPX readers (alphaTab / PyGuitarPro). A sector whose start is past the end still raises, so the malformed-file guard is preserved. Verified against two real GP6 files -- both now unpack to valid GPIF with all tracks. Adds the previously-missing positive BCFS round-trip coverage: partial-final-sector, multi-file, sector-aligned baseline, and the preserved out-of-range guard. Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: byrongamatos --- CHANGELOG.md | 1 + lib/gp2rs_gpx.py | 14 ++++++-- tests/test_gp2rs_gpx.py | 73 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd1009..91d7719 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Player frame-time hotspots removed (trace-backed) + weak-hardware hardening.** A Chrome performance trace of a 3D-highway session surfaced two core per-frame layout-thrash sources, now fixed: the highway's visibility check read `canvas.offsetParent` every rAF frame (forces style/layout recalc — now sampled every 10th frame with a cached value, force-refreshed on init/canvas-replace/resize/override-clear), and the v3 player chrome loop called `matches(':hover')` per frame and unconditionally rewrote the Up-Next pill's `textContent`/bar width at 6 Hz (now hover-tracked via mouseenter/mouseleave, DOM writes only on value change, progress bar moved from `width` to compositor-only `scaleX`). The 3D highway pre-warms shader programs (`ren.compile`) and deterministic label textures at init — and chart-dependent chord/section label textures on first draw — so first-appearance shader-compile/texture-upload frame spikes move into the load spinner. For weaker hardware: the per-frame renderer bundle is now a single reused object instead of a fresh ~35-field allocation per frame (object identity is stable and meaningless; array fields still swap reference on chart changes), custom viz get `bundle.lowerBoundT`/`bundle.lowerBoundTime` binary-search helpers for visible-window culling, the default 2D highway's beat lines no longer scan every beat in the song per frame, and the 3D highway stops reading `localStorage` per frame (1 Hz poll) and caches its lyrics text-measurement layout per displayed line instead of re-measuring every syllable every frame. A second, throttled-CPU trace pass additionally removed: shader-program re-resolution churn from label texture swaps (`material.needsUpdate` is now only set on a null↔texture transition — swapping between two cached label textures never changes the compiled program), the 3D highway's per-frame `getBoundingClientRect` layout read in its canvas-size self-check (now every 10th frame, still immediate on backing-store change), and the core 60 Hz HUD clock rewriting `textContent` on every tick (now write-on-change, ~1/s). The dominant residual — steady `getParameters` shader-program re-resolution (~4% of throttled main thread) — turned out to be Three r158+'s transparent-DoubleSide two-pass rendering, which sets `material.needsUpdate` twice per object per frame; all 18 of the 3D highway's transparent DoubleSide materials are flat unlit quads (labels, rails, chord frames, lanes), so they now declare `forceSinglePass: true`, eliminating the recompile churn and halving those objects' draw calls. ### Fixed +- **Guitar Pro 6 (`.gpx`) import no longer fails on every real file.** The GPX BCFS container reader (`lib/gp2rs_gpx.py`) rejected any file whose final sector wasn't a full `0x1000` block — but a real `.gpx`'s BCFZ-declared decompressed size isn't sector-aligned, so the last (small) container file always lands in a partial trailing sector. The bounds check *raised* `GPX BCFS sector pointer out of range (malformed file)` instead of clamping the tail read, so `_load_gpif` threw before `score.gpif` could be extracted and **no GP6 file could be imported into the song editor** (both real test files failed identically — this wasn't file-specific). GP7/GP8 `.gp` files were unaffected — they take the ZIP path, not BCFS, which is why prior GP-import work didn't surface it. The reader now **clamps the final sector read to the buffer end** (the per-file size field trims the padding anyway), matching canonical GPX readers (alphaTab / PyGuitarPro); a sector whose *start* is past the end still raises, preserving the malformed-file guard. Verified against two real GP6 files — both now unpack to valid GPIF with all tracks. Tests: `tests/test_gp2rs_gpx.py` (partial-final-sector round-trip, multi-file container, sector-aligned baseline, and the preserved out-of-range guard). - **v3 Songs grid: fixed the scroll stutter that "skips every so many scrolls," up or down.** The virtualized grid rebuilt its **entire** visible window (`grid.innerHTML = …` + a full `wireCards` pass) every time it slid by one row, so each row-boundary crossing was a heavy synchronous frame that stalled the main thread and buffered held-arrow key-repeats into a visible lurch (a tester's "super fast for a second then slowed down") at fixed scroll offsets — in **both directions and regardless of whether the page was already loaded** (the cost was DOM teardown, not fetching, which is why scrolling back up over cached songs hitched too). `renderWindow()` now **reconciles the window in place**: it reuses the card nodes that stay on-screen and builds only the row that enters/leaves (~6 nodes per slide instead of ~60), keyed by absolute index with a real-vs-skeleton + select-mode signature so hole-fills (after a page fetch) and select-mode toggles still rebuild exactly the nodes that changed. `wireCards`'s `data-wired` guard then wires only the freshly-built nodes, so per-slide listener churn drops with it. Follow-up to the stage-2 virtualized grid (got-feedback/feedBack#636 item 3). Frontend-only: `static/v3/songs.js`. Tests: `tests/js/v3_songs_window_recycle.test.js` (window stays `[start,end)` contiguous + in-window node identity reused across a down-then-up scroll; select-mode toggle and rail-seek jump rebuild correctly). - **Starter content seeds again (and now ships The Adicts' "Ode to Joy").** `_BUILTIN_STARTER_SOURCES` still listed `beethoven-ode_to_joy.feedpak` after that pack was deleted, and never wired up its replacement `the_adicts-ode-to-joy_vst_cover.feedpak` that landed on disk. The listed-but-missing file made the all-present gate never fire, so **no** starter songs seeded on first run. Synced the manifest to what's on disk (Für Elise, Star Spangled Banner, The Adicts' Ode to Joy). Tests: `tests/test_builtin_starter_seed.py` (the present/unlisted guards were red on `main`). - **Edit Metadata now writes into `.feedpak` files, not just legacy `.sloppak` ones.** `lib/songmeta.py`'s suffix gate predated the format rename — core reads both suffixes everywhere else (`sloppak.SONG_EXTS`), but the metadata writer only dispatched on `.sloppak`, so editing a zip-form `.feedpak`'s title/artist/album/year silently fell back to a DB-only update. That looked fine until the next **full library rescan** re-derived metadata from the file and reverted the edit (directory-form packages were unaffected — they dispatch on manifest presence, not suffix). The gate now accepts both package suffixes. Tests: `tests/test_songmeta.py` `TestWriteSongMetadata` (both zip suffixes, mixed-case suffix, directory form, unknown-suffix fallback). diff --git a/lib/gp2rs_gpx.py b/lib/gp2rs_gpx.py index 2b4dba6..ce6db41 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -121,10 +121,18 @@ def _parse_bcfs(bcfs: bytes) -> dict: while sc <= max_sectors: s = _gi(po + 4 * sc); sc += 1 if s == 0: break - so = s * SECTOR - if HDR + so + SECTOR > len(data): + start = HDR + s * SECTOR + # Real .gpx files' final sector is a few bytes short of a full + # 0x1000 block: the BCFZ-declared decompressed size isn't + # sector-aligned, so the last (small) container file lands in a + # partial trailing sector. Clamp the read to the buffer end — + # the per-file size field (`fs`, applied below) trims any + # padding — matching canonical GPX readers (alphaTab / + # PyGuitarPro slice-and-clamp). Only a sector whose *start* is + # past the end is genuinely malformed. + if start < 0 or start >= len(data): raise ValueError("GPX BCFS sector pointer out of range (malformed file)") - fb.extend(data[HDR + so: HDR + so + SECTOR]) + fb.extend(data[start: min(start + SECTOR, len(data))]) else: raise ValueError("GPX BCFS sector chain too long (malformed file)") files[fn] = bytes(fb[:fs]) diff --git a/tests/test_gp2rs_gpx.py b/tests/test_gp2rs_gpx.py index d677caf..4b91106 100644 --- a/tests/test_gp2rs_gpx.py +++ b/tests/test_gp2rs_gpx.py @@ -122,6 +122,79 @@ def test_parse_bcfs_rejects_bad_magic(): _parse_bcfs(b"NOPE" + b"\x00" * 16) +# ── _parse_bcfs container round-trip (GP6 .gpx partial final-sector) ───────── + +def _build_bcfs(entries, short_by=0): + """Assemble a minimal in-memory BCFS container for _parse_bcfs. + + ``entries`` is ``[(name: bytes, payload: bytes, data_sector: int), ...]``. + The directory entry for entry *i* is written to sector ``i + 1``; each + entry's payload goes in the sector index it names. ``short_by`` truncates + the final buffer by N bytes to emulate a real .gpx's partial trailing + sector (the BCFZ-declared decompressed size isn't 0x1000-aligned). Layout + mirrors the reader: a 4-byte ``BCFS`` header, then 0x1000-byte sectors, + with every value read at ``HDR + sector * 0x1000``. + """ + SECTOR = 0x1000 + HDR = 4 + max_sector = max([e[2] for e in entries] + [len(entries)]) + buf = bytearray(b"BCFS" + b"\x00" * ((max_sector + 1) * SECTOR)) + + def put_u32(off, val): + struct.pack_into(" sector i+1 + put_u32(dir_off + 0x00, 2) # entry type: file + nm = name[:127] + buf[HDR + dir_off + 0x04: HDR + dir_off + 0x04 + len(nm)] = nm + put_u32(dir_off + 0x8C, len(payload)) # declared file size + put_u32(dir_off + 0x94, data_sector) # first data-sector pointer + put_u32(dir_off + 0x94 + 4, 0) # chain terminator + dpos = HDR + data_sector * SECTOR + buf[dpos: dpos + len(payload)] = payload + if short_by: + del buf[len(buf) - short_by:] + return bytes(buf) + + +def test_parse_bcfs_reads_short_final_sector(): + """The regression: a real .gpx ends a byte short of a full 0x1000 sector, + so its last (small) container file lands in a partial trailing sector. The + reader must clamp that read, not reject the whole container — rejecting it + is what made every GP6 .gpx fail to import with 'sector pointer out of + range'.""" + bcfs = _build_bcfs([(b"score.gpif", b"hello", 2)], short_by=1) + assert (len(bcfs) - 4) % 0x1000 == 0x1000 - 1 # final sector is 1 short + assert _parse_bcfs(bcfs)["score.gpif"] == b"hello" + + +def test_parse_bcfs_full_sector_round_trip(): + """A sector-aligned container round-trips unchanged (baseline).""" + assert _parse_bcfs(_build_bcfs([(b"misc.xml", b"", 2)]))["misc.xml"] == b"" + + +def test_parse_bcfs_multi_file_short_final_sector(): + """Real-world shape: score.gpif plus small config files, the last one in + the partial trailing sector.""" + out = _parse_bcfs(_build_bcfs([ + (b"score.gpif", b"", 3), + (b"LayoutConfiguration", b"AB", 4), + ], short_by=1)) + assert out["score.gpif"] == b"" + assert out["LayoutConfiguration"] == b"AB" + + +def test_parse_bcfs_rejects_sector_starting_past_end(): + """A sector pointer whose *start* is beyond the container is genuinely + malformed and must still raise — the clamp tolerates a partial final + sector, not arbitrary out-of-range pointers.""" + bcfs = bytearray(_build_bcfs([(b"x", b"y", 2)])) + struct.pack_into("