Add configurable feedback high-pass cutoff with smoothing and WASM bindings#71
Add configurable feedback high-pass cutoff with smoothing and WASM bindings#71ovelhaaa wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable highpass filter cutoff frequency (feedbackHighpassHz) for the feedback loop in the orbit delay effect. It exposes the parameter through the C API, WASM bindings, and UI controls, while implementing parameter smoothing and clamping within the core engine. A critical thread-safety issue was identified in setFeedbackHighpassHz, where reading the non-atomic feedbackHighpassHz_ member variable concurrently with audio thread writes causes a data race. It is recommended to use the atomic pendingFeedbackHighpassHz_ as the fallback value instead.
| void OrbitDelayCore::setFeedbackHighpassHz(float value) { | ||
| pendingFeedbackHighpassHz_.store(clampf(sanitizeFinite(value, feedbackHighpassHz_), kMinFeedbackHighpassHz, kMaxFeedbackHighpassHz), | ||
| std::memory_order_relaxed); | ||
| pendingParamVersion_.fetch_add(1u, std::memory_order_release); | ||
| } |
There was a problem hiding this comment.
Thread Safety / Data Race Issue
There is a potential data race on feedbackHighpassHz_. The function setFeedbackHighpassHz is called from the control/UI thread, but it reads feedbackHighpassHz_ (a non-atomic member variable) as a fallback value for sanitizeFinite. Meanwhile, the audio thread writes to feedbackHighpassHz_ in applyPendingParamsIfNeeded() on every processing block.
Reading a non-atomic variable that is concurrently written to by another thread is undefined behavior in C++.
Solution
Instead of falling back to the non-atomic feedbackHighpassHz_, you can safely fall back to the atomic pendingFeedbackHighpassHz_.load(std::memory_order_relaxed). This avoids the data race entirely and ensures thread safety.
| void OrbitDelayCore::setFeedbackHighpassHz(float value) { | |
| pendingFeedbackHighpassHz_.store(clampf(sanitizeFinite(value, feedbackHighpassHz_), kMinFeedbackHighpassHz, kMaxFeedbackHighpassHz), | |
| std::memory_order_relaxed); | |
| pendingParamVersion_.fetch_add(1u, std::memory_order_release); | |
| } | |
| void OrbitDelayCore::setFeedbackHighpassHz(float value) { | |
| const float fallback = pendingFeedbackHighpassHz_.load(std::memory_order_relaxed); | |
| pendingFeedbackHighpassHz_.store(clampf(sanitizeFinite(value, fallback), kMinFeedbackHighpassHz, kMaxFeedbackHighpassHz), | |
| std::memory_order_relaxed); | |
| pendingParamVersion_.fetch_add(1u, std::memory_order_release); | |
| } |
Motivation
Description
OrbitDelayCore::setFeedbackHighpassHz(float)and stored statefeedbackHighpassHz_, pending atomicpendingFeedbackHighpassHz_and smootherfeedbackHighpassHzSm_incore/include/orbit_delay_core.h.applyPendingParamsIfNeeded()and mark smoothed targets dirty soupdateSmoothedTargetsIfDirty()andadvanceSmoothers()drive the smoother; use the smoother output inmaybeApplyHighpassCutoff()so runtime HPF updates use the smoothed target (clamped to musical bounds and Nyquist).orbit_set_feedback_highpass_hz()and WASM bridge functionorbit_wasm_set_feedback_highpass_hz()and added the function to Emscripten exported list intargets/wasm/CMakeLists.txt.feedbackHighpassHzviatargets/wasm/js/ui.jsto the new WASM bridge function.Testing
rgsearches forsetFeedbackHighpassHz,pendingFeedbackHighpassHz_,orbit_set_feedback_highpass_hzandorbit_wasm_set_feedback_highpass_hz, which succeeded.targets/wasm/CMakeLists.txtand the UI bindingfeedbackHighpassHzis present intargets/wasm/js/ui.jsvia automated search, which succeeded.Codex Task