Repository navigation
fix matcha tts zh-en model - #2851
Conversation
dosubot
Bot
added
the
size:S
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR modifies lexicon generation and TTS processing for Matcha TTS. Changes include switching from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
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 addresses an issue within the Matcha TTS Chinese-English model by refining its lexicon generation and token processing mechanisms. The core changes involve migrating the Python script responsible for lexicon creation to a more precise pinyin conversion method, adjusting C++ code to remove a potentially redundant space token insertion, and integrating new debug logging capabilities for token sequences to facilitate future troubleshooting. The author explicitly requests the regeneration of 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 aims to fix the Matcha TTS zh-en model by updating the lexicon generation script and C++ implementation. However, I've identified two critical issues that need to be addressed. First, the Python script for lexicon generation contains a bug that will cause it to crash due to incorrect handling of the data structure returned by the pinyin library. Second, in the C++ code, essential logic for preparing model inputs has been mistakenly moved into a debug-only conditional block, which will cause the TTS generation to fail in non-debug builds. Both of these issues are critical and will prevent the code from functioning as intended.
| for i in range(len(tokens)): | ||
| if tokens[i] == "shei2": | ||
| tokens[i] = "shui2" | ||
|
|
||
| if tokens[i][-1] not in ("1", "2", "3", "4", "5"): | ||
| tokens[i] += "1" | ||
|
|
||
| tokens = " ".join(tokens) | ||
| flatten = [t[0] for t in tokens] | ||
|
|
||
| tokens = " ".join(flatten) |
There was a problem hiding this comment.
The logic for processing pinyin tokens is incorrect after switching from lazy_pinyin to pinyin. The pinyin function returns a list of lists (e.g., [['pin1'], ['yin1']]), so tokens[i] in your loop is a list, not a string. The current loop attempts to perform string operations on this list, which will raise a TypeError and cause the script to fail.
The logic needs to be refactored to correctly handle the nested list structure returned by pinyin.
| for i in range(len(tokens)): | |
| if tokens[i] == "shei2": | |
| tokens[i] = "shui2" | |
| if tokens[i][-1] not in ("1", "2", "3", "4", "5"): | |
| tokens[i] += "1" | |
| tokens = " ".join(tokens) | |
| flatten = [t[0] for t in tokens] | |
| tokens = " ".join(flatten) | |
| processed_tokens = [] | |
| for t_list in tokens: | |
| t = t_list[0] | |
| if t == "shei2": | |
| t = "shui2" | |
| if t[-1] not in ("1", "2", "3", "4", "5"): | |
| t += "1" | |
| processed_tokens.append(t) | |
| tokens = " ".join(processed_tokens) |
| if (config_.model.debug) { | ||
| for (const auto &k : tokens) { | ||
| x.insert(x.end(), k.begin(), k.end()); | ||
| } | ||
| std::ostringstream oss; | ||
| for (int32_t i : x) { | ||
| oss << i << ", "; | ||
| } | ||
| oss << "\n"; | ||
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | ||
| } |
There was a problem hiding this comment.
The logic for populating the x vector, which is the input to the model, has been mistakenly moved inside a debug-only block. This means that when config_.model.debug is false (i.e., in a release build), x will remain empty. Running the model with an empty input tensor will lead to incorrect audio generation or a crash.
The x vector must be populated regardless of whether debug mode is enabled.
| if (config_.model.debug) { | |
| for (const auto &k : tokens) { | |
| x.insert(x.end(), k.begin(), k.end()); | |
| } | |
| std::ostringstream oss; | |
| for (int32_t i : x) { | |
| oss << i << ", "; | |
| } | |
| oss << "\n"; | |
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | |
| } | |
| for (const auto &k : tokens) { | |
| x.insert(x.end(), k.begin(), k.end()); | |
| } | |
| if (config_.model.debug) { | |
| std::ostringstream oss; | |
| for (int32_t i : x) { | |
| oss << i << ", "; | |
| } | |
| oss << "\n"; | |
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/matcha-tts/zh-en/generate_lexicon.py(3 hunks)sherpa-onnx/csrc/matcha-tts-lexicon.cc(0 hunks)sherpa-onnx/csrc/offline-tts-matcha-impl.h(1 hunks)
💤 Files with no reviewable changes (1)
- sherpa-onnx/csrc/matcha-tts-lexicon.cc
🔇 Additional comments (3)
scripts/matcha-tts/zh-en/generate_lexicon.py (3)
31-31: LGTM!The change to
pinyin()with[0][0]access correctly extracts the pinyin string from the nested list structure. Theneutral_tone_with_five=Trueparameter ensures neutral tones are explicitly marked, which aligns with the tone normalization logic at lines 36-37.
55-57: LGTM with dependency on loop fix!The flattening logic correctly extracts the first element from each sublist before joining. This is necessary because
pinyin()returns nested lists. However, this logic depends on the loop at lines 48-53 being fixed to properly modify the nested structure.
3-3: Confirm the pypinyin API change fromlazy_pinyintopinyin.The import update is correct. The
pinyin()function returns a list of lists (e.g.,[['zhōng'], ['xīn']]), requiring nested access patterns like[0][0]to get the first pinyin of the first character. It supports theneutral_tone_with_fiveparameter to mark neutral tones with "5", and flattening with[t[0] for t in result]extracts one pinyin per character as intended for phrase processing.
| tokens = pinyin(key, style=Style.TONE3, neutral_tone_with_five=True) | ||
| for i in range(len(tokens)): | ||
| if tokens[i] == "shei2": | ||
| tokens[i] = "shui2" | ||
|
|
||
| if tokens[i][-1] not in ("1", "2", "3", "4", "5"): | ||
| tokens[i] += "1" | ||
|
|
There was a problem hiding this comment.
Verify the loop logic with nested list structure.
After changing to pinyin(), the return value is a nested list (e.g., [['ni3'], ['hao3']]), but the loop at lines 48-53 accesses tokens[i] as if it were a string (checking tokens[i][-1] and comparing with strings). This suggests the loop should access tokens[i][0] instead of tokens[i].
Apply this diff to fix the nested list access:
tokens = pinyin(key, style=Style.TONE3, neutral_tone_with_five=True)
for i in range(len(tokens)):
- if tokens[i] == "shei2":
- tokens[i] = "shui2"
+ if tokens[i][0] == "shei2":
+ tokens[i][0] = "shui2"
- if tokens[i][-1] not in ("1", "2", "3", "4", "5"):
- tokens[i] += "1"
+ if tokens[i][0][-1] not in ("1", "2", "3", "4", "5"):
+ tokens[i][0] += "1"🤖 Prompt for AI Agents
In scripts/matcha-tts/zh-en/generate_lexicon.py around lines 47 to 54, tokens
returned by pinyin() are nested lists (e.g., [['ni3'], ['hao3']]) but the code
treats tokens[i] as a string; update the logic to operate on the inner string
tokens[i][0] (read, modify, then assign back into tokens[i][0]) so checks like
equality to "shei2" and the final-tone check/append use tokens[i][0] and write
modifications back into the nested list.
| if (config_.model.debug) { | ||
| for (const auto &k : tokens) { | ||
| x.insert(x.end(), k.begin(), k.end()); | ||
| } | ||
| std::ostringstream oss; | ||
| for (int32_t i : x) { | ||
| oss << i << ", "; | ||
| } | ||
| oss << "\n"; | ||
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | ||
| } |
There was a problem hiding this comment.
Critical bug: Token vector x is empty when debug mode is off.
The conditional at line 434 makes the population of vector x dependent on config_.model.debug. When debug is false (typical in production), x remains empty, causing an empty tensor to be passed to the model at line 455. This breaks audio generation entirely.
Apply this diff to fix the issue by separating token aggregation from debug logging:
std::vector<int64_t> x;
x.reserve(num_tokens);
-if (config_.model.debug) {
- for (const auto &k : tokens) {
- x.insert(x.end(), k.begin(), k.end());
- }
+for (const auto &k : tokens) {
+ x.insert(x.end(), k.begin(), k.end());
+}
+
+if (config_.model.debug) {
std::ostringstream oss;
for (int32_t i : x) {
oss << i << ", ";
}
oss << "\n";
SHERPA_ONNX_LOGE("%s\n", oss.str().c_str());
}📝 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 (config_.model.debug) { | |
| for (const auto &k : tokens) { | |
| x.insert(x.end(), k.begin(), k.end()); | |
| } | |
| std::ostringstream oss; | |
| for (int32_t i : x) { | |
| oss << i << ", "; | |
| } | |
| oss << "\n"; | |
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | |
| } | |
| for (const auto &k : tokens) { | |
| x.insert(x.end(), k.begin(), k.end()); | |
| } | |
| if (config_.model.debug) { | |
| std::ostringstream oss; | |
| for (int32_t i : x) { | |
| oss << i << ", "; | |
| } | |
| oss << "\n"; | |
| SHERPA_ONNX_LOGE("%s\n", oss.str().c_str()); | |
| } |
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/offline-tts-matcha-impl.h around lines 434 to 444, the token
aggregation into vector x is currently inside the debug-only conditional which
leaves x empty when debug is false and results in an empty tensor being passed
later; move the loop that appends tokens into x so it always runs (populate x
unconditionally before any debug checks), and keep only the ostringstream
construction and SHERPA_ONNX_LOGE call inside the config_.model.debug guard;
optionally reserve x size before the append to avoid reallocations.
1 participant
Please regenerate the lexicon.txt
or download the lexicon.txt below.
lexicon.txt
Fixes #2847
CC @lkocok
Test script
test_onnx_20251203.py
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.