Skip to content

Align the silence scale range between Validate() and ScaleSilence - #3747

Merged
csukuangfj merged 1 commit into
k2-fsa:masterfrom
ekenberg:pr/align-silence-scale-range
Jul 10, 2026
Merged

csukuangfj merged 1 commit into
k2-fsa:masterfrom
ekenberg:pr/align-silence-scale-range

Conversation

@ekenberg

@ekenberg ekenberg commented Jul 10, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #3745, from a review comment on it.

After #3745, ScaleSilence accepts [0.01, 2], but OfflineTtsConfig::Validate()
still only rejects silence_scale < 0.001. So --tts-silence-scale=0.005 or =5
passes validation and is then silently skipped at run time: the user asks for
scaling and gets unscaled audio back, with only a log line.

This puts both checks on one pair of constants, documents the range in the
--tts-silence-scale help text, and makes bad input fail before synthesis rather
than after it. The check inside ScaleSilence stays, because callers of the
Generate API pass GeneratedAudioConfig::silence_scale directly and bypass
Validate().

One question: should the upper bound be 2, or higher?

I raised it to 10 here. Happy to change it back to 2 — it is a one-line
revert, and it is your call.

The reason to raise it: nothing breaks until far higher. Overflow of the
int32_t conversion needs pause_length * scale > 2^31, which for a 1 s pause at
24 kHz means a scale around 89000, and for a 5 s pause around 17900. At
scale = 10 a 1 s pause becomes 10 s, which is 1 MB of float32. Only NaN,
infinity and negative values are actually dangerous, and those are rejected
regardless of where the ceiling sits.

2 also rules out reasonable uses of scaling a pause up — long dramatic pauses
in an audiobook, say. For comparison, speed has no upper bound at all, only
speed <= 0 is rejected.

If you prefer to keep the tighter limit, say so and I will set both constants
back to 2.

Verified

Built and run against kokoro-multi-lang-v1_0.

--tts-silence-scale before after
0.0099 passes Validate(), then silently skipped rejected by Validate()
0.01 scaled scaled
2.0 scaled scaled
2.001 passes Validate(), then silently skipped scaled
10 passes Validate(), then silently skipped scaled
10.001 passes Validate(), then silently skipped rejected by Validate()
0, -1, nan, inf, 1e9 Validate() catches 0/-1; nan/inf/1e9 skipped in ScaleSilence all rejected by Validate()

scale = 10 produces 19.25 s from a 4.17 s clip, and the count of samples above
the 0.01 silence threshold is unchanged (-1), so the added audio is silence.

Checked with clang-format --dry-run --Werror; no changes.

Summary by CodeRabbit

  • Bug Fixes
    • Expanded validation for the TTS silence scaling option.
    • Values must now fall within the supported range of 0.01 to 10, with clearer error messages for invalid settings.
    • Updated option guidance to reflect the valid range.

After k2-fsa#3745, ScaleSilence accepts [0.01, 2] but OfflineTtsConfig::Validate()
still only rejects silence_scale < 0.001. Values in [0.001, 0.01) and above 2
therefore pass validation and are then silently skipped at run time, so the
user asks for scaling and gets unscaled audio.

Use one pair of constants for both, document the range in the --tts-silence-scale
help text, and raise the upper bound from 2 to 10.

Rejecting in Validate() also makes bad input fail before synthesis rather than
after it. The check in ScaleSilence stays, because callers of the Generate API
pass GeneratedAudioConfig::silence_scale directly and bypass Validate().
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The TTS silence scale option now accepts values from 0.01 through 10.0. Shared bounds are used by audio scaling and configuration validation, and the command-line help and error messages describe the updated range.

Changes

TTS silence scale range

Layer / File(s) Summary
Silence scale range contract and validation
sherpa-onnx/csrc/offline-tts.cc
GeneratedAudio::ScaleSilence and OfflineTtsConfig::Validate enforce the [0.01, 10.0] range, while option help text and errors report both bounds.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: synchronizing the silence-scale validation range between Validate() and ScaleSilence().
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request increases the maximum allowed silence scale for offline text-to-speech from 2.0 to 10.0, introducing kMinSilenceScale and kMaxSilenceScale constants to enforce this range during validation and scaling. The command-line option description is also updated to reflect this new range. The reviewer suggests moving these constants to the header file offline-tts.h to improve maintainability and allow other parts of the codebase to access them.

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.

Comment on lines +37 to +38
static constexpr float kMinSilenceScale = 0.01f;
static constexpr float kMaxSilenceScale = 10.0f;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve maintainability and allow other parts of the codebase (or external language bindings) to access these validation limits, consider moving kMinSilenceScale and kMaxSilenceScale to the header file offline-tts.h (for example, as public static constexpr members of OfflineTtsConfig or within the sherpa_onnx namespace).

Currently, defining them as static constexpr in the .cc file hides them from other translation units, making it harder to reuse them for validation or documentation elsewhere.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
sherpa-onnx/csrc/offline-tts.cc (1)

41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider using 1.0f for explicit float comparison.

scale == 1 compares a float against an int literal. While 1 is exactly representable and the implicit conversion is harmless, using 1.0f is more idiomatic and avoids relying on implicit promotion.

♻️ Optional style tweak
-  if (scale == 1) {
+  if (scale == 1.0f) {
🤖 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 41, In the scale comparison within
the relevant TTS function, replace the integer literal 1 in `if (scale == 1)`
with the explicit float literal `1.0f`.
🤖 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.

Nitpick comments:
In `@sherpa-onnx/csrc/offline-tts.cc`:
- Line 41: In the scale comparison within the relevant TTS function, replace the
integer literal 1 in `if (scale == 1)` with the explicit float literal `1.0f`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b15503ac-3697-44a6-9064-9db2ae84c274

📥 Commits

Reviewing files that changed from the base of the PR and between 986a386 and 8e9ed8d.

📒 Files selected for processing (1)
  • sherpa-onnx/csrc/offline-tts.cc

@csukuangfj csukuangfj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants