Refactor Rust API for speech denoiser - #3371
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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, 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 refactors the Rust API for the speech denoiser by consolidating related structures and configurations into the Highlights
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. 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 refactors the Rust API for the speech denoiser by consolidating shared components. The changes move definitions from speech_denoiser.rs (which is now removed) into offline_speech_denoiser.rs. This is a good structural improvement that clarifies the dependency of the online denoiser on these shared components. I have one suggestion to reduce some code duplication in the newly moved code, which should improve long-term maintainability.
| pub struct OfflineSpeechDenoiserGtcrnModelConfig { | ||
| pub model: Option<String>, | ||
| } | ||
|
|
||
| impl OfflineSpeechDenoiserGtcrnModelConfig { | ||
| pub(crate) fn to_sys( | ||
| &self, | ||
| cstrings: &mut Vec<CString>, | ||
| ) -> sys::OfflineSpeechDenoiserGtcrnModelConfig { | ||
| sys::OfflineSpeechDenoiserGtcrnModelConfig { | ||
| model: to_c_ptr(&self.model, cstrings), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Default)] | ||
| pub struct OfflineSpeechDenoiserDpdfNetModelConfig { | ||
| pub model: Option<String>, | ||
| } | ||
|
|
||
| impl OfflineSpeechDenoiserDpdfNetModelConfig { | ||
| pub(crate) fn to_sys( | ||
| &self, | ||
| cstrings: &mut Vec<CString>, | ||
| ) -> sys::OfflineSpeechDenoiserDpdfNetModelConfig { | ||
| sys::OfflineSpeechDenoiserDpdfNetModelConfig { | ||
| model: to_c_ptr(&self.model, cstrings), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The structs OfflineSpeechDenoiserGtcrnModelConfig and OfflineSpeechDenoiserDpdfNetModelConfig and their to_sys implementations are identical. You can reduce code duplication by using a macro to generate these structs and their implementations. This will make the code more concise and easier to maintain.
macro_rules! define_denoiser_model_config {
($name:ident, $sys_name:ty) => {
#[derive(Clone, Debug, Default)]
pub struct $name {
pub model: Option<String>,
}
impl $name {
pub(crate) fn to_sys(
&self,
cstrings: &mut Vec<CString>,
) -> $sys_name {
<$sys_name> {
model: to_c_ptr(&self.model, cstrings),
}
}
}
};
}
define_denoiser_model_config!(
OfflineSpeechDenoiserGtcrnModelConfig,
sys::OfflineSpeechDenoiserGtcrnModelConfig
);
define_denoiser_model_config!(
OfflineSpeechDenoiserDpdfNetModelConfig,
sys::OfflineSpeechDenoiserDpdfNetModelConfig
);There was a problem hiding this comment.
Pull request overview
Refactors the Rust speech denoiser API by moving shared types/configs into the offline denoiser module and updating imports accordingly.
Changes:
- Removed
speech_denoiser.rsand migrated its configs/types intooffline_speech_denoiser.rs - Updated
online_speech_denoiser.rsto import denoiser types fromoffline_speech_denoiser - Removed
speech_denoisermodule and its public re-exports fromlib.rs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sherpa-onnx/rust/sherpa-onnx/src/speech_denoiser.rs | Deleted; its structs/conversion logic were relocated. |
| sherpa-onnx/rust/sherpa-onnx/src/offline_speech_denoiser.rs | Added the moved model config structs and DenoisedAudio implementation. |
| sherpa-onnx/rust/sherpa-onnx/src/online_speech_denoiser.rs | Updated imports to reference the new module location. |
| sherpa-onnx/rust/sherpa-onnx/src/lib.rs | Removed module declaration and re-export for speech_denoiser. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mod online_punctuation; | ||
| mod online_speech_denoiser; | ||
| mod speaker_embedding; | ||
| mod speech_denoiser; | ||
| mod tts; | ||
| mod utils; | ||
| mod vad; |
There was a problem hiding this comment.
offline_speech_denoiser.rs exists and online_speech_denoiser.rs now imports from crate::offline_speech_denoiser::..., but lib.rs does not declare mod offline_speech_denoiser; in the module list shown here. This will fail to compile with an unresolved import unless the module is declared elsewhere. Add mod offline_speech_denoiser; (and consider whether it needs to be pub/re-exported depending on intended public API).
| pub use online_punctuation::*; | ||
| pub use online_speech_denoiser::*; | ||
| pub use speaker_embedding::*; | ||
| pub use speech_denoiser::*; | ||
| pub use tts::*; | ||
| pub use utils::*; | ||
| pub use vad::*; |
There was a problem hiding this comment.
This PR removes pub use speech_denoiser::*; (and the entire speech_denoiser module), which is a breaking change for downstream crates importing sherpa_onnx::speech_denoiser::* or relying on those re-exports. If the intent is a non-breaking refactor, consider keeping a compatibility shim (e.g., reintroduce speech_denoiser as a module that re-exports the moved items, possibly marked #[deprecated]), or re-export the moved items from the crate root to preserve prior paths.
| pub(crate) fn from_ptr(ptr: *const sys::DenoisedAudio) -> Self { | ||
| if ptr.is_null() { | ||
| return Self::default(); | ||
| } | ||
|
|
||
| unsafe { | ||
| let n = (*ptr).n.max(0) as usize; | ||
| let samples = if (*ptr).samples.is_null() || n == 0 { | ||
| vec![] | ||
| } else { | ||
| slice::from_raw_parts((*ptr).samples, n).to_vec() | ||
| }; | ||
| let sample_rate = (*ptr).sample_rate; | ||
| sys::SherpaOnnxDestroyDenoisedAudio(ptr); | ||
| Self { | ||
| samples, | ||
| sample_rate, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
from_ptr always calls SherpaOnnxDestroyDenoisedAudio(ptr), so it takes ownership of the incoming pointer. That ownership contract isn’t obvious from the name/signature (especially since it accepts *const). To reduce the risk of accidental double-free/use-after-free by future callers, consider renaming to something that encodes ownership (e.g., from_owned_ptr), adding a brief doc comment stating it consumes/frees the pointer, and/or switching the parameter type to *mut sys::DenoisedAudio if the C API expects a mutable pointer for destroy.
Summary by CodeRabbit