feat(audio/effects): AudioEffect trait + EffectChain + bypass/wet-dry infrastructure (#30)#104
Merged
Conversation
…dioCommand/AudioEvent
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds audio effects plumbing: fundsp dependency, EffectsError, AudioEffect trait, EffectsCommand/EffectsEvent enums, EffectChain pipeline, Mixer integration, engine wiring, module re-exports, and a detailed implementation plan. ChangesAudio Effects Subsystem Plumbing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
cargo fmt --check failed on three files: - audio/effects/commands.rs: collapse multi-line assert!(matches!(...)) to one line - audio/engine.rs: expand SetEffectBypass struct variant to multi-line; collapse set_chain_bypass method chain to single line - audio/mod.rs: reorder mod declarations alphabetically (effects before engine)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md (2)
820-875: ⚡ Quick winRemove the unsafe implementation draft from the plan to prevent accidental adoption.
Even though you later provide a safe alternative, keeping an unsafe variant in the step-by-step plan increases the chance it gets copied into production code. Keeping only the final safe version would reduce ambiguity.
Also applies to: 877-927
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md` around lines 820 - 875, Remove the unsafe implementation draft that uses raw pointers from the plan and keep only the final safe, non-allocating version of process(); specifically delete the code that takes scratch.as_ptr() and constructs a slice via unsafe and the accompanying commentary, and ensure the retained text shows the safe approach that uses self.scratch, output.copy_from_slice, self.scratch[..n].copy_from_slice(&output[..n]), effect.process(&self.scratch[..n], &mut output[..n], sample_rate) (or an equivalent safe call) along with the wet/dry mix logic and references to scratch, effects, effect.process, and wet_dry so readers cannot accidentally copy the unsafe variant into production.
83-85: ⚡ Quick winUse repo-relative commands instead of machine-specific absolute paths.
Hardcoding
/home/adam/...makes the plan non-portable for other contributors and automation. Prefer repo-relative instructions (e.g., “run from repo root” and plaincargo ...).Also applies to: 110-112, 154-156, 178-179, 265-267, 347-349, 812-814, 932-934, 987-989, 1096-1098, 1105-1107, 1114-1116, 1159-1161, 1221-1223, 1230-1232, 1239-1241
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md` around lines 83 - 85, The plan uses machine-specific absolute paths (e.g., the command starting with "cd /home/adam/github/honkhonk/.worktrees/feat/issue-30" followed by "cargo test effects_error 2>&1 | head -30") which makes it non-portable; update those steps to be repo-relative by instructing contributors to run commands from the repo root (or use a generic repo-root placeholder) and use plain commands like "cargo test ..." and "head -30" without hardcoded /home/adam paths; apply the same change to the other occurrences you listed (lines around 110-112, 154-156, 178-179, 265-267, 347-349, 812-814, 932-934, 987-989, 1096-1098, 1105-1107, 1114-1116, 1159-1161, 1221-1223, 1230-1232, 1239-1241) so the document uses repo-relative instructions consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md`:
- Line 7: Update the plan text to use the actual Mixer API method name
process_block() instead of process_if_active(): search the document for any
occurrence of process_if_active (including the three places called out) and
replace with process_block(), and ensure any surrounding wording describing the
seam or no-op stub refers to process_block() exposing EffectChain via the
engine; keep references to EffectChain, src/audio/mixer.rs, and EffectsError
unchanged so readers can map to the implemented symbols.
In `@src/audio/effects/mod.rs`:
- Around line 14-16: The set_param signature currently returns () so invalid or
unknown parameter cases cannot propagate typed errors; change fn set_param(&mut
self, param: &str, value: f32) to return Result<(), ParamError> (or another
thiserror-declared enum) and implement a ParamError using thiserror with
variants like UnknownParameter { name: String } and OutOfRange { name: String,
value: f32, range: (f32,f32) }; update all implementations of the Effect trait
(methods named set_param) to return Err(...) with contextual info when
validation/normalization fails, and update callers (command handler) to
propagate or .context() the error using anyhow as appropriate so structured
errors cross the boundary.
In `@src/audio/engine.rs`:
- Around line 410-423: The SetEffectBypass and SetEffectParam handlers currently
drop the Result from ctx.mixer.borrow_mut().chain_mut().set_bypass/set_param;
instead capture and surface errors with context (don’t unwrap or ignore). Update
the AudioCommand::SetEffectBypass and AudioCommand::SetEffectParam branches to
handle the Result: on Err call .context(...) (anyhow) at the call site with
clear info (e.g. include index and param name/value) and then either
return/propagate a typed error (use thiserror for module boundary types) or emit
a UI/error event so the UI is informed of invalid index/param; ensure the log or
propagated error message contains the origin ("set_bypass"/"set_param") and the
offending index/param values.
In `@src/audio/mixer.rs`:
- Around line 50-53: Mixer::process_block() performs heap allocation by growing
output_buf when n > output_capacity; move allocations out of the real-time path
by introducing a non-realtime method (e.g.,
Mixer::ensure_output_capacity(required_capacity)) that does the actual buffer
growth (call Vec::resize or reserve and initialize) and updates output_capacity,
and remove the resize call from Mixer::process_block(); in process_block()
replace the resize branch with a debug_assert or early clamp/path that assumes n
<= output_capacity and zeroes only the used portion of output_buf, ensuring no
heap allocation happens inside the audio callback.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md`:
- Around line 820-875: Remove the unsafe implementation draft that uses raw
pointers from the plan and keep only the final safe, non-allocating version of
process(); specifically delete the code that takes scratch.as_ptr() and
constructs a slice via unsafe and the accompanying commentary, and ensure the
retained text shows the safe approach that uses self.scratch,
output.copy_from_slice, self.scratch[..n].copy_from_slice(&output[..n]),
effect.process(&self.scratch[..n], &mut output[..n], sample_rate) (or an
equivalent safe call) along with the wet/dry mix logic and references to
scratch, effects, effect.process, and wet_dry so readers cannot accidentally
copy the unsafe variant into production.
- Around line 83-85: The plan uses machine-specific absolute paths (e.g., the
command starting with "cd /home/adam/github/honkhonk/.worktrees/feat/issue-30"
followed by "cargo test effects_error 2>&1 | head -30") which makes it
non-portable; update those steps to be repo-relative by instructing contributors
to run commands from the repo root (or use a generic repo-root placeholder) and
use plain commands like "cargo test ..." and "head -30" without hardcoded
/home/adam paths; apply the same change to the other occurrences you listed
(lines around 110-112, 154-156, 178-179, 265-267, 347-349, 812-814, 932-934,
987-989, 1096-1098, 1105-1107, 1114-1116, 1159-1161, 1221-1223, 1230-1232,
1239-1241) so the document uses repo-relative instructions consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2c89b23-b1df-4879-aad5-8f9bba22b400
📒 Files selected for processing (10)
Cargo.tomldocs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.mdsrc/app.rssrc/audio/effects/chain.rssrc/audio/effects/commands.rssrc/audio/effects/mod.rssrc/audio/engine.rssrc/audio/error.rssrc/audio/mixer.rssrc/audio/mod.rs
…set_param - chain.rs: replace allocating resize in process() hot path with passthrough when block size exceeds pre-allocated scratch_capacity; upholds RT contract - mixer.rs: introduce ensure_output_capacity() cold-path pre-sizer; document that process_block() allocation is a fallback, not the intended RT path - effects/mod.rs: make AudioEffect::set_param fallible (-> Result<(), EffectsError>) so unknown/invalid parameters carry structured context across module boundaries - chain.rs: update GainEffect test impl to return Err(ParamUnknown) for unknown params; update EffectChain::set_param to propagate effect errors via ? - engine.rs: surface set_bypass/set_param errors via AudioEvent::Error instead of silently discarding them; avoids silent state drift in the UI - plan doc: align process_if_active() references to shipped process_block() name
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/audio/mixer.rs (1)
42-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove orphaned documentation comment.
Lines 42-47 contain the old doc comment for
process_block, but that method now has updated documentation at lines 60-68. These lines should have been removed whenensure_output_capacitywas inserted.📝 Proposed fix
- /// Process a block of mic audio through the effect chain. - /// - /// Returns a slice into the internal output buffer. Caller copies this - /// to the virtual sink's input buffer. - /// - /// No-op passthrough when chain is empty or bypassed. Real-time safe. /// Prepare `Mixer` to handle blocks up to `required_capacity` samples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/mixer.rs` around lines 42 - 47, Remove the orphaned documentation block that used to document process_block (the old doc comment currently preceding ensure_output_capacity); it duplicates the updated docs on process_block and should be deleted so only the current doc comment remains (keep the new documentation at the process_block definition and ensure_output_capacity remains as-is).
♻️ Duplicate comments (1)
src/audio/mixer.rs (1)
71-75:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHeap allocation remains in the real-time hot path.
process_block()still performs heap allocation (viaensure_output_capacity(n)→Vec::resize) when an incoming block exceeds the preallocated capacity. This violates the real-time safety constraint required for the PipeWire process callback and contradicts the past review guidance to replace the resize with adebug_assertor early clamp/passthrough.Per the PR objectives: "Respect real-time constraints for the PipeWire thread (no allocs/locks/syscalls in hot path)" and commit messages reference "passthrough behavior when block size exceeds scratch capacity". Replace the allocation fallback with one of:
- Option 1 (strictest):
debug_assert!(n <= self.output_capacity);to catch capacity violations in debug builds while assuming correct pre-sizing in release.- Option 2 (graceful degradation): Clamp to
self.output_capacityand process only the prefix, or return passthrough (copy input to output slice) when capacity is exceeded, avoiding allocation entirely.🔧 Proposed fix (Option 1: debug_assert)
pub fn process_block(&mut self, input: &[f32], sample_rate: u32) -> &[f32] { let n = input.len(); - if n > self.output_capacity { - // Allocation fallback — avoid by calling ensure_output_capacity() - // on a cold path before the audio callback runs. - self.ensure_output_capacity(n); - } + debug_assert!( + n <= self.output_capacity, + "process_block called with block size {n} exceeding capacity {}; \ + call ensure_output_capacity({n}) on a cold path before starting the audio callback", + self.output_capacity + ); + let n = n.min(self.output_capacity); self.chain .process(input, &mut self.output_buf[..n], sample_rate); &self.output_buf[..n]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/audio/mixer.rs` around lines 71 - 75, The process_block hot path currently calls ensure_output_capacity(n) causing a heap allocation when n > self.output_capacity; replace that allocation fallback by removing the ensure_output_capacity(n) call and instead enforce real-time safety: either (preferred) add debug_assert!(n <= self.output_capacity) in process_block to catch violations in debug builds and assume pre-sizing in release, or implement graceful degradation by clamping n to self.output_capacity (process only the prefix) or performing a passthrough copy from input to output without growing buffers; update references to ensure_output_capacity, process_block, and output_capacity accordingly and remove any Vec::resize fallback so no allocations occur in the PipeWire callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/audio/mixer.rs`:
- Around line 42-47: Remove the orphaned documentation block that used to
document process_block (the old doc comment currently preceding
ensure_output_capacity); it duplicates the updated docs on process_block and
should be deleted so only the current doc comment remains (keep the new
documentation at the process_block definition and ensure_output_capacity remains
as-is).
---
Duplicate comments:
In `@src/audio/mixer.rs`:
- Around line 71-75: The process_block hot path currently calls
ensure_output_capacity(n) causing a heap allocation when n >
self.output_capacity; replace that allocation fallback by removing the
ensure_output_capacity(n) call and instead enforce real-time safety: either
(preferred) add debug_assert!(n <= self.output_capacity) in process_block to
catch violations in debug builds and assume pre-sizing in release, or implement
graceful degradation by clamping n to self.output_capacity (process only the
prefix) or performing a passthrough copy from input to output without growing
buffers; update references to ensure_output_capacity, process_block, and
output_capacity accordingly and remove any Vec::resize fallback so no
allocations occur in the PipeWire callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62e905b3-878d-41b2-aa8c-46b9b564f233
📒 Files selected for processing (5)
docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.mdsrc/audio/effects/chain.rssrc/audio/effects/mod.rssrc/audio/engine.rssrc/audio/mixer.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/audio/effects/mod.rs
- src/audio/engine.rs
- src/audio/effects/chain.rs
- docs/superpowers/plans/2026-06-04-issue-30-effect-chain-plumbing.md
Lines 42-47 in mixer.rs retained the old process_block doc block after ensure_output_capacity was inserted between chain_mut and process_block. The stale comment described the wrong method. Deleted the orphaned lines; ensure_output_capacity now has only its own doc, and process_block's doc at lines 54-62 is unambiguous. Flagged by CodeRabbit (outside-diff comment, first review round).
Collaborator
Author
|
Fixed the orphaned doc comment in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AudioEffecttrait andEffectChaincomposable pipeline (PR 1 of 6 for issue feat(audio): sound effects (reverb, pitch shift) #18 Voice Effects)EffectsError,EffectsCommand,EffectsEvent, and aMixerstub as the integration seamfundsp = "0.23"toCargo.tomlper ADR-006 — no fundsp composition used yet, just establishing the depCloses #30
Files Changed
src/audio/effects/mod.rs(new) —AudioEffecttrait withprocess(),set_param(),bypass(),set_bypass(),latency_samples()src/audio/effects/chain.rs(new) —EffectChain: ordered pipeline, bypass, wet/dry, scratch-buffer ping-pong (no alloc in hot path)src/audio/effects/commands.rs(new) —EffectsCommand/EffectsEventenumssrc/audio/mixer.rs(new) —Mixerstub holdingEffectChain,process_block()passthroughsrc/audio/error.rs— addedEffectsError(ChainTooLong,ParamUnknown,IndexOutOfRange)src/audio/engine.rs— addedSetEffectBypass,SetEffectParam,SetEffectWetDry,SetEffectChainBypasstoAudioCommand;EffectsLatencyChangedtoAudioEvent;Mixerheld inEngineCtxsrc/app.rs— addedEffectsLatencyChangedmatch arm (no-op, reserved for Phase 4B UI)Cargo.toml—fundsp = "0.23"Design decisions
No software-level buffer processing in
mixer.rsin this PR. The current architecture uses PipeWire graph links for mic routing (ADR-007) — there is no application-level mic capture buffer.mixer.rsis a stub integration seam: it holds anEffectChainand exposesprocess_block(input, rate) -> &[f32]. Future PRs will plug this into apw_streamprocess callback when actual DSP is wired. When the chain is empty,process_blockis a transparent passthrough — existing mic routing behavior is completely unchanged.fundspadded but not used. The crate is declared per ADR-006 to avoid aCargo.tomlchange in every future effect PR. Actual fundsp graph composition (>>chains,|parallel,&mix) starts in PR 2+.Wet/dry mix at chain level, not per-effect. The issue specifies "wet/dry mix parameter on the chain."
EffectChain.wet_dryis af32in[0.0, 1.0]:0.0= input passthrough,1.0= fully processed. Applied after all effects run.Bypass passthrough copies
input → outputdirectly. Whenchain_bypass == trueOR all effects are bypassed OR the chain is empty,process()callsoutput.copy_from_slice(input)and returns immediately — no intermediate buffer touched.EffectsCommand/EffectsEventas variants on existing enums. Added toAudioCommand/AudioEventrather than separate channels, consistent with how all other commands (volume, passthrough, monitor device) work in this codebase. One channel, one match arm.EffectChain.process()uses pre-allocated scratch ping-pong — no alloc in the hot path. Strategy: copyinput → scratch, then per effect: copyscratch → output, runeffect.process(output, scratch). Finalscratch → outputcopy. Scratch is sized atEffectChain::new(block_size)and grown only inpush_effect()(cold path).Test Plan
New tests (28):
audio::effects::chain::tests::*— 15 tests covering passthrough, chained gains, bypass, wet/dry, error cases, latencyaudio::effects::commands::tests::*— 4 tests (constructibility)audio::effects::tests::*— 3 tests (trait via PassThrough mock)audio::error::effects_error_tests::*— 3 testsaudio::mixer::tests::*— 3 testsSummary by CodeRabbit