Repository navigation
Add JavaScript (node-addon) API for ten-vad - #2383
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant VadConfig
participant SileroVadConfig
participant TenVadConfig
User->>App: Start VAD process
App->>VadConfig: Initialize with SileroVadConfig and TenVadConfig
VadConfig->>SileroVadConfig: Construct silero config
VadConfig->>TenVadConfig: Construct tenvad config
App->>VadConfig: Use config for VAD
App->>App: On audio event, select window size based on active model
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
nodejs-addon-examples/test_vad_asr_non_streaming_sense_voice_microphone.js (1)
73-73: Update dynamic VAD config selection in ASR non-streaming testCurrently this file always uses
vad.config.sileroVad.windowSize, but since TenVad support was added, it needs the same conditional logic used in test_vad_microphone.js.• nodejs-addon-examples/test_vad_asr_non_streaming_sense_voice_microphone.js
– At line 75, replace:
js const windowSize = vad.config.sileroVad.windowSize;
with:
js const windowSize = vad.config.sileroVad.model !== '' ? vad.config.sileroVad.windowSize : vad.config.tenVad.windowSize;This ensures both SileroVad and TenVad configurations are handled dynamically.
🧹 Nitpick comments (3)
nodejs-addon-examples/test_vad_asr_non_streaming_zipformer_ctc_microphone.js (1)
73-73: Consider TenVad support for completeness.Like other test files, this only uses
sileroVad.windowSize. Consider adding dynamic VAD config selection if TenVad support is part of the broader PR scope.nodejs-addon-examples/test_vad_asr_non_streaming_whisper_microphone.js (1)
96-99: LGTM! Filename sanitization completed across all test files.The consistent application of colon-to-hyphen replacement ensures all test files generate valid filenames across platforms.
Consider simplifying the filename construction logic across all test files:
- const filename = `${index}-${text}-${ - new Date() - .toLocaleTimeString('en-US', {hour12: false}) - .split(' ')[0]}.wav` - .replace(/:/g, '-'); + const timestamp = new Date().toLocaleTimeString('en-US', {hour12: false}).replace(/:/g, '-'); + const filename = `${index}-${text}-${timestamp}.wav`;harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/ets/components/Vad.ets (1)
37-52: Consider refactoring to reduce code duplication.The
TenVadConfigclass is identical toSileroVadConfigin structure, properties, and behavior. This creates unnecessary code duplication that could lead to maintenance issues.Consider these refactoring options:
Option 1: Use a base class
+export abstract class BaseVadConfig { + public model: string; + public threshold: number; + public minSpeechDuration: number; + public minSilenceDuration: number; + public windowSize: number; + + public constructor(model: string, threshold: number, minSpeechDuration: number, minSilenceDuration: number, windowSize: number) { + this.model = model; + this.threshold = threshold; + this.minSpeechDuration = minSpeechDuration; + this.minSilenceDuration = minSilenceDuration; + this.windowSize = windowSize; + } +} + +export class SileroVadConfig extends BaseVadConfig {} +export class TenVadConfig extends BaseVadConfig {}Option 2: Use a generic approach with a type parameter
+export class VadModelConfig { + public model: string; + public threshold: number; + public minSpeechDuration: number; + public minSilenceDuration: number; + public windowSize: number; + public type: 'silero' | 'ten'; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
harmony-os/SherpaOnnxHar/sherpa_onnx/Index.ets(1 hunks)harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/vad.cc(3 hunks)harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/ets/components/Vad.ets(2 hunks)harmony-os/SherpaOnnxVadAsr/entry/src/main/ets/workers/NonStreamingAsrWithVadWorker.ets(4 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_moonshine_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_nemo_ctc_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_paraformer_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_sense_voice_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_transducer_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_whisper_microphone.js(1 hunks)nodejs-addon-examples/test_vad_asr_non_streaming_zipformer_ctc_microphone.js(1 hunks)nodejs-addon-examples/test_vad_microphone.js(3 hunks)nodejs-addon-examples/test_vad_spoken_language_identification_microphone.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/vad.cc (1)
nodejs-addon-examples/test_vad_microphone.js (1)
windowSize(64-66)
nodejs-addon-examples/test_vad_microphone.js (1)
nodejs-addon-examples/test_vad_spoken_language_identification_microphone.js (4)
config(11-22)config(32-40)windowSize(68-68)vad(45-45)
🔇 Additional comments (21)
nodejs-addon-examples/test_vad_asr_non_streaming_sense_voice_microphone.js (1)
98-101: LGTM! Cross-platform filename compatibility improved.The replacement of colons with hyphens ensures filenames work across all file systems, particularly Windows which prohibits colons in filenames.
nodejs-addon-examples/test_vad_asr_non_streaming_zipformer_ctc_microphone.js (1)
96-99: LGTM! Consistent filename sanitization applied.Good improvement for cross-platform compatibility. The pattern matches other test files in this PR.
nodejs-addon-examples/test_vad_asr_non_streaming_paraformer_microphone.js (1)
95-98: LGTM! Filename sanitization applied consistently.The colon-to-hyphen replacement ensures cross-platform filename validity. Good consistent implementation across test files.
nodejs-addon-examples/test_vad_asr_non_streaming_nemo_ctc_microphone.js (1)
97-100: LGTM! Proper filename sanitization implemented.Consistent with other test files in ensuring cross-platform filename compatibility.
harmony-os/SherpaOnnxHar/sherpa_onnx/Index.ets (1)
3-3: LGTM! Proper export of the new TenVadConfig class.The addition of
TenVadConfigto the export statement correctly makes the new VAD configuration class available for external use, maintaining consistency with the existingSileroVadConfigexport.nodejs-addon-examples/test_vad_spoken_language_identification_microphone.js (1)
94-97: LGTM! Improved filename compatibility across filesystems.The replacement of colons with hyphens in the timestamp ensures the generated filenames are valid across different operating systems, particularly Windows where colons are not allowed in filenames.
nodejs-addon-examples/test_vad_asr_non_streaming_moonshine_microphone.js (1)
100-103: LGTM! Consistent filename formatting improvement.The colon-to-hyphen replacement maintains consistency with other example files and ensures cross-platform filename compatibility.
nodejs-addon-examples/test_vad_asr_non_streaming_transducer_microphone.js (1)
100-103: LGTM! Consistent cross-platform filename handling.The filename formatting maintains consistency across all example files and ensures proper cross-platform compatibility.
harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/ets/components/Vad.ets (2)
61-63: LGTM! Proper constructor update for dual VAD support.The constructor correctly adds the
tenVadparameter and assigns it to the instance property, maintaining API consistency with the existing pattern.
56-56: LGTM! Consistent property addition.The
tenVadproperty addition follows the same pattern as the existingsileroVadproperty, maintaining consistency in the class structure.harmony-os/SherpaOnnxVadAsr/entry/src/main/ets/workers/NonStreamingAsrWithVadWorker.ets (4)
9-9: LGTM: Import addition is correct.The
TenVadConfigimport properly extends the existing VAD configuration support.
35-41: LGTM: TenVadConfig initialization is well-structured.The configuration follows the same pattern as
SileroVadConfigwith appropriate default values. The empty model string serves as a clear placeholder, and the window size of 256 is appropriate for the ten-vad model.
104-108: LGTM: Window size selection logic is correct.The conditional logic properly selects the window size based on which VAD model is active. Using
!= ''to check for non-empty model string is a reliable approach.
154-158: LGTM: Consistent implementation across functions.The same window size selection logic is correctly applied in the
decodeMicfunction, maintaining consistency with thedecodeFilefunction.nodejs-addon-examples/test_vad_microphone.js (4)
11-15: LGTM: Helpful documentation addition.The comments provide clear guidance on where to download the ten-vad.onnx model, maintaining consistency with the existing silero_vad.onnx documentation.
25-32: LGTM: TenVadConfig structure is consistent.The
tenVadconfiguration object follows the same structure assileroVadwith appropriate defaults. The empty model string clearly indicates this is a placeholder configuration.
64-66: LGTM: Window size selection logic is sound.The conditional logic correctly selects the appropriate window size based on which VAD model is configured. This matches the implementation in the HarmonyOS worker file.
86-89: LGTM: Filename sanitization improvement.Replacing colons with dashes in filenames is a good practice for cross-platform compatibility, as colons are invalid characters in Windows filenames.
harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/vad.cc (3)
297-314: LGTM: GetTenVadConfig implementation follows established patterns.The function correctly mirrors the
GetSileroVadConfigimplementation:
- Proper struct initialization with
memset- Appropriate null checks for the
tenVadobject- Consistent use of attribute assignment macros
- All expected VAD parameters are handled
The implementation maintains consistency with the existing codebase.
361-361: LGTM: Proper integration of TenVadConfig.The call to
GetTenVadConfigis correctly placed alongside the existingGetSileroVadConfigcall, ensuring both VAD configurations are available during detector creation.
392-392: LGTM: Memory cleanup is handled correctly.The addition of
SHERPA_ONNX_DELETE_C_STR(c.ten_vad.model)ensures proper memory management for the dynamically allocated model string, preventing memory leaks.
There was a problem hiding this comment.
Pull Request Overview
Adds support for a new Ten VAD model alongside the existing Silero VAD, updates configuration and binding code, and sanitizes output filenames by replacing colons.
- Introduce
TenVadConfigin both JavaScript examples and HarmonyOS components, and extendVadConfigto include it. - Update N-API bindings (
vad.cc) to parse Ten VAD parameters and free associated strings. - Sanitize generated filenames in all example scripts by replacing
:with-.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs-addon-examples/test_vad_spoken_language_identification_microphone.js | Added filename sanitization with replace(/:/g) |
| nodejs-addon-examples/test_vad_microphone.js | Added tenVad config, dynamic window size logic |
| nodejs-addon-examples/test_vad_asr_non_streaming__microphone.js (9 files) | Added filename sanitization in all ASR examples |
| harmony-os/.../NonStreamingAsrWithVadWorker.ets | Registered TenVadConfig, dynamic window size |
| harmony-os/.../Vad.ets | Added TenVadConfig class and maxSpeechDuration |
| harmony-os/.../vad.cc | Implemented GetTenVadConfig and integrated into wrapper |
| harmony-os/.../Index.ets | Exported TenVadConfig |
Comments suppressed due to low confidence (1)
nodejs-addon-examples/test_vad_microphone.js:25
- New
tenVadconfiguration paths are introduced but not covered by existing tests. Add or update tests to ensure behavior whentenVad.modelis set and the correct VAD model is used.
tenVad: {
| const windowSize = vad.config.sileroVad.model != '' ? | ||
| vad.config.sileroVad.windowSize : | ||
| vad.config.tenVad.windowSize; |
There was a problem hiding this comment.
The condition checks sileroVad.model to choose the window size but should instead check tenVad.model to properly select the Ten VAD configuration when provided.
| const windowSize = vad.config.sileroVad.model != '' ? | |
| vad.config.sileroVad.windowSize : | |
| vad.config.tenVad.windowSize; | |
| const windowSize = vad.config.tenVad.model != '' ? | |
| vad.config.tenVad.windowSize : | |
| vad.config.sileroVad.windowSize; |
| const filename = `${index}-${fullLang}-${ | ||
| new Date() | ||
| .toLocaleTimeString('en-US', {hour12: false}) | ||
| .split(' ')[0]}.wav`; | ||
| new Date() | ||
| .toLocaleTimeString('en-US', {hour12: false}) | ||
| .split(' ')[0]}.wav` | ||
| .replace(/:/g, '-'); |
There was a problem hiding this comment.
[nitpick] The filename sanitization logic is duplicated across multiple example scripts. Consider extracting a helper function (e.g., sanitizeFilename) to improve reuse and reduce duplication.
Summary by CodeRabbit
New Features
Bug Fixes