From 2ebe54305ee19efdf4d29ff97214328e4ccc249a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 2 Jun 2026 07:36:50 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20patchSmith=20spectralBalance=20calibrati?= =?UTF-8?q?on=20+=20Phase=201=20v1=E2=86=92v2=20replay=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects surfaced by an independent review of the loudness-v2 program (#128–#133): patchSmith read spectralBalance bands as "dB above/below the mix average", but the analyzer emits absolute band energy (10·log10(mean_energy), ~ -100..0). On real tracks the sub oscillator never engaged (guard subBass<=1.0 against a real ~ -41 dB reading) and the acid cutoff pinned to the 200 Hz floor, while the cited rationale misstated the measurement (invariant #2). Fix locally: add meanBandDb() and read each band as a prominence relative to the 7-band mix average — the analyzer contract is unchanged (mixDoctor/dashboard rely on the absolute scale). Rationales are now true; tests use realistic absolute dB with a regression case that fails on the old logic. phase1Version was emitted but never consumed, so persisted pre-#128 runs (stored verbatim in analysis_runtime, never migrated) replayed v1-unit values into v2 consumers: a linear truePeak of 0.98 read as a +0.98 dBTP over (false TRUE_PEAK_OVER / MISSING_LOUDNESS_ACTION) and a raw bpmConfidence of 3.2 rendered as 320%. Add _migrate_v1_loudness_units() at the top of _build_phase1 (the single chokepoint every persisted snapshot passes through): a payload lacking "phase1.v2" gets truePeak linear→dBTP, bpmConfidence raw→0-1, plr dropped and recomputed, then stamped v2. No-op for v2 payloads. Verified: tsc clean; 750 frontend unit tests; vite build; 1102 backend unittests (skipped=3). Playwright smoke could not run (chromium download blocked by the sandbox network allowlist); both changes are pure logic covered by the suites that ran. https://claude.ai/code/session_01C2VkJk4JAUGtz3b9BWqVGE --- apps/backend/server_phase1.py | 38 ++++++++++- apps/backend/tests/test_server.py | 49 ++++++++++++++ apps/ui/src/services/patchSmith.ts | 52 +++++++++++++-- apps/ui/tests/services/patchSmith.test.ts | 81 +++++++++++++++++++---- 4 files changed, 198 insertions(+), 22 deletions(-) diff --git a/apps/backend/server_phase1.py b/apps/backend/server_phase1.py index 7ae13aee..2a12164a 100644 --- a/apps/backend/server_phase1.py +++ b/apps/backend/server_phase1.py @@ -2,7 +2,7 @@ import sys from datetime import datetime -from math import ceil, isfinite +from math import ceil, isfinite, log10 from typing import Any from fastapi.responses import JSONResponse @@ -124,7 +124,43 @@ def _normalize_stem_analysis(stem_analysis: Any) -> dict[str, Any] | None: return out +def _migrate_v1_loudness_units(payload: dict[str, Any]) -> dict[str, Any]: + """Convert a persisted Phase 1 *v1* payload's loudness fields to v2 units. + + Schema v1 (pre-#128) stored ``truePeak`` as a linear amplitude proxy and + ``bpmConfidence`` as raw Essentia confidence (~0-5.32). The analyzer has + emitted ``phase1Version`` since v2, so a payload that lacks ``"phase1.v2"`` + is a persisted v1 run replayed from SQLite (``analysis_runtime`` stores raw + analyze.py output and never migrates it). Without this, v2 consumers misread + the stored values — a linear ``truePeak`` of 0.98 reads as a +0.98 dBTP + inter-sample over (false ``TRUE_PEAK_OVER`` / ``MISSING_LOUDNESS_ACTION``), + and a raw ``bpmConfidence`` of 3.2 renders as 320%. Convert in place so the + rest of ``_build_phase1`` (including its PLR fallback) operates on v2-correct + units. No-op for v2 payloads (idempotent). + """ + if not isinstance(payload, dict) or payload.get("phase1Version") == "phase1.v2": + return payload + migrated = dict(payload) + # truePeak: linear amplitude proxy -> dBTP (None for <= 0, matching v2 silence). + true_peak_linear = _coerce_nullable_number(payload.get("truePeak")) + if true_peak_linear is not None: + migrated["truePeak"] = ( + round(20.0 * log10(true_peak_linear), 1) if true_peak_linear > 0 else None + ) + # bpmConfidence: raw Essentia (~0-5.32) -> 0-1 (/5, clamped), matching analyze_core. + bpm_confidence = _coerce_nullable_number(payload.get("bpmConfidence")) + if bpm_confidence is not None: + migrated["bpmConfidence"] = round(min(max(bpm_confidence, 0.0) / 5.0, 1.0), 3) + # The stored v1 plr mixed a linear peak with dB LUFS (incoherent); drop it so + # the _build_phase1 fallback recomputes it from the now-dBTP truePeak. + migrated["plr"] = None + # Stamp the generation so the normalized result advertises v2 units. + migrated["phase1Version"] = "phase1.v2" + return migrated + + def _build_phase1(payload: dict[str, Any]) -> dict[str, Any]: + payload = _migrate_v1_loudness_units(payload) stereo_detail = payload.get("stereoDetail") if not isinstance(stereo_detail, dict): stereo_detail = {} diff --git a/apps/backend/tests/test_server.py b/apps/backend/tests/test_server.py index e4807ea9..3c92554e 100644 --- a/apps/backend/tests/test_server.py +++ b/apps/backend/tests/test_server.py @@ -2639,6 +2639,7 @@ class BuildPhase1CoercionTests(unittest.TestCase): def _minimal_payload(self, **overrides) -> dict: base = { + "phase1Version": "phase1.v2", "bpm": 128, "bpmConfidence": 0.92, "key": "A minor", @@ -2663,6 +2664,54 @@ def _minimal_payload(self, **overrides) -> dict: base.update(overrides) return base + # ── Phase 1 v1 -> v2 replay migration (persisted pre-#128 runs) ────────── + # A run completed before #128 was stored with a LINEAR truePeak, RAW + # bpmConfidence (~0-5.32), an incoherent v1 plr, and NO phase1Version. + # _build_phase1 must migrate those to v2 units on replay so v2 consumers + # (loudnessGuardrails dBTP>0, exportUtils *100) don't misread them. + def _v1_payload(self, **overrides) -> dict: + payload = self._minimal_payload(**overrides) + payload.pop("phase1Version", None) # pre-#128 runs lack the marker + return payload + + def test_v1_linear_true_peak_is_converted_to_dbtp(self) -> None: + # Linear 0.98 (a hot-but-not-over v1 peak) -> ~ -0.2 dBTP, NOT a false + # +0.98 dBTP inter-sample over. + phase1 = server._build_phase1(self._v1_payload(truePeak=0.98)) + self.assertAlmostEqual(phase1["truePeak"], -0.2, places=1) + + def test_v1_full_scale_linear_true_peak_becomes_zero_dbtp(self) -> None: + phase1 = server._build_phase1(self._v1_payload(truePeak=1.0)) + self.assertEqual(phase1["truePeak"], 0.0) + + def test_v1_silent_linear_true_peak_becomes_none(self) -> None: + phase1 = server._build_phase1(self._v1_payload(truePeak=0.0)) + self.assertIsNone(phase1["truePeak"]) + + def test_v1_raw_bpm_confidence_is_normalized(self) -> None: + # Raw 3.2 (reliable on the old scale) -> 0.64, not a 320%-rendering 3.2. + phase1 = server._build_phase1(self._v1_payload(bpmConfidence=3.2)) + self.assertAlmostEqual(phase1["bpmConfidence"], 0.64, places=2) + + def test_v1_plr_is_recomputed_as_dbtp_minus_lufs(self) -> None: + # v1 stored an incoherent plr (24.8); it must be dropped and recomputed + # from the migrated dBTP truePeak: 0.0 - (-8.2) = 8.2. + phase1 = server._build_phase1( + self._v1_payload(truePeak=1.0, lufsIntegrated=-8.2, plr=24.8) + ) + self.assertAlmostEqual(phase1["plr"], 8.2, places=1) + + def test_v1_payload_is_stamped_v2_after_migration(self) -> None: + phase1 = server._build_phase1(self._v1_payload(truePeak=0.9)) + self.assertEqual(phase1["phase1Version"], "phase1.v2") + + def test_v2_payload_loudness_fields_are_not_migrated(self) -> None: + # A v2 payload (phase1Version set) must pass through untouched: a -0.1 + # dBTP peak stays -0.1, not re-logged into nonsense, and 0.92 stays 0.92. + phase1 = server._build_phase1(self._minimal_payload(truePeak=-0.1, bpmConfidence=0.92)) + self.assertEqual(phase1["truePeak"], -0.1) + self.assertEqual(phase1["bpmConfidence"], 0.92) + def test_lufs_range_nan_is_coerced_to_none(self) -> None: phase1 = server._build_phase1(self._minimal_payload(lufsRange=float("nan"))) self.assertIsNone(phase1["lufsRange"]) diff --git a/apps/ui/src/services/patchSmith.ts b/apps/ui/src/services/patchSmith.ts index 99b4889c..dc34239e 100644 --- a/apps/ui/src/services/patchSmith.ts +++ b/apps/ui/src/services/patchSmith.ts @@ -144,6 +144,32 @@ export function detuneCentsToVital(cents: number): number { return round(clamp(cents / 7.0, 0, 10), 3); } +/** + * Mean of the 7 `spectralBalance` bands — the "mix average" reference. + * + * CRITICAL: the bands are ABSOLUTE band energy in dB (`10·log10(mean_energy)`, + * roughly −100…0; see `apps/backend/analyze_core.py`), NOT relative to the mix + * average. A real track reads e.g. `subBass −41`, `highs −62`. So any + * "X dB above/below the mix average" reasoning MUST subtract this mean first — + * comparing a raw band value against a small absolute threshold (the original + * bug) never fires on real input. + */ +export function meanBandDb(sb: Phase1Result["spectralBalance"] | null | undefined): number | null { + if (!sb) return null; + const bands = [sb.subBass, sb.lowBass, sb.lowMids, sb.mids, sb.upperMids, sb.highs, sb.brilliance]; + const finite = bands.filter((v): v is number => typeof v === "number" && Number.isFinite(v)); + if (finite.length === 0) return null; + return finite.reduce((sum, v) => sum + v, 0) / finite.length; +} + +/** + * dB above the mix average at which the sub band is "prominent" enough to + * warrant a dedicated octave-down oscillator. First cut — tuned so a genuinely + * sub-forward mix engages it while ordinary low-end does not; revisit against + * the recommendation corpus (GOAL.md). + */ +const SUB_PROMINENCE_DB = 3.0; + // ── Mapping engine ───────────────────────────────────────────────────────── const CONF_RANK: Record = { HIGH: 2, MED: 1, LOW: 0 }; @@ -212,8 +238,16 @@ function mapSupersaw(phase1: Phase1Result, acc: PatchAccumulator): void { /** Strong measured sub energy → a sub-octave second oscillator. */ function mapSubLayer(phase1: Phase1Result, acc: PatchAccumulator): void { - const subBass = phase1.spectralBalance?.subBass; - if (typeof subBass !== "number" || subBass <= 1.0) return; + const sb = phase1.spectralBalance; + const subBass = sb?.subBass; + if (typeof subBass !== "number") return; + // The bands are absolute dB, so reduce to a prominence vs the mix average + // before thresholding (see meanBandDb) — the old `subBass <= 1.0` guard + // compared an absolute ~−41 dB reading against 1.0 and never fired. + const meanDb = meanBandDb(sb); + if (meanDb === null) return; + const subProminenceDb = subBass - meanDb; + if (subProminenceDb <= SUB_PROMINENCE_DB) return; applyCitation(acc, { label: "Sub oscillator", @@ -221,7 +255,7 @@ function mapSubLayer(phase1: Phase1Result, acc: PatchAccumulator): void { value: 1.0, display: "on", phase1Fields: ["spectralBalance.subBass"], - rationale: `Sub band sits ${subBass.toFixed(1)} dB above the mix average — add an octave-down oscillator for that weight.`, + rationale: `Sub band sits ${subProminenceDb.toFixed(1)} dB above the mix average — add an octave-down oscillator for that weight.`, confidence: "MED", }); // The octave-down transpose and balance are what "sub layer" means; cite them @@ -273,10 +307,14 @@ function mapAcidFilter(phase1: Phase1Result, acc: PatchAccumulator): void { confidence, }); - // Place the cutoff from spectral brightness (always-present measurement). + // Place the cutoff from spectral brightness *relative to the mix average*. + // The bands are absolute dB; the 1200 Hz center assumes a relative reading, + // so without subtracting the mean (the old bug) brightness ≈ −63 and the + // cutoff pinned to the 200 Hz floor on every real track. const sb = phase1.spectralBalance; - if (sb && typeof sb.highs === "number" && typeof sb.brilliance === "number") { - const brightness = (sb.highs + sb.brilliance) / 2; // dB above/below average + const meanDb = meanBandDb(sb); + if (sb && meanDb !== null && typeof sb.highs === "number" && typeof sb.brilliance === "number") { + const brightness = (sb.highs + sb.brilliance) / 2 - meanDb; // dB above/below mix average const cutoffHz = clamp(1200 * 2 ** (brightness / 6), 200, 16000); applyCitation(acc, { label: "Filter cutoff", @@ -284,7 +322,7 @@ function mapAcidFilter(phase1: Phase1Result, acc: PatchAccumulator): void { value: hzToCutoffSemitone(cutoffHz), display: `${Math.round(cutoffHz)} Hz`, phase1Fields: ["spectralBalance.highs", "spectralBalance.brilliance"], - rationale: `High/brilliance balance (${brightness >= 0 ? "+" : ""}${brightness.toFixed(1)} dB) sets the cutoff at ≈${Math.round(cutoffHz)} Hz.`, + rationale: `High/brilliance balance (${brightness >= 0 ? "+" : ""}${brightness.toFixed(1)} dB vs mix average) sets the cutoff at ≈${Math.round(cutoffHz)} Hz.`, confidence: "MED", }); } diff --git a/apps/ui/tests/services/patchSmith.test.ts b/apps/ui/tests/services/patchSmith.test.ts index 86b79555..8df1ee18 100644 --- a/apps/ui/tests/services/patchSmith.test.ts +++ b/apps/ui/tests/services/patchSmith.test.ts @@ -42,13 +42,15 @@ const richSupersaw = makePhase1({ spectralComplexity: 0.7, }, spectralBalance: { - subBass: 2.0, // strong sub → osc 2 - lowBass: 1.0, - lowMids: 0.0, - mids: -0.5, - upperMids: 0.5, - highs: 2.0, - brilliance: 1.0, + // Absolute band energy in dB (~ -100..0), as real Phase 1 emits. subBass + // sits ~10 dB above the 7-band mean (-18), so the sub-osc mapping engages. + subBass: -8.0, + lowBass: -10.0, + lowMids: -22.0, + mids: -26.0, + upperMids: -20.0, + highs: -16.0, + brilliance: -24.0, }, }); @@ -117,13 +119,15 @@ describe("buildPatch — mapping & citations", () => { bassRhythmDensity: 0.6, }, spectralBalance: { - subBass: 0, - lowBass: 0, - lowMids: 0, - mids: 0, - upperMids: 0, - highs: 3.0, - brilliance: 2.0, + // Absolute dB; highs/brilliance sit ~10 dB above the mix average, + // so the cutoff opens well above the 200 Hz floor. + subBass: -30, + lowBass: -28, + lowMids: -22, + mids: -18, + upperMids: -12, + highs: -6, + brilliance: -10, }, }), ); @@ -167,6 +171,55 @@ describe("buildPatch — mapping & citations", () => { }); }); +describe("buildPatch — spectralBalance read relative to the mix average", () => { + // Regression for the v1 calibration bug: spectralBalance bands are ABSOLUTE + // dB (~ -100..0), not relative to the mix. The old guards compared raw + // absolute values against small thresholds and so never fired on real input. + it("engages the sub oscillator when sub is prominent vs the mix average (realistic absolute dB)", () => { + const result = buildPatch( + makePhase1({ + spectralBalance: { + subBass: -8, lowBass: -10, lowMids: -22, mids: -26, + upperMids: -20, highs: -16, brilliance: -24, + }, + }), + ); + // subBass (-8) sits ~10 dB above the 7-band mean (-18) → sub layer engages. + expect(citation(result, "osc_2_on")?.value).toBe(1.0); + expect(citation(result, "osc_2_on")?.rationale).toMatch(/above the mix average/); + }); + + it("skips the sub oscillator when sub sits below the mix average (golden-like absolute dB)", () => { + const result = buildPatch( + makePhase1({ + spectralBalance: { + subBass: -41.6, lowBass: -4.9, lowMids: -6.1, mids: -41.0, + upperMids: -53.4, highs: -62.2, brilliance: -64.7, + }, + }), + ); + // Real golden track: sub is far below the dominant low-bass body → no sub osc. + expect(citation(result, "osc_2_on")).toBeUndefined(); + }); + + it("opens the acid cutoff above the 200 Hz floor when the top is relatively bright", () => { + const result = buildPatch( + makePhase1({ + acidDetail: { isAcid: true, confidence: 0.8, resonanceLevel: 0.7 } as never, + // A dark overall mix whose top is still the brightest part: absolute + // highs/brilliance are low but ABOVE the mix mean. The old absolute + // formula pinned the cutoff to 200 Hz; the relative one opens it. + spectralBalance: { + subBass: -50, lowBass: -52, lowMids: -55, mids: -58, + upperMids: -54, highs: -45, brilliance: -48, + }, + }), + ); + // 200 Hz floor ≈ semitone 55; a responsive cutoff sits clearly above it. + expect(citation(result, "filter_1_cutoff")?.value).toBeGreaterThan(70); + }); +}); + describe("buildPatch — honest uncertainty (invariant #4)", () => { it("hedges a low-confidence supersaw and marks the params LOW", () => { const result = buildPatch(