fix(loudness): thread sample_rate through analyze_loudness - #34
Conversation
Closes the open finding from the Track 1 verification spike (#33 / docs/track1-spike-outcome-2026-05-13.md). Before: analyze_core.analyze_loudness instantiated es.LoudnessEBUR128() with no sampleRate (Essentia default 44100), while the full pipeline at analyze.py:1497 passed in a stereo array from load_stereo at the source file's native rate (no upstream resample). Any source above 44.1 kHz got measured against K-weighting coefficients tuned for the wrong rate. The bias is small at 1 kHz but grows with frequency. After: - analyze_loudness now takes sample_rate: int = 44_100 and passes it to LoudnessEBUR128(sampleRate=sample_rate). - analyze.py:1503 (full pipeline) threads sr from load_stereo through. - analyze.py:1242 (stem path) hardcodes 44_100 because Demucs always writes stems at 44.1 kHz regardless of source rate (see the inline comment for why the function's sample_rate parameter is not the right value for this call site). - analyze_fast.py was already correct; analyze_segments.py was already correct. No change to those. Regression test: - New TestLoudnessR128ThroughAnalyzeLoudnessAt48kHz class in tests/test_loudness_r128.py asserts -23.0 ±0.1 LUFS through analyze_loudness(stereo, sample_rate=48000). This would have failed on the prior code (the integrated LUFS would have drifted from the expected value because the K-filter was mis-tuned). Docs: - track1-spike-outcome-2026-05-13.md updated to mark the open finding as resolved, with a pointer to this branch and the new regression test. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
slittycode
left a comment
slittycode
left a comment
There was a problem hiding this comment.
Verdict: COMMENT
Summary
Correct fix for a genuine measurement bug: analyze_loudness was instantiating LoudnessEBUR128() with no sampleRate argument on the full-mix path, so any 48/96 kHz source got K-weighting coefficients tuned for 44.1 kHz. The approach — backward-compatible default, thread sr from load_stereo on the full-mix path, hardcode 44_100 on the stem path — is right. The Demucs stem hardcode is independently confirmed by analyze_audio_io._load_stem_stereo's docstring and line 112 (sample_rate = 44100 # HDEMUCS_HIGH_MUSDB_PLUS target rate). No phase violations. CI still running at review time.
Findings
Should fix
tests/test_loudness_r128.py:21–28 — module docstring is now stale. Two claims are false after this PR:
analyze_core.py:197callsLoudnessEBUR128()with no explicitsampleRate, defaulting to 44100
That's no longer true — line 204 now passes sampleRate=sample_rate.
The gap between
analyze_coreandanalyze_faston sample-rate threading is logged as a follow-up finding from this spike (not fixed here — out of scope for a verification-only spike).
This PR is exactly that fix. A future reader tracing the test file's intent will be misled into thinking the gap is still open. The docstring needs updating in this PR since it's in the same file being modified.
Worth considering
The new test TestLoudnessR128ThroughAnalyzeLoudnessAt48kHz uses a 1 kHz sine. K-weighting bias between 44.1 kHz and 48 kHz coefficient sets at 1 kHz is well under 0.05 LU — the test would likely pass even if sample_rate were not threaded through to Essentia (i.e., if the parameter existed but the fix was reverted). The test proves the function is callable and the parameter is wired, but it's not a tight regression gate for the bug itself. A broadband signal (or even a 6–8 kHz tone, where the K-weighting shelf makes the filter more sample-rate-sensitive) would actually fail against ±0.1 LU tolerance on the pre-fix code. This is worth knowing if you add more coverage later; it doesn't block this PR.
Test results
CI in progress at review time. Existing suite has three tests in this module: the two 44.1 kHz Tech 3341 cases (unaffected — they call analyze_loudness(stereo) without the new parameter, using the default, which is unchanged) and the direct-Essentia 48 kHz probe. All three pass before and after the fix by design. The new test would have failed on the prior code — that's the only new pass gate, and it depends on the 1 kHz caveat above.
Phase boundary check
Clean. No files under phase2/ or phase3/ touched. Phase 1 output schema unchanged — same fields, corrected values for non-44.1 kHz inputs.
Generated by Claude Code
… limit Per PR #34 review: 1. Should fix: module docstring was stale after the fix. It still claimed analyze_core.py:197 calls LoudnessEBUR128() without sampleRate (no longer true — sampleRate=sample_rate is now passed) and that the gap to analyze_fast was a follow-up (this PR closes that gap). Rewrote the docstring to match the post-fix state and the four tests now in the file. 2. Worth considering: the new 48 kHz test through analyze_loudness uses a 1 kHz tone, which the reviewer noted is too K-weighting- insensitive to tightly prove the sample_rate argument actually reaches Essentia (it could be silently swallowed and the test would still pass). Added a NOTE block at the bottom of the file documenting this coverage limit and the two paths a tighter wiring test could take (mocking, or broadband fixture against a pre-computed reference). Did not add a divergence-based wiring test here because Essentia's K-filter coefficient adaptation behavior is uncertain enough that any divergence threshold I'd pick risks failing the test for the wrong reason. Filed as a future improvement instead of risking flaky coverage. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Per PR #34 review (Worth considering): the 1 kHz Tech 3341 tests cannot tightly prove that the sample_rate argument actually reaches Essentia, because K-weighting bias between 44.1 kHz and 48 kHz coefficient sets is under 0.05 LU at 1 kHz and analyze_loudness rounds to one decimal — so a silently-swallowed parameter would pass the tolerance assertion. This commit closes that coverage gap with three white-box mock tests in TestAnalyzeLoudnessThreadsSampleRateToEssentia. Each patches analyze_core.es.LoudnessEBUR128 and asserts: - explicit sample_rate=48_000 reaches LoudnessEBUR128(sampleRate=…) - default (no kwarg) results in sampleRate=44_100 - sample_rate=96_000 is passed verbatim (no clamping or normalization) The previous "NOTE on coverage limits" block is removed — the tests now cover what the note flagged. Module docstring updated to list the wiring class alongside the four tolerance tests. Trade-off acknowledged: this is white-box, tied to the constructor shape es.LoudnessEBUR128(sampleRate=…). If Essentia ever renames the kwarg, these tests will fail and that's the right behavior — the production code would break too. The wiring contract is the kwarg name. https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
2 participants
Closes the open finding from PR #33's Track 1 verification spike (
docs/track1-spike-outcome-2026-05-13.md).The bug
analyze_core.pydefinedanalyze_loudness(stereo)and instantiatedes.LoudnessEBUR128()with nosampleRateargument (Essentia's default is 44100). But the full pipeline atanalyze.pycallsload_stereo(audio_path)which returns audio at the source file's native sample rate without resampling. Any 48 kHz / 96 kHz reference track got its loudness measured against K-weighting filters tuned for 44.1 kHz. The bias is small at 1 kHz but grows with frequency on broadband program material.analyze_fast.py:100andanalyze_segments.py:287already threadedsampleRatecorrectly. The full-mix and stem paths inanalyze.pydid not. This PR closes that gap.What this PR changes
apps/backend/analyze_core.pyanalyze_loudness(stereo, sample_rate: int = 44_100) -> dict— backward-compatible signature; passessampleRate=sample_ratetoLoudnessEBUR128. Docstring explains the K-weighting filter's sample-rate dependence and points to the regression test.apps/backend/analyze.py:1503(full pipeline)srfromload_stereothrough:analyze_loudness(stereo, sample_rate=sr).apps/backend/analyze.py:1242(stem path)sample_rate=44_100. Demucs writes stems at 44.1 kHz regardless of source rate —_load_stem_stereodoes not resample, and the function'ssample_rateparameter targets the mono path (resampled), not the stereo stem. Inline comment captures the rationale.apps/backend/tests/test_loudness_r128.pyTestLoudnessR128ThroughAnalyzeLoudnessAt48kHzclass — end-to-end regression gate. Assertsanalyze_loudness(stereo, sample_rate=48000)yields -23.0 ±0.1 LUFS on the EBU Tech 3341 case 1 signal.docs/track1-spike-outcome-2026-05-13.mdWhat this PR does NOT change
analyze_fast.pyandanalyze_segments.pywere already correct. Untouched.Phase1ResultJSON shape, the HTTP envelope, orEXPECTED_TOP_LEVEL_KEYS. The fix is internal to the measurement path; downstream consumers see the same fields with corrected values for non-44.1 kHz sources.Verification
tests/test_loudness_r128.py(cases 1 and 2 at 44.1 kHz, plus the direct-Essentia 48 kHz probe) continue to assert ±0.1 LU compliance. They pass before and after this change because they don't go through the buggy code path.analyze_loudnessat 48 kHz — it would have failed on the prior code and passes after the fix. CI is the proof.https://claude.ai/code/session_01Uh2eNT1oqjixcox8UX3BTg
Generated by Claude Code