Repository navigation
Support passing multiple lexicon files for matcha tts models. - #2765
Conversation
dosubot
Bot
added
the
size:L
|
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. WalkthroughUpdated Matcha TTS lexicon loading to accept comma-separated files and load multiple lexicons; added IsAlphaOrPunct helper and ASCII-aware backward scanning in phrase matching; refined tokenization and punctuation handling; changed OnlinePunctuationModelConfig::debug type from Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Calling Code
participant Lex as MatchaTtsLexicon
participant Init as InitLexicon(string)
participant File as File I/O
Caller->>Lex: Construct with lexicon string
Lex->>Init: InitLexicon(comma_separated_paths)
Init->>Init: Split string by ','
loop for each path
Init->>File: open(path)
File-->>Init: stream/data
Init->>Init: parse tokens & merge lexicon
end
Init-->>Lex: lexicons loaded
Lex-->>Caller: ready
sequenceDiagram
participant PM as PhraseMatcher
participant Check as IsAlphaOrPunct
participant Lex as Lexicon Lookup
PM->>Check: Is current word start ASCII?
alt Non-ASCII start
PM->>PM: Set window (i..i+max_search_len_)
PM->>PM: Backward scan to find candidate end
else ASCII start
PM->>PM: Use current position as start
end
PM->>Lex: Lookup candidate substring
alt Match found
PM->>PM: accept match, advance index, log if debug
else No match
PM->>PM: fallback single-word
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (3)sherpa-onnx/csrc/text-utils.h (1)
sherpa-onnx/csrc/phrase-matcher.cc (1)
sherpa-onnx/csrc/matcha-tts-lexicon.cc (2)
🔇 Additional comments (8)
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 enhances the flexibility and robustness of Matcha TTS models by enabling the use of multiple lexicon files. This change allows for more comprehensive phonetic mappings, especially beneficial for multilingual applications, while also refining text processing and phrase matching logic to improve overall accuracy and debugging capabilities. 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 successfully adds support for passing multiple lexicon files for Matcha TTS models by parsing a comma-separated string of file paths. The changes are logical and cover both configuration and lexicon loading. My review focuses on improving code robustness and correctness. I've pointed out a potential bug in a punctuation mapping, several places where operations on potentially empty strings could lead to undefined behavior, and an unused variable. I've also included suggestions to adhere to modern C++ practices by using C++ standard headers.
| std::string text = _text; | ||
| std::vector<std::pair<std::string, std::string>> replace_str_pairs = { | ||
| {"，", ","}, {":", ","}, {"、", ","}, {"；", ";"}, {"：", ":"}, | ||
| {"，", ","}, {"：", ":"}, {"、", ","}, {"；", ";"}, {"：", ":"}, |
There was a problem hiding this comment.
There seems to be a copy-paste error here. The pair {"：", ":"} is duplicated, and the original {":", ","} has been removed. This is likely unintentional and could affect text normalization. Please restore the original mapping and remove the duplicate.
{"，", ","}, {":", ","}, {"、", ","}, {"；", ";"}, {"：", ":"},| } | ||
| } | ||
|
|
||
| if (isalpha(w.front())) { |
There was a problem hiding this comment.
Calling w.front() on an empty string w would lead to undefined behavior. It's safer to add a check to ensure the string is not empty before accessing its first character.
if (!w.empty() && isalpha(w.front())) {| auto this_word = GetWord(words, start, end); | ||
| if (debug_) { | ||
|
|
||
| if (!isascii(words[i].front())) { |
There was a problem hiding this comment.
Calling words[i].front() on an empty string would lead to undefined behavior. It's safer to add a check to ensure the string is not empty before accessing its first character.
| if (!isascii(words[i].front())) { | |
| if (!words[i].empty() && !isascii(words[i].front())) { |
|
|
||
| while (end > start) { | ||
| auto this_word = GetWord(words, start, end); | ||
| if (isascii(this_word.back())) { |
There was a problem hiding this comment.
The GetWord function can return an empty string. Calling .back() on an empty string results in undefined behavior. Please add a check to ensure this_word is not empty before accessing its last character.
| if (isascii(this_word.back())) { | |
| if (!this_word.empty() && isascii(this_word.back())) { |
|
|
||
| #include "sherpa-onnx/csrc/matcha-tts-lexicon.h" | ||
|
|
||
| #include <ctype.h> |
There was a problem hiding this comment.
For better C++ practice, it's recommended to use C++ headers instead of C headers. Please include <cctype> and use functions from the std namespace, like std::isalpha (used on line 273).
#include <cctype>| private: | ||
| std::vector<int32_t> ConvertWordToIds(const std::string &w) const { | ||
| std::vector<int32_t> ans; | ||
| int32_t space_id = token2id_.at(" "); |
There was a problem hiding this comment.
This space_id variable is initialized but never used. Later, at line 274, token2id_.at(" ") is called again. To avoid the redundant map lookup, please use this space_id variable at line 274.
| // Copyright (c) 2025 Xiaomi Corporation | ||
| #include "sherpa-onnx/csrc/phrase-matcher.h" | ||
|
|
||
| #include <ctype.h> |
There was a problem hiding this comment.
For better C++ practice, it's recommended to use C++ headers instead of C headers. Please include <cctype> and use functions from the std namespace, like std::isascii (used on lines 64 and 69).
| #include <ctype.h> | |
| #include <cctype> |
![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
🧹 Nitpick comments (2)
sherpa-onnx/csrc/matcha-tts-lexicon.cc (1)
7-7: Prefer<cctype>for C++ code.While
<ctype.h>works, the C++ standard recommends using<cctype>for C++ code.Apply this diff:
-#include <ctype.h> +#include <cctype>sherpa-onnx/csrc/phrase-matcher.cc (1)
64-98: Add documentation for the non-ASCII phrase matching algorithm.The new logic introduces complex conditional behavior (ASCII domain checks, backward scanning, candidate filtering) without explanatory comments. This makes the code difficult to understand and maintain.
Consider adding a comment block explaining:
- Why non-ASCII words trigger multi-word matching
- Why candidates ending with ASCII characters are skipped (line 69-72)
- The algorithm's purpose (e.g., correctly handling mixed-language phrases like Chinese text with embedded English words)
Example:
std::string w; + + // For words starting with non-ASCII (e.g., Chinese, Japanese), + // attempt to match multi-word phrases from the lexicon. + // Skip candidates ending with ASCII to avoid breaking up + // mixed-language phrases incorrectly (e.g., "你好world" should + // not match "你好" if "你好world" is in the lexicon). if (!isascii(words[i].front())) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sherpa-onnx/c-api/cxx-api.h(1 hunks)sherpa-onnx/csrc/matcha-tts-lexicon.cc(8 hunks)sherpa-onnx/csrc/offline-tts-matcha-model-config.cc(2 hunks)sherpa-onnx/csrc/phrase-matcher.cc(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sherpa-onnx/csrc/offline-tts-matcha-model-config.cc (2)
sherpa-onnx/csrc/matcha-tts-lexicon.cc (2)
lexicon(326-338)lexicon(326-326)sherpa-onnx/csrc/text-utils.cc (2)
SplitStringToVector(130-142)SplitStringToVector(130-132)
sherpa-onnx/csrc/matcha-tts-lexicon.cc (3)
sherpa-onnx/csrc/kokoro-multi-lang-lexicon.cc (12)
InitLexicon(484-497)InitLexicon(484-484)lexicon(470-481)lexicon(470-470)w(178-211)w(178-178)is(433-433)is(445-468)is(445-445)is(478-478)is(499-543)is(499-499)sherpa-onnx/csrc/text-utils.cc (3)
SplitStringToVector(130-142)SplitStringToVector(130-132)i(114-114)sherpa-onnx/csrc/character-lexicon.cc (8)
w(194-225)w(194-194)is(47-47)is(52-52)is(227-258)is(227-227)is(260-312)is(260-260)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: rknn shared OFF
- GitHub Check: rknn shared ON
- GitHub Check: Debug shared tts-ON
- GitHub Check: Debug shared-ON tts-ON
- GitHub Check: Release shared tts-ON
- GitHub Check: swift (macos-latest)
- GitHub Check: Debug shared-OFF tts-OFF
- GitHub Check: Release shared-OFF tts-OFF
- GitHub Check: Debug shared-ON tts-OFF
- GitHub Check: swift (macos-13)
- GitHub Check: Release shared-OFF tts-ON
- GitHub Check: ubuntu-24.04 3.12
- GitHub Check: ubuntu-24.04 3.13
- GitHub Check: ubuntu-24.04 3.11
- GitHub Check: Release shared-ON tts-ON
- GitHub Check: ubuntu-24.04 3.10
- GitHub Check: ubuntu-24.04 3.9
- GitHub Check: ubuntu-24.04 3.8
- GitHub Check: ascend (gpustack/devel-ascendai-cann:8.0.rc3.beta1-310p-ubuntu20.04-v2, 8.0.0-310p)
🔇 Additional comments (10)
sherpa-onnx/c-api/cxx-api.h (1)
745-745: LGTM! Type correctness improvement.The change from
int32_ttoboolfor the debug field is appropriate and improves type safety.sherpa-onnx/csrc/offline-tts-matcha-model-config.cc (3)
11-11: LGTM!The include is necessary for the
SplitStringToVectorfunction used in the validation logic.
19-22: LGTM! Clear documentation of multi-file support.The help text clearly explains that multiple lexicon files can be passed as a comma-separated list, which will help users understand the new capability.
88-99: LGTM! Robust validation logic.The validation correctly splits the comma-separated lexicon string and verifies the existence of each file individually, providing clear error messages if any file is missing.
sherpa-onnx/csrc/matcha-tts-lexicon.cc (6)
62-62: LGTM! Cleaner multi-file lexicon loading.The change from directly opening a file stream to calling the new
InitLexicon(const std::string &)method enables multi-file lexicon support consistently.
88-95: LGTM! Consistent multi-file loading implementation.The template constructor correctly splits the comma-separated lexicon string and loads each file individually, consistent with the non-template constructor implementation.
108-108: LGTM! Removed duplicate mapping.The duplicate
{"：", ":"}entry has been correctly removed from the punctuation mapping list.
210-220: LGTM! Helpful debug logging.The debug logging for sentence boundaries provides useful visibility into the tokenization process.
273-275: Verify the space appending behavior for alphabetic words.The code now appends a space token to the end of the token ID sequence when a word starts with an alphabetic character. This changes the tokenization behavior and could affect TTS output quality.
Ensure this behavior is intentional and that:
- It correctly handles mixed Chinese/English text
- It doesn't create unexpected pauses or artifacts in the generated speech
- The space is appended at the correct position relative to other processing
326-338: LGTM! Well-structured multi-file lexicon loading.The new
InitLexicon(const std::string &)overload correctly implements multi-file lexicon loading by splitting the comma-separated string and loading each file individually. This approach is consistent with similar implementations in the codebase (e.g.,kokoro-multi-lang-lexicon.cc).
| #include <ctype.h> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Consider portability of isascii().
The isascii() function is a POSIX extension, not standard C++. If the codebase targets non-POSIX platforms (e.g., certain embedded systems, Windows without POSIX compatibility layer), this may cause compilation failures.
Consider using a standard alternative:
-#include <ctype.h>
-
+// Helper for ASCII check (standard C++ compatible)
+inline bool IsAsciiChar(char c) {
+ return static_cast<unsigned char>(c) <= 127;
+}Then replace isascii() calls with IsAsciiChar().
Alternatively, verify that all target platforms support POSIX isascii():
🏁 Script executed:
#!/bin/bash
# Check if the codebase already uses other POSIX-specific functions
rg -n 'isascii|isalpha|isdigit|isalnum' --type cpp -g '!*test*' -C 2Length of output: 2662
---
🏁 Script executed:
```shell
#!/bin/bash
# Check phrase-matcher.cc includes and look for platform/build configuration
head -20 sherpa-onnx/csrc/phrase-matcher.cc
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Check if codebase has platform-specific configurations
fd -t f '(CMakeLists|Makefile|BUILD|bazel)' --type f | head -20Length of output: 907
🏁 Script executed:
#!/bin/bash
# Check how other files handle character checking for consistency
rg -n 'std::isalpha|std::isdigit' sherpa-onnx/csrc/phrase-matcher.ccLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Find GetWord() implementation to check if it can return empty strings
rg -n 'GetWord\s*\(' sherpa-onnx/csrc/phrase-matcher.cc -A 10
Length of output: 354
🏁 Script executed:
#!/bin/bash
# Check CMakeLists.txt for platform support and POSIX requirement statements
grep -i 'posix\|platform\|windows\|embed' CMakeLists.txt | head -20Length of output: 122
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for GetWord() function definition in the file
rg -n 'std::string.*GetWord|GetWord.*{' sherpa-onnx/csrc/phrase-matcher.cc -A 15
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Also check phrase-matcher.h for GetWord signature
cat sherpa-onnx/csrc/phrase-matcher.h | grep -A 10 'GetWord'Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for GetWord method definition with simpler pattern
rg -n 'GetWord' sherpa-onnx/csrc/phrase-matcher.cc -B 2 -A 8
Length of output: 330
🏁 Script executed:
#!/bin/bash
# View the complete phrase-matcher.cc to understand full structure
cat sherpa-onnx/csrc/phrase-matcher.ccLength of output: 3618
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for GetWord definition in header and related files
rg -n 'GetWord' sherpa-onnx/csrc/phrase-matcher.h
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for GetWord in all cpp/h files in csrc
fd -e cpp -e h . sherpa-onnx/csrc | xargs grep -l 'GetWord' 2>/dev/nullLength of output: 29
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for GetWord definition in text-utils
rg -n 'GetWord' sherpa-onnx/csrc/text-utils.h -B 2 -A 12
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Also check if text-utils.h is included in phrase-matcher.cc
grep -n 'text-utils' sherpa-onnx/csrc/phrase-matcher.ccLength of output: 43
---
🏁 Script executed:
```shell
#!/bin/bash
# Find GetWord implementation in text-utils.cc
rg -n 'std::string GetWord' sherpa-onnx/csrc/text-utils.cc -A 20
Length of output: 594
Replace isascii() with standard C++ alternative for cross-platform compatibility.
The isascii() function is a POSIX extension, not part of the C++ standard library. While other files use std::isdigit() and std::isalpha(), this file uses bare isascii(), creating an inconsistency. On platforms without POSIX support (e.g., certain embedded systems, Windows without POSIX compatibility), this will cause compilation failures.
Recommended fix:
Replace #include <ctype.h> with a standard-compatible ASCII check:
-#include <ctype.h>
+// Helper for standard C++ compatible ASCII checking
+inline bool IsAsciiChar(char c) {
+ return static_cast<unsigned char>(c) <= 127;
+}Then replace isascii() calls at lines 64 and 69 with IsAsciiChar(). Additionally, add defensive empty-string checks before .front() and .back() calls to avoid undefined behavior on edge cases.
Committable suggestion skipped: line range outside the PR's diff.
| auto this_word = GetWord(words, start, end); | ||
| if (debug_) { | ||
|
|
||
| if (!isascii(words[i].front())) { |
There was a problem hiding this comment.
Add empty string checks to prevent undefined behavior.
Calling .front() on line 64 and .back() on line 69 without checking if the strings are empty invokes undefined behavior and can cause crashes.
Apply this diff to add safety checks:
- if (!isascii(words[i].front())) {
+ if (!words[i].empty() && !isascii(words[i].front())) {
int32_t end = std::min(i + max_search_len_ - 1, num_words - 1);
while (end > start) {
auto this_word = GetWord(words, start, end);
- if (isascii(this_word.back())) {
+ if (this_word.empty() || isascii(this_word.back())) {
--end;
continue;
}Also applies to: 69-69
🤖 Prompt for AI Agents
In sherpa-onnx/csrc/phrase-matcher.cc around lines 64 and 69, the code calls
words[i].front() and words[i].back() without verifying the string is non-empty,
which is undefined behavior; add checks that words[i].empty() is false (or
!words[i].empty()) before calling .front() or .back(), and skip or handle empty
strings appropriately (e.g., continue the loop or treat them as
non-ascii/mismatch) so the front()/back() calls are only executed on non-empty
strings.
See also #2763
Summary by CodeRabbit
New Features
Improvements