fix: use cubic interpolation for QualityQuick to match SOXR#13
fix: use cubic interpolation for QualityQuick to match SOXR#13
Conversation
Root cause: QualityQuick was using polyphase filtering instead of cubic interpolation like SOXR_QQ, resulting in poor THD (-88.23 dB vs SOXR's -152.72 dB). Changes: - Updated CubicStage to use SOXR's exact cubic formula from cr-core.c:59-61 - Made CubicStage generic to support float32/float64 - Modified NewResampler to use CubicStage when quality == QualityQuick - Updated passband ripple threshold to 5.5 dB (matches SOXR's 5.067 dB) Results: - THD now matches SOXR (within 0.3-1.4 dB across test cases) - Passband ripple matches SOXR perfectly (5.063 dB vs 5.067 dB) - All tests pass Note: Cubic interpolation inherently has ~5.1 dB passband ripple vs polyphase's ~1.3 dB. This is expected - cubic trades passband flatness for speed and better THD performance.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce a generic Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resampler
participant CubicStage
participant Pipeline as Standard<br/>Pipeline
Client->>Resampler: NewResampler(ratio, QualityQuick)
Resampler->>Resampler: Validate ratio bounds
alt ratio valid
Resampler->>CubicStage: Initialize cubicStage
else ratio invalid
Resampler-->>Client: Error
end
Client->>Resampler: Process(input)
alt cubicStage exists
Resampler->>CubicStage: Process(input)
CubicStage-->>Resampler: output
else cubicStage nil
Resampler->>Pipeline: Process via standard stages
Pipeline-->>Resampler: output
end
Resampler-->>Client: output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 of ChangesHello @tphakala, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant THD regression in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the THD regression for QualityQuick by switching to cubic interpolation, aligning its performance with SOXR. While the implementation is generally clean and well-executed, a critical security vulnerability has been identified: the lack of validation for the resampling ratio. Extremely large or infinite ratios can trigger Denial of Service conditions, including infinite loops and memory exhaustion panics. It is recommended to add bounds checking for the resampling ratio during resampler initialization to mitigate this risk.
| if quality == QualityQuick { | ||
| cubicStage := NewCubicStage[F](ratio) |
There was a problem hiding this comment.
The resampling ratio, calculated from outputRate / inputRate, is not validated to be within a sane, finite range. If an extremely large or infinite ratio is provided (e.g., if inputRate is extremely small), it can lead to several Denial of Service (DoS) conditions:
- Infinite Loop: In
CubicStage.Process(andLinearStage.Process), the phase increment1.0 / c.ratiocan become zero or smaller than the floating-point epsilon when the ratio is extremely large. This causes the processing loopfor c.phase < 1.0to hang indefinitely, consuming 100% CPU. - Memory Exhaustion (OOM): The estimated output size
int(math.Ceil(float64(len(input)) * c.ratio))can become extremely large, leading to a massive memory allocation attempt and a subsequent panic or process crash. - Division by Zero: In
PolyphaseStage.Process, a zerostepvalue (derived from a large ratio) leads to a division by zero panic when calculatingnumOut.
To remediate this, add validation in NewResampler to ensure the resampling ratio is finite and does not exceed a reasonable maximum (e.g., 1024 or 65536, depending on the application's requirements).
| outputSize := int(math.Ceil(float64(len(input)) * c.ratio)) | ||
| output := make([]float64, 0, outputSize) | ||
| output := make([]F, 0, outputSize) |
There was a problem hiding this comment.
The outputSize calculation and subsequent make call are susceptible to memory exhaustion or panics if c.ratio is extremely large. Additionally, the processing loop (not shown in this diff but present in the file) can enter an infinite loop if 1.0 / c.ratio is too small to advance the c.phase variable. Validation should be added to the constructor or here to ensure the ratio is within a safe range.
Add validation for resampling ratio following SOXR's pattern (1/256 to 256). Prevents: - Integer overflow in output size calculation with extreme ratios - Memory exhaustion from attempting to allocate huge output buffers - DoS attacks using absurdly large or small sample rates Added test cases for boundary conditions and extreme ratios. Addresses Gemini code review feedback.
TestPrecisionComparison_Summary was timing out during race detection due to O(N²) DFT calculations. Added testing.Short() check to skip this expensive test in CI.
1 participant
Summary
Fixes QualityQuick THD regression by using cubic interpolation instead of polyphase filtering, matching SOXR_QQ implementation.
Root Cause
QualityQuick was using polyphase filtering which resulted in:
The issue was that SOXR_QQ uses cubic interpolation (fast, good THD) while higher quality presets use polyphase filtering (slower, better frequency response).
Changes
cr-core.c:59-61float32andfloat64CubicStagewhenquality == QualityQuickResults
THD Performance (matches SOXR)
Passband Ripple (matches SOXR)
Test Status
✅ All tests pass
Technical Notes
Cubic interpolation inherently has ~5.1 dB passband ripple compared to polyphase filtering's ~1.3 dB. This is expected and matches SOXR's behavior:
The ~5 dB ripple is a fundamental characteristic of cubic interpolation, not a bug.
Testing
Verified with SOXR reference tool:
./test-reference/test_quality 44100 48000 ripple quick # ripple = 5.067022 dBSummary by CodeRabbit
New Features
Bug Fixes
Tests