refactor: extract DSP utilities, remove dead code, fix RT safety and minor issues#198
refactor: extract DSP utilities, remove dead code, fix RT safety and minor issues#198
Conversation
OpenSauce
commented
Feb 28, 2026
- Extract shared DSP primitives (db_to_lin, calculate_coefficient, DcBlocker, EnvelopeFollower) into stages/common.rs and reuse across compressor, noise_gate, preamp, multiband_saturator, and poweramp
- Remove dead code: ir/model.rs, downsampled_buffer(), set_selected()
- Replace .expect() with safe early returns in pitch shifter and convolver FFT paths for real-time safety
- Fix metronome: replace unwrap() with error handling, println! with debug!, fix method signatures (is_enabled/bpm take &self not &mut self)
- Fix engine: StopRecording() -> StopRecording unit variant, fix error message
- Extract try_connect/try_disconnect helpers in audio manager
- Remove redundant match arm in sanitize_filename
- Standardize "Unknown parameter" error messages across stages
…minor issues - Extract shared DSP primitives (db_to_lin, calculate_coefficient, DcBlocker, EnvelopeFollower) into stages/common.rs and reuse across compressor, noise_gate, preamp, multiband_saturator, and poweramp - Remove dead code: ir/model.rs, downsampled_buffer(), set_selected() - Replace .expect() with safe early returns in pitch shifter and convolver FFT paths for real-time safety - Fix metronome: replace unwrap() with error handling, println! with debug!, fix method signatures (is_enabled/bpm take &self not &mut self) - Fix engine: StopRecording() -> StopRecording unit variant, fix error message - Extract try_connect/try_disconnect helpers in audio manager - Remove redundant match arm in sanitize_filename - Standardize "Unknown parameter" error messages across stages
There was a problem hiding this comment.
Pull request overview
This PR refactors shared DSP utilities into a common module, removes unused/dead code, and improves runtime safety/logging behavior in several audio and UI components.
Changes:
- Extract shared DSP helpers (
db_to_lin,calculate_coefficient,DcBlocker,EnvelopeFollower) intosrc/amp/stages/common.rsand reuse them across multiple stages. - Remove dead/unused code paths (IR model module, GUI control setter, sampler buffer accessor) and simplify filename sanitization.
- Improve real-time safety and logging by replacing panic-on-error FFT paths and unwrap/println usage, plus minor engine/audio-manager API cleanups.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/amp/stages/common.rs | Introduces shared DSP primitives used across multiple stages. |
| src/amp/stages/mod.rs | Exposes the new common module. |
| src/amp/stages/compressor.rs | Switches to shared envelope follower/coeff helpers; standardizes unknown-parameter errors. |
| src/amp/stages/noise_gate.rs | Replaces bespoke envelope/coeff logic with shared utilities; standardizes unknown-parameter errors. |
| src/amp/stages/preamp.rs | Replaces inline DC-block implementation with shared DcBlocker. |
| src/amp/stages/poweramp.rs | Uses shared coefficient calculation for sag attack/release. |
| src/amp/stages/multiband_saturator.rs | Removes local DC blocker/envelope follower in favor of shared implementations. |
| src/ir/convolver/fft.rs | Replaces .expect() with error handling/early returns in FFT paths. |
| src/audio/pitch_shifter.rs | Replaces .expect() with early returns on FFT/IFFT failure for RT safety. |
| src/metronome/mod.rs | Replaces unwrap/println! with error handling and debug!; adjusts method signatures. |
| src/audio/engine.rs | Makes StopRecording a unit variant and improves send error message. |
| src/audio/manager.rs | Extracts port connect/disconnect helpers to reduce duplication. |
| src/preset/manager.rs | Removes redundant match arm in sanitize_filename. |
| src/audio/samplers.rs | Removes unused downsampled_buffer() accessor. |
| src/gui/components/control.rs | Removes unused set_selected() method. |
| src/ir/model.rs | Deletes unused IR model type. |
| src/ir/mod.rs | Removes model module export. |
| .gitignore | Ignores /claude directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| .expect("IFFT failed"); | ||
| .is_err() | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
process_tail_partition returns early on inverse-FFT failure without advancing overlap-add/write position. Because the caller continues to move the read pointer each sample, skipping the ola_write update can break the timing relationship and lead to misaligned tail output once FFTs succeed again. Update the state consistently on error (and emit silence for that partition) to keep buffer alignment intact.
| .process_with_scratch(&mut self.frame, &mut self.spectrum, &mut self.r2c_scratch) | ||
| .expect("FFT failed"); | ||
| .is_err() | ||
| { |
There was a problem hiding this comment.
On analysis FFT failure, process_frame returns early without advancing output_write. Meanwhile process_block continues advancing output_read, so a later successful frame will overlap-add into already-read (past) regions of output_accum, causing timing glitches. Consider still incrementing output_write by HOP_SIZE (and leaving that hop silent) to keep read/write pointers aligned.
| { | |
| { | |
| // On FFT failure, emit a silent hop but still advance the output write | |
| // pointer to keep it aligned with the read pointer. | |
| let out_len = self.output_accum.len(); | |
| for i in 0..HOP_SIZE { | |
| let idx = (self.output_write + i) % out_len; | |
| self.output_accum[idx] = 0.0; | |
| } | |
| self.output_write = (self.output_write + HOP_SIZE) % out_len; |
| ) | ||
| .expect("IFFT failed"); | ||
| .is_err() | ||
| { |
There was a problem hiding this comment.
On synthesis IFFT failure, process_frame returns early without advancing output_write, but output_read will still advance every sample in process_block. This can desynchronize the overlap-add ring buffer and produce mis-timed or overwritten output on subsequent frames. Keep timing state consistent on error (advance output_write and output silence for that frame).
| { | |
| { | |
| // Keep timing state consistent on error: advance output_write and | |
| // contribute silence for this frame (no overlap-add performed). | |
| self.output_write = (self.output_write + HOP_SIZE) % OUTPUT_SIZE; |
| let samples: Vec<f32> = reader | ||
| .samples::<i16>() | ||
| .filter_map(|s| s.ok()) |
There was a problem hiding this comment.
filter_map(|s| s.ok()) silently drops decode errors from hound, which can truncate the tick sample without any indication (and make debugging bad WAVs hard). Prefer collecting into a Result<Vec<_>, _> and logging/returning on the first error (or at least logging when an error is encountered) instead of skipping samples.
| let samples: Vec<f32> = reader | |
| .samples::<i16>() | |
| .filter_map(|s| s.ok()) | |
| let raw_samples: Vec<i16> = match reader.samples::<i16>().collect::<Result<Vec<_>, _>>() { | |
| Ok(s) => s, | |
| Err(e) => { | |
| error!("Failed to decode samples from WAV file '{file_path}': {e}"); | |
| return; | |
| } | |
| }; | |
| let samples: Vec<f32> = raw_samples | |
| .into_iter() |
| ) | ||
| .expect("FFT failed"); | ||
| .is_err() | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
process_tail_partition returns early on forward-FFT failure without advancing internal overlap-add state (e.g., ola_write). Since process_sample will still advance input_base/ola_read, this can desynchronize the ring-buffer alignment and cause subsequent blocks to be written to the wrong region. Consider advancing ola_write (and any other timing state) even on FFT error, and leave the corresponding output region as silence.
…bad WAV - Pitch shifter: advance output_write on analysis/synthesis FFT failure to keep read/write pointers aligned (emit silence instead of desyncing) - Convolver: advance ola_write on forward/inverse FFT failure to keep overlap-add buffer timing consistent - Metronome: collect WAV samples into Result instead of silently dropping decode errors via filter_map