Repository navigation
Add Pascal API for ten-vad - #2388
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSupport for the ten-vad voice activity detection (VAD) model was added to the Pascal API and example suite. This includes new configuration records, a new Pascal example program for silence removal using ten-vad, an accompanying shell script for setup and execution, workflow adjustments to test the new example, and relevant updates to documentation and ignore files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShellScript as run-remove-silence-ten-vad.sh
participant PascalApp as remove_silence_ten_vad.pas
participant SherpaAPI as SherpaOnnx Pascal API
participant TenVadModel
User->>ShellScript: Execute script
ShellScript->>ShellScript: Build dependencies, download model/audio
ShellScript->>PascalApp: Compile and run remove_silence_ten_vad
PascalApp->>SherpaAPI: Configure VAD with ten-vad model
loop For each audio window
PascalApp->>SherpaAPI: Feed audio window
SherpaAPI->>TenVadModel: Perform VAD
TenVadModel-->>SherpaAPI: Speech segments
SherpaAPI-->>PascalApp: Speech segments
end
PascalApp->>SherpaAPI: Flush VAD
SherpaAPI-->>PascalApp: Remaining segments
PascalApp->>PascalApp: Concatenate speech, write output WAV
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the new Ten-VAD voice activity detection model in the Pascal API, updates example scripts, and extends CI to run the ten-vad example.
- Introduce
TSherpaOnnxTenVadModelConfigand integrate it into the existing VAD model records and C API. - Implement
ToStringandInitializeforTenVad, and update the parentToStringformatting to include the new field. - Add example scripts and Pascal sample for ten-vad, update
.gitignore, and extend the GitHub Actions workflow to run the new example.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sherpa-onnx/pascal-api/sherpa_onnx.pas | Add TenVad config record, update C binding, ToString, and constructor mapping. |
| pascal-api-examples/vad/run-remove-silence-ten-vad.sh | New shell script to download assets, build, and run the ten-vad Pascal example. |
| pascal-api-examples/vad/remove_silence_ten_vad.pas | New Pascal example showing how to remove silence with ten-vad. |
| pascal-api-examples/vad/remove_silence.pas | Clarify comment to specify silero-vad in the existing example. |
| pascal-api-examples/vad/.gitignore | Ignore generated artifacts for the ten-vad example. |
| .github/workflows/pascal.yaml | Add a CI step to run the Pascal ten-vad example alongside existing VAD tests. |
Comments suppressed due to low confidence (1)
pascal-api-examples/vad/remove_silence_ten_vad.pas:27
- [nitpick] The variable
AllSpeechSegmentholds multiple segments but uses a singular name. Consider renaming it toAllSpeechSegmentsfor clarity.
AllSpeechSegment: array of TSherpaOnnxSpeechSegment;
| 'Provider := %s, ' + | ||
| 'Debug := %s' + | ||
| 'Debug := %s, ' + | ||
| 'SileroVad := %s' + |
There was a problem hiding this comment.
The last field label in the TSherpaOnnxVadModelConfig.ToString format string is duplicated as 'SileroVad'. It should read 'TenVad := %s' to match the added field.
| 'SileroVad := %s' + | |
| 'TenVad := %s' + |
| end; | ||
|
|
||
| class operator TSherpaOnnxTenVadModelConfig.Initialize({$IFDEF FPC}var{$ELSE}out{$ENDIF} Dest: TSherpaOnnxTenVadModelConfig); | ||
| begin |
There was a problem hiding this comment.
The Initialize operator sets default numeric values but does not initialize the Model string. Consider adding Dest.Model := ''; for clarity.
| begin | |
| begin | |
| Dest.Model := ''; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pascal-api-examples/vad/remove_silence_ten_vad.pas (1)
61-78: Consider refactoring to reduce code duplication.The VAD processing logic is identical to the silero-vad example. Consider creating a shared utility function or a more generic example that accepts VAD type as a parameter to reduce code duplication.
For example, you could extract the processing logic into a shared procedure:
procedure ProcessVadSegments(var Vad: TSherpaOnnxVoiceActivityDetector; Wave: TSherpaOnnxWave; WindowSize: Integer; SampleRate: Integer; OutputFilename: AnsiString);Also applies to: 82-93, 95-109
pascal-api-examples/vad/run-remove-silence-ten-vad.sh (2)
3-4: Harden bash options
set -exprints commands and aborts on non-zero exit but misses two common foot-guns:set -euo pipefail•
-uaborts on unset variables (avoids typos)
•-o pipefailsurfaces failures hidden inside pipelines.Adding them costs nothing and makes the script more robust.
40-43: Prepend only if the dir isn’t already presentRepeated runs append duplicate entries to
LD_LIBRARY_PATH/DYLD_LIBRARY_PATH. Use a lightweight check to avoid path bloat:add_path() { case ":$1:" in *":$2:"*) ;; # already present *) export "$1"="$2:${!1}" ;; esac } add_path LD_LIBRARY_PATH "$SHERPA_ONNX_DIR/build/install/lib" add_path DYLD_LIBRARY_PATH "$SHERPA_ONNX_DIR/build/install/lib"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pascal.yaml(1 hunks)pascal-api-examples/vad/.gitignore(1 hunks)pascal-api-examples/vad/remove_silence.pas(1 hunks)pascal-api-examples/vad/remove_silence_ten_vad.pas(1 hunks)pascal-api-examples/vad/run-remove-silence-ten-vad.sh(1 hunks)sherpa-onnx/pascal-api/sherpa_onnx.pas(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: pascal (ubuntu-latest)
- GitHub Check: pascal (ubuntu-22.04-arm)
- GitHub Check: pascal (windows-latest)
- GitHub Check: windows-latest
- GitHub Check: ubuntu-22.04
🔇 Additional comments (14)
sherpa-onnx/pascal-api/sherpa_onnx.pas (6)
429-438: LGTM! Well-structured ten-vad configuration.The new
TSherpaOnnxTenVadModelConfigrecord follows the same pattern as the existingTSherpaOnnxSileroVadModelConfig, ensuring API consistency.
845-852: Consistent native C structure implementation.The native C-compatible
SherpaOnnxTenVadModelConfigrecord properly mirrors the Pascal record structure for interoperability.
1933-1946: Excellent ToString implementation.The ToString method for
TSherpaOnnxTenVadModelConfigprovides comprehensive string representation of all configuration parameters.
1957-1964: Good default configuration with appropriate window size.The Initialize operator sets sensible defaults, with a window size of 256 which appears to be the appropriate default for ten-vad models (different from silero-vad's 512).
2128-2133: Proper configuration copying for ten-vad.The constructor correctly copies all TenVad configuration parameters to the native C structure, ensuring proper initialization.
1973-1977: Fix duplicate field name in ToString method.The ToString method incorrectly shows 'SileroVad := %s' twice instead of including the TenVad field.
- 'Debug := %s, ' + - 'SileroVad := %s' + + 'Debug := %s, ' + + 'TenVad := %s' +Likely an incorrect or invalid review comment.
pascal-api-examples/vad/remove_silence.pas (1)
4-4: Good clarification for VAD model specificity.The comment update clearly identifies this example as using silero-vad, which helps distinguish it from the new ten-vad example.
pascal-api-examples/vad/.gitignore (1)
4-4: Appropriate gitignore entry for new example.The addition of
remove_silence_ten_vadfollows the existing pattern and ensures compiled artifacts are properly excluded from version control..github/workflows/pascal.yaml (1)
139-158: Excellent CI integration for ten-vad testing.The workflow properly integrates the new ten-vad example into the test suite, maintaining existing tests while adding the new functionality. The timing measurements and clear separation with echo statements improve test observability.
pascal-api-examples/vad/remove_silence_ten_vad.pas (4)
1-5: Well-documented example for ten-vad usage.The header comment clearly explains the purpose and distinguishes this example from the silero-vad version.
43-50: Appropriate ten-vad configuration parameters.The configuration uses appropriate defaults for ten-vad: window size of 256 and threshold of 0.25, which differ from silero-vad settings.
46-46: Correct usage of TenVad configuration.The example properly uses
Config.TenVad.Modelinstead ofConfig.SileroVad.Model, demonstrating the new API correctly.
111-111: Good output filename differentiation.Using a distinct output filename (
lei-jun-test-no-silence-ten-vad.wav) prevents conflicts with the silero-vad example output.pascal-api-examples/vad/run-remove-silence-ten-vad.sh (1)
34-38: Quote FPC flags and pass absolute pathsUnquoted
-Fu/-Flvalues break when the project lives under a directory containing spaces. Also, relying on./remove_silence_ten_vad.pashas the same pwd issue mentioned earlier.-fpc \ - -dSHERPA_ONNX_USE_SHARED_LIBS \ - -Fu$SHERPA_ONNX_DIR/sherpa-onnx/pascal-api \ - -Fl$SHERPA_ONNX_DIR/build/install/lib \ - ./remove_silence_ten_vad.pas +fpc \ + -dSHERPA_ONNX_USE_SHARED_LIBS \ + -Fu"$SHERPA_ONNX_DIR/sherpa-onnx/pascal-api" \ + -Fl"$SHERPA_ONNX_DIR/build/install/lib" \ + "$SCRIPT_DIR/remove_silence_ten_vad.pas"Ensure CI invokes the script after these path changes; the compile step should still succeed.
| SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd ) | ||
| SHERPA_ONNX_DIR=$(cd $SCRIPT_DIR/../.. && pwd) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use $SHERPA_ONNX_DIR instead of hard-coded relative paths & always quote variables
Several commands assume the caller’s pwd matches the script location (../../build/..., ./ten-vad.onnx, etc.).
If the script is invoked from any other directory (e.g. by CI or via an absolute path), those relatives resolve incorrectly and the build/downloads will silently fail.
- if [[ ! -f ../../build/install/lib/libsherpa-onnx-c-api.dylib ...
+if [[ ! -f "$SHERPA_ONNX_DIR/build/install/lib/libsherpa-onnx-c-api.dylib" \
+ && ! -f "$SHERPA_ONNX_DIR/build/install/lib/libsherpa-onnx-c-api.so" \
+ && ! -f "$SHERPA_ONNX_DIR/build/install/lib/sherpa-onnx-c-api.dll" ]]; then
- mkdir -p ../../build
- pushd ../../build
+ mkdir -p "$SHERPA_ONNX_DIR/build"
+ pushd "$SHERPA_ONNX_DIR/build"
...
-if [[ ! -f ./ten-vad.onnx ]]; then
- curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/ten-vad.onnx
+MODEL_PATH="$SCRIPT_DIR/ten-vad.onnx"
+if [[ ! -f "$MODEL_PATH" ]]; then
+ curl -fLso "$MODEL_PATH" \
+ https://github.com/k2-fsa/sherpa-onnx/releases/download/asr-models/ten-vad.onnxQuoting also guards against spaces in $SCRIPT_DIR and $SHERPA_ONNX_DIR.
Fail-fast curl -f prevents silent 404/5xx downloads.
Also applies to: 10-12, 26-33
🤖 Prompt for AI Agents
In pascal-api-examples/vad/run-remove-silence-ten-vad.sh around lines 5 to 7,
replace all hard-coded relative paths with the variable $SHERPA_ONNX_DIR to
ensure paths resolve correctly regardless of the current working directory.
Additionally, quote all variable expansions like "$SCRIPT_DIR" and
"$SHERPA_ONNX_DIR" to handle spaces safely. Also, update curl commands to use
the -f flag to fail fast on HTTP errors. Apply these changes similarly to lines
10-12 and 26-33.
Summary by CodeRabbit