Guard ScaleSilence against invalid silence scales - #3745
Conversation
dosubot
Bot
added
the
size:S
📝 WalkthroughWalkthrough
ChangesSilence scaling validation
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 |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sherpa-onnx/csrc/offline-tts.cc`:
- Around line 82-95: Use double-precision arithmetic for the silence-length
scaling in the interval-processing logic, replacing the float-based
multiplication before the int32_t conversion. Update the code associated with
the scaling operation (including the expression using len) so it matches the
double-based overflow guard and prevents out-of-range conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9858fbbb-ed90-4454-8815-6a5870995b6f
📒 Files selected for processing (1)
sherpa-onnx/csrc/offline-tts.cc
| // A finite but very large scale can still overflow the int32_t conversion | ||
| // below, so check the longest interval before scaling anything. | ||
| int32_t max_len = 0; | ||
| for (const auto &interval : intervals) { | ||
| max_len = std::max(max_len, interval.end - interval.start); | ||
| } | ||
|
|
||
| if (static_cast<double>(max_len) * scale > | ||
| std::numeric_limits<int32_t>::max()) { | ||
| SHERPA_ONNX_LOGE("Silence scale %f is too large. Skip scaling silence.", | ||
| scale); | ||
| return *this; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Overflow check uses double but the computation at line 106 uses float — narrow UB window remains.
The guard at line 89 checks static_cast<double>(max_len) * scale > INT32_MAX, but the actual scaling at line 106 computes static_cast<int32_t>(len * scale) where len (int32_t) is promoted to float for the multiplication. For len > 2²⁴ (~16M samples), static_cast<float>(len) can round up, making float(len) * scale exceed INT32_MAX even when double(len) * scale does not. This leaves a narrow path to the exact UB the PR aims to prevent.
Fix: use double at line 106 to match the guard's arithmetic. Since int32_t→double is always exact and len ≤ max_len, the guard becomes a perfect bound.
🔧 Proposed fix for line 106
- int32_t n = static_cast<int32_t>(len * scale);
+ int32_t n = static_cast<int32_t>(static_cast<double>(len) * scale);📝 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.
| // A finite but very large scale can still overflow the int32_t conversion | |
| // below, so check the longest interval before scaling anything. | |
| int32_t max_len = 0; | |
| for (const auto &interval : intervals) { | |
| max_len = std::max(max_len, interval.end - interval.start); | |
| } | |
| if (static_cast<double>(max_len) * scale > | |
| std::numeric_limits<int32_t>::max()) { | |
| SHERPA_ONNX_LOGE("Silence scale %f is too large. Skip scaling silence.", | |
| scale); | |
| return *this; | |
| } | |
| int32_t n = static_cast<int32_t>(static_cast<double>(len) * scale); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/offline-tts.cc` around lines 82 - 95, Use double-precision
arithmetic for the silence-length scaling in the interval-processing logic,
replacing the float-based multiplication before the int32_t conversion. Update
the code associated with the scaling operation (including the expression using
len) so it matches the double-based overflow guard and prevents out-of-range
conversion.
|
Can you limit the value of The motivation of using scale is to reduce long pauses in the generated speech and the expected value of scale is usually in the range The current changes make the code complicated. |
n below is computed as interval_length * scale and converted to an int32_t. Converting NaN or infinity is undefined behaviour, and a large finite scale overflows the conversion. In practice each of these aborts the process: --tts-silence-scale=nan terminate called after throwing std::length_error --tts-silence-scale=inf terminate called after throwing std::length_error --tts-silence-scale=1e9 terminate called after throwing std::length_error Since scale exists to shorten long pauses and is normally within (0, 1), reject anything outside [0.01, 2], log, and return the audio unscaled. Any comparison with NaN is false, so a single range check rejects NaN too. Negative values were already rejected by OfflineTtsConfig::Validate(), which requires silence_scale >= 0.001. NaN passed that check, because NaN < 0.001 is false, and callers of the Generate API pass GeneratedAudioConfig::silence_scale directly and bypass Validate() entirely.
756374f to
355d51c
Compare
|
Thanks — good call, that's much simpler. Replaced my version with the range check you suggested: log and return unless Since any comparison with NaN is false, that single check also covers One detail: the literals are @coderabbitai the float/double mismatch you flagged is moot now — with Verified: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd offline-tts.cc --exec cat -n {} \; | sed -n '1,80p'Length of output: 3569 🏁 Script executed: #!/bin/bash
fd offline-tts.cc sherpa-onnx/csrc --exec cat -n {} \;Length of output: 25010
if (!(scale >= 0.01f && scale <= 2.0f)) {
SHERPA_ONNX_LOGE("Silence scale %f is not in [0.01, 2]. Skip scaling.",
scale);
return *this;
}Using ✏️ Learnings added
|
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/csrc/offline-tts.cc (1)
94-94: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winInteger overflow in
static_cast<int32_t>(len * scale)— no overflow guard exists.The PR summary states "skips scaling when a finite scale would overflow that type," but the code has no such check. The range [0.01, 2.0] bounds
scalebut not the productlen * scale. Withscale = 2.0andlen > INT32_MAX / 2(~1.07 billion samples, ~6.7 hours at 48 kHz), the product exceedsINT32_MAXand thestatic_cast<int32_t>is undefined behavior. If the result wraps negative,n <= lenat line 96 is true, causing iterator UB at lines 97–98; orn - lenat line 106 converts to a hugesize_t, triggering a massive allocation.Additionally,
len(int32_t) is promoted tofloatfor the multiplication, losing precision forlen > 2²⁴(~16M). This was flagged in a prior review and remains unaddressed.Suggested fix: use
doublearithmetic and add an overflow guard.🔧 Proposed fix
int32_t len = interval.end - interval.start; - int32_t n = static_cast<int32_t>(len * scale); + double product = static_cast<double>(len) * static_cast<double>(scale); + if (product > static_cast<double>(INT32_MAX)) { + // Skip scaling for this interval to avoid overflow. + ans.samples.insert(ans.samples.end(), samples.begin() + interval.start, + samples.begin() + interval.end); + continue; + } + int32_t n = static_cast<int32_t>(product);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sherpa-onnx/csrc/offline-tts.cc` at line 94, Fix the sample-count calculation around n by using double-precision arithmetic for len * scale, checking that the result is finite and within the int32_t range before casting, and skipping scaling or handling the overflow safely when it is not. Preserve the existing behavior for valid results while preventing invalid iterator ranges or oversized allocations in the subsequent n <= len and resize logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sherpa-onnx/csrc/offline-tts.cc`:
- Around line 44-48: Align OfflineTtsConfig::Validate() with ScaleSilence by
validating silence_scale within the inclusive range [0.01, 2.0], rejecting
values below 0.01 or above 2.0 and reporting the validation error consistently.
Update the existing silence_scale validation logic in
OfflineTtsConfig::Validate() so all values accepted by configuration are handled
by ScaleSilence without being silently skipped.
- Around line 44-48: Update the range validation in ScaleSilence to allow scale
= 0 by changing the lower bound from 0.01f to 0.0f, preserving the existing
behavior that removes silence intervals when scaling by zero.
---
Outside diff comments:
In `@sherpa-onnx/csrc/offline-tts.cc`:
- Line 94: Fix the sample-count calculation around n by using double-precision
arithmetic for len * scale, checking that the result is finite and within the
int32_t range before casting, and skipping scaling or handling the overflow
safely when it is not. Preserve the existing behavior for valid results while
preventing invalid iterator ranges or oversized allocations in the subsequent n
<= len and resize logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51682f94-2ec9-45e5-8f6d-d16fbe0cbab4
📒 Files selected for processing (1)
sherpa-onnx/csrc/offline-tts.cc
| if (!(scale >= 0.01f && scale <= 2.0f)) { | ||
| SHERPA_ONNX_LOGE("Silence scale %f is not in [0.01, 2]. Skip scaling.", | ||
| scale); | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Config validation range [0.001, ∞) doesn't match ScaleSilence range [0.01, 2.0].
OfflineTtsConfig::Validate() at line 224 only rejects silence_scale < 0.001 with no upper bound, but ScaleSilence rejects values outside [0.01, 2.0]. Values in [0.001, 0.01) or above 2.0 pass config validation but are silently no-op'd at runtime — the user gets unscaled audio with no indication beyond a log message.
Align the two ranges to avoid surprising behavior:
🔧 Proposed fix for Validate()
- if (silence_scale < 0.001) {
- SHERPA_ONNX_LOGE("--tts-silence-scale '%.3f' is too small", silence_scale);
+ if (silence_scale < 0.01f || silence_scale > 2.0f) {
+ SHERPA_ONNX_LOGE("--tts-silence-scale '%.3f' is not in [0.01, 2]",
+ silence_scale);
return false;
}Also applies to: 224-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/offline-tts.cc` around lines 44 - 48, Align
OfflineTtsConfig::Validate() with ScaleSilence by validating silence_scale
within the inclusive range [0.01, 2.0], rejecting values below 0.01 or above 2.0
and reporting the validation error consistently. Update the existing
silence_scale validation logic in OfflineTtsConfig::Validate() so all values
accepted by configuration are handled by ScaleSilence without being silently
skipped.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Range check rejects scale = 0, breaking existing pause-removal behavior.
The PR summary states "Scale 0 retains its existing behavior of removing pauses," but 0 < 0.01f fails the range check, so ScaleSilence(0) now logs an error and returns unscaled audio with pauses intact. Previously, scale = 0 produced n = 0, effectively removing all silence intervals.
If scale = 0 should still remove pauses, lower the bound to 0.0f:
🔧 Proposed fix
- if (!(scale >= 0.01f && scale <= 2.0f)) {
- SHERPA_ONNX_LOGE("Silence scale %f is not in [0.01, 2]. Skip scaling.",
+ if (!(scale >= 0.0f && scale <= 2.0f)) {
+ SHERPA_ONNX_LOGE("Silence scale %f is not in [0, 2]. Skip scaling.",
scale);If rejecting scale = 0 is intentional, the PR summary should be corrected to reflect this behavior change.
📝 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 (!(scale >= 0.01f && scale <= 2.0f)) { | |
| SHERPA_ONNX_LOGE("Silence scale %f is not in [0.01, 2]. Skip scaling.", | |
| scale); | |
| return *this; | |
| } | |
| if (!(scale >= 0.0f && scale <= 2.0f)) { | |
| SHERPA_ONNX_LOGE("Silence scale %f is not in [0, 2]. Skip scaling.", | |
| scale); | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sherpa-onnx/csrc/offline-tts.cc` around lines 44 - 48, Update the range
validation in ScaleSilence to allow scale = 0 by changing the lower bound from
0.01f to 0.0f, preserving the existing behavior that removes silence intervals
when scaling by zero.
2 participants
Follow-up to #3744. A review comment on that PR pointed out that a
negative or NaN
scalecould produce a negativen. It was partly correct:there is undefined behaviour here, but not quite where it said, and the
suggested fix would not have removed it.
What is actually wrong
ScaleSilencecomputesConverting NaN or infinity to
int32_tis undefined, and a finite but verylarge
scaleoverflows the conversion. All three abort the process today:The bot suggested clamping
nto0after the cast. That would not help:the undefined behaviour is at the cast, so NaN and infinity still get through,
and it does not address overflow at all.
Negative values are already handled.
OfflineTtsConfig::Validate()requiressilence_scale >= 0.001and the CLI calls it, so--tts-silence-scale=-1isrejected cleanly. But:
NaN < 0.001isfalse, so NaN passesValidate().GenerateAPI passGeneratedAudioConfig::silence_scaledirectly and bypass
Validate()entirely.The change
Reject any scale outside
[0.01, 2], log, and return the audio unscaled ratherthan aborting.
Edited after review: the merged version uses the single range check
@csukuangfj suggested, not the guard originally proposed here. Note that
scale == 0is now rejected too; it was already unreachable from the CLI, sinceValidate()requiressilence_scale >= 0.001, but a caller of theGenerateAPI could previously pass
0to remove pauses.Before / after
Same build,
kokoro-multi-lang-v1_0,--sid=10,"First sentence here. Second sentence follows."--tts-silence-scalenanstd::length_error)infstd::length_error)1e9std::length_error)-infValidate()Validate()-1Validate()Validate()2.01.0Checked with
clang-format --dry-run --Werrorusing the repo's own.clang-format; it reports no changes.Summary by CodeRabbit