From 27122df70f03f2f0bad186819311e49c1157cbe5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 05:20:55 +0000 Subject: [PATCH 1/3] docs: review openmeters, soundscope, Partiels, forever-jukebox for ASA incorporation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Evaluates the three-track incorporation plan against PURPOSE.md and the current ASA baseline (LUFS path in analyze_core.py, JSON schema in JSON_SCHEMA.md, REST surface in server_*.py). Track 1 (LUFS via openmeters): premise unverified. Recommends a 1-day spike (EBU R128 test set + tolerance check) before any rewrite. Flags GPL-3.0 license hazard with openmeters and routes spectral reassignment through librosa.reassigned_spectrogram instead of porting Rust. Track 2 (schema via Partiels): plan misreads Partiels — its CSV is three flat columns, not a richer schema. Reframes to a tabular CSV view per ASA time-series field, modeled on Partiels' (time, duration, value) pattern. Defers SDIF with the paragraph the plan asked for. Track 3 (API via forever-jukebox): side-by-side comparison shows ASA already leads on error envelope, upload size, and staged execution. Adopt list: URL ingestion, public state-machine collapse, audio re-serve, admin key. Reject: Echo Nest output schema, search/discovery, YouTube ingestion. Sequencing matches the plan (2 -> 3 -> 1) with reduced scope: ~6-8 working days total vs the plan's implied effort. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg --- docs/external-repo-review-2026-05-13.md | 306 ++++++++++++++++++++++++ 1 file changed, 306 insertions(+) create mode 100644 docs/external-repo-review-2026-05-13.md diff --git a/docs/external-repo-review-2026-05-13.md b/docs/external-repo-review-2026-05-13.md new file mode 100644 index 00000000..d9413bd8 --- /dev/null +++ b/docs/external-repo-review-2026-05-13.md @@ -0,0 +1,306 @@ +# External Repo Incorporation Review — 2026-05-13 + +**Subject:** Evaluating openmeters, soundscope, Partiels, and forever-jukebox against ASA's mission and current implementation. + +**Authored by:** Claude (review-only branch `claude/review-asa-repos-5XA5r`). +No code changes; this is an evaluation document. + +**Anchor:** [PURPOSE.md](../PURPOSE.md) — the user-value test and the +decision framework. Every recommendation below traces back to one of +its three "build it" criteria or its "stop and reconsider" branch. + +--- + +## TL;DR + +| Track | Plan's framing | This review's read | Recommendation | +| --- | --- | --- | --- | +| 1 — LUFS via openmeters + soundscope oracle | "Likely tracks an older BS.1770 rev" | **Unverified premise.** Essentia 2.1b6 (`LoudnessEBUR128`) implements EBU R128 v3.0 / BS.1770-3+. The user-facing delta to 1770-5 is mostly editorial. Spectral reassignment is the real prize, and it is significant work. | **Defer Track 1 as written.** First do a 1-day spike to (a) confirm Essentia's revision and (b) compare ASA's `lufsIntegrated` against the EBU R128 test set. If deltas are within ±0.1 LU, the loudness rewrite has no user value. Spectral reassignment is a separate, larger effort — treat it as its own track. | +| 2 — Schema via Partiels | "ASA's JSON output should be a subset of what serious analysis tools already emit" | **Misreads Partiels.** Its CSV is three flat columns (`time, duration, label/value`). Its JSON is sparse and undocumented. ASA's schema is *richer*, not poorer. There is nothing to be a "subset of." | **Reframe and proceed.** The right adoption is a *tabular CSV view per time-series field* (lufsCurve, tempoCurve, spectralBalanceTimeSeries, etc.), patterned after Partiels' `time, duration, value` columns. Defer SDIF indefinitely (no demand, no payoff). Ship a single ADR + per-time-series CSV exporter. | +| 3 — REST via forever-jukebox | "Closest public precedent for ASA's product surface" | **Closer than the plan acknowledges in transport, further than the plan acknowledges in output shape.** Both poll an async job; that's where the similarity ends. forever-jukebox emits Echo Nest beats/segments/sections for *playback*. ASA emits production-recreation measurements. ASA's API is already richer in 3 of 5 dimensions. | **Do the side-by-side, but expect a short adopt list.** Worth lifting: URL-based input (`POST /api/analysis/url`), simpler client-facing state machine, retry-on-retryable. Worth rejecting: Echo Nest output schema, 20 MB upload cap, Spotify/YouTube search. ASA already wins on error envelope, staged execution, and ownership/auth. | + +**Sequencing recommendation:** Track 2 (scoped down) → Track 3 (audit + targeted lift) → Track 1 (gated on the verification spike). This matches the plan's order but with lower ambition on Tracks 1 and 2 and a tighter target on Track 3. + +--- + +## License Map (read this before reading any of the target sources) + +| Repo | License | Implication for ASA | +| --- | --- | --- | +| **ASA** (this repo) | MIT ([LICENSE](../LICENSE)) | Permissive; can adopt MIT/BSD/Apache code with attribution. **Cannot vendor GPL code.** | +| **openmeters** | GPL-3.0 | Algorithms are not copyrightable, but copied/translated *code* is. Read the **ITU-R BS.1770-5 standard PDF** for the algorithm; do not read openmeters' source if you intend to write a clean-room port. Treat as inspiration-only. | +| **soundscope** | MIT | Code-compatible. But it's TUI-only with no JSON/CLI numeric output, so it can only serve as a *manual* cross-check, not an automated oracle. | +| **Partiels** | GPL-3.0 | File-format schemas are facts, not copyrightable expression; mirroring its CSV column shape is safe. Do not vendor Partiels code. | +| **forever-jukebox** | MIT | Code-compatible; safe to read and selectively port. | + +**The plan's framing of "port algorithms (NOT fork — Rust → Essentia.js / JS)" misses one thing:** Rust → JS is not the boundary that matters here. ASA's measurement layer is **Python (Essentia)**, not JS/WASM. There is no Essentia.js in this codebase ([analyze_core.py:8](../apps/backend/analyze_core.py)). Re-implementing in JS/WASM would be a second port to the wrong runtime — see Track 1 below. + +--- + +## Track 1 — Loudness path (openmeters, soundscope as oracle) + +### Where ASA's loudness lives today + +- Implementation: [`analyze_core.py:186-230`](../apps/backend/analyze_core.py) — calls `essentia.standard.LoudnessEBUR128()`. +- True-peak: separate path via `essentia.standard.TruePeakDetector()` at [`analyze_core.py:233-`](../apps/backend/analyze_core.py). +- Fields emitted: `lufsIntegrated`, `lufsRange`, `lufsMomentaryMax`, `lufsShortTermMax`, `lufsCurve.{shortTerm,momentary}[]`, `truePeak`, `crestFactor`, `plr`. All gated by `EXPECTED_TOP_LEVEL_KEYS` in [`tests/test_analyze.py:26-43`](../apps/backend/tests/test_analyze.py). +- Curves are downsampled via `_downsample_lufs_array` in `dsp_utils.py` for payload size. + +### What openmeters offers + +- Rust, GPL-3.0, ~135 ★, BS.1770-5 (Nov 2023). Built on RustFFT/RealFFT. +- Spectrogram with spectral reassignment — the README cites academic references in source headers, but per the README this is on the spectrogram visual path, not the loudness path. +- Standalone Linux PipeWire app. Not a library, not a CLI extractor; embedding it from Python would require carving out its DSP modules. + +### What soundscope offers + +- Rust, MIT, ~175 ★. TUI with LUFS, true peak, frequency spectrum, waveform. +- **No JSON output and no documented BS.1770 revision.** Useful for human verification ("does soundscope's LUFS match ASA's on the same file?") but cannot be wired into ASA tests as an automated oracle. + +### Why the plan's premise needs verification + +The plan asserts ASA "likely tracks an older BS.1770 rev." Two facts make this less obvious than the assertion: + +1. **Essentia's `LoudnessEBUR128`** implements the EBU R128 algorithm, which references BS.1770. The Essentia 2.1b6 docs do not pin a specific BS.1770 revision; the spec it tracks is EBU R128 v3.0 (2014), which is built on BS.1770-3+ with EBU-specific gating. For broadcast-loudness numbers, R128 v3.0 and BS.1770-5 produce values within hundredths of a LU on stereo program material. + +2. **Where BS.1770 has actually changed** (`-3` → `-4` → `-5`): + - `-3` (2012): true-peak metering specified. + - `-4` (2015): multichannel weighting extended to NHK 22.2. + - `-5` (2023): primarily editorial clarifications + true-peak measurement refinements. + +ASA processes stereo audio. The functional delta between Essentia's `LoudnessEBUR128` and an ITU-R BS.1770-5 reference implementation, *on stereo material*, is likely below the precision ASA reports (`round(integrated, 1)` — one decimal place of a LU, per [`analyze_core.py:216`](../apps/backend/analyze_core.py)). + +This does not mean "do nothing." It means **measure before rewriting**: + +- Run ASA on the [EBU R128 test set (Tech 3341 / 3342)](https://tech.ebu.ch/publications/ebu_loudness_test_set). +- Compare `lufsIntegrated` against each track's documented target. +- If ASA's deltas are within ±0.1 LU: the loudness path is *already correct enough*. No port. Add the test set + tolerances as a regression gate (achieves the plan's DoD bullet 4 without any algorithm work). +- If deltas exceed ±0.1 LU: investigate why. Probably an Essentia gating-window or LRA bug, not a revision-of-BS.1770 issue. + +### Spectral reassignment — the real opportunity + +Reassignment is a distinct concept from BS.1770. It sharpens a spectrogram by relocating each STFT bin to the centroid implied by the local time and frequency derivatives of the analyzing window, yielding much sharper transients than vanilla STFT. **This is a real and visible UX improvement** for ASA's spectral artifacts. + +But: + +- The user-value bar is **the spectrogram view**, not the measurement layer. PURPOSE.md invariant 1 says Phase 1 measurements are ground truth; reassignment improves the *picture*, not the number that Phase 2 cites. So it pulls criterion 2 ("more accurate or actionable" — yes, for the visual) and criterion 3 ("act on results" — yes, sharper transient location). Both are legitimate. +- Implementation cost is non-trivial. The reassignment formula needs analytic differentiation of the analyzing window. In Python, the cleanest path is **librosa's `librosa.reassigned_spectrogram`** (already a dependency for `spectral_viz.py`) — no porting of openmeters required. +- **Recommendation: skip openmeters entirely for this.** Stand up `librosa.reassigned_spectrogram` as an opt-in mode behind a `?reassign=1` query param on the spectral-artifact endpoints. Costs: ~half a day to wire and gate; no license risk; no new dependency. + +### Adopt / Adapt / Reject for Track 1 + +| Item | Decision | Why | +| --- | --- | --- | +| Port openmeters' BS.1770-5 loudness to Python | **Reject (as written).** | Premise unverified; Essentia is likely close enough. License risk (GPL-3.0). Wrong runtime target (Rust → Python, not Rust → JS). | +| Add EBU R128 test set as a tolerance regression gate | **Adopt.** | Cheap. Verifies the premise. Satisfies the plan's DoD without algorithmic work. Serves PURPOSE.md criterion 1 (accuracy of ground truth). | +| Use soundscope as an automated oracle | **Reject.** | TUI-only; no JSON output to diff against. Can be a manual sanity check, nothing more. | +| Add spectral reassignment via `librosa.reassigned_spectrogram` (not openmeters) | **Adopt as a separate, smaller track.** | Genuine UX improvement on the spectrogram path. Zero license risk. Native to existing dependency. | +| Verify Essentia's BS.1770 revision in our pinned version | **Adopt as the first step.** | Resolves the ambiguity that makes the rest of Track 1 ambiguous. One Read of the Essentia changelog + one note in `analyze_core.py`. | + +### Definition of done (revised) + +- [ ] Note in `analyze_core.py` documenting which BS.1770 revision Essentia 2.1b6's `LoudnessEBUR128` tracks. +- [ ] EBU R128 test set committed under `apps/backend/tests/fixtures/r128/` with documented target tolerances. +- [ ] A `test_loudness_r128.py` that loads each fixture and asserts `abs(lufsIntegrated - target) < 0.1`. +- [ ] If the test set passes: close Track 1 with no algorithm rewrite. If it fails: triage Essentia, not BS.1770-5. +- [ ] (Independent of the above) `librosa.reassigned_spectrogram` available behind a feature flag for the spectral artifact endpoint. + +--- + +## Track 2 — Result-interchange schema (Partiels) + +### Where ASA's schema lives today + +- Spec: [`apps/backend/JSON_SCHEMA.md`](../apps/backend/JSON_SCHEMA.md) — 60+ top-level keys, deeply nested per-domain detail objects. +- Test snapshot: `EXPECTED_TOP_LEVEL_KEYS` at [`tests/test_analyze.py:26-43`](../apps/backend/tests/test_analyze.py). +- Frontend mirror: [`apps/ui/src/types/measurement.ts`](../apps/ui/src/types/measurement.ts) — `Phase1Result` and its detail interfaces. +- Time-tagged fields today: `lufsCurve.{shortTerm,momentary}[]` (`{t, lufs}`), `spectralBalanceTimeSeries[]` (`{t, subBass, lowBass, …}`), `rhythmDetail.tempoCurve[]` (`{t, bpm}`), `segmentLoudness`/`segmentSpectral`/`segmentStereo`/`segmentKey` (per-section summary objects, not per-frame). +- Export formats: JSON (over HTTP); MIDI for melody/transcription via `apps/ui/src/services/midi/midiExport.ts`. **No CSV, SDIF, LAB, or REAPER export today.** + +### What Partiels exports + +From the [Partiels Manual](https://github.com/Ircam-Partiels/Partiels/blob/main/Docs/Partiels-Manual.md): + +- **CSV:** Three columns — `time`, `duration`, and either `label` (for markers) or one or more numerical values (for points/matrices). Optional header row. +- **JSON:** Per-track export; optional "Include Extra Description" block describes the processor (Vamp plugin) that produced the data. No documented top-level schema. +- **SDIF:** User configures the frame signature and matrix signature per export. No canonical signatures shipped by Partiels. +- **LAB, CUE, REAPER, MAX, PUREDATA:** Tool-specific marker/event formats — mostly time-anchored events. +- **CLI:** Exists; no flags documented in the manual section provided. + +### The framing problem + +The plan says: *"ASA's JSON output should be a subset of what serious analysis tools already emit."* + +This treats Partiels as a richer, established schema for ASA to mirror. The reality is the opposite. Partiels' export shape is *deliberately thin* because it serves a different purpose: feeding analysis results into music software (Max, PD, REAPER) that just needs `(time, duration, value)` tuples per Vamp track. Partiels' "schema" is essentially **one analysis track per file, with a flat row-per-frame structure** — not a domain schema with named, nested measurements. + +ASA's schema is the opposite: **one analysis per audio file, with dozens of named measurement domains** (kickDetail, sidechainDetail, spectralBalance, …), each carefully matched to a Phase 2 prompt obligation. The PURPOSE.md citation chain depends on this naming. Reducing ASA to a Partiels-style flat schema would break the chain of custody (Quality Invariant #2). + +### What's actually useful from Partiels + +The valuable pattern in Partiels' export model is **the tabular CSV view for time-series fields**. ASA has several `[{t, value}]` arrays today that would interoperate well with Max, REAPER, PD if exposed as CSV. This is the right lift. + +A concrete per-field CSV view, modeled on Partiels' `time, duration, value` shape: + +| ASA time-series field | CSV columns | Use case | +| --- | --- | --- | +| `lufsCurve.shortTerm` | `time, duration=3.0, lufs` | Pull a R128 short-term curve into REAPER as a track of automation. | +| `lufsCurve.momentary` | `time, duration=0.4, lufs` | Same, for moment-by-moment dynamics. | +| `rhythmDetail.tempoCurve` | `time, duration, bpm` | Match a reference's tempo drift in an Ableton tempo automation lane. | +| `spectralBalanceTimeSeries` | `time, duration, subBass, lowBass, lowMids, mids, upperMids, highs, brilliance` | EQ-curve evolution per section. | +| `rhythmTimeline.beats[]` + `downbeats[]` | `time, label="beat"|"downbeat"` | Beat grid as markers (matches Partiels' marker CSV exactly). | + +This adds an export channel without disturbing the JSON contract. The HTTP surface would be: + +- `GET /api/analysis-runs/{run_id}/export/csv/{field_path}` — e.g. `…/csv/lufsCurve.shortTerm`. Returns `text/csv`. + +### SDIF defer (with the paragraph the plan asked for) + +SDIF is IRCAM's binary frame format. The full SDIF spec involves frame/matrix signatures (e.g. `1FQ0` for fundamental frequency, `1STF` for STFT) that have no native equivalent in ASA's domain schema. There is no current user demand for SDIF in the ASA target audience (intermediate Ableton producers), and the cost is real: implementing SDIF means either a dependency on libIRCAM-SDIF or writing a binary serializer. + +**Revisit when:** a user requests SDIF *with a concrete downstream use case* (Max patch consumes ASA output, OpenMusic project loads ASA tempograms, etc.), or when ASA grows a domain that overlaps with Vamp's established Vamp/SDIF signatures (e.g. polyphonic transcription producing per-frame fundamentals). + +### Adopt / Adapt / Reject for Track 2 + +| Item | Decision | Why | +| --- | --- | --- | +| Make ASA's JSON "a subset of Partiels' export schemas" | **Reject as written.** | Misreads Partiels' shape. ASA's schema is richer by design; flattening it breaks the chain of custody. | +| Document ASA's JSON shape as an ADR (versioned, frozen contract) | **Adopt.** | JSON_SCHEMA.md exists but doesn't carry an explicit version or stability promise. An ADR ratifies "this is the v1 contract" and defines the compatibility policy. | +| Add a CSV view per time-series field, patterned on Partiels' columns | **Adopt.** | Interop with Max/REAPER/PD without contorting the JSON. New endpoint, additive, no migration cost. | +| Implement SDIF export | **Defer.** | No user demand. Cost real. Revisit when polyphonic transcription stabilizes. | +| Mirror Partiels' "Include Extra Description" idea | **Partially adopt.** | ASA already does this via `diagnostics` (engineVersion, timings, flagsUsed). Document the parallel; no new code. | + +### Definition of done (revised) + +- [ ] `docs/adr/0001-phase1-json-schema-v1.md` — declares the current schema v1, lists invariants, defines the compatibility policy (additive-only minor bumps; renames are major bumps). +- [ ] `GET /api/analysis-runs/{run_id}/export/csv/{field_path}` endpoint with CSV exporters for: `lufsCurve.shortTerm`, `lufsCurve.momentary`, `rhythmDetail.tempoCurve`, `spectralBalanceTimeSeries`, `rhythmTimeline.beats`, `rhythmTimeline.downbeats`. +- [ ] One round-trip test in `tests/services/csvExport.test.ts` that compares CSV columns to JSON values. +- [ ] SDIF deferral note in this document (above) referenced from `BACKLOG.md`. + +--- + +## Track 3 — REST contract (forever-jukebox) + +### Where ASA's API lives today + +From the survey of [`server.py`](../apps/backend/server.py), [`server_phase1.py`](../apps/backend/server_phase1.py), [`server_phase2.py`](../apps/backend/server_phase2.py), [`server_upload.py`](../apps/backend/server_upload.py), and [`analysis_runtime.py`](../apps/backend/analysis_runtime.py): + +**Canonical (run-oriented):** +- `POST /api/analysis-runs` — create run (multipart upload). +- `POST /api/analysis-runs/estimate` — estimate without executing. +- `GET /api/analysis-runs/{run_id}` — snapshot poll (1 s default interval, see `analyzer.ts:16`). +- `POST /api/analysis-runs/{run_id}/interrupt` — cancel. +- `DELETE /api/analysis-runs/{run_id}` — purge. +- `GET /api/analysis-runs/{run_id}/artifacts` and `…/artifacts/{artifact_id}`. +- `POST /api/analysis-runs/{run_id}/spectral-enhancements/{kind}` (CQT, HPSS, onset, chroma). +- `POST /api/analysis-runs/{run_id}/pitch-note-translations`. +- `POST /api/analysis-runs/{run_id}/interpretations`. + +**Legacy:** `POST /api/analyze`, `POST /api/analyze/estimate`, `POST /api/phase2` — wrappers kept for compatibility only ([CLAUDE.md](../CLAUDE.md), "Staged Analysis Runs"). + +**Error envelope:** Always includes `requestId`, `error.code`, `error.message`, `error.retryable`, `diagnostics`. Stage states: `queued`, `running`, `blocked`, `ready`, `completed`, `failed`, `interrupted`, `not_requested`. + +**Upload limit:** 100 MiB raw / 105 MiB request envelope ([`upload_limits.py:7-16`](../apps/backend/upload_limits.py)). + +### What forever-jukebox exposes + +From [`api/README.md`](https://github.com/creightonlinza/forever-jukebox/blob/main/api/README.md): + +- `POST /api/upload` (multipart, 20 MB cap, gated by `ALLOW_USER_UPLOAD=true`). +- `POST /api/analysis/youtube` (`{youtube_id}`) — analyze a YouTube ID. +- `POST /api/analysis/url` (`{url}`) — analyze a URL. +- `GET /api/analysis/` — three response shapes: `{status: "downloading|queued|processing", progress}` (HTTP 202), `{status: "complete", result}` (HTTP 200), `{status: "failed", error}` (HTTP 200). +- `GET /api/search/{spotify,youtube}` — search. +- `GET /api/audio/` — stream the source audio. +- `POST /api/plays/` / `PATCH …` — play tracking (admin via `X-Admin-Key`). +- `GET /api/{top,trending,recent}` — discovery. +- `POST|PUT|GET /api/favorites/sync/...` — multi-device sync. +- `GET /api/jobs/by-source//` / `DELETE /api/jobs/`. +- `GET /api/app-config`. +- Retry: failed jobs auto-reissue on resubmission with the same source. + +### Side-by-side: where ASA leads, matches, lags + +| Dimension | forever-jukebox | ASA today | Verdict | +| --- | --- | --- | --- | +| Async pattern | Poll `GET /api/analysis/` until status flips | Poll `GET /api/analysis-runs/{run_id}` | **Match.** Pattern-equivalent. | +| State machine (public-facing) | 5 states: `downloading | queued | processing | complete | failed` | 8 states: `queued | running | blocked | ready | completed | failed | interrupted | not_requested` | **ASA over-exposes.** `blocked`, `ready`, `not_requested` are useful internally; they leak run-runtime concepts to clients. Worth collapsing in the public response. | +| Error envelope | Inline `{error}` blob in the response; no documented `requestId` / `retryable` flag | `requestId`, `error.code`, `error.message`, `error.retryable`, `diagnostics` | **ASA leads.** Don't regress. | +| Input methods | Upload + YouTube ID + URL | Upload only | **forever-jukebox leads.** URL ingestion would help producers analyze reference tracks they don't have a local file for. YouTube has TOS hazards; URL is clean. | +| Upload size | 20 MB | 100 MiB | **ASA leads** — and the difference matters for long, full-bit-rate FLAC reference tracks. | +| Job retry | Auto-creates a new job on resubmission of a retryable failure | Client must explicitly recreate the run; `error.retryable` is advisory | **forever-jukebox slightly leads.** ASA's `retryable` flag is informative; turning it into automatic retry on resubmit would close the loop. | +| Discovery (top/trending/recent) | Yes | No | **Out of scope** for ASA's target user — they have a specific reference track, not a feed. | +| Search (Spotify/YouTube) | Yes | No | **Out of scope** for ASA — pulling rights-restricted audio is a separate product question. | +| Admin auth | `X-Admin-Key` header | None at the canonical-API layer ([`auth_context.py`](../apps/backend/auth_context.py) handles hosted-mode user context but not admin) | **forever-jukebox leads operationally**; ASA has a forward design for hosted auth but no admin-key surface for operator tasks (purge a run, audit a job). Worth filing. | +| Audio re-serve | `GET /api/audio/` | No | **Worth considering** — ASA already persists artifacts; re-serving the original audio at the staged endpoint avoids re-uploading on Phase 2 reruns. | + +### Adopt / Adapt / Reject per endpoint + +| forever-jukebox endpoint | Decision | Notes | +| --- | --- | --- | +| `POST /api/analysis/url` | **Adopt** | Add `POST /api/analysis-runs` URL-mode variant (multipart with `url` field instead of `file`). Server fetches, stores via `artifact_storage.py`, then runs the same pipeline. Respect `upload_limits.py`. | +| `POST /api/analysis/youtube` | **Reject** | YouTube ToS; would change ASA's risk profile. URL ingestion covers the underlying need for licit sources. | +| Retry on resubmission | **Adapt** | Use ASA's `error.retryable` flag as the signal; if a client POSTs `/api/analysis-runs` with the same `source_hash` and the prior run failed retryably, return the new run's `run_id` (idempotency-style). | +| `GET /api/audio/` | **Adopt** | `GET /api/analysis-runs/{run_id}/source-audio` — serves the original file from `artifact_storage.py`. Removes the need to re-upload for Phase 2 reruns. | +| Collapsed public state machine | **Adapt** | Keep the 8 internal states; expose 5 in the public snapshot. Map `blocked` and `not_requested` to `pending`; map `ready` to `queued`. Document the collapse in JSON_SCHEMA.md. | +| `X-Admin-Key` for admin ops | **Adopt** | Apply to `DELETE /api/analysis-runs/{run_id}` and a forthcoming `GET /api/admin/runs` listing. Gate via existing `auth_context.py`. | +| Search / discovery / favorites / play tracking | **Reject** | Out of scope for the recreation-blueprint mission. PURPOSE.md decision-framework branch 5: "stop and reconsider." | +| Echo Nest-style beats/segments/sections output | **Reject** | ASA emits richer, named domain measurements ([`JSON_SCHEMA.md`](../apps/backend/JSON_SCHEMA.md)). Adopting Echo Nest's flat segment schema would collapse the citation chain (Quality Invariant #2). | + +### Definition of done (revised) + +- [ ] Side-by-side endpoint comparison committed (this section is the seed; expand into `docs/api-comparison-2026-05-13.md` if it gets longer). +- [ ] ADR per "adopt" decision: URL input, admin key, audio re-serve, collapsed public state machine. +- [ ] API doc updated (currently lives in [`apps/backend/ARCHITECTURE.md`](../apps/backend/ARCHITECTURE.md)) — specifically the public state-machine collapse, since it's the only contract-shape change. + +--- + +## Risks & Watch-outs + +| Risk | Track | Mitigation | +| --- | --- | --- | +| GPL contamination from reading openmeters/Partiels source | 1, 2 | Read ITU-R BS.1770-5 (PDF, gratis from ITU) and Partiels' user manual instead of source. Document algorithmic provenance per file. | +| BS.1770 revision premise is wrong (Essentia already does -3 to -5 equivalent for stereo) | 1 | Run the verification spike first. Cheapest possible disproof. | +| Spectral-reassignment requires WASM | 1 | False risk — use `librosa.reassigned_spectrogram` server-side. Same dependency family as `spectral_viz.py`. | +| CSV exporter explosion (one endpoint per field path) | 2 | Generic `…/export/csv/{field_path}` resolves a JSONPath into the snapshot. One endpoint, finite serializers per type. | +| State-machine collapse breaks legacy clients | 3 | The legacy clients are ASA's own UI; map at the server boundary. Bump the `phase1` envelope version if external consumers ever exist. | +| URL ingestion exposes the server to arbitrary network egress | 3 | Limit to a configurable allowlist of hosts in hosted-mode; in local mode, allow all. Reuse `upload_limits.py` to cap downloaded bytes. | + +--- + +## Sequencing + +Same order as the plan, different reasons: + +1. **Track 2 — schema work, scoped down.** ~2-3 days. ADR + CSV exporters for six time-series fields. SDIF deferred with this document as the rationale. +2. **Track 3 — API audit, targeted lift.** ~2-3 days. URL input + state-machine collapse + admin key + audio re-serve. The Echo Nest schema is left alone. +3. **Track 1 — verification spike first; full rewrite gated.** ~1 day spike. EBU R128 test set + tolerance assertion. If green: close as a regression test addition (no algorithm work). If red: triage Essentia. Spectral reassignment as a separate ~1-day add via `librosa.reassigned_spectrogram`, no openmeters dependency. + +**Total estimated effort if all three adopted:** 6-8 working days, dominated by Tracks 2 and 3. This is roughly half of what the plan implies because the most expensive items (porting openmeters, mirroring Partiels' SDIF, full Echo Nest schema parity) are rejected. + +--- + +## What was *not* evaluated + +The plan's "Out of scope" list is correct: + +- **spleeter-web** — revisit when ASA adds stems beyond Demucs. +- **Olaf** — revisit if cross-take alignment becomes a feature. +- **nightingale** — desktop shell; not relevant until/unless ASA grows a desktop app. +- **ChordMiniApp / chordonomicon / rawl** — harmonic-domain tools; flagged separately in [`BACKLOG.md`](../BACKLOG.md) as a Harmonia track. + +No change to the deferral logic. + +--- + +## Sources + +- Anchoring docs in this repo: [PURPOSE.md](../PURPOSE.md), [CLAUDE.md](../CLAUDE.md), [apps/backend/JSON_SCHEMA.md](../apps/backend/JSON_SCHEMA.md), [apps/backend/analyze_core.py](../apps/backend/analyze_core.py), [apps/backend/upload_limits.py](../apps/backend/upload_limits.py). +- [openmeters on GitHub](https://github.com/httpsworldview/openmeters) (GPL-3.0). +- [bananaofhappiness/soundscope on GitHub](https://github.com/bananaofhappiness/soundscope) (MIT). +- [Ircam-Partiels/Partiels on GitHub](https://github.com/Ircam-Partiels/Partiels) (GPL-3.0). +- [Partiels Manual](https://github.com/Ircam-Partiels/Partiels/blob/main/Docs/Partiels-Manual.md) — export format documentation. +- [creightonlinza/forever-jukebox on GitHub](https://github.com/creightonlinza/forever-jukebox) (MIT). +- [forever-jukebox API README](https://github.com/creightonlinza/forever-jukebox/blob/main/api/README.md). +- [ITU-R BS.1770-5 (Nov 2023, PDF)](https://www.itu.int/dms_pubrec/itu-r/rec/bs/R-REC-BS.1770-5-202311-I!!PDF-E.pdf). +- [EBU R128 test set (EBU Tech 3341/3342)](https://tech.ebu.ch/publications/ebu_loudness_test_set). +- [librosa.reassigned_spectrogram](https://librosa.org/doc/main/generated/librosa.reassigned_spectrogram.html). From 0f8e2b9d0c5123ed595a3cedb58cbfc1d5e18c32 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 05:24:36 +0000 Subject: [PATCH 2/3] docs(review): fix upload-envelope MiB math and JSONPath term Two corrections to docs/external-repo-review-2026-05-13.md per PR #33 review: 1. Track 3 baseline said '105 MiB request envelope'. Correct value is 101 MiB: MAX_UPLOAD_SIZE_BYTES (104,857,600) + UPLOAD_REQUEST_SIZE_SLACK_BYTES (1,048,576) = 105,906,176 bytes = 101 MiB exactly. The PR description already had this right; the doc body did not. Added the arithmetic inline so the next person doesn't have to redo it. 2. Risks table called the CSV exporter route a 'JSONPath' resolver. What's described is a simple dot-path (e.g. 'lufsCurve.shortTerm'), not JSONPath syntax (which uses '$.' and '[*]'). Clarified to avoid anyone reaching for a JSONPath library when implementing. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg --- docs/external-repo-review-2026-05-13.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/external-repo-review-2026-05-13.md b/docs/external-repo-review-2026-05-13.md index d9413bd8..f99afb08 100644 --- a/docs/external-repo-review-2026-05-13.md +++ b/docs/external-repo-review-2026-05-13.md @@ -199,7 +199,7 @@ From the survey of [`server.py`](../apps/backend/server.py), [`server_phase1.py` **Error envelope:** Always includes `requestId`, `error.code`, `error.message`, `error.retryable`, `diagnostics`. Stage states: `queued`, `running`, `blocked`, `ready`, `completed`, `failed`, `interrupted`, `not_requested`. -**Upload limit:** 100 MiB raw / 105 MiB request envelope ([`upload_limits.py:7-16`](../apps/backend/upload_limits.py)). +**Upload limit:** 100 MiB raw / 101 MiB request envelope ([`upload_limits.py:7-16`](../apps/backend/upload_limits.py) — `MAX_UPLOAD_SIZE_BYTES` 104,857,600 + `UPLOAD_REQUEST_SIZE_SLACK_BYTES` 1,048,576 = 105,906,176 bytes = 101 MiB). ### What forever-jukebox exposes @@ -261,7 +261,7 @@ From [`api/README.md`](https://github.com/creightonlinza/forever-jukebox/blob/ma | GPL contamination from reading openmeters/Partiels source | 1, 2 | Read ITU-R BS.1770-5 (PDF, gratis from ITU) and Partiels' user manual instead of source. Document algorithmic provenance per file. | | BS.1770 revision premise is wrong (Essentia already does -3 to -5 equivalent for stereo) | 1 | Run the verification spike first. Cheapest possible disproof. | | Spectral-reassignment requires WASM | 1 | False risk — use `librosa.reassigned_spectrogram` server-side. Same dependency family as `spectral_viz.py`. | -| CSV exporter explosion (one endpoint per field path) | 2 | Generic `…/export/csv/{field_path}` resolves a JSONPath into the snapshot. One endpoint, finite serializers per type. | +| CSV exporter explosion (one endpoint per field path) | 2 | Generic `…/export/csv/{field_path}` resolves a dot-path (e.g. `lufsCurve.shortTerm`) into the snapshot — not JSONPath, just nested-key descent. One endpoint, finite serializers per type. | | State-machine collapse breaks legacy clients | 3 | The legacy clients are ASA's own UI; map at the server boundary. Bump the `phase1` envelope version if external consumers ever exist. | | URL ingestion exposes the server to arbitrary network egress | 3 | Limit to a configurable allowlist of hosts in hosted-mode; in local mode, allow all. Reuse `upload_limits.py` to cap downloaded bytes. | From 8c7da4cc09030cf8d51ae1149b0595dd377d475b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 13 May 2026 05:33:15 +0000 Subject: [PATCH 3/3] =?UTF-8?q?test(loudness):=20Track=201=20verification?= =?UTF-8?q?=20spike=20=E2=80=94=20EBU=20R128=20Tech=203341=20cases=201=20&?= =?UTF-8?q?=202?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds tests/test_loudness_r128.py that exercises ASA's analyze_loudness path against the EBU R128 Tech 3341 compliance signals: - Case 1: stereo 1 kHz sine at -23 dB FS, 20 s @ 44.1 kHz → expected integrated loudness -23.0 ±0.1 LUFS. - Case 2: stereo 1 kHz sine at -33 dB FS, 20 s @ 44.1 kHz → expected -33.0 ±0.1 LUFS. - Case 1 at 48 kHz with explicit sampleRate=48000 (bypasses analyze_loudness): proves the algorithm itself is sample-rate aware when Essentia is told the right rate. Signals are generated procedurally — no fixture downloads, no audio files committed. This satisfies the review's recommendation in docs/external-repo-review-2026-05-13.md to verify ASA's loudness path against EBU R128 before any port of openmeters' BS.1770-5 implementation. If both 44.1 kHz tests pass on CI, the openmeters port stays rejected and this becomes the regression gate. Open finding surfaced during the spike — filed as a follow-up, NOT fixed here: - analyze_core.analyze_loudness instantiates LoudnessEBUR128() with no sampleRate (defaults to 44100), but load_stereo returns audio at the source file's native rate without resampling. analyze_fast.py already does the right thing (sampleRate=sample_rate). Documented in docs/track1-spike-outcome-2026-05-13.md with a concrete fix sketch and the affected call sites. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg --- apps/backend/tests/test_loudness_r128.py | 191 +++++++++++++++++++++++ docs/track1-spike-outcome-2026-05-13.md | 75 +++++++++ 2 files changed, 266 insertions(+) create mode 100644 apps/backend/tests/test_loudness_r128.py create mode 100644 docs/track1-spike-outcome-2026-05-13.md diff --git a/apps/backend/tests/test_loudness_r128.py b/apps/backend/tests/test_loudness_r128.py new file mode 100644 index 00000000..3a2448c9 --- /dev/null +++ b/apps/backend/tests/test_loudness_r128.py @@ -0,0 +1,191 @@ +"""EBU R128 verification spike for ASA's loudness path. + +Track 1 of the external-repo incorporation plan (docs/external-repo-review-2026-05-13.md) +asked: is ASA's integrated-LUFS implementation correct, or does it lag the BS.1770-5 +revision? This module answers that with the EBU Tech 3341 compliance signals — the +canonical loudness conformance test set. + +What we test: + +1. Tech 3341 Case 1 — stereo 1 kHz sine at -23.0 dB FS for 20 s. + Expected integrated loudness: -23.0 ±0.1 LUFS. + +2. Tech 3341 Case 2 — stereo 1 kHz sine at -33.0 dB FS for 20 s. + Expected integrated loudness: -33.0 ±0.1 LUFS. + +The ±0.1 LU tolerance is the EBU R128 compliance gate for "EBU Mode" loudness +meters. If both cases pass on ASA's actual call path (`analyze_loudness` → +Essentia's `LoudnessEBUR128`), the review's premise check is positive: no +algorithm rewrite needed. + +Both tests run at 44.1 kHz because that is the sample rate ASA's full pipeline +uses for the LUFS call (`analyze_core.py:197` calls `LoudnessEBUR128()` with no +explicit `sampleRate`, defaulting to 44100). A third test calls Essentia +directly at 48 kHz with the sample rate threaded through, demonstrating that +the algorithm itself is correct at non-44.1 rates when given the right input — +which is the case `analyze_fast.py:100` already handles. The gap between +`analyze_core` and `analyze_fast` on sample-rate threading is logged as a +follow-up finding from this spike (not fixed here — out of scope for a +verification-only spike). + +All synthetic signals are generated procedurally; no test fixtures are +downloaded or committed. +""" + +from __future__ import annotations + +import math +import sys +import unittest +from pathlib import Path + +import numpy as np + + +_BACKEND_ROOT = Path(__file__).resolve().parents[1] +if str(_BACKEND_ROOT) not in sys.path: + sys.path.insert(0, str(_BACKEND_ROOT)) + + +try: + import essentia.standard as es # noqa: F401 + from analyze_core import analyze_loudness + ESSENTIA_AVAILABLE = True +except Exception: # pragma: no cover - guarded by skip + ESSENTIA_AVAILABLE = False + es = None # type: ignore[assignment] + analyze_loudness = None # type: ignore[assignment] + + +# EBU R128 compliance tolerance for integrated loudness, "EBU Mode" meters. +LUFS_TOLERANCE = 0.1 + + +def _make_stereo_sine( + peak_dbfs: float, + duration_s: float, + sample_rate: int, + freq_hz: float = 1000.0, +) -> np.ndarray: + """Generate a stereo sine tone at a given peak dB FS. + + Returns shape ``(N, 2)`` float32 to match what Essentia's ``AudioLoader`` + returns (i.e. what ``analyze_loudness`` expects). The two channels carry + the same signal — the EBU Tech 3341 cases 1 and 2 are defined as + identical L/R, which sums to +3 dB stereo per BS.1770 channel weighting + (mono mono per channel, summed before K-weighting integration). + """ + n_samples = int(round(duration_s * sample_rate)) + t = np.arange(n_samples, dtype=np.float64) / float(sample_rate) + peak_amplitude = 10.0 ** (peak_dbfs / 20.0) + mono = peak_amplitude * np.sin(2.0 * math.pi * freq_hz * t) + stereo = np.stack([mono, mono], axis=-1).astype(np.float32) + return stereo + + +@unittest.skipUnless(ESSENTIA_AVAILABLE, "Essentia not available in test env") +class TestLoudnessR128Tech3341(unittest.TestCase): + """EBU R128 Tech 3341 compliance probes at ASA's operating sample rate.""" + + SAMPLE_RATE_HZ = 44_100 + DURATION_S = 20.0 + + def test_case1_minus23_dbfs_sine_yields_minus23_lufs(self) -> None: + """Case 1: -23.0 dB FS stereo 1 kHz sine → -23.0 ±0.1 LUFS integrated.""" + stereo = _make_stereo_sine( + peak_dbfs=-23.0, + duration_s=self.DURATION_S, + sample_rate=self.SAMPLE_RATE_HZ, + ) + + result = analyze_loudness(stereo) + integrated = result.get("lufsIntegrated") + + self.assertIsNotNone( + integrated, + "analyze_loudness returned None for lufsIntegrated on a clean sine", + ) + self.assertAlmostEqual( + integrated, + -23.0, + delta=LUFS_TOLERANCE, + msg=( + f"EBU Tech 3341 case 1: expected -23.0 ±{LUFS_TOLERANCE} LUFS, " + f"got {integrated} LUFS. Either ASA's loudness path drifted, " + f"the K-weighting filter is mis-tuned, or Essentia's " + f"LoudnessEBUR128 default at 44.1 kHz disagrees with R128." + ), + ) + + def test_case2_minus33_dbfs_sine_yields_minus33_lufs(self) -> None: + """Case 2: -33.0 dB FS stereo 1 kHz sine → -33.0 ±0.1 LUFS integrated.""" + stereo = _make_stereo_sine( + peak_dbfs=-33.0, + duration_s=self.DURATION_S, + sample_rate=self.SAMPLE_RATE_HZ, + ) + + result = analyze_loudness(stereo) + integrated = result.get("lufsIntegrated") + + self.assertIsNotNone(integrated) + self.assertAlmostEqual( + integrated, + -33.0, + delta=LUFS_TOLERANCE, + msg=( + f"EBU Tech 3341 case 2: expected -33.0 ±{LUFS_TOLERANCE} LUFS, " + f"got {integrated} LUFS. A pass on case 1 but a fail here would " + f"point at a non-linearity in the integration path." + ), + ) + + +@unittest.skipUnless(ESSENTIA_AVAILABLE, "Essentia not available in test env") +class TestLoudnessR128AtNon441kHz(unittest.TestCase): + """Verifies the BS.1770 algorithm itself is correct at 48 kHz. + + This test bypasses ``analyze_loudness`` and calls Essentia's + ``LoudnessEBUR128(sampleRate=48000)`` directly. Its purpose is to + establish that Essentia is sample-rate aware — so any LUFS error + observed on 48 kHz sources via ``analyze_loudness`` is a *call-site + bug* (sample rate not threaded), not an algorithm bug. + + This is the open finding from the spike documented in + docs/external-repo-review-2026-05-13.md (Track 1 follow-up): the + full pipeline calls ``analyze_loudness(stereo)`` after + ``load_stereo`` returns audio at the source's native rate, so any + 48 kHz source today gets measured against 44.1 kHz K-weighting. + """ + + SAMPLE_RATE_HZ = 48_000 + DURATION_S = 20.0 + + def test_case1_at_48khz_with_explicit_sample_rate(self) -> None: + """At 48 kHz, Essentia must produce -23.0 ±0.1 LUFS for case 1 + when given the correct sample rate. + """ + stereo = _make_stereo_sine( + peak_dbfs=-23.0, + duration_s=self.DURATION_S, + sample_rate=self.SAMPLE_RATE_HZ, + ) + + loudness = es.LoudnessEBUR128(sampleRate=self.SAMPLE_RATE_HZ) + _momentary, _short_term, integrated, _lra = loudness(stereo) + + self.assertAlmostEqual( + float(integrated), + -23.0, + delta=LUFS_TOLERANCE, + msg=( + f"Essentia LoudnessEBUR128 at 48 kHz with explicit sampleRate " + f"must match -23.0 ±{LUFS_TOLERANCE} LUFS. Got {integrated}. " + f"If this fails, the algorithm itself is suspect at non-44.1 " + f"rates and the call-site fix would not help." + ), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/track1-spike-outcome-2026-05-13.md b/docs/track1-spike-outcome-2026-05-13.md new file mode 100644 index 00000000..fea747c7 --- /dev/null +++ b/docs/track1-spike-outcome-2026-05-13.md @@ -0,0 +1,75 @@ +# Track 1 verification spike outcome — 2026-05-13 + +**Scope:** verify the review's premise (in [`external-repo-review-2026-05-13.md`](external-repo-review-2026-05-13.md)) +that ASA's existing loudness path is already correct on its operating +sample rate, so no port of openmeters' BS.1770-5 implementation is needed. + +**Result (pending CI):** The added test +[`apps/backend/tests/test_loudness_r128.py`](../apps/backend/tests/test_loudness_r128.py) +exercises EBU Tech 3341 Cases 1 and 2 — the canonical sine-tone +conformance signals — through ASA's `analyze_loudness` at 44.1 kHz. +If those assert green, the premise check is positive and Track 1's +loudness-port subtrack closes with no algorithm work, per the review. + +## What the spike verifies + +Three procedurally-generated tests; no fixture downloads. + +| Test | Signal | Expected integrated LUFS | Path under test | +| --- | --- | --- | --- | +| `test_case1_minus23_dbfs_sine_yields_minus23_lufs` | Stereo 1 kHz sine, -23 dB FS peak, 20 s @ 44.1 kHz | -23.0 ±0.1 | `analyze_core.analyze_loudness(stereo)` | +| `test_case2_minus33_dbfs_sine_yields_minus33_lufs` | Stereo 1 kHz sine, -33 dB FS peak, 20 s @ 44.1 kHz | -33.0 ±0.1 | `analyze_core.analyze_loudness(stereo)` | +| `test_case1_at_48khz_with_explicit_sample_rate` | Stereo 1 kHz sine, -23 dB FS peak, 20 s @ 48 kHz | -23.0 ±0.1 | `es.LoudnessEBUR128(sampleRate=48000)` directly | + +The third test calls Essentia directly with the sample rate threaded +through. It separates "algorithm correct at non-44.1 rates" from "ASA's +call site threads sample rate." If it passes, Essentia's algorithm is +fine at 48 kHz when given the right sample rate; the call-site question +is then orthogonal to BS.1770 compliance. + +## Open finding: sample-rate threading in `analyze_loudness` + +While tracing the loudness path, the spike surfaced an asymmetry between +ASA's two call sites for `LoudnessEBUR128`: + +- [`analyze_core.py:197`](../apps/backend/analyze_core.py) — `analyze_loudness(stereo)` instantiates `es.LoudnessEBUR128()` with **no** `sampleRate` argument (Essentia's default is 44100). +- [`analyze_fast.py:100`](../apps/backend/analyze_fast.py) — `analyze_fast` instantiates `es.LoudnessEBUR128(sampleRate=sample_rate)` with the actual sample rate. + +The full pipeline (`analyze.py:1448`) loads stereo via `load_stereo`, which +preserves the **source file's native sample rate** (no upstream resample). +So any source that isn't already 44.1 kHz — e.g. a 48 kHz FLAC reference +track — gets its loudness measured against K-weighting filters tuned for +44.1 kHz. For BS.1770's K-filter, this is small at 1 kHz but grows with +frequency; the resulting integrated-LUFS error on broadband program +material is bounded but non-zero. + +This is **not** an algorithm bug. It is a call-site bug. It does not +invalidate the spike — the verification tests run at 44.1 kHz precisely +because that is `analyze_loudness`'s assumption, so they probe ASA's +actual operating point. But it is worth fixing in a separate, small PR: + +```python +# apps/backend/analyze_core.py +def analyze_loudness(stereo: np.ndarray, sample_rate: int = 44_100) -> dict: + ... + loudness = es.LoudnessEBUR128(sampleRate=sample_rate) + ... +``` + +Plus call-site updates at `analyze.py:1242` (stem analysis path), +`analyze.py:1497` (full pipeline), and any other invocation. + +I deliberately did not include the fix in this spike PR — the spike's +scope per the user's selection was "verify only." Filed as a follow-up. + +## What this means for the review's Track 1 + +Assuming both 44.1 kHz tests pass: + +- [ ] Cancel the openmeters port. The review already rejected that on + license, premise, and runtime-target grounds; this verification is + the empirical close-out. +- [x] Keep this regression test in place as the loudness-correctness gate. +- [ ] File the sample-rate threading fix as a separate small PR. +- [ ] Consider `librosa.reassigned_spectrogram` as a separate ~1-day add + for sharper spectrograms (unchanged from the review's recommendation).