Fix process abort when espeak-ng rejects the voice (e.g. invalid --kokoro-lang)#3749
Fix process abort when espeak-ng rejects the voice (e.g. invalid --kokoro-lang)#3749ekenberg wants to merge 1 commit into
Conversation
piper::phonemize_eSpeak() throws std::runtime_error when espeak_SetVoiceByName() fails, for example sherpa-onnx-offline-tts --kokoro-lang=en-gb ... (the espeak-ng data shipped with kokoro-multi-lang-v1_0 contains en-GB-x-rp and other en-GB variants, but no plain en-GB). Nothing catches the exception on its way out of Generate(), so the process dies with SIGABRT after the model has already been loaded -- and a library user of the C/C++ API is taken down with it. Catch it in CallPhonemizeEspeak(), the single espeak boundary shared by the kokoro, piper and matcha frontends: log the text, the voice and the reason, and return no phonemes. The callers already handle empty phoneme lists, so generation fails with the usual "Error in generating audio" message and a non-zero exit code, consistent with how other invalid generation parameters are treated.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPhonemization error handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request wraps the call to piper::phonemize_eSpeak in a try-catch block to handle exceptions when an unsupported voice is configured, preventing process termination. The reviewer recommends adding a defensive null check for the phonemes pointer before dereferencing it to avoid potential crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // keep multi threads from calling into piper::phonemize_eSpeak | ||
| std::lock_guard<std::mutex> lock(espeak_mutex); |
There was a problem hiding this comment.
To prevent potential undefined behavior or crashes from dereferencing a null pointer, it is highly recommended to add a defensive null check for the phonemes pointer before locking the mutex and calling piper::phonemize_eSpeak.
if (!phonemes) {
SHERPA_ONNX_LOGE("phonemes pointer is null");
return;
}
// keep multi threads from calling into piper::phonemize_eSpeak
std::lock_guard<std::mutex> lock(espeak_mutex);There was a problem hiding this comment.
All callers pass the address of a local vector, so phonemes cannot be null here — and the surrounding code does not null-check output parameters either. Happy to add it if the maintainer prefers.
Problem
An unsupported
--kokoro-langvalue kills the whole process:Easy to hit innocently: the espeak-ng data in
kokoro-multi-lang-v1_0hasen-GB-x-rp,en-GB-scotlandetc., but no plainen-GB— so asking for British English dumps core, after the model has already been loaded.Cause
piper::phonemize_eSpeak()throwsstd::runtime_errorwhenespeak_SetVoiceByName()fails (piper-phonemizesrc/phonemize.cpp)Generate()Fix
Catch it in
CallPhonemizeEspeak()— the single espeak boundary shared by the kokoro, matcha and piper frontends. Log the text, the voice and the reason, return no phonemes. The callers already handle empty phoneme lists, so generation fails the same way other invalid parameters do:Exit code 1, no core dump.
Tested
Linux x86_64, kokoro-multi-lang-v1_0:
--kokoro-lang=en-gb,--kokoro-lang=xyzzy: SIGABRT before, clean exit 1 after--kokoro-lang=fr,--kokoro-lang=en-GB-x-rp, default (no lang): unchanged, audio verifiedTwo notes on scope.
std::exceptionis caught rather than onlystd::runtime_error, since anything escaping through this boundary kills the process the same way. And validating the voice name up front (atValidate()time, before the model loads) would give an even earlier error but needs espeak initialized first — happy to do that as a follow-up if you think it's worth it.Summary by CodeRabbit