Refactor TTS C API - #3127
Refactor TTS C API#3127
Conversation
dosubot
Bot
added
the
size:L
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 refactoring the C API for Text-to-Speech (TTS) functionality within Highlights
Changelog
Ignored Files
Activity
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
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR adds a GitHub Actions step to test PocketTTS, tightens null-checks and input validation across TTS C API entry points, makes GenerationConfig parameters const-correct in public/internal APIs, adjusts callback wrapper usage, and exports a new TTS symbol. Changes
Sequence Diagram(s)(omitted — changes are API-level and CI step; no multi-component sequential flow requiring diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
![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 refactors the TTS C API, improving its robustness by adding null pointer checks and fixing bugs related to null callbacks. The changes are generally positive, but I've identified a critical issue where an optional parameter has been made mandatory, which constitutes a breaking change. I've also pointed out a minor redundancy in the code. Please see my detailed comments.
| if (!config->reference_audio) { | ||
| SHERPA_ONNX_LOGE("Reference audio is nullptr"); | ||
| return nullptr; | ||
| } | ||
|
|
||
| if (config->reference_audio_len <= 0) { | ||
| SHERPA_ONNX_LOGE("Invalid reference audio len: %d", | ||
| config->reference_audio_len); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
This change makes reference_audio a mandatory parameter for SherpaOnnxOfflineTtsGenerateWithConfig. Previously, it was optional. This is a breaking change and will likely cause issues for users who are not using voice cloning features and thus not providing a reference audio. The checks for reference_audio and reference_audio_len should be conditional on reference_audio being provided.
A possible fix would be to change the logic to something like this:
sherpa_onnx::GenerationConfig cfg;
if (config->reference_audio) {
if (config->reference_audio_len <= 0) {
SHERPA_ONNX_LOGE("Invalid reference audio len: %d",
config->reference_audio_len);
return nullptr;
}
cfg.reference_audio.assign(
config->reference_audio,
config->reference_audio + config->reference_audio_len);
}
// ... continue setting other cfg fields| if (!callback) return 1; | ||
| return callback(samples, n, progress, arg); |
There was a problem hiding this comment.
The if (!callback) check on line 1631 is redundant because this lambda is only created inside the if (callback) block on line 1628. You can simplify the lambda body.
return callback(samples, n, progress, arg);
There was a problem hiding this comment.
Pull request overview
This PR refactors the TTS C API with a focus on improving parameter validation, error handling, and API consistency. The changes add a new SherpaOnnxOfflineTtsGenerateWithConfig function to the symbol exports and make the GenerationConfig parameter const-qualified across the API.
Changes:
- Added comprehensive null pointer checks and error logging for TTS generation functions
- Changed
GenerationConfig*toconst GenerationConfig*to indicate the config is not modified - Refactored callback wrapper logic to handle null callbacks explicitly
- Updated config field handling to use
SHERPA_ONNX_ORmacro for default values - Added CI workflow test for PocketTTS with the new API
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| sherpa-onnx/c-api/sherpa-onnx-symbols-c.exp | Added symbol export for SherpaOnnxOfflineTtsGenerateWithConfig |
| sherpa-onnx/c-api/c-api.h | Changed GenerationConfig* to const GenerationConfig* parameter |
| sherpa-onnx/c-api/c-api.cc | Added null checks, refactored validation logic, improved error messages, and restructured callback handling |
| .github/workflows/c-api.yaml | Added CI test for PocketTTS example using the new API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cfg.silence_scale = SHERPA_ONNX_OR(config->silence_scale, 0.2); | ||
| cfg.speed = SHERPA_ONNX_OR(config->speed, 1.0); |
There was a problem hiding this comment.
Weakened validation for config fields: The old code used if (config->silence_scale > 0) to validate that the value is positive before using it. The new code uses SHERPA_ONNX_OR which treats any non-zero value as valid, including negative values. This means negative values like -0.5 would now be accepted and passed through, whereas they would have been rejected before (falling back to defaults).
If silence_scale and speed should only accept positive values, restore the > 0 check or add validation to reject negative values.
| cfg.silence_scale = SHERPA_ONNX_OR(config->silence_scale, 0.2); | |
| cfg.speed = SHERPA_ONNX_OR(config->speed, 1.0); | |
| if (config->silence_scale > 0) { | |
| cfg.silence_scale = config->silence_scale; | |
| } else { | |
| cfg.silence_scale = 0.2; | |
| } | |
| if (config->speed > 0) { | |
| cfg.speed = config->speed; | |
| } else { | |
| cfg.speed = 1.0; | |
| } |
|
|
||
| if (config->extra) { | ||
| try { | ||
| auto json = nlohmann::json::parse(config->extra); |
There was a problem hiding this comment.
Weakened validation for num_steps: The old code used if (config->num_steps > 0) to validate that the value is positive before using it. The new code uses SHERPA_ONNX_OR which treats any non-zero value as valid, including negative values. This means negative values would now be accepted and passed through to the underlying implementation, whereas they would have been rejected before (falling back to the default of 5).
If num_steps should only accept positive values, restore the > 0 check or add validation to reject negative values.
| if (config->reference_audio) { | ||
| if (config->reference_audio_len <= 0) { | ||
| SHERPA_ONNX_LOGE("Invalid reference audio len: %d", | ||
| config->reference_audio_len); | ||
| return nullptr; | ||
| } | ||
|
|
||
| cfg.reference_audio.assign( | ||
| config->reference_audio, | ||
| config->reference_audio + config->reference_audio_len); | ||
| } |
There was a problem hiding this comment.
The reference audio validation logic has been changed from optional to mandatory. The old code checked if (config->reference_audio_len > 0 && config->reference_audio) before using reference audio, making it optional. The new code returns an error if reference_audio is nullptr, which breaks backward compatibility for callers who don't provide reference audio.
This is a breaking API change. Consider making reference_audio optional by checking both pointers first: if reference_audio is nullptr, skip the reference audio processing entirely instead of returning an error. This would maintain backward compatibility while adding the validation for cases where reference_audio is provided.
| if (!text) { | ||
| SHERPA_ONNX_LOGE("text is nullptr"); | ||
| return nullptr; |
There was a problem hiding this comment.
Inconsistent error logging: This function checks if tts is nullptr at line 1563 but doesn't log an error message before returning nullptr. This is inconsistent with the error logging pattern used in other similar functions (e.g., SherpaOnnxOfflineTtsGenerate at line 1444, SherpaOnnxOfflineTtsGenerateWithCallback at line 1460). Add a SHERPA_ONNX_LOGE statement before returning nullptr for consistency.
| if (!text) { | ||
| SHERPA_ONNX_LOGE("text is nullptr"); | ||
| return nullptr; |
There was a problem hiding this comment.
Inconsistent error logging: This function checks if tts is nullptr at line 1614 but doesn't log an error message before returning nullptr. This is inconsistent with the error logging pattern used in other similar functions (e.g., SherpaOnnxOfflineTtsGenerate at line 1444). Add a SHERPA_ONNX_LOGE statement before returning nullptr for consistency.
| std::move(wrapper)); | ||
| } else { |
There was a problem hiding this comment.
Redundant null check: The lambda checks if callback is nullptr at line 1631, but this code is already inside an if (callback) block that starts at line 1628. This check is unnecessary and can be removed. The lambda should simply call the callback directly.
| std::string ptext_s = prompt_text; | ||
|
|
||
| std::vector<float> prompt_vec; | ||
| if (prompt_samples && n_prompt > 0) { |
There was a problem hiding this comment.
Redundant null check: The condition if (prompt_samples && n_prompt > 0) checks prompt_samples again, but this pointer is already validated at line 1577 where the function returns nullptr if prompt_samples is null. The check for prompt_samples here is redundant and can be simplified to just if (n_prompt > 0).
| if (prompt_samples && n_prompt > 0) { | |
| if (n_prompt > 0) { |
|
|
||
| cfg.reference_text = SHERPA_ONNX_OR(config->reference_text, ""); | ||
| cfg.num_steps = SHERPA_ONNX_OR(config->num_steps, 5); | ||
|
|
There was a problem hiding this comment.
Missing validation for reference_sample_rate: The C++ implementation (offline-tts-pocket-impl.h:526) validates that reference_sample_rate must be greater than 0. However, the C API doesn't validate this before passing it to the C++ layer. If a caller doesn't set reference_sample_rate (leaving it as 0), the error will only be caught later in the C++ layer. Consider adding validation here: if (config->reference_sample_rate <= 0) { SHERPA_ONNX_LOGE("Invalid reference_sample_rate: %d", config->reference_sample_rate); return nullptr; }
Summary by CodeRabbit
New Features
Bug Fixes
Chores