Use a shorter name for Zipvoice models. - #2894
Conversation
dosubot
Bot
added
the
size:L
WalkthroughThis PR systematically renames ZipVoice offline TTS model configuration fields across the codebase: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
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 focuses on refining the naming conventions for Zipvoice Text-to-Speech (TTS) model components within the Sherpa-Onnx library. The primary goal is to enhance code clarity and maintainability by introducing more descriptive and concise names for model parts. Specifically, the Highlights
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 introduces a consistent renaming of Zipvoice model parameters across the entire codebase, changing textModel to encoder and flowMatchingModel to decoder. It also standardizes PinyinDict to Lexicon in various API bindings. The changes are applied consistently across multiple languages including Dart, Go, C#, Python, C++, and Pascal. The related example scripts and documentation have also been updated to reflect these changes. The code formatting has been improved in some files as well. Overall, this is a solid refactoring that improves code clarity and consistency. I have not found any issues of medium or higher severity.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/c-api/cxx-api.cc (1)
149-160: FixDecodeloops incrementingninstead ofiBoth:
OnlineRecognizer::Decode(const OnlineStream *ss, int32_t n)andKeywordSpotter::Decode(const OnlineStream *ss, int32_t n)use
for (int32_t i = 0; i != n; ++n), which incrementsninstead ofi, causing a non-terminating loop and repeatedly writingstreams[0].This is a correctness bug and can also lead to UB if
noverflows.Suggested fix:
@@ - std::vector<const SherpaOnnxOnlineStream *> streams(n); - for (int32_t i = 0; i != n; ++n) { - streams[i] = ss[i].Get(); - } + std::vector<const SherpaOnnxOnlineStream *> streams(n); + for (int32_t i = 0; i != n; ++i) { + streams[i] = ss[i].Get(); + } @@ - std::vector<const SherpaOnnxOnlineStream *> streams(n); - for (int32_t i = 0; i != n; ++n) { - streams[i] = ss[i].Get(); - } + std::vector<const SherpaOnnxOnlineStream *> streams(n); + for (int32_t i = 0; i != n; ++i) { + streams[i] = ss[i].Get(); + }Also applies to: 561-567
🧹 Nitpick comments (5)
scripts/dotnet/OfflineTtsZipVoiceModelConfig.cs (1)
7-46: .NET ZipVoice config fields correctly mirror C struct after rename
Encoder/Decoder/Lexiconreplace the old names while preserving field order and types, so the marshaled layout stays compatible withSherpaOnnxOfflineTtsZipvoiceModelConfig. Just be aware this is a source-breaking rename for existing .NET callers and may warrant a brief note in release docs.sherpa-onnx/csrc/offline-tts-model-config.cc (1)
40-48: ZipVoice validation gate correctly follows decoder renameSwitching the guard to
!zipvoice.decoder.empty()maintains the prior behavior that “having the main ZipVoice model path set” triggerszipvoice.Validate(). If in the future both encoder and decoder become strictly required, you might want to gate on both here for symmetry with the creation logic.sherpa-onnx/c-api/cxx-api.h (1)
429-441: C++ ZipVoice wrapper fields renamed consistently to encoder/decoder
OfflineTtsZipvoiceModelConfignow exposesencoder/decoder, matching the underlying C struct and other model configs. This is a source-level rename for C++ callers but doesn’t affect layout or interop.sherpa-onnx/csrc/offline-tts-zipvoice-model-config.cc (1)
15-36: ZipVoice config validation and diagnostics now align with encoder/decoder
- CLI options
--zipvoice-encoder/--zipvoice-decoderare correctly registered and validated, including file-existence checks.ToString()and error messages now reportencoder/decoder, matching the renamed fields.You might optionally tweak the help strings from “text model / flow-matching decoder model” to “encoder model / decoder model” for consistency, but behavior is already correct.
Also applies to: 38-67, 123-139
sherpa-onnx/csrc/offline-tts-zipvoice-model.cc (1)
35-54: ZipVoice encoder/decoder sessions are wired correctlyThe ZipVoice implementation now:
- Loads
config.zipvoice.encoderandconfig.zipvoice.decoder,- Instantiates the first
Ort::Sessionfrom the encoder buffer and the second from the decoder buffer, and- Adjusts debug output headings to
---encoder---/---decoder---.Behavior matches the previous text/flow-matching setup; only naming changed. If desired, you could rename
text_buftoencoder_buffor readability.Also applies to: 193-273
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
flutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dart(1 hunks)flutter/sherpa_onnx/lib/src/tts.dart(17 hunks)go-api-examples/non-streaming-tts/main.go(1 hunks)go-api-examples/non-streaming-tts/run-zipvoice.sh(1 hunks)python-api-examples/offline-zeroshot-tts.py(3 hunks)scripts/dotnet/OfflineTtsZipVoiceModelConfig.cs(3 hunks)scripts/go/sherpa_onnx.go(2 hunks)sherpa-onnx/c-api/c-api.cc(1 hunks)sherpa-onnx/c-api/c-api.h(1 hunks)sherpa-onnx/c-api/cxx-api.cc(1 hunks)sherpa-onnx/c-api/cxx-api.h(1 hunks)sherpa-onnx/csrc/offline-tts-impl.cc(2 hunks)sherpa-onnx/csrc/offline-tts-model-config.cc(1 hunks)sherpa-onnx/csrc/offline-tts-zipvoice-model-config.cc(3 hunks)sherpa-onnx/csrc/offline-tts-zipvoice-model-config.h(2 hunks)sherpa-onnx/csrc/offline-tts-zipvoice-model.cc(6 hunks)sherpa-onnx/csrc/sherpa-onnx-offline-zeroshot-tts.cc(1 hunks)sherpa-onnx/pascal-api/sherpa_onnx.pas(4 hunks)sherpa-onnx/python/csrc/offline-tts-zipvoice-model-config.cc(1 hunks)swift-api-examples/SherpaOnnx.swift(2 hunks)wasm/tts/sherpa-onnx-tts.js(4 hunks)wasm/tts/sherpa-onnx-wasm-main-tts.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
wasm/tts/sherpa-onnx-tts.js (1)
wasm/asr/sherpa-onnx-asr.js (45)
encoderLen(95-95)encoderLen(130-130)encoderLen(643-643)encoderLen(780-780)encoderLen(824-824)encoderLen(870-870)encoderLen(918-918)decoderLen(96-96)decoderLen(131-131)decoderLen(644-644)decoderLen(781-781)decoderLen(825-825)decoderLen(919-919)n(99-99)n(133-133)n(157-157)n(173-173)n(189-189)n(647-647)n(678-678)n(695-695)n(712-712)n(729-729)n(746-746)n(763-763)n(785-785)n(829-829)n(876-877)n(921-921)buffer(101-101)buffer(134-134)buffer(158-158)buffer(174-174)buffer(190-190)buffer(285-285)buffer(379-379)buffer(402-402)buffer(464-464)buffer(649-649)buffer(680-680)buffer(697-697)buffer(714-714)buffer(731-731)buffer(748-748)buffer(765-765)
scripts/dotnet/OfflineTtsZipVoiceModelConfig.cs (1)
sherpa-onnx/csrc/vocoder.h (1)
Vocoder(17-31)
sherpa-onnx/csrc/offline-tts-zipvoice-model-config.cc (1)
sherpa-onnx/c-api/cxx-api.cc (2)
FileExists(808-810)FileExists(808-808)
swift-api-examples/SherpaOnnx.swift (2)
sherpa-onnx/kotlin-api/OfflineRecognizer.kt (4)
encoder(17-21)encoder(48-54)encoder(56-62)encoder(64-67)sherpa-onnx/kotlin-api/OnlineRecognizer.kt (2)
encoder(17-21)encoder(23-26)
go-api-examples/non-streaming-tts/main.go (2)
sherpa-onnx/csrc/lexicon.cc (4)
Lexicon(105-122)Lexicon(125-145)Lexicon(402-405)Lexicon(409-412)sherpa-onnx/csrc/lexicon.h (1)
Lexicon(20-62)
🔇 Additional comments (21)
go-api-examples/non-streaming-tts/run-zipvoice.sh (1)
20-25: Command invocation correctly applies the model field renames.The updates to flag names (
--zipvoice-encoder,--zipvoice-decoder,--zipvoice-lexicon) and file paths (encoder.int8.onnx,decoder.int8.onnx,lexicon.txt) are consistent with the renaming scheme outlined in the PR objectives.sherpa-onnx/c-api/c-api.h (1)
1069-1080: Zipvoice C struct rename keeps ABI while aligning with encoder/decoder namingRenaming
text_model/flow_matching_modeltoencoder/decoderhere preserves field order and types, so the struct layout and existing sizeof/asserts remain valid while matching the rest of the API’s encoder/decoder terminology.flutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dart (1)
217-235: Dart FFI ZipVoice struct stays layout-compatible with C APISwitching to
encoder/decoderkeeps the Pointer/Utf8 field order identical to the C struct (tokens, encoder, decoder, vocoder, data_dir, lexicon, …), so FFI layout remains correct while matching the new naming.wasm/tts/sherpa-onnx-wasm-main-tts.cc (1)
76-86: WASM ZipVoice debug output now matches encoder/decoder namingPrinting
zipvoice->encoderandzipvoice->decoderaligns the debug output with the renamed struct fields, with no impact on the surrounding size checks or logic.sherpa-onnx/csrc/offline-tts-impl.cc (1)
39-52: ZipVoice selection now correctly keyed on encoder+decoder in both factory overloadsBoth
OfflineTtsImpl::Createoverloads now chooseOfflineTtsZipvoiceImplonly whenzipvoice.encoderandzipvoice.decoderare non-empty, preserving the previous “both paths required” behavior under the new names and keeping the model-choice ordering intact.Also applies to: 59-73
swift-api-examples/SherpaOnnx.swift (1)
930-953: Swift ZipVoice helper cleanly updated to encoder/decoderThe Swift factory now accepts
encoder/decoderand forwards them in the correct order toSherpaOnnxOfflineTtsZipvoiceModelConfig, staying consistent with the C struct layout and other encoder/decoder-based configs.sherpa-onnx/c-api/c-api.cc (1)
1243-1263: ZipVoice encoder/decoder wiring in C API is consistent
tts_config.model.zipvoice.encoder/decodernow map directly fromconfig->model.zipvoice.encoder/decoder, aligned with the new struct fields; tokens/vocoder/data_dir/lexicon and numeric params are untouched and remain correctly forwarded.wasm/tts/sherpa-onnx-tts.js (1)
265-327: ZipVoice WASM struct packing matches new encoder/decoder layoutThe ZipVoice JS binding now correctly:
- Computes buffer size as
tokens + encoder + decoder + vocoder + dataDir + lexicon,- Writes strings in that order, and
- Stores 6 pointers followed by 4 floats (featScale, tShift, targetRMS, guidanceScale) into a 10×4-byte struct,
which matches the updated C
SherpaOnnxOfflineTtsZipVoiceModelConfiglayout. The defaultofflineTtsZipVoiceModelConfigalso exposesencoder/decoderkeys, so external configs can set the new names directly.Also applies to: 375-387
sherpa-onnx/pascal-api/sherpa_onnx.pas (1)
104-118: Pascal ZipVoice Encoder/Decoder/Lexicon mapping is coherent end‑to‑endThe Pascal API now consistently uses
Encoder/Decoder/Lexicon:
- Public record, C-interop record,
ToString, andTSherpaOnnxOfflineTts.Createall agree on field names and order.
Interoperability with the C struct remains sound.Also applies to: 984-995, 2422-2438, 2530-2539
sherpa-onnx/c-api/cxx-api.cc (1)
416-425: OfflineTts C++ ZipVoice encoder/decoder mapping looks correct
OfflineTts::Createnow forwardsconfig.model.zipvoice.encoder/decoderintoc.model.zipvoice.encoder/decoder, matching the updated C API struct; other ZipVoice fields are unchanged and correctly wired.sherpa-onnx/csrc/sherpa-onnx-offline-zeroshot-tts.cc (1)
23-47: Updated CLI example correctly reflects new ZipVoice flagsThe usage block now demonstrates
--zipvoice-encoder,--zipvoice-decoder,--zipvoice-lexicon,--zipvoice-tokens, and--zipvoice-vocoderconsistent with the new ZipVoice configuration; no behavioral changes to main logic.go-api-examples/non-streaming-tts/main.go (1)
52-58: Go ZipVoice flags now target Encoder/Decoder/Lexicon fields correctlyThe Go example’s CLI now populates
OfflineTtsZipVoiceModelConfig.Encoder/Decoder/Lexiconvia--zipvoice-encoder/--zipvoice-decoder/--zipvoice-lexicon, aligning with the binding changes; the zero‑shotGenerateWithZipvoicepath reuses these without further changes.Also applies to: 64-68
python-api-examples/offline-zeroshot-tts.py (2)
19-20: LGTM! CLI interface updated consistently.The renaming from
--zipvoice-text-modeland--zipvoice-flow-matching-modelto--zipvoice-encoderand--zipvoice-decoderis applied consistently in both the usage example and argument definitions. The help text accurately reflects the new naming.Also applies to: 52-63
238-239: LGTM! Configuration construction properly uses renamed fields.The
OfflineTtsZipvoiceModelConfigis correctly instantiated with the newencoderanddecoderparameters.scripts/go/sherpa_onnx.go (2)
963-968: LGTM! Struct definition updated with clear field names.The
OfflineTtsZipvoiceModelConfigstruct now uses the more conciseEncoderandDecoderfield names with appropriately updated comments.
1139-1143: LGTM! C interop updated correctly.The C string conversion and memory management properly use the renamed
EncoderandDecoderfields from the Go config struct.sherpa-onnx/csrc/offline-tts-zipvoice-model-config.h (1)
17-18: LGTM! Core C++ config struct renamed consistently.The
OfflineTtsZipvoiceModelConfigstruct members, constructor parameters, and initializer list are all updated to useencoderanddecoderinstead of the more verbosetext_modelandflow_matching_model. The rename improves clarity.Also applies to: 32-39
sherpa-onnx/python/csrc/offline-tts-zipvoice-model-config.cc (1)
22-29: LGTM! Python bindings updated to match C++ rename.The Pybind11 bindings correctly expose the renamed
encoderanddecoderfields, with proper argument names and default values. Python users will have access to the new field names.flutter/sherpa_onnx/lib/src/tts.dart (3)
198-244: LGTM! Dart model class updated comprehensively.The
OfflineTtsZipVoiceModelConfigclass is updated consistently:
- Constructor parameters use
encoderanddecoder- JSON serialization/deserialization uses the correct keys
- String representation reflects the new field names
- Field declarations match the new naming
408-409: LGTM! Native interop and memory management updated correctly.The FFI code properly converts the renamed
encoderanddecoderfields to native UTF8 strings, and the memory cleanup correctly frees these pointers.Also applies to: 436-437
39-46: Nice formatting improvement!Adding trailing commas to the
toJsonmap literals is a good Dart practice that improves code formatting and makes future diffs cleaner when adding new fields.Also applies to: 88-95, 137-144, 181-186, 230-240
| if [ ! -f ./sherpa-onnx-zipvoice-distill-int8-zh-en-emilia/fm_decoder.onnx ]; then | ||
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/tts-models/sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | ||
| tar xf sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | ||
| rm sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | ||
| fi |
There was a problem hiding this comment.
Update the file existence check to match the new int8 model filenames.
Line 6 checks for fm_decoder.onnx, which is the old filename from the non-int8 model. However, the int8 model variant (downloaded on line 7) contains decoder.int8.onnx. Subsequent script runs will fail to find the check file and unnecessarily re-download the model.
Apply this diff to fix the file existence check:
-if [ ! -f ./sherpa-onnx-zipvoice-distill-int8-zh-en-emilia/fm_decoder.onnx ]; then
+if [ ! -f ./sherpa-onnx-zipvoice-distill-int8-zh-en-emilia/decoder.int8.onnx ]; then📝 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 ./sherpa-onnx-zipvoice-distill-int8-zh-en-emilia/fm_decoder.onnx ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/tts-models/sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| tar xf sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| rm sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| fi | |
| if [ ! -f ./sherpa-onnx-zipvoice-distill-int8-zh-en-emilia/decoder.int8.onnx ]; then | |
| curl -SL -O https://github.com/k2-fsa/sherpa-onnx/releases/download/tts-models/sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| tar xf sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| rm sherpa-onnx-zipvoice-distill-int8-zh-en-emilia.tar.bz2 | |
| fi |
🤖 Prompt for AI Agents
In go-api-examples/non-streaming-tts/run-zipvoice.sh around lines 6 to 10 the
file-existence check uses the old non-int8 name `fm_decoder.onnx`; update that
check to the int8 model filename `decoder.int8.onnx` so the script detects the
downloaded int8 tarball correctly and avoids re-downloading; leave the curl, tar
and rm steps unchanged.
1 participant
Summary by CodeRabbit
Breaking Changes
Updates
✏️ Tip: You can customize this high-level summary in your review settings.