Repository navigation
Add CXX API for PocketTTS - #3128
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughAdds PocketTTS support: introduces a unified GenerationConfig with reference-audio fields, updates C and C++ TTS APIs and overloads, adds a C++ PocketTTS example and C example updates, wires Pocket model parameters in the CXX layer, and adds a CI step to build/run the pocket-tts C++ example and upload WAV artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant Example as C++ Example
participant CXX as sherpa-onnx CXX API
participant CApi as sherpa-onnx C API
participant Model as PocketTTS Model / Filesystem
Note over Example,CXX: Generate(text, GenerationConfig)
Example->>CXX: OfflineTts::Generate(text, config, callback)
CXX->>CApi: SherpaOnnxOfflineTtsGenerateWithConfig(tts, text, sherpaConfig, callback)
CApi->>Model: load model artifacts (lm, encoder, decoder, vocab, token_scores)
CApi->>Model: run generation (uses reference_audio if provided)
Model-->>CApi: generated audio buffer
CApi-->>CXX: SherpaOnnxGeneratedAudio result
CXX-->>Example: GeneratedAudio -> write WAV
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 expands the 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
|
![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 C++ API for PocketTTS, which is a great addition. The changes include a new C++ example, updates to CMake, and modifications to the C/C++ APIs to support a more flexible generation configuration.
My review focuses on improving the newly introduced GenerationConfig in the C++ API for better flexibility and correctness. I've identified a potential issue with how extra parameters are handled, which could lead to parsing errors for non-string values. I've also found a critical issue with struct initialization that could lead to undefined behavior. I've provided specific suggestions to address these points in the relevant files.
Overall, the changes are well-structured, and with a few adjustments, the new API will be more robust and easier to use.
| const GenerationConfig &config, | ||
| OfflineTtsCallback callback /*= nullptr*/, | ||
| void *arg /*= nullptr*/) const { | ||
| SherpaOnnxGenerationConfig c; |
There was a problem hiding this comment.
The SherpaOnnxGenerationConfig struct c is not initialized. This can lead to undefined behavior as its members will contain garbage values, especially pointers which might not be nullptr. It's important to zero-initialize C-style structs.
| SherpaOnnxGenerationConfig c; | |
| SherpaOnnxGenerationConfig c{}; |
|
|
||
| #include <memory> | ||
| #include <string> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
To support a more flexible GenerationConfig::extra field that can handle various data types (not just strings), it would be beneficial to use nlohmann::json. Please include its header here. I've added another comment with more details on the GenerationConfig struct itself.
#include "nlohmann/json.hpp"| int32_t num_steps = 5; // number of steps in flow matching | ||
|
|
||
| // extra attrs , model specific | ||
| std::unordered_map<std::string, std::string> extra; |
There was a problem hiding this comment.
The current design of GenerationConfig::extra as std::unordered_map<std::string, std::string> is restrictive as it only allows string values. This can lead to issues when numerical or boolean parameters are needed, as they are passed as strings and may be parsed incorrectly downstream. For instance, a value like "10" becomes "\"10\"" after JSON serialization, which fails to parse as a number.
To improve flexibility and prevent such parsing errors, I recommend changing the type of extra to nlohmann::json and initializing it as an empty object. This allows for storing various data types (numbers, booleans, strings) correctly.
nlohmann::json extra = nlohmann::json::object();| nlohmann::json j = config.extra; | ||
| std::string s = j.dump(); | ||
| c.extra = s.c_str(); |
There was a problem hiding this comment.
Following the suggested change in cxx-api.h to use nlohmann::json for GenerationConfig::extra, this logic for JSON conversion should be updated. You can now directly dump config.extra if it's not empty, which simplifies the code and correctly handles various data types.
| nlohmann::json j = config.extra; | |
| std::string s = j.dump(); | |
| c.extra = s.c_str(); | |
| std::string s; | |
| if (!config.extra.empty()) { | |
| s = config.extra.dump(); | |
| c.extra = s.c_str(); | |
| } |
| Wave wave = ReadWave(reference_audio_file); | ||
| cfg.reference_audio = std::move(wave.samples); | ||
| cfg.reference_sample_rate = wave.sample_rate; | ||
| cfg.extra["max_reference_audio_len"] = "10"; |
There was a problem hiding this comment.
With the proposed change to use nlohmann::json for GenerationConfig::extra, you should use a numeric type for this parameter instead of a string to avoid parsing issues and leverage the flexibility of the new API design.
| cfg.extra["max_reference_audio_len"] = "10"; | |
| cfg.extra["max_reference_audio_len"] = 10.0; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cxx-api-examples/pocket-tts-en-cxx-api.cc`:
- Around line 67-86: ReadWave can fail and leave Wave.sample_rate uninitialized,
so after calling ReadWave(reference_audio_file) validate the returned Wave
(e.g., ensure wave.sample_rate > 0 and wave.samples is non-empty) before
assigning cfg.reference_audio and cfg.reference_sample_rate; if invalid, handle
the error path (log/throw/return or set sane defaults) so
GenerationConfig::reference_sample_rate is never assigned an uninitialized value
— update the code around the ReadWave call and the assignments to
cfg.reference_audio and cfg.reference_sample_rate in the block that constructs
GenerationConfig (where ReadWave, Wave, and
GenerationConfig::reference_sample_rate are used).
In `@sherpa-onnx/c-api/cxx-api.cc`:
- Around line 512-535: The code dereferences the result of
SherpaOnnxOfflineTtsGenerateWithConfig without checking for nullptr; modify the
call site (where SherpaOnnxOfflineTtsGenerateWithConfig is invoked and its
result stored in audio) to check if audio == nullptr and return an
empty/appropriate GeneratedAudio (e.g., samples empty and sample_rate 0) instead
of dereferencing; also guard the reference_audio assignment (c.reference_audio =
config.reference_audio.data()) so you only set c.reference_audio and
c.reference_audio_len when config.reference_audio is non-empty to avoid
triggering the C API validation path that returns nullptr.
In `@sherpa-onnx/c-api/cxx-api.h`:
- Around line 487-498: The GenerationConfig struct has reference_sample_rate
uninitialized which can lead to indeterminate values; update the struct
definition (GenerationConfig) to give reference_sample_rate a default value
(e.g., = 0) to match the Java API and ensure deterministic behavior, leaving
other fields unchanged; modify the declaration of reference_sample_rate in
GenerationConfig to include the default initializer.
| auto tts = OfflineTts::Create(config); | ||
| GenerationConfig cfg; | ||
| cfg.speed = 1.0; | ||
|
|
||
| std::string reference_audio_file = | ||
| "./sherpa-onnx-pocket-tts-int8-2026-01-26/test_wavs/bria.wav"; | ||
|
|
||
| Wave wave = ReadWave(reference_audio_file); | ||
| cfg.reference_audio = std::move(wave.samples); | ||
| cfg.reference_sample_rate = wave.sample_rate; | ||
| cfg.extra["max_reference_audio_len"] = "10"; | ||
|
|
||
| #if 0 | ||
| // If you don't want to use a callback, then please enable this branch | ||
| GeneratedAudio audio = tts.Generate(text, cfg); | ||
| #else | ||
| GeneratedAudio audio = tts.Generate(text, cfg, ProgressCallback); | ||
| #endif | ||
|
|
||
| WriteWave(filename, {audio.samples, audio.sample_rate}); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ReadWave function and GenerationConfig struct definitions
echo "=== Searching for ReadWave ==="
rg -n "Wave ReadWave" --type cc --type h -C 3
echo "=== Searching for GenerationConfig struct ==="
rg -n "struct GenerationConfig|class GenerationConfig" --type cc --type h -C 3Repository: k2-fsa/sherpa-onnx
Length of output: 209
🏁 Script executed:
#!/bin/bash
# Search for ReadWave function and GenerationConfig struct definitions
echo "=== Searching for ReadWave ==="
rg "Wave ReadWave" -C 3
echo "=== Searching for GenerationConfig struct ==="
rg "struct GenerationConfig|class GenerationConfig" -C 3
echo "=== Check the file structure ==="
find . -name "cxx-api.h" -o -name "cxx-api.cc" | head -10Repository: k2-fsa/sherpa-onnx
Length of output: 2788
🏁 Script executed:
#!/bin/bash
# Get full ReadWave implementation
echo "=== ReadWave implementation ==="
rg -A 15 "^Wave ReadWave" sherpa-onnx/c-api/cxx-api.cc
echo "=== Wave struct definition ==="
rg -B 2 -A 5 "^struct Wave" sherpa-onnx/c-api/cxx-api.h
echo "=== GenerationConfig in offline-tts.h ==="
rg -A 20 "^struct GenerationConfig" sherpa-onnx/csrc/offline-tts.h
echo "=== GenerationConfig in cxx-api.h ==="
rg -A 20 "^struct GenerationConfig" sherpa-onnx/c-api/cxx-api.hRepository: k2-fsa/sherpa-onnx
Length of output: 2209
🏁 Script executed:
#!/bin/bash
# Check the actual file being reviewed
echo "=== File context around lines 67-86 ==="
sed -n '60,90p' cxx-api-examples/pocket-tts-en-cxx-api.cc
echo "=== Check includes to see which GenerationConfig is used ==="
head -20 cxx-api-examples/pocket-tts-en-cxx-api.ccRepository: k2-fsa/sherpa-onnx
Length of output: 1807
🏁 Script executed:
#!/bin/bash
# Get full includes section
echo "=== Full includes in the file ==="
head -50 cxx-api-examples/pocket-tts-en-cxx-api.cc | grep -A 50 "include"
echo "=== Check if ReadWave and Wave are from cxx-api ==="
rg "^#include.*cxx-api" cxx-api-examples/pocket-tts-en-cxx-api.ccRepository: k2-fsa/sherpa-onnx
Length of output: 1282
Add validation after ReadWave to prevent undefined behavior from uninitialized sample_rate.
ReadWave() performs default initialization on failure, leaving the Wave struct's sample_rate field uninitialized. Assigning this garbage value to GenerationConfig::reference_sample_rate causes undefined behavior. Check the result before use and ensure proper initialization.
Suggested fix
- GenerationConfig cfg;
+ GenerationConfig cfg{};
cfg.speed = 1.0;
std::string reference_audio_file =
"./sherpa-onnx-pocket-tts-int8-2026-01-26/test_wavs/bria.wav";
Wave wave = ReadWave(reference_audio_file);
+ if (wave.samples.empty() || wave.sample_rate <= 0) {
+ fprintf(stderr, "Failed to read %s\n", reference_audio_file.c_str());
+ return -1;
+ }
+ cfg.reference_sample_rate = wave.sample_rate;
cfg.reference_audio = std::move(wave.samples);
- cfg.reference_sample_rate = wave.sample_rate;
cfg.extra["max_reference_audio_len"] = "10";📝 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.
| auto tts = OfflineTts::Create(config); | |
| GenerationConfig cfg; | |
| cfg.speed = 1.0; | |
| std::string reference_audio_file = | |
| "./sherpa-onnx-pocket-tts-int8-2026-01-26/test_wavs/bria.wav"; | |
| Wave wave = ReadWave(reference_audio_file); | |
| cfg.reference_audio = std::move(wave.samples); | |
| cfg.reference_sample_rate = wave.sample_rate; | |
| cfg.extra["max_reference_audio_len"] = "10"; | |
| #if 0 | |
| // If you don't want to use a callback, then please enable this branch | |
| GeneratedAudio audio = tts.Generate(text, cfg); | |
| #else | |
| GeneratedAudio audio = tts.Generate(text, cfg, ProgressCallback); | |
| #endif | |
| WriteWave(filename, {audio.samples, audio.sample_rate}); | |
| auto tts = OfflineTts::Create(config); | |
| GenerationConfig cfg{}; | |
| cfg.speed = 1.0; | |
| std::string reference_audio_file = | |
| "./sherpa-onnx-pocket-tts-int8-2026-01-26/test_wavs/bria.wav"; | |
| Wave wave = ReadWave(reference_audio_file); | |
| if (wave.samples.empty() || wave.sample_rate <= 0) { | |
| fprintf(stderr, "Failed to read %s\n", reference_audio_file.c_str()); | |
| return -1; | |
| } | |
| cfg.reference_sample_rate = wave.sample_rate; | |
| cfg.reference_audio = std::move(wave.samples); | |
| cfg.extra["max_reference_audio_len"] = "10"; | |
| `#if` 0 | |
| // If you don't want to use a callback, then please enable this branch | |
| GeneratedAudio audio = tts.Generate(text, cfg); | |
| `#else` | |
| GeneratedAudio audio = tts.Generate(text, cfg, ProgressCallback); | |
| `#endif` | |
| WriteWave(filename, {audio.samples, audio.sample_rate}); |
🤖 Prompt for AI Agents
In `@cxx-api-examples/pocket-tts-en-cxx-api.cc` around lines 67 - 86, ReadWave can
fail and leave Wave.sample_rate uninitialized, so after calling
ReadWave(reference_audio_file) validate the returned Wave (e.g., ensure
wave.sample_rate > 0 and wave.samples is non-empty) before assigning
cfg.reference_audio and cfg.reference_sample_rate; if invalid, handle the error
path (log/throw/return or set sane defaults) so
GenerationConfig::reference_sample_rate is never assigned an uninitialized value
— update the code around the ReadWave call and the assignments to
cfg.reference_audio and cfg.reference_sample_rate in the block that constructs
GenerationConfig (where ReadWave, Wave, and
GenerationConfig::reference_sample_rate are used).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@sherpa-onnx/c-api/cxx-api.cc`:
- Around line 500-504: The early-return path creates a GeneratedAudio ans and
returns it without initializing its sample_rate (and potentially other fields);
update both early-return sites (the null-audio checks in the function that
declares GeneratedAudio ans) to set ans.sample_rate to a safe default (e.g., 0)
before returning, and ensure any other relevant members are set to known
defaults so callers do not read indeterminate values from GeneratedAudio.
🧹 Nitpick comments (1)
sherpa-onnx/c-api/cxx-api.cc (1)
560-569: Preferstd::make_sharedfor the newGenerate2overload.It avoids a separate allocation and is the usual C++ idiom.
♻️ Proposed refactor
- GeneratedAudio *ans = new GeneratedAudio; - ans->samples = std::move(audio.samples); - ans->sample_rate = audio.sample_rate; - - return std::shared_ptr<GeneratedAudio>(ans); + auto ans = std::make_shared<GeneratedAudio>(); + ans->samples = std::move(audio.samples); + ans->sample_rate = audio.sample_rate; + return ans;
| GeneratedAudio ans; | ||
|
|
||
| if (!audio) { | ||
| return ans; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the GeneratedAudio struct definition
find . -type f -name "*.h" -o -name "*.cc" | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 968
🏁 Script executed:
# Search for GeneratedAudio struct definition
rg "struct GeneratedAudio|class GeneratedAudio" --type cpp --type cc -A 10Repository: k2-fsa/sherpa-onnx
Length of output: 88
🏁 Script executed:
# Read the specific lines in cxx-api.cc
grep -n "GeneratedAudio" sherpa-onnx/c-api/cxx-api.cc | head -20Repository: k2-fsa/sherpa-onnx
Length of output: 762
🏁 Script executed:
# Read the specific lines around 500-504
sed -n '495,515p' sherpa-onnx/c-api/cxx-api.ccRepository: k2-fsa/sherpa-onnx
Length of output: 657
🏁 Script executed:
# Read the specific lines around 536-540
sed -n '530,545p' sherpa-onnx/c-api/cxx-api.ccRepository: k2-fsa/sherpa-onnx
Length of output: 528
🏁 Script executed:
# Find the GeneratedAudio struct definition
rg "struct GeneratedAudio" --type cpp -A 15Repository: k2-fsa/sherpa-onnx
Length of output: 1991
Initialize GeneratedAudio.sample_rate on early return paths.
The GeneratedAudio struct contains an int32_t sample_rate member without a default constructor. When returning early on the null-audio path with GeneratedAudio ans; return ans;, the sample_rate field is left uninitialized (indeterminate value), leading to undefined behavior if callers read it. Initialize it to a safe default before returning.
Proposed fix
GeneratedAudio ans;
if (!audio) {
+ ans.sample_rate = 0;
return ans;
}This applies to both locations (lines 500-504 and 536-540).
📝 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.
| GeneratedAudio ans; | |
| if (!audio) { | |
| return ans; | |
| } | |
| GeneratedAudio ans; | |
| if (!audio) { | |
| ans.sample_rate = 0; | |
| return ans; | |
| } |
🤖 Prompt for AI Agents
In `@sherpa-onnx/c-api/cxx-api.cc` around lines 500 - 504, The early-return path
creates a GeneratedAudio ans and returns it without initializing its sample_rate
(and potentially other fields); update both early-return sites (the null-audio
checks in the function that declares GeneratedAudio ans) to set ans.sample_rate
to a safe default (e.g., 0) before returning, and ensure any other relevant
members are set to known defaults so callers do not read indeterminate values
from GeneratedAudio.
Summary by CodeRabbit
New Features
API
Testing