fix(audio): resample MLX outputs to the declared sample rate - #2106
Conversation
|
The PR appears safe to merge with no concrete correctness, security, or repository-rule violations identified. SummaryNormalizes MLX-Audio output to the backend’s declared 24 kHz contract.
Reviews (3) · Last reviewed commit: "fix(mlx): retain resampling context betw..." |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughMLX audio output now groups consecutive chunks by reported sample rate and resamples each group to the backend rate before concatenation. Tests cover primary and fallback generation, mixed rates, grouped resampling, and raw audio without rate metadata. Documentation and the changelog describe the behavior. ChangesMLX audio sample-rate normalization
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Audio from supported MLX generation paths is normalized to the declared output rate while preserving the expected output shape. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 7 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (7 passed)
Full details: Title checkExplanation The title follows Conventional Commit style and accurately describes the MLX sample-rate fix, but it does not include the required issue reference. The provided description also does not contain an issue reference. Full details: Docstring CoverageExplanation Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
MLX-Audio results from a non-24 kHz model are currently played at the wrong speed and pitch. The adapter always advertises 24,000 Hz but discards each
GenerationResult.sample_rate; the locked mlx-audio 0.5.0 Dia implementation emits 44,100 Hz. A one-second Dia waveform is consequently treated as 1.8375 seconds of audio. This also affects duration calculations, inline pauses, crossfades and export._run_backend_inferencereads the rate before callinggenerate, so changing the adapter's rate only after generation would leave that first render inconsistent. The GPT-SoVITS adapter already resamples foreign-rate outputs to its declared rate.Changes
Type
Testing
On this standalone MLX branch, the regression and neighboring VoiceDesign tests were rerun with the existing isolated Python environment:
python -m pytest tests/test_mlx_audio_sample_rate.py tests/test_mlx_audio_instruct_1405.py -q --tb=short— 13 passed. The test harness explicitly asserts before and after pytest thatservices.tts_backend.__file__resolves inside this worktree.git apply --checkandgit diff --cached --checkpassed.Current-main integration check: this patch was also applied alone to
dad59c1318f18492b93ec81e55f49bb2bee458e5in a separate worktree. The two targeted files reproduced 5 failures / 8 passes before the production fix; the seven MLX/engine/chunk/dub test files listed below passed 88 tests afterward. The imported adapter path was asserted before and after pytest, and its MLX class matches this PR. The environment used temporary application data,HF_HUB_OFFLINE=1and an empty HF cache. The newer main branch's full backend/frontend/native matrix was not rerun.Validation scope: the broader checks listed below (87 related Python tests, 25 policy checks, frontend, Rust, and full-suite attempts) were run earlier on the reviewed SHA with both the MLX sample-rate patch and the independent ffprobe sibling-path patch applied. They were not rerun wholesale on this standalone branch. The ffprobe patch is absent from this PR.
HF_HUB_OFFLINE=1 HF_HUB_CACHE=<empty-cache> .venv/bin/python -m pytest tests/test_mlx_audio_sample_rate.py tests/test_mlx_audio_instruct_1405.py tests/test_mlx_gate_390.py tests/test_mlx_supported.py tests/test_engine_routing.py tests/test_chunked_tts.py tests/test_dub_batch_engine_selection.py -q --tb=short— 87 passed on macOS arm64, Python 3.11, Torch/torchaudio 2.8.0.eaf8bb953855cab3b687d547b3835f86fa38b308.pytest tests/ -q --tb=short --maxfail=5stopped at 510 passed, 5 failed, 10 skipped, 1 xfailed; failures include missing Transformers/ASR packages and existing Mac isolation expectations.pytest backend/tests/ -q --tb=short --maxfail=3finished 353 passed, 2 failed, 2 skipped, with the same native-preload assumption on this Mac. This is not a full CI-green claim.git diff --checkpasses.Checklist
Earlier local gates on the combined two-patch tree
bun install --frozen-lockfile,bun run lint,bun run typecheck:ci,bun run format:check, andbun run buildpassed.bun run test --maxWorkers=4passed 346 files / 2,872 tests;node --experimental-strip-types --no-warnings --test ../tests/frontend/*.test.mjspassed 100 tests. Manifests and lockfiles are unchanged.CI=1 E2E_PROD_PORT=44173 PLAYWRIGHT_CHROMIUM=<system-Chrome> bun run test:prod-bundlepassed 4 tests against a freshly built production bundle. Offline Pythonpytest tests/smoke/ -q --tb=shortpassed 4 tests. Install-document validation passed; AppRun's model-free shell harness passed 26 cases.cargo check --locked --target aarch64-apple-darwin --message-format=short -j 4passed.cargo test --locked --lib --target aarch64-apple-darwin --message-format=short -j 4 -- --test-threads=1passed 271 tests; the corresponding--test backend_lifecycleinvocation passed 25 tests. The first parallel lib-test run had 270 passed / 1 failed, because an existing test temporarily sets HOME while another asserts the log directory; serial execution resolves that race without a code change.bun run typecheckreports 4,028 existing checkJs errors (the actual CI typecheck passes),bun run lint:hooksreports 113 errors / 23 warnings, andbun run knipreports existing unused/duplicate exports. Developmentbun run e2e --workers=2was attempted without a full seeded backend: 12 passed / 6 failed waiting for gallery/support elements; this does not establish full development E2E success.bash scripts/inject-apprun.test.shexits 2 on macOS because the injector intentionally does nothing outside Linux and its fixture is absent.bash scripts/verify-apprun-bundle.test.shexits 1 at GNU-stylesed -iunder BSD sed; the first two checks pass. Linux CI owns these native packaging checks.docker info --format '{{.ServerVersion}}'exits 1, with no daemon socket. Windows/Linux native packaging requires those hosts and their platform SDKs. No release bundle or signing/publishing action was performed. The current thin installer bundles source and uv/ffmpeg/ffprobe, not model weights; Cargo's zero-byte sidecar placeholders cannot validate release binaries. The full desktop smoke script was not run because it resets application data and stops/launches local app processes; it needs a dedicated installation test host.MLXAudioBackend.generatenow groups consecutive chunks by sample rate and resamples each group to the declared 24 kHz rate before concatenation, including the fallback path. This fixes duration and pitch errors for 44.1 kHz and 48 kHz models while preserving 24 kHz and bare-array behavior. No additional merge-blocking risk is identified in the supplied findings.