Fix ScaleSilence copying past the pause interval when scale > 1 - #3744
Conversation
GeneratedAudio::ScaleSilence() lengthens each detected pause by copying n = (interval.end - interval.start) * scale samples starting at interval.start. When scale > 1 that is n > interval.end - interval.start, so the copy runs past the end of the pause and into the audio that follows it. Two consequences: - The beginning of the next word is duplicated, which is audible as a stutter after every pause. - On the last interval, interval.end == num_samples, so samples.begin() + interval.start + n is past samples.end() and the insert reads out of bounds. Copy the pause once and pad with zeros to the requested length instead. For scale <= 1 the behaviour is unchanged, and scale == 1 still returns early before reaching this code.
dosubot
Bot
added
the
size:S
|
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
ChangesSilence scaling
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 |
![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 updates the silence scaling logic in GeneratedAudio::ScaleSilence to prevent out-of-bounds reads and audio artifacts when the scale factor is greater than 1 by copying the pause once and appending silence. The reviewer identified a potential issue where a negative or NaN scale factor could result in a negative sample count, leading to undefined behavior, and suggested clamping the sample count to a minimum of zero.
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.
| int32_t len = interval.end - interval.start; | ||
| int32_t n = static_cast<int32_t>(len * scale); | ||
|
|
||
| if (n <= len) { |
There was a problem hiding this comment.
If scale is negative or NaN, n can be negative (or a large negative value like INT_MIN). This would make n <= len true, and the iterator arithmetic samples.begin() + interval.start + n would point to a memory location before samples.begin() + interval.start, leading to undefined behavior or a crash during std::vector::insert.
To prevent this, we should clamp n to be at least 0.
int32_t len = interval.end - interval.start;
int32_t n = static_cast<int32_t>(len * scale);
if (n < 0) {
n = 0;
}
if (n <= len) {
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
2 participants
GeneratedAudio::ScaleSilence()lengthens each detected pause by copyingn = (interval.end - interval.start) * scalesamples starting atinterval.start:When
scale > 1,nexceeds the length of the interval, so the copy runs pastthe end of the pause. Three consequences:
stutter after every pause.
end of the buffer then
interval.end == num_samples, andsamples.begin() + interval.start + nis pastsamples.end().scaling silently does not happen.
Copying the pause once and padding with zeros gives the intended behaviour. For
scale <= 1nothing changes, andscale == 1still returns early beforereaching this code.
Measured
Built at
97293f0, Kokorokokoro-multi-lang-v1_0,--sid=10, text"First sentence here. Second sentence follows. Third one ends it."Same binary before and after the patch,
--tts-silence-scale=2.0, comparedagainst the
--tts-silence-scale=1.0output as reference (scale 1 returns early,so it is unscaled ground truth). Counting samples above the same
0.01thresholdScaleSilenceitself uses to detect silence:scale=1.0scale=2.0scale=2.0The unpatched build invents 28379 speech-like samples — 1.18 s of audio that
the reference does not contain. That is duplicated speech pasted into the pauses.
The patched build adds silence only; the +102 residue is boundary rounding.
Applying the same pause detector to the outputs is another way to see it: the
reference has 4 pauses and the patched output has 4, but the unpatched output
reports 7 — the duplicated words split each pause into two.
The out-of-bounds read (2) follows by construction from
interval.end == num_samples; it was not separately exercised under a sanitizer.Reproducing
sherpa-onnx-offline-tts \ --kokoro-model=model.onnx --kokoro-voices=voices.bin \ --kokoro-tokens=tokens.txt --kokoro-data-dir=espeak-ng-data \ --kokoro-lexicon=lexicon-us-en.txt \ --sid=10 --tts-silence-scale=2.0 \ --output-filename=out.wav \ "First sentence here. Second sentence follows. Third one ends it."The onset of each sentence is audibly repeated inside the pause preceding it.
Checked with
clang-format --dry-run --Werrorusing the repo's own.clang-format; it reports no changes.Summary by CodeRabbit