Skip to content

Add configurable boundaries for speed parameter - #4445

Open
michalkulakowski wants to merge 3 commits into
mainfrom
mkulakow/speed_parameter_bounding
Open

Add configurable boundaries for speed parameter#4445
michalkulakowski wants to merge 3 commits into
mainfrom
mkulakow/speed_parameter_bounding

Conversation

@michalkulakowski

Copy link
Copy Markdown
Collaborator

🛠 Summary

JIRA/Issue if applicable.
Describe the changes.

🧪 Checklist

  • Unit tests added.
  • The documentation updated.
  • Change follows security best practices.
    ``

Copilot AI lite review requested due to automatic review settings August 11, 2026 09:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds configurable validation bounds for the Text-to-Speech request speed parameter via T2sCalculatorOptions, and adds a size-cap guard for synthesized audio output to prevent oversized allocations/responses.

Changes:

  • Add speed_min / speed_max options to T2sCalculatorOptions (with defaults) and enforce them during request processing.
  • Add a defensive size check in prepareAudioOutput() consistent with existing decode-path size limits.
  • Extend unit tests to cover default/custom speed bounds and synthesized-output size-cap behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/test/audio/text2speech_test.cpp Adds request-level tests for default speed bounds and config-level test for custom bounds.
src/test/audio/audio_utils_test.cpp Adds tests ensuring prepareAudioOutput() enforces size caps and rejects invalid parameters.
src/audio/text_to_speech/t2s_calculator.proto Introduces configurable speed_min / speed_max options with defaults.
src/audio/text_to_speech/t2s_calculator.cc Enforces speed bounds when parsing TTS requests.
src/audio/audio_utils.cpp Adds overflow/size-cap validation for synthesized WAV output buffers.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/audio/audio_utils.cpp
Comment on lines +231 to +235
const size_t bytesPerSample = bitsPerSample / 8;
if (bytesPerSample == 0 || speechSize > std::numeric_limits<size_t>::max() / bytesPerSample) {
throw std::runtime_error("Synthesized audio buffer size overflows maximum representable value");
}
validateAudioFileSizeAgainstMaxValue(speechSize * bytesPerSample);
Comment on lines +141 to +145
const auto& calcOptions = cc->Options<T2sCalculatorOptions>();
const float speedMin = calcOptions.speed_min();
const float speedMax = calcOptions.speed_max();
if (speed < speedMin || speed > speedMax) {
return absl::InvalidArgumentError(
const float speedMax = calcOptions.speed_max();
if (speed < speedMin || speed > speedMax) {
return absl::InvalidArgumentError(
absl::StrCat("speed must be between ", speedMin, " and ", speedMax));

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.

Does it propagate to the logs? I would use speed_min and speed_max to match naming in graph.pbtxt

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/audio/text_to_speech/t2s_calculator.cc:149

  • The current bounds check allows NaN (or misconfigured NaN bounds) to slip through because both (speed < speedMin) and (speed > speedMax) are false for NaN. This can bypass validation and pass an invalid speed into the GenAI pipeline. Consider rewriting the check to use a positive-range predicate and explicitly validate the configured bounds (e.g., speed_min <= speed_max).
                // Validate speed bounds regardless of whether it came from request or default
                const auto& calcOptions = cc->Options<T2sCalculatorOptions>();
                const float speedMin = calcOptions.speed_min();
                const float speedMax = calcOptions.speed_max();
                if (speed < speedMin || speed > speedMax) {

src/audio/audio_utils.cpp:264

  • If drwav_write_pcm_frames() fails (framesWritten != totalSamples), the function throws without calling drwav_uninit() or freeing the partially built *ppData, which can leak memory. Since this function now has additional throw paths, it would be safer to make the whole write + post-write size validation exception-safe and free/uninit on every failure path.
    // Validate the actual WAV container size (includes RIFF/fmt/fact/data header
    // overhead that the pre-write check did not account for).
    try {
        validateAudioFileSizeAgainstMaxValue(pDataSize);
    } catch (...) {

src/test/audio/text2speech_test.cpp:389

  • The new CustomSpeedBoundsConfigured test verifies that the graph config accepts speed_min / speed_max, but it doesn’t verify that these configured bounds actually affect request handling (e.g., speed=0.4 rejected when speed_min=0.5). Since the PR’s main feature is configurable bounds, it would be helpful to add an integration-style test that loads a graph/config with custom bounds and asserts both reject/accept behaviors at runtime.
TEST_F(Text2SpeechConfigTest, CustomSpeedBoundsConfigured) {
    ConstructorEnabledModelManager manager;
    std::string testPbtxt = R"(
    input_stream: "HTTP_REQUEST_PAYLOAD:input"
    output_stream: "HTTP_RESPONSE_PAYLOAD:output"

Comment thread src/audio/audio_utils.cpp
Comment on lines +262 to +269
try {
validateAudioFileSizeAgainstMaxValue(pDataSize);
} catch (...) {
drwav_free(*ppData, nullptr);
*ppData = nullptr;
pDataSize = 0;
throw;
}

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.

  1. Doesn't validateAudioFileSizeAgainstMaxValue also need a try catch?
  2. What is the result of rethrowing here regarding what user - client and admin - see in the response message and logs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/audio/text_to_speech/t2s_calculator.cc:92

  • Open() treats an invalid speed_min/speed_max configuration as an InternalError. This is a user configuration error and should use InvalidArgumentError so it is surfaced as a client/config validation issue rather than an internal server failure.
        const float speedMax = calcOptions.speed_max();
        // !(speedMin <= speedMax) is true for inverted ranges and for any NaN bound.
        if (!(speedMin <= speedMax)) {
            return absl::InternalError(
                absl::StrCat("Invalid T2sCalculatorOptions: speed_min (", speedMin, ") must be <= speed_max (", speedMax, ")"));

src/test/audio/audio_utils_test.cpp:497

  • prepareAudioOutput allocates the WAV buffer via dr_wav; freeing it with free() can be incorrect if dr_wav.h is configured with custom allocators (the production path uses drwav_free). This can lead to allocator-mismatch crashes in tests under some builds.
    if (ppData) {
        free(ppData);  // drwav allocates via DRWAV_MALLOC
    }

src/audio/text_to_speech/t2s_calculator.cc:158

  • The out-of-range speed error message does not include the actual invalid speed value, which makes debugging client requests harder.
                if (!(speedMin <= speed && speed <= speedMax)) {
                    return absl::InvalidArgumentError(
                        absl::StrCat("speed must be between speed_min (", speedMin, ") and speed_max (", speedMax, ")"));

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants