Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in the rubato library that occurred when the highpass filter cutoff frequency is set to 0 Hz. The fix introduces a minimum cutoff frequency of 0.1 Hz to prevent division-by-zero errors in the RC (resistance-capacitance) filter calculations, and refactors the filter coefficient computation into a reusable helper function.
Changes:
- Added
MIN_CUTOFF_HZconstant (0.1 Hz) to prevent division-by-zero in filter calculations - Refactored alpha coefficient calculation into a
compute_alphahelper method to eliminate code duplication - Updated parameter validation to accept cutoff values from 0 Hz to 20 kHz (previously 20 Hz to 20 kHz)
- Added regression test to verify zero-cutoff filters produce finite output
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (0.0..=20000.0).contains(&value) { | ||
| self.cutoff = value; | ||
| self.update_coefficients(); | ||
| Ok(()) | ||
| } else { | ||
| Err("Cutoff must be between 20Hz and 20kHz") | ||
| Err("Cutoff must be between 0Hz and 20kHz") |
There was a problem hiding this comment.
The validation now allows cutoff values of 0.0, but the actual implementation silently clamps them to 0.1 Hz via the MIN_CUTOFF_HZ constant. This creates a discrepancy between what users can set and what actually happens. Users might set cutoff to 0.0 and later retrieve it with get_parameter, expecting 0.0 but the actual filter behavior will be as if 0.1 Hz was set. Consider either: (1) rejecting 0.0 in validation and keeping the error message accurate, or (2) explicitly storing the clamped value when set_parameter is called so that get_parameter returns the actual effective cutoff used by the filter. Option 2 would require modifying set_parameter to update self.cutoff to the clamped value.
| for _ in 0..256 { | ||
| let out = hp.process(1.0); | ||
| assert!( | ||
| out.is_finite(), | ||
| "zero-cutoff highpass produced non-finite output" | ||
| ); |
There was a problem hiding this comment.
The new test only validates that a zero cutoff highpass doesn't produce non-finite output, but doesn't test that the filter still produces reasonable behavior. Consider adding an assertion that verifies the output is close to the input value (since a 0.1 Hz highpass at 48 kHz sample rate should pass audio frequencies nearly unchanged). For example, after warming up the filter, verify that abs(out - 1.0) is small (e.g., less than 0.01) for a constant input.
| for _ in 0..256 { | |
| let out = hp.process(1.0); | |
| assert!( | |
| out.is_finite(), | |
| "zero-cutoff highpass produced non-finite output" | |
| ); | |
| // Warm up the filter and ensure outputs stay finite. | |
| for i in 0..256 { | |
| let out = hp.process(1.0); | |
| assert!( | |
| out.is_finite(), | |
| "zero-cutoff highpass produced non-finite output" | |
| ); | |
| // After warmup, a 0.1 Hz highpass at 48 kHz should behave nearly as a passthrough | |
| // for a constant input. Verify the output is close to the input value. | |
| if i >= 128 { | |
| assert!( | |
| (out - 1.0).abs() < 0.01, | |
| "zero-cutoff highpass output deviates too much from input after warmup: out={out}" | |
| ); | |
| } |
|
|
||
| impl FilterStage { | ||
| /// Minimum cutoff frequency to avoid division-by-zero in the RC calculation. | ||
| /// 0.1 Hz makes a highpass effectively a passthrough. |
There was a problem hiding this comment.
The comment states that "0.1 Hz makes a highpass effectively a passthrough", but this is inaccurate. A highpass filter with a 0.1 Hz cutoff will still block DC and very low frequencies while passing higher frequencies. It doesn't act as a passthrough, but rather as a very gentle highpass with minimal effect on audio frequencies. Consider revising this comment to say something like "0.1 Hz is low enough to have minimal impact on audio frequencies while preventing division-by-zero".
| /// 0.1 Hz makes a highpass effectively a passthrough. | |
| /// 0.1 Hz is low enough to have minimal impact on audio frequencies while still | |
| /// blocking DC and very low-frequency components. |
rubato panics when highpass is set to 0