Skip to content

Restore mixer throughput without changing DSP output - #1978

Open
skylartaylor wants to merge 2 commits into
mainfrom
codex/independent-mixer-throughput-20261015
Open

skylartaylor wants to merge 2 commits into
mainfrom
codex/independent-mixer-throughput-20261015

Conversation

@skylartaylor

@skylartaylor skylartaylor commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

Restore mixer throughput without changing DSP output. Constant EQ/dynamics/reverb chains process once per request, reverb uses separate settled/ramping loops, and scalar automation evaluates outer holds once per block. Recursive arithmetic, the 32-sample gain-update grid, boundaries and live-release behavior retain their existing correctness coverage.

Current head: cd45768. Base remains 9ed9650; this follow-up does not rebase the DSP work. Removed the unrelated dev_icon.rs MIME hunk after verifying it exactly matches the separate open PR #1958. The net diff is three DSP/mixer files plus docs/testing.md. No dependency or benchmark-threshold changes.

The testing docs correct the stale --lib benchmark command: perf_24_tracks_3_effects_realtime_factor is in the mixer_perf integration test. cargo xtask ci invokes release tests without setting profile overrides; checked-in release defaults are thin LTO and one codegen unit. The reported pool profile (LTO off, 16 codegen units) must be reproduced explicitly. The docs now give serial commands for both profiles, the before revision, and the required receipts. The unchanged release assertion remains strictly >4x. Thread CPU time limits scheduling noise but does not normalize CPU speed or thermal behavior, so one 4.3x result cannot establish a stable margin across hosts/profiles.

Exact-head local validation, macOS arm64, Rust/Cargo 1.99.1, one workload at a time with CARGO_BUILD_JOBS=1, cargo -j1 and --test-threads=1:

  • cargo fmt --all -- --check: passed.
  • cargo test -j1 -p filmcraft-audio-dsp --lib --locked --offline reverb -- --test-threads=1: 8 passed, 0 failed, 0 ignored.
  • cargo test -j1 -p filmcraft-render --lib --locked --offline mixer_tests -- --test-threads=1: 25 passed, 0 failed, 0 ignored.
  • cargo test -j1 -p filmcraft-render --lib --locked --offline lane_span_tests -- --test-threads=1: 3 passed, 0 failed, 0 ignored.
  • git diff --check: passed.

Historical only: the previous PR description reported Linux CI and one production-profile run at 4.3x (10 seconds rendered in 2.313 CPU seconds). These historical claims were not independently rerun here and do not qualify the current head.

Pending: native Linux formatting, requested strict locked/offline workspace Clippy, reverb/mixer correctness checks, and repeated before/after measurements of the unchanged release mixer gate in BOTH LTO-off/16-unit and thin-LTO/1-unit profiles on the same host/toolchain. No before measurement or new realtime factor is claimed. No supported Linux pool tool was exposed; no new SSH connection, VM/container or installation was used. Full workspace builds were prohibited by the user's resource restrictions. Physical audio/input and remaining package/platform qualification also remain pending. This PR is not yet claimed ready against the Linux review gate.

Summary by CodeRabbit

  • Performance
    • Improved audio rendering efficiency for reverb and mixer processing, including requests with steady parameters.
  • Reliability
    • Preserved consistent audio output when rendering with different request sizes, including when effect settings change during playback.
    • Added coverage for automation edge cases, reverb gain-update boundaries, and multiple channel configurations.
  • Documentation
    • Clarified mixer throughput benchmarks, platform-specific timing, and instructions for running and comparing performance checks.

@skylartaylor
skylartaylor marked this pull request as ready for review October 15, 2026 01:19

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not found a correctness problem, but I'm holding this as a comment for two reasons.

  1. The MimeType= line in apps/filmcraft/src/dev_icon.rs has nothing to do with mixer speed. It is the same fix as #1958, which is open. Please drop it from this PR so the speed change stands alone.
  2. The stated margin is thin and is measured in a different profile from the one CI uses. The text says the unchanged ">4x realtime" gate passes at 4.3x under the production thin-LTO, one codegen-unit build, while "CI/native GUI builds use LTO off and 16 codegen units". Does the gate test pass, and by how much, in the profile cargo xtask ci and the pool use? If it fails or is within noise there, a 7% margin is not a stable gate.

What I did check: the reverb is split into process_block::<FIXED, SETTLED> variants (audio-dsp/src/effects/time.rs) and lines becomes an array, so the recursive arithmetic and the 32-sample gain-update schedule are untouched by construction, and the bit-for-bit checks against the original loop (mono, stereo, multichannel) are the right proof for a speed-only change. The measured number is a single production-profile run, one machine; please add the before figure, so the gain is visible.

Hosted CI does not build or test filmcraft, so I'm leaving this as a comment. I'll approve once the profile question is answered and the receipt for this exact head is in.

Validation request (head 8bc2f7df81afd5a25099217f9ce9b1b4584fb41a):

  • os: linux
  • commands: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --locked --offline -- -D warnings; cargo test -p filmcraft-audio-dsp --lib --locked --offline reverb; cargo test -p filmcraft-render --lib --locked --offline mixer_tests; the existing >4x realtime gate test in release mode (please name the test and command you used)
  • expected: all pass, 0 ignored; please report counts and the measured realtime factor of the gate in both profiles
  • holds: the profile question; drop the dev_icon.rs hunk (#1958)

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c16494c6-02cb-40c7-8d33-df5437bf174e
📥 Commits

Reviewing files that changed from the base of the PR and between 9ed9650 and cd45768.

📒 Files selected for processing: 4
  • crates/audio-dsp/src/effects/time.rs
  • crates/render/src/mixer.rs
  • crates/render/src/mixer_tests.rs
  • docs/testing.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Reverb processing now dispatches by block-level parameter and delay states. Mixer lane filling and insert processing use optimized paths for qualifying requests. Tests cover reverb output, lane behavior, and render partitioning. Benchmark documentation defines thresholds and reproduction procedures.

Changes: Audio rendering

Layer / File(s) Summary
Reverb block processing
crates/audio-dsp/src/effects/time.rs
Reverb stores its delay-line buffers in a fixed-size array and dispatches block processing according to delay and parameter states. Tests cover automation timing and bit-exact output for 1, 2, 3, and 6 channels.
Mixer automation and insert processing
crates/render/src/mixer.rs, crates/render/src/mixer_tests.rs, docs/testing.md
Lane filling returns one value for qualifying requests outside the keyed interval. Insert processing returns for empty buffers and uses one call when parameter refreshes are not needed. Tests check lane cases and exact output across request partitions. Benchmark documentation specifies thresholds, commands, and measurement records.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: echelon

Merge Risk: ⚪ Minimal · up to cd457

No actionable merge-blocking risk remains in the reviewed audio-processing changes after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly summarizes the main objective: restore mixer throughput without changing DSP output.
Full details: Docstring Coverage

Explanation

Docstring coverage is 58.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@skylartaylor

Copy link
Copy Markdown
Member Author

Thanks @FredApps, the MIME change was unrelated here. I've verified that it matches #1958 and removed it from this PR at cd45768, keeping the DSP changes intact.

The local macOS checks pass: 8 reverb tests, 25 mixer tests and 3 automation-lane tests, with none ignored. I've corrected the benchmark command to use the mixer_perf integration test and documented explicit commands for both build profiles and the before/after comparison. The strict >4x gate is unchanged.

The requested native Linux checks and repeated measurements in both profiles are still outstanding, so I'm not presenting the historical 4.3x result as current-head validation. Could you re-review the scope and benchmark instructions in the meantime? I appreciate you helping make the evidence clearer.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants