Repository navigation
Fix various language bindings - #3166
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThis PR extends offline ASR model configuration across multiple language bindings and C++ core. It adds timestamp flags (enable_token_timestamps, enable_segment_timestamps) to Whisper model config and language customization fields (language, itn, hotwords) to FunASR Nano model config. Changes include field declarations, constructor updates, C struct mappings, and memory cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 enhances the configuration capabilities for both FunASR Nano and Whisper models by introducing new parameters related to language, text normalization, hotwords, and timestamp generation. These changes are propagated across multiple language bindings, including Go, Java, Kotlin, and Pascal, to ensure that developers using these languages can leverage the expanded functionality. The update aims to provide greater flexibility and control over the automatic speech recognition process. Highlights
Changelog
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
This pull request correctly adds new configuration options for the FunASR Nano and Whisper models. The changes are consistently propagated across various language bindings, including C++ for HarmonyOS, Go, Java, Kotlin, and Pascal. The new fields are properly handled in data structures, configuration parsing, and memory management. The code is clean and I found no issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/pascal-api/sherpa_onnx.pas (1)
2238-2245:⚠️ Potential issue | 🟡 MinorChange Pascal default for
UseItntoTrueto align with other language bindings.All other bindings (C++, Java, Kotlin, Python) default
itntotrue. The Pascal implementation at line 2244 currently setsUseItn := False, creating an inconsistency across the API surface. Update it to match the standard default.Fix
- Dest.UseItn := False; + Dest.UseItn := True;
There was a problem hiding this comment.
Pull request overview
This PR updates multiple language bindings to expose newly added offline model configuration options (Whisper timestamp toggles; FunASR Nano language/ITN/hotwords) so they can be set consistently across supported SDKs.
Changes:
- Pascal API: add FunASR Nano
Language/UseItn/Hotwordsand WhisperEnableTokenTimestamps/EnableSegmentTimestamps, includingToString, defaults, and C-struct conversion. - JVM/JNI APIs: add Whisper timestamp flags in Kotlin + Java, and plumb them through JNI to the native config.
- Go + HarmonyOS bindings: add the same fields and wire them into C config conversion/freeing.
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 | Adds new config fields for FunASR Nano + Whisper and converts them into the C config struct. |
| sherpa-onnx/kotlin-api/OfflineRecognizer.kt | Exposes Whisper timestamp toggles in Kotlin config. |
| sherpa-onnx/jni/offline-recognizer.cc | Reads Whisper timestamp toggles from Kotlin config into native config. |
| sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineWhisperModelConfig.java | Adds Whisper timestamp toggles to the Java builder/config object. |
| scripts/go/sherpa_onnx.go | Adds new config fields to Go binding and wires them into C config creation/freeing; minor formatting tweak in ToBuffer(). |
| harmony-os/SherpaOnnxHar/sherpa_onnx/src/main/cpp/non-streaming-asr.cc | Adds FunASR Nano + Whisper fields for HarmonyOS config parsing and frees newly allocated strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fs := C.SherpaOnnxWaveFileSize(C.int(n)) // SHERPA_ONNX_API int64_t SherpaOnnxWaveFileSize(int32_t n_samples); | ||
| buf := make([]byte, fs) |
There was a problem hiding this comment.
fs is a C.int64_t (from SherpaOnnxWaveFileSize) but it is passed directly to make([]byte, fs), which requires an int length. This will not compile with Go; convert fs to int (with a range check to avoid overflow on 32-bit platforms) before allocating the buffer.
| fs := C.SherpaOnnxWaveFileSize(C.int(n)) // SHERPA_ONNX_API int64_t SherpaOnnxWaveFileSize(int32_t n_samples); | |
| buf := make([]byte, fs) | |
| fsC := C.SherpaOnnxWaveFileSize(C.int(n)) // SHERPA_ONNX_API int64_t SherpaOnnxWaveFileSize(int32_t n_samples); | |
| if fsC <= 0 { | |
| return nil | |
| } | |
| fs := int64(fsC) | |
| maxInt := int64(^uint(0) >> 1) | |
| if fs > maxInt { | |
| // Avoid integer overflow when converting to int (especially on 32-bit platforms). | |
| return nil | |
| } | |
| bufLen := int(fs) | |
| buf := make([]byte, bufLen) |
| @@ -1472,10 +1484,12 @@ func (audio *GeneratedAudio) Save(filename string) bool { | |||
| } | |||
There was a problem hiding this comment.
Save() can panic when audio.Samples is empty because it takes &audio.Samples[0] earlier in the function. Consider adding a len(audio.Samples)==0 guard (returning false) to match the defensive check used in ToBuffer().
Summary by CodeRabbit