Repository navigation
Add Rust API for VAD - #3213
Add Rust API for VAD#3213
Conversation
dosubot
Bot
added
the
size:XL
📝 WalkthroughWalkthroughAdds FFI and high-level Rust support for voice-activity-detection (VAD) including Silero/TenVad configs, circular buffer and detector types, WAV writing, a runnable silence-removal example + script, C API null guards, and version bumps across Rust crates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Example as Rust Example
participant RustVad as Rust VAD Wrapper
participant Sys as sherpa-onnx-sys (C API)
participant Model as VAD Model (ONNX)
participant File as WAV I/O
User->>Example: run silero_vad_remove_silence --input --output --model
Example->>File: read input WAV samples
Example->>RustVad: build VadModelConfig (paths, params)
RustVad->>Sys: convert config & call SherpaOnnxCreateVoiceActivityDetector
Example->>RustVad: feed samples in chunks (accept_waveform)
RustVad->>Sys: SherpaOnnxVoiceActivityDetectorAcceptWaveform
Sys->>RustVad: detection events / SpeechSegment pointers
Example->>RustVad: collect segments, flush detector
RustVad->>Sys: SherpaOnnxVoiceActivityDetectorFlush
Example->>File: write collected speech samples via SherpaOnnxWriteWave
File->>User: output no-silence.wav created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 @csukuangfj, 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 significantly expands the Rust API by integrating Voice Activity Detection (VAD) capabilities. It introduces new modules and structures to allow Rust applications to perform VAD, specifically demonstrated through an example that removes silent segments from audio files. This addition enhances the functionality of 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
The pull request introduces a new Rust API for Voice Activity Detection (VAD) using SileroVAD, along with an example demonstrating how to remove silences from a WAV file. It also updates the Cargo.lock and Cargo.toml files to reflect version changes for rust-api-examples, sherpa-onnx, and sherpa-onnx-sys. Additionally, it includes a shell script to run the new VAD example and modifies the C API to add a null check before accessing SherpaOnnxVoiceActivityDetectorFront.
Overall, the changes are well-structured and add valuable functionality. The new VAD example is clear and demonstrates the API usage effectively. The version bumps and Cargo.lock updates are standard for new feature additions. The C API change improves robustness.
One minor improvement opportunity is to ensure consistency in error handling within the Rust VAD example, particularly when writing the output WAV file. Also, the copyright year in the new Rust file should be updated to reflect the current year or a more appropriate future year if it's a forward-looking statement.
| @@ -0,0 +1,109 @@ | |||
| // Copyright (c) 2026 Xiaomi Corporation | |||
There was a problem hiding this comment.
The copyright year is set to 2026. It's generally good practice to either use the current year or a range (e.g., 2023-2024) if the project is actively maintained. If 2026 is intentional for some future-proofing, it might be worth a small comment explaining why.
| let ok = sherpa_onnx::write(&args.output, &speech_samples, sample_rate); | ||
| if ok { | ||
| println!("Saved speech-only audio to {}", args.output); | ||
| } else { | ||
| println!("Failed to save speech-only audio to {}", args.output); | ||
| } |
There was a problem hiding this comment.
The sherpa_onnx::write function returns a boolean indicating success or failure. While the if ok block handles the success case, the else block only prints a failure message. It might be beneficial to return an anyhow::Result from main that propagates this error, or at least log the error more formally if this is a critical operation.
let ok = sherpa_onnx::write(&args.output, &speech_samples, sample_rate);
if !ok {
anyhow::bail!("Failed to save speech-only audio to {}", args.output);
}
println!("Saved speech-only audio to {}", args.output);| if (SherpaOnnxVoiceActivityDetectorEmpty(p)) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Adding a null check for SherpaOnnxVoiceActivityDetectorEmpty(p) before calling p->impl->Front() is a good defensive programming practice. This prevents potential crashes if Front() is called on an empty detector, improving the robustness of the C API.
There was a problem hiding this comment.
Pull request overview
Adds Rust bindings and examples for voice activity detection (VAD) and WAV writing via the SherpaOnnx C API.
Changes:
- Introduces a safe-ish Rust wrapper for the SherpaOnnx VAD C API (configs, circular buffer, speech segments, detector).
- Adds WAV write support to both
sherpa-onnx-sys(FFI) andsherpa-onnx(safe wrapper). - Adds a Silero VAD Rust example + CI script invocation.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/rust/sherpa-onnx/src/wave.rs | Adds high-level WAV writing helpers calling into C API |
| sherpa-onnx/rust/sherpa-onnx/src/vad.rs | New Rust VAD wrapper types and functions over the C API |
| sherpa-onnx/rust/sherpa-onnx/src/lib.rs | Exposes the new vad module from the crate |
| sherpa-onnx/rust/sherpa-onnx/Cargo.toml | Bumps crate and sys dependency versions for the new API |
| sherpa-onnx/rust/sherpa-onnx-sys/src/wave.rs | Adds FFI declaration for SherpaOnnxWriteWave |
| sherpa-onnx/rust/sherpa-onnx-sys/src/vad.rs | New raw FFI bindings for VAD-related C API |
| sherpa-onnx/rust/sherpa-onnx-sys/src/lib.rs | Exposes the new vad bindings module |
| sherpa-onnx/rust/sherpa-onnx-sys/Cargo.toml | Bumps sys crate version |
| sherpa-onnx/c-api/c-api.cc | Adds empty-check guard to VAD Front() C API |
| rust-api-examples/run-silero-vad-remove-silence.sh | New runnable script to fetch assets and execute the example |
| rust-api-examples/examples/silero_vad_remove_silence.rs | New Rust example demonstrating VAD-based silence removal |
| rust-api-examples/README.md | Documents the new example |
| rust-api-examples/Cargo.toml | Bumps examples crate + sherpa-onnx dependency version |
| .github/scripts/test-rust.sh | Adds the new example script to CI runs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn write(&self, filename: &str) -> bool { | ||
| let c_filename = CString::new(filename).unwrap(); | ||
| unsafe { | ||
| sys::SherpaOnnxWriteWave( | ||
| (*self.inner).samples, | ||
| (*self.inner).num_samples, | ||
| (*self.inner).sample_rate, | ||
| c_filename.as_ptr(), | ||
| ) == 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
CString::new(filename).unwrap() will panic if filename contains an interior NUL byte, which is a surprising failure mode for a method that reports errors via bool. Prefer handling the error (e.g., return false) or changing the API to return a Result so the caller can see why it failed.
| pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool { | ||
| let c_filename = CString::new(filename).unwrap(); | ||
| unsafe { | ||
| sys::SherpaOnnxWriteWave( | ||
| samples.as_ptr(), | ||
| samples.len() as i32, | ||
| sample_rate, | ||
| c_filename.as_ptr(), | ||
| ) == 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
samples.len() as i32 can truncate/overflow for large inputs, producing an incorrect (potentially negative) n passed to the C API. Use a checked conversion (e.g., try_into()) and return false (or an error) when the length doesn’t fit in i32.
| } | ||
|
|
||
| pub fn samples(&self) -> &[f32] { | ||
| unsafe { slice::from_raw_parts((*self.ptr).samples, (*self.ptr).n as usize) } |
There was a problem hiding this comment.
Casting (*self.ptr).n (an i32) to usize without validation can cause UB if n is negative (it becomes a huge usize length). It would be safer to clamp non-positive values to an empty slice and use a checked conversion for positive values (or change the API to return Option<&[f32]>).
| unsafe { slice::from_raw_parts((*self.ptr).samples, (*self.ptr).n as usize) } | |
| unsafe { | |
| let n = (*self.ptr).n; | |
| if n <= 0 { | |
| // Clamp non-positive lengths to an empty slice to avoid UB. | |
| return &[]; | |
| } | |
| let len = match usize::try_from(n) { | |
| Ok(len) => len, | |
| // If conversion somehow fails, return an empty slice as a safe fallback. | |
| Err(_) => return &[], | |
| }; | |
| slice::from_raw_parts((*self.ptr).samples, len) | |
| } |
| pub fn get(&self, start_index: i32, n: i32) -> Vec<f32> { | ||
| unsafe { | ||
| let p = sys::SherpaOnnxCircularBufferGet(self.ptr, start_index, n); | ||
| if p.is_null() { | ||
| return vec![]; | ||
| } | ||
| let slice = slice::from_raw_parts(p, n as usize); | ||
| let result = slice.to_vec(); | ||
| sys::SherpaOnnxCircularBufferFree(p); | ||
| result | ||
| } | ||
| } |
There was a problem hiding this comment.
Accepting n: i32 allows callers to pass negative lengths, which then become a huge usize in from_raw_parts and can trigger UB. Consider making n a usize (and similarly for indices if appropriate), or validate n >= 0 before calling into C and before converting to usize.
|
|
||
| ./run-version.sh | ||
|
|
||
| ./run-silero-vad-remove-silence.sh |
There was a problem hiding this comment.
This adds a network-dependent step (downloads model/audio via curl) to the primary Rust test script, which can make CI flaky and slower. Consider gating it behind an env flag (e.g., RUN_NETWORK_TESTS=1), adding retries/checksums, and/or caching the assets in CI to keep the test pipeline deterministic.
| ./run-silero-vad-remove-silence.sh | |
| if [ "${RUN_NETWORK_TESTS:-0}" = "1" ]; then | |
| ./run-silero-vad-remove-silence.sh | |
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-api-examples/examples/silero_vad_remove_silence.rs`:
- Around line 83-89: The code currently ignores failures from sherpa_onnx::write
by always returning Ok(()) from main; change main (the function returning
anyhow::Result<()>) to propagate write failures: check the boolean result of
sherpa_onnx::write(&args.output, &speech_samples, sample_rate) and if false
return an Err (e.g., anyhow::anyhow! or anyhow::bail!) with a descriptive
message mentioning args.output so the process exits non-zero; keep or adjust the
println! messages as needed but ensure the false branch returns an error instead
of continuing to Ok(()).
In `@rust-api-examples/run-silero-vad-remove-silence.sh`:
- Around line 5-11: The curl invocations that download "./silero_vad.onnx" and
"./lei-jun-test.wav" should use the --fail flag so the script fails on HTTP
4xx/5xx; update the two lines containing "curl -SL -O
https://.../silero_vad.onnx" and "curl -SL -O https://.../lei-jun-test.wav" to
include -f (e.g., "curl -fSL -O ..." or "--fail -SL -O ...") so set -e will stop
the script on download errors.
In `@sherpa-onnx/rust/sherpa-onnx/src/vad.rs`:
- Around line 101-112: The get method (CircularBuffer::get / pub fn get) must
validate that n is non-negative before converting to usize to avoid UB from
slice::from_raw_parts; add a guard like if n <= 0 { return vec![] } (or return
an Err if you prefer) before calling sys::SherpaOnnxCircularBufferGet, then
safely cast n to usize for slice::from_raw_parts and still check p.is_null() and
call sys::SherpaOnnxCircularBufferFree(p) as currently done.
- Around line 81-83: The FFI wrapper structs CircularBuffer, SpeechSegment, and
VoiceActivityDetector currently auto-derive Send/Sync because they contain raw
pointers; add a PhantomData<*mut ()> field to each struct to opt out of
auto-derived Send/Sync (preventing unsound concurrent use from methods that
mutate C++ state such as CircularBuffer::push/CircularBuffer::pop,
VoiceActivityDetector::accept_waveform/flush/reset, and SpeechSegment::clear),
then only add explicit unsafe impl Send/Sync for any of these types if you can
guarantee the underlying C++ object is thread-safe; update the struct
definitions to include PhantomData<*mut ()> and adjust any constructors or trait
impls accordingly.
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs`:
- Around line 79-87: The cast samples.len() as i32 in write() can truncate large
buffers; add a checked conversion before calling sys::SherpaOnnxWriteWave:
validate samples.len() fits in i32 (use i32::try_from or usize::try_into) and
return false early on failure, then pass the safely converted length to
sys::SherpaOnnxWriteWave; apply the same pattern to the other occurrences in
vad.rs, online_asr.rs, and offline_asr.rs that cast buffer lengths to i32.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust-api-examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/scripts/test-rust.shrust-api-examples/Cargo.tomlrust-api-examples/README.mdrust-api-examples/examples/silero_vad_remove_silence.rsrust-api-examples/run-silero-vad-remove-silence.shsherpa-onnx/c-api/c-api.ccsherpa-onnx/rust/sherpa-onnx-sys/Cargo.tomlsherpa-onnx/rust/sherpa-onnx-sys/src/lib.rssherpa-onnx/rust/sherpa-onnx-sys/src/vad.rssherpa-onnx/rust/sherpa-onnx-sys/src/wave.rssherpa-onnx/rust/sherpa-onnx/Cargo.tomlsherpa-onnx/rust/sherpa-onnx/src/lib.rssherpa-onnx/rust/sherpa-onnx/src/vad.rssherpa-onnx/rust/sherpa-onnx/src/wave.rs
| // Write speech-only samples to output WAV | ||
| let ok = sherpa_onnx::write(&args.output, &speech_samples, sample_rate); | ||
| if ok { | ||
| println!("Saved speech-only audio to {}", args.output); | ||
| } else { | ||
| println!("Failed to save speech-only audio to {}", args.output); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n rust-api-examples/examples/silero_vad_remove_silence.rs | head -100Repository: k2-fsa/sherpa-onnx
Length of output: 3855
🏁 Script executed:
cat -n rust-api-examples/examples/silero_vad_remove_silence.rs | tail -20Repository: k2-fsa/sherpa-onnx
Length of output: 816
🏁 Script executed:
find . -name "Cargo.toml" -path "*/rust-api-examples/*" -o -name "Cargo.toml" -path "*" | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 210
🏁 Script executed:
cat rust-api-examples/Cargo.tomlRepository: k2-fsa/sherpa-onnx
Length of output: 586
Propagate write failures via non-zero exit.
The function returns anyhow::Result<()> but masks write failures by returning Ok(()) regardless of the write result. This allows the program to exit with code 0 even when writing fails, making it impossible for CI to detect the failure.
Suggested fix
- let ok = sherpa_onnx::write(&args.output, &speech_samples, sample_rate);
- if ok {
- println!("Saved speech-only audio to {}", args.output);
- } else {
- println!("Failed to save speech-only audio to {}", args.output);
- }
+ if !sherpa_onnx::write(&args.output, &speech_samples, sample_rate) {
+ anyhow::bail!("Failed to save speech-only audio to {}", args.output);
+ }
+ println!("Saved speech-only audio to {}", args.output);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Write speech-only samples to output WAV | |
| let ok = sherpa_onnx::write(&args.output, &speech_samples, sample_rate); | |
| if ok { | |
| println!("Saved speech-only audio to {}", args.output); | |
| } else { | |
| println!("Failed to save speech-only audio to {}", args.output); | |
| } | |
| // Write speech-only samples to output WAV | |
| if !sherpa_onnx::write(&args.output, &speech_samples, sample_rate) { | |
| anyhow::bail!("Failed to save speech-only audio to {}", args.output); | |
| } | |
| println!("Saved speech-only audio to {}", args.output); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-api-examples/examples/silero_vad_remove_silence.rs` around lines 83 -
89, The code currently ignores failures from sherpa_onnx::write by always
returning Ok(()) from main; change main (the function returning
anyhow::Result<()>) to propagate write failures: check the boolean result of
sherpa_onnx::write(&args.output, &speech_samples, sample_rate) and if false
return an Err (e.g., anyhow::anyhow! or anyhow::bail!) with a descriptive
message mentioning args.output so the process exits non-zero; keep or adjust the
println! messages as needed but ensure the false branch returns an error instead
of continuing to Ok(()).
| if [ ! -f "./silero_vad.onnx" ]; then | ||
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/silero_vad.onnx | ||
| fi | ||
|
|
||
| if [ ! -f ./lei-jun-test.wav ]; then | ||
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/lei-jun-test.wav | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the script file
find . -name "*run-silero-vad-remove-silence.sh" -type fRepository: k2-fsa/sherpa-onnx
Length of output: 114
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "./rust-api-examples/run-silero-vad-remove-silence.sh" ]; then
cat -n "./rust-api-examples/run-silero-vad-remove-silence.sh"
fiRepository: k2-fsa/sherpa-onnx
Length of output: 696
🌐 Web query:
curl -SL -O HTTP 4xx 5xx error behavior exit code
💡 Result:
By default, curl -SL -O <url> does not treat HTTP 4xx/5xx as a failure. If the HTTP response is received successfully (even 404/500), curl typically exits 0 and (with -O) will save the server’s error page into the output file. (ec.haxx.se)
To make curl fail (non‑zero exit) on HTTP errors, add one of these:
- Fail on HTTP ≥ 400 and discard the body (good for
-Odownloads):curl -fSL -O <url>-f/--failmakes HTTP errors return exit code 22 (“HTTP not found / requested page not found”), and this code only appears if--failis used. (curl.se)
- Fail on HTTP ≥ 400 but keep the body (useful for debugging API errors):
curl --fail-with-body -SL <url>- This also returns exit code 22 while still outputting/saving the response body. (ec.haxx.se)
Notes on your flags:
-Lfollows redirects; it doesn’t change 4xx/5xx exit behavior by itself. (ec.haxx.se)-S(“show error”) is commonly paired with-s(silent) to still print errors; on its own it doesn’t make HTTP 4xx/5xx fail—you still need-for--fail-with-body. (superuser.com)
Citations:
- 1: https://ec.haxx.se/http/response.html?utm_source=openai
- 2: https://curl.se/mail/archive-2003-03/0081.html?utm_source=openai
- 3: https://ec.haxx.se/http/response.html?utm_source=openai
- 4: https://ec.haxx.se/http/response.html?utm_source=openai
- 5: https://superuser.com/questions/590099/can-i-make-curl-fail-with-an-exitcode-different-than-0-if-the-http-status-code-i?utm_source=openai
🌐 Web query:
curl -f flag fail on HTTP error exit code
💡 Result:
curl -f / --fail makes curl treat HTTP response codes ≥ 400 as an error, so the command exits non‑zero instead of 0. Specifically, it returns exit code 22 (CURLE_HTTP_RETURNED_ERROR). By default it also suppresses the response body for those HTTP errors (“fail silently”). [1][2]
Common patterns:
- Fail the script on 4xx/5xx (no body on error):
curl -f https://example.com echo $?
- Show an error message while still being silent otherwise:
curl -fsS https://example.com
- If you want to fail but still keep the body on HTTP errors (curl ≥ 7.76):
curl --fail-with-body https://example.com ``` [3]
Sources: [1] [2] [3]
Fail fast on download errors.
curl -SL -O doesn't fail on HTTP 4xx/5xx by default, so the script can proceed with corrupt or missing files. Add -f to make set -e effective.
🔧 Suggested fix
- curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/silero_vad.onnx
+ curl -fSL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/silero_vad.onnx
...
- curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/lei-jun-test.wav
+ curl -fSL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/lei-jun-test.wav📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -f "./silero_vad.onnx" ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/silero_vad.onnx | |
| fi | |
| if [ ! -f ./lei-jun-test.wav ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/lei-jun-test.wav | |
| fi | |
| if [ ! -f "./silero_vad.onnx" ]; then | |
| curl -fSL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/silero_vad.onnx | |
| fi | |
| if [ ! -f ./lei-jun-test.wav ]; then | |
| curl -fSL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/lei-jun-test.wav | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-api-examples/run-silero-vad-remove-silence.sh` around lines 5 - 11, The
curl invocations that download "./silero_vad.onnx" and "./lei-jun-test.wav"
should use the --fail flag so the script fails on HTTP 4xx/5xx; update the two
lines containing "curl -SL -O https://.../silero_vad.onnx" and "curl -SL -O
https://.../lei-jun-test.wav" to include -f (e.g., "curl -fSL -O ..." or "--fail
-SL -O ...") so set -e will stop the script on download errors.
| pub struct CircularBuffer { | ||
| ptr: *const sys::CircularBuffer, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the vad.rs file structure and the specific lines mentioned
wc -l sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 105
🏁 Script executed:
# View the specific lines mentioned in the review
sed -n '75,90p' sherpa-onnx/rust/sherpa-onnx/src/vad.rs
sed -n '130,145p' sherpa-onnx/rust/sherpa-onnx/src/vad.rs
sed -n '155,170p' sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 1199
🏁 Script executed:
# Check for existing Send/Sync or PhantomData implementations
rg -n "PhantomData|unsafe impl (Send|Sync)" sherpa-onnx/rust/sherpa-onnx/srcRepository: k2-fsa/sherpa-onnx
Length of output: 44
🏁 Script executed:
# Find all raw pointer struct patterns in the FFI wrappers
rg -n "struct .* \{" sherpa-onnx/rust/sherpa-onnx/src/vad.rs -A 2Repository: k2-fsa/sherpa-onnx
Length of output: 629
🏁 Script executed:
# View the full CircularBuffer implementation
sed -n '81,115p' sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 1075
🏁 Script executed:
# View the full SpeechSegment implementation
sed -n '137,160p' sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 556
🏁 Script executed:
# View the full VoiceActivityDetector implementation
sed -n '161,235p' sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 2017
🏁 Script executed:
# Check if sys crate opaque types have Send/Sync bounds
grep -r "CircularBuffer\|SpeechSegment\|VoiceActivityDetector" sherpa-onnx/rust/sherpa-onnx/src/lib.rsRepository: k2-fsa/sherpa-onnx
Length of output: 44
🏁 Script executed:
# Search for thread-safety comments or documentation in the Rust code
rg -i "thread|safe|concurrent|Send|Sync" sherpa-onnx/rust/sherpa-onnx/src/vad.rsRepository: k2-fsa/sherpa-onnx
Length of output: 1353
🏁 Script executed:
# Check the sys bindings or C headers for any thread-safety info
fd "\.rs$" sherpa-onnx/rust/ | xargs rg -l "CircularBuffer|VoiceActivityDetector" | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 145
🏁 Script executed:
# Check if there's any documentation or unsafe reasoning comments
sed -n '1,100p' sherpa-onnx/rust/sherpa-onnx/src/vad.rs | grep -i "safety\|safety\|thread\|concurrent"Repository: k2-fsa/sherpa-onnx
Length of output: 130
Add PhantomData<*mut ()> to opt out of auto-derived Send/Sync for FFI handles.
CircularBuffer, SpeechSegment, and VoiceActivityDetector auto-derive Send/Sync due to raw pointers, yet their &self methods mutate underlying C++ state (e.g., push, accept_waveform, pop, clear, reset, flush). If the C++ types aren't thread-safe, this enables unsound concurrent access. Add a PhantomData<*mut ()> field to each struct to opt out, then only unsafe impl Send/Sync with explicit thread-safety guarantees.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/vad.rs` around lines 81 - 83, The FFI
wrapper structs CircularBuffer, SpeechSegment, and VoiceActivityDetector
currently auto-derive Send/Sync because they contain raw pointers; add a
PhantomData<*mut ()> field to each struct to opt out of auto-derived Send/Sync
(preventing unsound concurrent use from methods that mutate C++ state such as
CircularBuffer::push/CircularBuffer::pop,
VoiceActivityDetector::accept_waveform/flush/reset, and SpeechSegment::clear),
then only add explicit unsafe impl Send/Sync for any of these types if you can
guarantee the underlying C++ object is thread-safe; update the struct
definitions to include PhantomData<*mut ()> and adjust any constructors or trait
impls accordingly.
| pub fn get(&self, start_index: i32, n: i32) -> Vec<f32> { | ||
| unsafe { | ||
| let p = sys::SherpaOnnxCircularBufferGet(self.ptr, start_index, n); | ||
| if p.is_null() { | ||
| return vec![]; | ||
| } | ||
| let slice = slice::from_raw_parts(p, n as usize); | ||
| let result = slice.to_vec(); | ||
| sys::SherpaOnnxCircularBufferFree(p); | ||
| result | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against negative n to avoid UB in CircularBuffer::get.
n is an i32, and a negative value will cast to a huge usize and make from_raw_parts unsound. Since this is a safe API, validate n before use.
🛡️ Suggested fix
pub fn get(&self, start_index: i32, n: i32) -> Vec<f32> {
- unsafe {
+ if n <= 0 {
+ return vec![];
+ }
+ unsafe {
let p = sys::SherpaOnnxCircularBufferGet(self.ptr, start_index, n);
if p.is_null() {
return vec![];
}
- let slice = slice::from_raw_parts(p, n as usize);
+ let slice = slice::from_raw_parts(p, n as usize);
let result = slice.to_vec();
sys::SherpaOnnxCircularBufferFree(p);
result
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get(&self, start_index: i32, n: i32) -> Vec<f32> { | |
| unsafe { | |
| let p = sys::SherpaOnnxCircularBufferGet(self.ptr, start_index, n); | |
| if p.is_null() { | |
| return vec![]; | |
| } | |
| let slice = slice::from_raw_parts(p, n as usize); | |
| let result = slice.to_vec(); | |
| sys::SherpaOnnxCircularBufferFree(p); | |
| result | |
| } | |
| } | |
| pub fn get(&self, start_index: i32, n: i32) -> Vec<f32> { | |
| if n <= 0 { | |
| return vec![]; | |
| } | |
| unsafe { | |
| let p = sys::SherpaOnnxCircularBufferGet(self.ptr, start_index, n); | |
| if p.is_null() { | |
| return vec![]; | |
| } | |
| let slice = slice::from_raw_parts(p, n as usize); | |
| let result = slice.to_vec(); | |
| sys::SherpaOnnxCircularBufferFree(p); | |
| result | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/vad.rs` around lines 101 - 112, The get
method (CircularBuffer::get / pub fn get) must validate that n is non-negative
before converting to usize to avoid UB from slice::from_raw_parts; add a guard
like if n <= 0 { return vec![] } (or return an Err if you prefer) before calling
sys::SherpaOnnxCircularBufferGet, then safely cast n to usize for
slice::from_raw_parts and still check p.is_null() and call
sys::SherpaOnnxCircularBufferFree(p) as currently done.
| pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool { | ||
| let c_filename = CString::new(filename).unwrap(); | ||
| unsafe { | ||
| sys::SherpaOnnxWriteWave( | ||
| samples.as_ptr(), | ||
| samples.len() as i32, | ||
| sample_rate, | ||
| c_filename.as_ptr(), | ||
| ) == 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd sherpa-onnx/rust/sherpa-onnx && find . -name "wave.rs" -o -name "*.rs" | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 174
🏁 Script executed:
cd sherpa-onnx && cat -n rust/sherpa-onnx/src/wave.rs | head -100Repository: k2-fsa/sherpa-onnx
Length of output: 2960
🏁 Script executed:
cd sherpa-onnx && rg "SherpaOnnxWriteWave" --type rust -A 2 -B 2Repository: k2-fsa/sherpa-onnx
Length of output: 1022
🏁 Script executed:
cd sherpa-onnx && rg "fn write" --type rust -A 10 -B 2 sherpa-onnx/rust/sherpa-onnx/src/wave.rsRepository: k2-fsa/sherpa-onnx
Length of output: 142
🏁 Script executed:
cd sherpa-onnx && find . -name "*.h" -o -name "*.hpp" | xargs grep -l "SherpaOnnxWriteWave" 2>/dev/null | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 77
🏁 Script executed:
cd sherpa-onnx && rg "write.*samples" --type rust -B 5 -A 5 | head -50Repository: k2-fsa/sherpa-onnx
Length of output: 745
🏁 Script executed:
cd sherpa-onnx && rg "write\(" --type rust -B 3 -A 3 | grep -A 5 -B 5 "samples" | head -60Repository: k2-fsa/sherpa-onnx
Length of output: 844
🏁 Script executed:
cd sherpa-onnx && rg "samples\.len\(\)" --type rust -B 2 -A 2Repository: k2-fsa/sherpa-onnx
Length of output: 1552
🏁 Script executed:
cd sherpa-onnx && find . -type f -name "*.md" | xargs grep -l "buffer\|sample\|size" 2>/dev/null | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 44
🏁 Script executed:
cd sherpa-onnx && cat c-api/c-api.h | grep -A 20 "SherpaOnnxWriteWave"Repository: k2-fsa/sherpa-onnx
Length of output: 1486
🏁 Script executed:
cd sherpa-onnx && find . -path "*test*" -name "*.rs" | xargs grep -l "write\|wave" 2>/dev/null | head -5Repository: k2-fsa/sherpa-onnx
Length of output: 44
Guard against usize→i32 truncation when converting sample buffer length. The cast samples.len() as i32 can overflow for buffers exceeding i32::MAX samples. While this is unlikely in typical audio workloads (i32::MAX ≈ 46 days at 48kHz), it represents unsound type conversion. Use a checked conversion with early return to handle oversized buffers safely.
Note: This same pattern appears in other functions (vad.rs, online_asr.rs, offline_asr.rs). Consider applying the fix systematically across the codebase.
Suggested fix
pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool {
let c_filename = CString::new(filename).unwrap();
+ let n = match i32::try_from(samples.len()) {
+ Ok(n) => n,
+ Err(_) => return false,
+ };
unsafe {
sys::SherpaOnnxWriteWave(
samples.as_ptr(),
- samples.len() as i32,
+ n,
sample_rate,
c_filename.as_ptr(),
) == 1
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool { | |
| let c_filename = CString::new(filename).unwrap(); | |
| unsafe { | |
| sys::SherpaOnnxWriteWave( | |
| samples.as_ptr(), | |
| samples.len() as i32, | |
| sample_rate, | |
| c_filename.as_ptr(), | |
| ) == 1 | |
| pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool { | |
| let c_filename = CString::new(filename).unwrap(); | |
| let n = match i32::try_from(samples.len()) { | |
| Ok(n) => n, | |
| Err(_) => return false, | |
| }; | |
| unsafe { | |
| sys::SherpaOnnxWriteWave( | |
| samples.as_ptr(), | |
| n, | |
| sample_rate, | |
| c_filename.as_ptr(), | |
| ) == 1 | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/rust/sherpa-onnx/src/wave.rs` around lines 79 - 87, The cast
samples.len() as i32 in write() can truncate large buffers; add a checked
conversion before calling sys::SherpaOnnxWriteWave: validate samples.len() fits
in i32 (use i32::try_from or usize::try_into) and return false early on failure,
then pass the safely converted length to sys::SherpaOnnxWriteWave; apply the
same pattern to the other occurrences in vad.rs, online_asr.rs, and
offline_asr.rs that cast buffer lengths to i32.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sherpa-onnx/csrc/voice-activity-detector.cc (1)
144-155:static SpeechSegment tmpwill havestart == 0, not the-1sentinel used elsewhere.C++ zero-initializes static-storage-duration variables, so
tmp.startwill be0, while every other "invalid" sentinel in this file (cur_segment_.start = -1,start_ = -1) uses-1. Any direct C++ caller that inspectsFront().startwithout first checkingEmpty()will get a misleading value.The error is already logged and the C-API wrapper now guards against the empty case, so this is low-risk in practice — but worth being explicit.
🔧 Proposed fix: explicitly set the error-sentinel
const SpeechSegment &Front() const { - static SpeechSegment tmp; + static SpeechSegment tmp = {/*.start=*/-1, /*.samples=*/{}}; if (Empty()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/csrc/voice-activity-detector.cc` around lines 144 - 155, The Front() method returns a static SpeechSegment tmp that is zero-initialized, causing tmp.start == 0 instead of the file's -1 sentinel; change the sentinel by explicitly setting tmp.start = -1 (and any other sentinel fields used elsewhere if needed) before returning when Empty() is true so callers that inspect Front().start get the correct invalid value; locate this in the Front() implementation in voice-activity-detector.cc near the Empty() check and adjust the static tmp initialization accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Around line 1188-1191: SherpaOnnxCreateVoiceActivityDetectorOHOS currently
calls GetVadModelConfig(config) on the OHOS branch without ensuring the returned
config is non-null; update SherpaOnnxCreateVoiceActivityDetectorOHOS to check
the result of GetVadModelConfig(config) (and/or verify mgr and config before
calling) and handle a nullptr by logging an error and returning nullptr instead
of proceeding, so GetVadModelConfig(nullptr) cannot be dereferenced; reference
the GetVadModelConfig function and the SherpaOnnxCreateVoiceActivityDetectorOHOS
caller to locate where to add the null check and error handling.
---
Nitpick comments:
In `@sherpa-onnx/csrc/voice-activity-detector.cc`:
- Around line 144-155: The Front() method returns a static SpeechSegment tmp
that is zero-initialized, causing tmp.start == 0 instead of the file's -1
sentinel; change the sentinel by explicitly setting tmp.start = -1 (and any
other sentinel fields used elsewhere if needed) before returning when Empty() is
true so callers that inspect Front().start get the correct invalid value; locate
this in the Front() implementation in voice-activity-detector.cc near the
Empty() check and adjust the static tmp initialization accordingly.
| if (!config) { | ||
| SHERPA_ONNX_LOGE("vad config is nullptr"); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
OHOS path skips the same null guard when mgr != nullptr.
The new check correctly prevents GetVadModelConfig(nullptr) from dereferencing a null pointer. However, SherpaOnnxCreateVoiceActivityDetectorOHOS (around Line 2879) calls GetVadModelConfig(config) directly without a prior null check on the mgr != nullptr branch, leaving a null-dereference gap there.
🛡️ Proposed fix for the OHOS path
const SherpaOnnxVoiceActivityDetector *
SherpaOnnxCreateVoiceActivityDetectorOHOS(
const SherpaOnnxVadModelConfig *config, float buffer_size_in_seconds,
NativeResourceManager *mgr) {
if (mgr == nullptr) {
return SherpaOnnxCreateVoiceActivityDetector(config,
buffer_size_in_seconds);
}
+ if (!config) {
+ SHERPA_ONNX_LOGE("vad config is nullptr");
+ return nullptr;
+ }
+
auto vad_config = GetVadModelConfig(config);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/c-api/c-api.cc` around lines 1188 - 1191,
SherpaOnnxCreateVoiceActivityDetectorOHOS currently calls
GetVadModelConfig(config) on the OHOS branch without ensuring the returned
config is non-null; update SherpaOnnxCreateVoiceActivityDetectorOHOS to check
the result of GetVadModelConfig(config) (and/or verify mgr and config before
calling) and handle a nullptr by logging an error and returning nullptr instead
of proceeding, so GetVadModelConfig(nullptr) cannot be dereferenced; reference
the GetVadModelConfig function and the SherpaOnnxCreateVoiceActivityDetectorOHOS
caller to locate where to add the null check and error handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// | ||
| /// Returns true if succeeded, false otherwise. | ||
| pub fn write(&self, filename: &str) -> bool { | ||
| let c_filename = CString::new(filename).unwrap(); |
There was a problem hiding this comment.
The unwrap() will panic if the filename contains null bytes, but provides no context about what went wrong. Consider using expect() with a descriptive message like 'Filename must not contain null bytes'.
| /// | ||
| /// Returns true on success, false otherwise. | ||
| pub fn write(filename: &str, samples: &[f32], sample_rate: i32) -> bool { | ||
| let c_filename = CString::new(filename).unwrap(); |
There was a problem hiding this comment.
The unwrap() will panic if the filename contains null bytes, but provides no context about what went wrong. Consider using expect() with a descriptive message like 'Filename must not contain null bytes'.
| pub sample_rate: i32, | ||
| pub num_threads: i32, | ||
| pub provider: *const c_char, | ||
| pub debug: i32, | ||
| pub ten_vad: TenVadModelConfig, |
There was a problem hiding this comment.
The field order in VadModelConfig doesn't match the order in the Rust wrapper (vad.rs lines 56-61), where ten_vad comes before sample_rate. This inconsistency could lead to confusion. The struct layout should match the logical ordering in the high-level API.
| pub sample_rate: i32, | |
| pub num_threads: i32, | |
| pub provider: *const c_char, | |
| pub debug: i32, | |
| pub ten_vad: TenVadModelConfig, | |
| pub ten_vad: TenVadModelConfig, | |
| pub sample_rate: i32, | |
| pub num_threads: i32, | |
| pub provider: *const c_char, | |
| pub debug: i32, |
Summary by CodeRabbit
New Features
Bug Fixes
Chores