Refactor Kokoro TTS to use the new Generate API - #3363
Conversation
dosubot
Bot
added
the
size:L
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates offline TTS generation examples across multiple language bindings (C, C++, Dart, .NET, Java, JavaScript, Pascal, Swift) to use a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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)
📝 Coding Plan
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, 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 streamlines the Text-to-Speech (TTS) generation process for Kokoro models by introducing a standardized 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. Footnotes
|
![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 (1)
swift-api-examples/tts-kokoro-zh-en.swift (1)
31-42: Unusedprogressparameter in callback.The callback signature now includes a
progressparameter (third argument), but it's not being used in the callback body. Consider logging or utilizing the progress value for user feedback, similar to how the C examples print progress.💡 Optional: Log progress
let callback: TtsProgressCallbackWithArg = { samples, n, progress, arg in let o = Unmanaged<MyClass>.fromOpaque(arg!).takeUnretainedValue() + print("Progress: \(progress * 100)%") var savedSamples: [Float] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swift-api-examples/tts-kokoro-zh-en.swift` around lines 31 - 42, The TtsProgressCallbackWithArg closure currently ignores its third parameter progress; update the callback body (the closure assigned to callback) to use progress (e.g., call a logging/UI method or pass it into MyClass) so callers get feedback: extract o via Unmanaged<MyClass>.fromOpaque(arg!).takeUnretainedValue(), then either log or forward the progress value (e.g., o.updateProgress(progress) or o.log("progress: \(progress)") before assembling savedSamples and calling o.playSamples(samples: savedSamples); keep returning 1 to continue generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/csrc/offline-tts-kokoro-impl.h`:
- Around line 465-467: The check treating 0.2f as a sentinel causes ambiguity
because GenerationConfig::silence_scale (and the Generate() parameter) can be
explicitly set to 0.2; change the logic to distinguish "unset" from an explicit
value by either (a) switching GenerationConfig::silence_scale default to an
out-of-band sentinel (e.g., -1.0f) and update callers, then replace the if
(silence_scale == 0.2f) branch in offline-tts-kokoro-impl.h to check for the new
sentinel (e.g., silence_scale < 0.0f) and fall back to config_.silence_scale, or
(b) make silence_scale an std::optional<float> on the Generate() API and in
GenerationConfig, and use has_value() to decide whether to use the config
default; update references to config_.silence_scale and the Generate() signature
accordingly so an explicit 0.2 is honored.
---
Nitpick comments:
In `@swift-api-examples/tts-kokoro-zh-en.swift`:
- Around line 31-42: The TtsProgressCallbackWithArg closure currently ignores
its third parameter progress; update the callback body (the closure assigned to
callback) to use progress (e.g., call a logging/UI method or pass it into
MyClass) so callers get feedback: extract o via
Unmanaged<MyClass>.fromOpaque(arg!).takeUnretainedValue(), then either log or
forward the progress value (e.g., o.updateProgress(progress) or o.log("progress:
\(progress)") before assembling savedSamples and calling o.playSamples(samples:
savedSamples); keep returning 1 to continue generation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c006a582-5eff-4ad6-b2a8-872730e3c8d6
📒 Files selected for processing (21)
c-api-examples/kokoro-tts-en-c-api.cc-api-examples/kokoro-tts-zh-en-c-api.ccxx-api-examples/kokoro-tts-en-cxx-api.cccxx-api-examples/kokoro-tts-zh-en-cxx-api.ccdart-api-examples/tts/bin/kokoro-en.dartdart-api-examples/tts/bin/kokoro-zh-en.dartdotnet-examples/kokoro-tts-play/Program.csdotnet-examples/kokoro-tts/Program.csjava-api-examples/NonStreamingTtsKokoroEn.javajava-api-examples/NonStreamingTtsKokoroZhEn.javanodejs-addon-examples/test_tts_non_streaming_kokoro_en.jsnodejs-addon-examples/test_tts_non_streaming_kokoro_zh_en.jsnodejs-examples/test-offline-tts-kokoro-en.jsnodejs-examples/test-offline-tts-kokoro-zh-en.jspascal-api-examples/tts/kokoro-en-playback.paspascal-api-examples/tts/kokoro-en.paspascal-api-examples/tts/kokoro-zh-en-playback.paspascal-api-examples/tts/kokoro-zh-en.passherpa-onnx/csrc/offline-tts-kokoro-impl.hswift-api-examples/tts-kokoro-en.swiftswift-api-examples/tts-kokoro-zh-en.swift
| if (silence_scale == 0.2f) { | ||
| silence_scale = config_.silence_scale; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the default value of silence_scale in GenerationConfig
rg -n "silence_scale\s*=" sherpa-onnx/csrc/offline-tts.h -A2 -B2
# Check how other TTS implementations handle silence_scale
ast-grep --pattern 'silence_scale == $_'Repository: k2-fsa/sherpa-onnx
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Get full context around the problematic code
cat -n sherpa-onnx/csrc/offline-tts-kokoro-impl.h | sed -n '450,480p'Repository: k2-fsa/sherpa-onnx
Length of output: 1101
🏁 Script executed:
#!/bin/bash
# Find the function containing this code to understand how silence_scale is passed
rg -n "silence_scale" sherpa-onnx/csrc/offline-tts-kokoro-impl.h -B5 -A5 | head -100Repository: k2-fsa/sherpa-onnx
Length of output: 2923
🏁 Script executed:
#!/bin/bash
# Check if silence_scale is ever set explicitly to 0.2 anywhere in call sites
rg -n "silence_scale\s*=" sherpa-onnx/csrc/ --type cpp --type h | grep -v "0\.2" | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 804
🏁 Script executed:
#!/bin/bash
# Look for optional or sentinel value patterns used elsewhere in the codebase
rg -n "std::optional" sherpa-onnx/csrc/ | head -10Repository: k2-fsa/sherpa-onnx
Length of output: 497
Sentinel value 0.2f creates ambiguity—user cannot explicitly set silence_scale to 0.2.
The GenerationConfig::silence_scale defaults to 0.2 and the code checks for this value as a sentinel to mean "unset/use config default." This creates an impossible situation: if a user explicitly sets silence_scale = 0.2 when calling Generate(), the code incorrectly treats it as "unset" and overrides it with config_.silence_scale.
The documentation at line 156–157 even acknowledges this pattern ("If left at 0.2, it falls back..."), but this is problematic API design. Other TTS backends in the codebase (kitten, matcha, vits) avoid this by directly using the config value without sentinel checks.
Use a different sentinel (e.g., a negative value like -1.0f or a guard value outside the valid range) or switch to std::optional<float> to properly distinguish "explicitly set" from "unset."
💡 Suggested fix using a negative sentinel
Change the default in GenerationConfig to a sentinel like -1.0f, then:
- if (silence_scale == 0.2f) {
+ if (silence_scale < 0) {
silence_scale = config_.silence_scale;
}This would require updating GenerationConfig::silence_scale default from 0.2 to -1.0f in offline-tts.h, and updating all call sites that rely on the default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sherpa-onnx/csrc/offline-tts-kokoro-impl.h` around lines 465 - 467, The check
treating 0.2f as a sentinel causes ambiguity because
GenerationConfig::silence_scale (and the Generate() parameter) can be explicitly
set to 0.2; change the logic to distinguish "unset" from an explicit value by
either (a) switching GenerationConfig::silence_scale default to an out-of-band
sentinel (e.g., -1.0f) and update callers, then replace the if (silence_scale ==
0.2f) branch in offline-tts-kokoro-impl.h to check for the new sentinel (e.g.,
silence_scale < 0.0f) and fall back to config_.silence_scale, or (b) make
silence_scale an std::optional<float> on the Generate() API and in
GenerationConfig, and use has_value() to decide whether to use the config
default; update references to config_.silence_scale and the Generate() signature
accordingly so an explicit 0.2 is honored.
![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 consistently refactors the Kokoro TTS examples and implementation across various languages to adopt a new GenerationConfig-based API, which is a good step towards a more extensible interface. My review focuses on improving code clarity in the examples and points out a potential design issue in the C++ implementation concerning how default parameter values are handled.
| SherpaOnnxGenerationConfig cfg = {0}; | ||
| cfg.silence_scale = 0.2f; | ||
| cfg.sid = sid; | ||
| cfg.speed = speed; |
There was a problem hiding this comment.
The initialization of SherpaOnnxGenerationConfig can be made more concise and readable by using C99 designated initializers. This avoids initializing the struct to zero and then assigning members individually.
| SherpaOnnxGenerationConfig cfg = {0}; | |
| cfg.silence_scale = 0.2f; | |
| cfg.sid = sid; | |
| cfg.speed = speed; | |
| SherpaOnnxGenerationConfig cfg = {.silence_scale = 0.2f, .sid = sid, .speed = speed}; |
| SherpaOnnxGenerationConfig cfg = {0}; | ||
| cfg.silence_scale = 0.2f; | ||
| cfg.sid = sid; | ||
| cfg.speed = speed; |
There was a problem hiding this comment.
The initialization of SherpaOnnxGenerationConfig can be made more concise and readable by using C99 designated initializers. This avoids initializing the struct to zero and then assigning members individually.
| SherpaOnnxGenerationConfig cfg = {0}; | |
| cfg.silence_scale = 0.2f; | |
| cfg.sid = sid; | |
| cfg.speed = speed; | |
| SherpaOnnxGenerationConfig cfg = {.silence_scale = 0.2f, .sid = sid, .speed = speed}; |
| int32_t sid = 0; | ||
| float speed = 1.0; // larger -> faster in speech speed | ||
| GenerationConfig gen_config; | ||
| gen_config.sid = sid; | ||
| gen_config.speed = speed; | ||
| gen_config.silence_scale = 0.2f; |
There was a problem hiding this comment.
The sid and speed variables are only used to initialize gen_config and are then discarded. You can make the code more direct by removing these intermediate variables and assigning the values directly to the gen_config members.
| int32_t sid = 0; | |
| float speed = 1.0; // larger -> faster in speech speed | |
| GenerationConfig gen_config; | |
| gen_config.sid = sid; | |
| gen_config.speed = speed; | |
| gen_config.silence_scale = 0.2f; | |
| GenerationConfig gen_config; | |
| gen_config.sid = 0; | |
| gen_config.speed = 1.0; // larger -> faster in speech speed | |
| gen_config.silence_scale = 0.2f; |
| int32_t sid = 50; | ||
| float speed = 1.0; // larger -> faster in speech speed | ||
| GenerationConfig gen_config; | ||
| gen_config.sid = sid; | ||
| gen_config.speed = speed; | ||
| gen_config.silence_scale = 0.2f; |
There was a problem hiding this comment.
The sid and speed variables are only used to initialize gen_config and are then discarded. You can make the code more direct by removing these intermediate variables and assigning the values directly to the gen_config members.
| int32_t sid = 50; | |
| float speed = 1.0; // larger -> faster in speech speed | |
| GenerationConfig gen_config; | |
| gen_config.sid = sid; | |
| gen_config.speed = speed; | |
| gen_config.silence_scale = 0.2f; | |
| GenerationConfig gen_config; | |
| gen_config.sid = 50; | |
| gen_config.speed = 1.0; // larger -> faster in speech speed | |
| gen_config.silence_scale = 0.2f; |
| OfflineTtsGenerationConfig genConfig = new OfflineTtsGenerationConfig(); | ||
| genConfig.Sid = sid; | ||
| genConfig.Speed = speed; |
There was a problem hiding this comment.
For more concise and idiomatic C# code, you can use an object initializer to set the properties of OfflineTtsGenerationConfig upon instantiation.
var genConfig = new OfflineTtsGenerationConfig { Sid = sid, Speed = speed };| int sid = 0; | ||
| float speed = 1.0f; | ||
| GenerationConfig genConfig = new GenerationConfig(); | ||
| genConfig.setSid(sid); | ||
| genConfig.setSpeed(speed); | ||
| genConfig.setSilenceScale(0.2f); |
There was a problem hiding this comment.
The sid and speed variables are only used for initializing genConfig. You can simplify the code by removing these variables and setting the values directly.
| int sid = 0; | |
| float speed = 1.0f; | |
| GenerationConfig genConfig = new GenerationConfig(); | |
| genConfig.setSid(sid); | |
| genConfig.setSpeed(speed); | |
| genConfig.setSilenceScale(0.2f); | |
| GenerationConfig genConfig = new GenerationConfig(); | |
| genConfig.setSid(0); | |
| genConfig.setSpeed(1.0f); | |
| genConfig.setSilenceScale(0.2f); |
| int sid = 0; // this model has 53 speakers. You can use sid in the range 0-52 | ||
| float speed = 1.0f; | ||
| GenerationConfig genConfig = new GenerationConfig(); | ||
| genConfig.setSid(sid); | ||
| genConfig.setSpeed(speed); | ||
| genConfig.setSilenceScale(0.2f); |
There was a problem hiding this comment.
The sid and speed variables are only used for initializing genConfig. You can simplify the code by removing these variables and setting the values directly. The comment for sid can be moved to clarify the direct assignment.
| int sid = 0; // this model has 53 speakers. You can use sid in the range 0-52 | |
| float speed = 1.0f; | |
| GenerationConfig genConfig = new GenerationConfig(); | |
| genConfig.setSid(sid); | |
| genConfig.setSpeed(speed); | |
| genConfig.setSilenceScale(0.2f); | |
| // this model has 53 speakers. You can use sid in the range 0-52 | |
| GenerationConfig genConfig = new GenerationConfig(); | |
| genConfig.setSid(0); | |
| genConfig.setSpeed(1.0f); | |
| genConfig.setSilenceScale(0.2f); |
| const generationConfig = { | ||
| sid: speakerId, | ||
| speed: speed, | ||
| silenceScale: 0.2, | ||
| }; |
There was a problem hiding this comment.
You can use ES6 property value shorthand for speed to make the object initialization more concise.
| const generationConfig = { | |
| sid: speakerId, | |
| speed: speed, | |
| silenceScale: 0.2, | |
| }; | |
| const generationConfig = { | |
| sid: speakerId, | |
| speed, | |
| silenceScale: 0.2, | |
| }; |
| const generationConfig = { | ||
| sid: speakerId, | ||
| speed: speed, | ||
| silenceScale: 0.2, | ||
| }; |
There was a problem hiding this comment.
You can use ES6 property value shorthand for speed to make the object initialization more concise.
| const generationConfig = { | |
| sid: speakerId, | |
| speed: speed, | |
| silenceScale: 0.2, | |
| }; | |
| const generationConfig = { | |
| sid: speakerId, | |
| speed, | |
| silenceScale: 0.2, | |
| }; |
| if (silence_scale == 0.2f) { | ||
| silence_scale = config_.silence_scale; | ||
| } |
There was a problem hiding this comment.
Using 0.2f as a magic number to detect if silence_scale was set by the user is fragile. If a user explicitly wants to set silence_scale to 0.2f, their value will be unexpectedly overridden by config_.silence_scale. A more robust approach would be to use a sentinel value (e.g., a negative number, since scale must be non-negative) or std::optional<float> in GenerationConfig to represent an unset value. This would make the intent clearer and avoid potential bugs.
There was a problem hiding this comment.
Pull request overview
Refactors Kokoro offline TTS examples and implementation to use the new “Generate with GenerationConfig” API (and updated progress-callback signatures) across multiple language bindings.
Changes:
- Updated Swift/Node.js/Java/.NET/Dart/C/C++/Pascal examples to construct and pass a GenerationConfig instead of (sid, speed) arguments.
- Extended Kokoro TTS backend to accept
GenerationConfig, add extra optionlang, and routesilence_scalethrough processing. - Kept backward compatibility by adding a deprecated overload that adapts legacy parameters into
GenerationConfig.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| swift-api-examples/tts-kokoro-zh-en.swift | Migrates Swift example to generateWithConfig + progress callback w/ arg |
| swift-api-examples/tts-kokoro-en.swift | Migrates Swift example to generateWithConfig + progress callback w/ arg |
| sherpa-onnx/csrc/offline-tts-kokoro-impl.h | Implements Generate(text, GenerationConfig, callback), adds lang extra and silence_scale plumbing |
| pascal-api-examples/tts/kokoro-zh-en.pas | Updates Pascal example to pass TSherpaOnnxGenerationConfig |
| pascal-api-examples/tts/kokoro-zh-en-playback.pas | Updates Pascal playback example callback signature + config-based generation |
| pascal-api-examples/tts/kokoro-en.pas | Updates Pascal example to pass TSherpaOnnxGenerationConfig |
| pascal-api-examples/tts/kokoro-en-playback.pas | Updates Pascal playback example callback signature + config-based generation |
| nodejs-examples/test-offline-tts-kokoro-zh-en.js | Switches Node.js example to generateWithConfig |
| nodejs-examples/test-offline-tts-kokoro-en.js | Switches Node.js example to generateWithConfig |
| nodejs-addon-examples/test_tts_non_streaming_kokoro_zh_en.js | Uses addon GenerationConfig and passes it to generate |
| nodejs-addon-examples/test_tts_non_streaming_kokoro_en.js | Uses addon GenerationConfig and passes it to generate |
| java-api-examples/NonStreamingTtsKokoroZhEn.java | Uses GenerationConfig and config-based generate API |
| java-api-examples/NonStreamingTtsKokoroEn.java | Uses GenerationConfig and config-based generate API |
| dotnet-examples/kokoro-tts/Program.cs | Migrates .NET example to config-based generation + callback-with-arg |
| dotnet-examples/kokoro-tts-play/Program.cs | Migrates .NET playback example to config-based generation + callback-with-arg |
| dart-api-examples/tts/bin/kokoro-zh-en.dart | Switches Dart example to OfflineTtsGenerationConfig + generateWithConfig |
| dart-api-examples/tts/bin/kokoro-en.dart | Switches Dart example to OfflineTtsGenerationConfig + generateWithConfig |
| cxx-api-examples/kokoro-tts-zh-en-cxx-api.cc | Updates C++ example to pass GenerationConfig |
| cxx-api-examples/kokoro-tts-en-cxx-api.cc | Updates C++ example to pass GenerationConfig |
| c-api-examples/kokoro-tts-zh-en-c-api.c | Updates C example to SherpaOnnxGenerationConfig + new callback signature |
| c-api-examples/kokoro-tts-en-c-api.c | Updates C example to SherpaOnnxGenerationConfig + new callback signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (silence_scale == 0.2f) { | ||
| silence_scale = config_.silence_scale; | ||
| } |
| const std::string &_text, const GenerationConfig &gen_config, | ||
| GeneratedAudioCallback callback = nullptr) const override { | ||
| if (config_.model.debug) { | ||
| SHERPA_ONNX_LOGE("%s", gen_config.ToString().c_str()); |
| var MyCallback = (IntPtr samples, int n, float progress) => | ||
| OfflineTtsGenerationConfig genConfig = new OfflineTtsGenerationConfig(); | ||
| genConfig.Sid = sid; | ||
| genConfig.Speed = speed; |
Summary by CodeRabbit
New Features
Refactor