Support ten vad - #2377
Conversation
WalkthroughThe changes introduce support for the TEN-VAD voice activity detection model across the C++ core, Python bindings, and example scripts. This includes new configuration and model classes for TEN-VAD, integration into model selection logic, updates to command-line arguments, and Python API exposure. Minor typo corrections and build script updates are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonScript
participant VadModelConfig
participant VadModelFactory
participant SileroVadModel
participant TenVadModel
User->>PythonScript: Run with --silero-vad-model or --ten-vad-model
PythonScript->>VadModelConfig: Parse and validate config
PythonScript->>VadModelFactory: Create(config)
alt silero_vad.model is set
VadModelFactory->>SileroVadModel: Instantiate
else ten_vad.model is set
VadModelFactory->>TenVadModel: Instantiate
else
VadModelFactory->>PythonScript: Error (no model)
end
PythonScript->>User: Process audio with selected VAD model
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🪧 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 TEN VAD model alongside the existing Silero VAD in both C++ and Python interfaces.
- Introduces
TenVadModelConfigwith Python bindings and integrates it intoVadModelConfig. - Implements
TenVadModelin C++ and extends the factory (VadModel::Create) and detector logic to choose between Silero and TEN VAD. - Updates build files (CMake), fixes a spelling typo, and extends the Python example script to demonstrate
--ten-vad-model.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/sherpa_onnx/init.py | Import TenVadModelConfig |
| python/csrc/vad-model-config.cc | Extend Python binding for VadModelConfig with ten_vad |
| python/csrc/ten-vad-model-config.h/cc | Add Python binding for TenVadModelConfig |
| python/csrc/CMakeLists.txt | Include ten-vad-model-config.cc in build |
| csrc/voice-activity-detector.cc | Handle ten_vad in detector implementation |
| csrc/vad-model.cc | Extend factory to create TenVadModel |
| csrc/vad-model-config.h/cc | Integrate TenVadModelConfig into config struct and parsing |
| csrc/ten-vad-model.h/cc | Implement TenVadModel class |
| csrc/silero-vad-model-config.cc | Fix spelling from “perfomance” to “performance” |
| csrc/CMakeLists.txt | Add TEN VAD model files to build |
| python-api-examples/generate-subtitles.py | Update example to support --ten-vad-model |
| cxx-api-examples/zipformer-...-microphone.cc | Minor comment formatting fix |
| cmake/kaldi-native-fbank.cmake | Bump kaldi-native-fbank dependency to v1.21.3 |
Comments suppressed due to low confidence (3)
sherpa-onnx/python/csrc/vad-model-config.cc:25
- The constructor argument name 'ten' mismatches the property 'ten_vad'. Rename the argument to 'ten_vad' for consistency with the
.def_readwrite("ten_vad", ...)binding.
py::arg("ten") = TenVadModelConfig{}, py::arg("sample_rate") = 16000,
sherpa-onnx/csrc/ten-vad-model-config.cc:42
- Consider adding unit tests for
TenVadModelConfig::Validateto ensure both valid and invalid configurations are correctly detected.
bool TenVadModelConfig::Validate() const {
sherpa-onnx/csrc/silero-vad-model-config.h:27
- [nitpick] Consider restoring the removed comment about valid
window_sizevalues for 8000 Hz (e.g., 256, 512, 768 samples) to help users configure the model correctly.
int32_t window_size = 512; // in samples
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
sherpa-onnx/csrc/transpose.h (1)
23-24: Spelling now inconsistent between the two doc-blocksGreat catch fixing “data type” here, but the earlier comment for
Transpose01(line 13) still says “dataype”, and line 15 uses “datatype” (no space). For consistency and clarity, fix both instances.- * @param v A 3-D tensor of shape (B, T, C). Its dataype is type. - * - * @return Return a 3-D tensor of shape (T, B, C). Its datatype is type. + * @param v A 3-D tensor of shape (B, T, C). Its data type is type. + * + * @return Return a 3-D tensor of shape (T, B, C). Its data type is type.sherpa-onnx/python/csrc/CMakeLists.txt (1)
54-55: Source list stays consistent – optional alphabetical placement
ten-vad-model-config.ccis correctly appended and will be built.
For long lists it’s easier to scan when kept alphabetical (like the surrounding block), but this is purely cosmetic.sherpa-onnx/csrc/CMakeLists.txt (1)
126-127: Remember to add headers for IDE code-navigationThe new
.ccfiles are registered, but the corresponding headers (ten-vad-model.h,ten-vad-model-config.h) are not listed anywhere.
Some IDEs/clang-tools rely on header entries in the build graph for proper include-path deduction. Consider adding them to aninstall(FILES …)clause or an IDE helper target.sherpa-onnx/python/sherpa_onnx/__init__.py (1)
67-69: Silence Ruff F401 for re-exported symbol
TenVadModelConfigis intentionally imported for re-export, but Ruff flags it as unused.
Add a file-level or per-line# noqa: F401to keep the lint pipeline green:- TenVadModelConfig, + TenVadModelConfig, # noqa: F401 (re-export)sherpa-onnx/python/csrc/vad-model-config.cc (1)
22-25: Consider consistent parameter naming.The constructor parameter is named
tenbut the class property isten_vad. This naming inconsistency could cause confusion for users.Consider renaming the parameter for consistency:
- .def(py::init<const SileroVadModelConfig &, const TenVadModelConfig &, - int32_t, int32_t, const std::string &, bool>(), - py::arg("silero_vad") = SileroVadModelConfig{}, - py::arg("ten") = TenVadModelConfig{}, py::arg("sample_rate") = 16000, + .def(py::init<const SileroVadModelConfig &, const TenVadModelConfig &, + int32_t, int32_t, const std::string &, bool>(), + py::arg("silero_vad") = SileroVadModelConfig{}, + py::arg("ten_vad") = TenVadModelConfig{}, py::arg("sample_rate") = 16000,sherpa-onnx/csrc/ten-vad-model-config.cc (1)
36-40: Remove trailing space and consider enforcing the recommended window sizes.The warning message has a trailing space after "256". Additionally, since the warning strongly recommends using 160 or 256, consider validating these specific values in the
Validate()method.po->Register( "ten-vad-window-size", &window_size, "In samples. Audio chunks of --ten-vad-window-size samples are fed " - "to the ten VAD model. WARNING! Please use 160 or 256 "); + "to the ten VAD model. WARNING! Please use 160 or 256");sherpa-onnx/csrc/ten-vad-model.cc (3)
168-172: Align window size validation with configuration recommendations.The validation allows window sizes up to 768, but the configuration file strongly recommends using 160 or 256. Consider tightening this validation or at least warning about non-recommended values.
if (config_.ten_vad.window_size > 768) { SHERPA_ONNX_LOGE("Windows size %d for ten-vad is too large", config_.ten_vad.window_size); SHERPA_ONNX_EXIT(-1); } + + if (config_.ten_vad.window_size != 160 && config_.ten_vad.window_size != 256) { + SHERPA_ONNX_LOGW("Recommended window sizes are 160 or 256. Given: %d", + config_.ten_vad.window_size); + }
352-354: Add more context to the pitch feature comment.While setting pitch to 0 is mentioned in the PR objectives, the inline comment could be more descriptive for future maintainers.
// we use 0 for the pitch + // Note: The ten-vad model expects a pitch feature, but we set it to 0 + // as a simplification. This may reduce performance as noted in the PR. features_.back() = 0;
284-288: Add documentation for the Scale method.The method scales normalized float samples to int16 range, but this purpose isn't documented.
+ // Scales normalized float samples [-1, 1] to int16 range [-32768, 32767] static void Scale(const float *samples, int32_t n, float *out) { for (int32_t i = 0; i != n; ++i) { out[i] = samples[i] * 32768; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmake/kaldi-native-fbank.cmake(2 hunks)cxx-api-examples/zipformer-transducer-simulate-streaming-microphone-cxx-api.cc(1 hunks)python-api-examples/generate-subtitles.py(4 hunks)sherpa-onnx/csrc/CMakeLists.txt(1 hunks)sherpa-onnx/csrc/silero-vad-model-config.cc(1 hunks)sherpa-onnx/csrc/silero-vad-model-config.h(0 hunks)sherpa-onnx/csrc/ten-vad-model-config.cc(1 hunks)sherpa-onnx/csrc/ten-vad-model-config.h(1 hunks)sherpa-onnx/csrc/ten-vad-model.cc(1 hunks)sherpa-onnx/csrc/ten-vad-model.h(1 hunks)sherpa-onnx/csrc/transpose.h(1 hunks)sherpa-onnx/csrc/vad-model-config.cc(2 hunks)sherpa-onnx/csrc/vad-model-config.h(2 hunks)sherpa-onnx/csrc/vad-model.cc(3 hunks)sherpa-onnx/csrc/voice-activity-detector.cc(3 hunks)sherpa-onnx/python/csrc/CMakeLists.txt(1 hunks)sherpa-onnx/python/csrc/ten-vad-model-config.cc(1 hunks)sherpa-onnx/python/csrc/ten-vad-model-config.h(1 hunks)sherpa-onnx/python/csrc/vad-model-config.cc(1 hunks)sherpa-onnx/python/sherpa_onnx/__init__.py(1 hunks)
💤 Files with no reviewable changes (1)
- sherpa-onnx/csrc/silero-vad-model-config.h
🧰 Additional context used
🧬 Code Graph Analysis (4)
sherpa-onnx/csrc/vad-model-config.cc (5)
sherpa-onnx/csrc/silero-vad-model-config.h (1)
po(36-36)sherpa-onnx/csrc/ten-vad-model-config.h (1)
po(36-36)sherpa-onnx/csrc/vad-model-config.h (1)
po(38-38)sherpa-onnx/csrc/silero-vad-model-config.cc (2)
ToString(102-114)ToString(102-102)sherpa-onnx/csrc/ten-vad-model-config.cc (2)
ToString(97-109)ToString(97-97)
sherpa-onnx/csrc/voice-activity-detector.cc (2)
sherpa-onnx/csrc/ten-vad-model.cc (2)
config_(146-146)config_(148-148)sherpa-onnx/csrc/silero-vad-model.cc (2)
config_(157-157)config_(159-161)
sherpa-onnx/python/csrc/vad-model-config.cc (4)
sherpa-onnx/python/csrc/silero-vad-model-config.cc (2)
PybindSileroVadModelConfig(14-45)PybindSileroVadModelConfig(14-14)sherpa-onnx/python/csrc/silero-vad-model-config.h (1)
PybindSileroVadModelConfig(12-12)sherpa-onnx/python/csrc/ten-vad-model-config.h (1)
PybindTenVadModelConfig(12-12)sherpa-onnx/python/csrc/ten-vad-model-config.cc (2)
PybindTenVadModelConfig(14-45)PybindTenVadModelConfig(14-14)
sherpa-onnx/csrc/ten-vad-model.h (2)
sherpa-onnx/csrc/ten-vad-model.cc (21)
TenVadModel(435-436)TenVadModel(439-440)TenVadModel(442-442)TenVadModel(471-472)TenVadModel(476-477)samples(74-144)samples(74-74)samples(284-288)samples(284-284)samples(290-300)samples(290-290)samples(302-307)samples(302-303)samples(334-362)samples(334-334)samples(364-395)samples(364-364)s(154-156)s(154-154)threshold(158-158)threshold(158-158)sherpa-onnx/csrc/voice-activity-detector.cc (2)
samples(44-123)samples(44-44)
🪛 Ruff (0.11.9)
sherpa-onnx/python/sherpa_onnx/__init__.py
67-67: _sherpa_onnx.TenVadModelConfig imported but unused
(F401)
🔇 Additional comments (31)
cxx-api-examples/zipformer-transducer-simulate-streaming-microphone-cxx-api.cc (1)
4-5: Comment wording tweak is fine – no action neededOnly a comment re-flow; no functional impact.
sherpa-onnx/csrc/silero-vad-model-config.cc (1)
40-44: Typo fix looks good“performance” spelling corrected – no further issues.
sherpa-onnx/python/csrc/ten-vad-model-config.h (1)
1-17: LGTM! Clean header file structure.The header file follows proper conventions with include guards, namespace usage, and function declaration for Python bindings.
cmake/kaldi-native-fbank.cmake (2)
15-19: File paths updated consistently.All local file path references have been updated to reflect the new version number.
4-6: Dependency update verified: kaldi-native-fbank v1.21.3 archive is accessible and its SHA256 (d409eddae5a46dc796f0841880f489ff0728b96ae26218702cd438c28667c70e) matches the CMake setting. No further action required.sherpa-onnx/csrc/vad-model-config.cc (3)
16-17: Good integration of TEN-VAD registration.The registration of both VAD models is implemented correctly, maintaining consistency with the existing pattern.
52-63: Excellent validation logic for dual VAD model support.The validation approach correctly handles either VAD model configuration:
- Validates silero_vad if provided
- Falls back to ten_vad if silero_vad is not provided
- Provides clear error message if neither is configured
This maintains backward compatibility while enabling the new TEN-VAD support.
69-70: ToString method properly updated.The string representation now includes both VAD model configurations, maintaining consistency with the existing format.
sherpa-onnx/csrc/vad-model-config.h (3)
11-11: Proper header inclusion.The inclusion of the TEN-VAD model configuration header is correctly placed.
16-17: Member addition follows existing pattern.The
ten_vadmember is added consistently with the existingsilero_vadmember.
28-36: No action needed: constructor parameters are in correct orderI’ve verified that the constructor’s parameter list (
silero_vad,ten_vad,sample_rate,num_threads,provider,debug) matches the declaration order of the struct members. No changes required.sherpa-onnx/csrc/voice-activity-detector.cc (3)
21-21: Appropriate header inclusion for error handling.The inclusion of
macros.hprovides the necessary error logging and exit macros used in the conditional logic.
49-58: Excellent conditional logic for dual VAD model support.The implementation correctly handles both VAD models:
- Checks silero_vad first for backward compatibility
- Falls back to ten_vad if silero_vad is not configured
- Provides clear error handling for unknown models
The error handling using
SHERPA_ONNX_LOGEandSHERPA_ONNX_EXITis appropriate for this critical configuration error.
172-181: Consistent initialization logic.The
Initmethod uses the same conditional pattern asAcceptWaveform, ensuring consistent behavior across the class. The error handling for unsupported VAD models is appropriate.sherpa-onnx/python/csrc/ten-vad-model-config.cc (1)
1-48: Well-structured pybind11 bindings implementation.The implementation follows established patterns from the existing SileroVadModelConfig bindings and provides appropriate default values for the TEN-VAD model configuration parameters.
sherpa-onnx/python/csrc/vad-model-config.cc (3)
11-11: LGTM: Correct include addition.The include statement properly adds the TEN-VAD model configuration header.
17-17: LGTM: Proper registration of TEN-VAD bindings.The call to
PybindTenVadModelConfig(m)correctly registers the new TEN-VAD model configuration class.
29-29: LGTM: Proper property exposure.The
ten_vadproperty is correctly exposed as a read-write attribute.sherpa-onnx/csrc/vad-model.cc (6)
22-22: LGTM: Proper include addition.The include statement correctly adds the TEN-VAD model header.
29-34: LGTM: Proper RKNN provider validation.The implementation correctly validates that only Silero VAD is supported for RKNN provider and provides appropriate error handling.
44-50: LGTM: Well-structured fallback logic.The factory method properly implements fallback logic by checking for Silero VAD first, then TEN-VAD, ensuring at least one model is provided.
52-53: LGTM: Appropriate error handling.The error logging and null return provide proper handling when no VAD model is specified.
61-66: LGTM: Consistent RKNN validation in template method.The template method correctly mirrors the same RKNN provider validation logic as the main factory method.
75-84: LGTM: Consistent fallback logic in template method.The template method properly implements the same fallback logic as the main factory method.
sherpa-onnx/csrc/ten-vad-model-config.h (1)
1-45: Well-structured configuration header.The
TenVadModelConfigstruct follows the established pattern fromSileroVadModelConfigwith appropriate default values and standard methods. The default window size of 256 is consistent with the comment indicating valid values of 160 or 256.python-api-examples/generate-subtitles.py (3)
22-26: LGTM: Good documentation update.The documentation properly explains the new TEN-VAD model option and usage.
131-140: LGTM: Proper argument handling for dual VAD support.The command-line argument changes correctly make both VAD models optional while requiring at least one to be specified.
513-518: LGTM: Proper validation logic.The validation ensures at least one VAD model is provided with appropriate error handling.
sherpa-onnx/csrc/ten-vad-model-config.cc (2)
60-65: Clarify if threshold value 1.0 should be allowed.The current validation rejects threshold values >= 1. If 1.0 represents 100% probability and is a valid threshold, consider using
> 1instead.
97-109: LGTM!The
ToString()method is well-implemented with proper formatting.sherpa-onnx/csrc/ten-vad-model.cc (1)
243-250: Great error messaging for model validation!The error message clearly explains the issue and provides actionable guidance for users to download the correct model version with metadata.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes #2329
Note that the pitch feature is set to 0, which may degrade the performance but can greatly simplify the implementation.
We use TEN-framework/ten-vad#36 as a reference.
CC @shenjinti
Please see
https://github.com/k2-fsa/sherpa-onnx/blob/master/python-api-examples/generate-subtitles.py
for how to use ten-vad.onnx with ASR for generating subtitles.
File size comparison between silero-vad (version 4) and ten-vad
ten-vad.onnxis from https://github.com/TEN-framework/ten-vad/blob/main/src/onnx_model/ten-vad.onnxBut we have added some meta data to it, e.g.,
meanandinv_stddevfor feature normalization. So you have to use the model with metadata from us.Summary by CodeRabbit
New Features
TenVadModelConfigin the Python API for advanced VAD configuration.Bug Fixes
Documentation
Chores