Add configurable boundaries for speed parameter

[michalkulakowski]

🛠 Summary

JIRA/Issue if applicable.
Describe the changes.

🧪 Checklist

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

[Copilot]

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.

[mzegla]

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

[Copilot]

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"

[mzegla]

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]

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, ")"));

[Copilot]

Pull request overview

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

Suppressed comments (6)

src/audio/text_to_speech/t2s_calculator.cc:93

• speed_min/speed_max being invalid is a configuration/user-input problem, not an internal failure. Returning absl::InternalError can lead to misleading error classification and potentially incorrect HTTP mapping; use absl::InvalidArgumentError (or another config-appropriate status) here so it’s treated as a bad config rather than a server fault.

        const auto& calcOptions = cc->Options<T2sCalculatorOptions>();
        const float speedMin = calcOptions.speed_min();
        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/audio/audio_utils.cpp:235

• This allows non-byte-aligned bitsPerSample values (e.g., 12) to pass with a truncated bytesPerSample, making the size check inaccurate and potentially letting oversized outputs slip through. Consider explicitly rejecting unsupported bitsPerSample values (at least require bitsPerSample % 8 == 0), and update the error message to reflect invalid bitsPerSample vs. overflow.

    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);

src/test/audio/text2speech_test.cpp:436

• ASSERT_NE(..., StatusCode::OK) makes the test pass for unrelated failures (e.g., parsing/path issues). Since this test is specifically about inverted speed bounds, assert the exact expected failure code (e.g., StatusCode::MEDIAPIPE_GRAPH_CONFIG_FILE_INVALID) to ensure the validation is what’s being exercised.

    ASSERT_NE(validateText2SpeechGraphConfig(manager, testPbtxt), StatusCode::OK);

src/audio/text_to_speech/t2s_calculator.proto:52

• The comment promises an HTTP 400, but the code path shown returns an absl::InvalidArgumentError which may or may not be mapped to 400 depending on the surrounding HTTP/status translation layer. If the mapping is not guaranteed, consider rewording to refer to a 'bad request' / 'invalid argument' error (or adjust the translation layer/tests so the behavior is consistently HTTP 400).

    // Minimum allowed value for the "speed" request parameter.
    // Requests with speed < speed_min are rejected with HTTP 400.
    // Default matches the OpenAI TTS API lower bound.
    optional float speed_min = 5 [default = 0.25];

    // Maximum allowed value for the "speed" request parameter.
    // Requests with speed > speed_max are rejected with HTTP 400.
    // Default matches the OpenAI TTS API upper bound.
    optional float speed_max = 6 [default = 4.0];

src/audio/text_to_speech/t2s_calculator.cc:153

• This re-reads calculator options on every request. Since speed_min/speed_max are static per node instance, consider caching them as T2sCalculator members initialized in Open() and reusing them in Process() to reduce per-request overhead and duplicated logic (optional).

                // 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();

src/audio/text_to_speech/tts_node_initializer.cpp:68

• The PR description still contains placeholders (no linked issue/JIRA, no concrete change description, and checklist is unchecked). Please update the PR description to reflect the actual changes (configurable speed bounds + synthesized output size cap) and any relevant tracking links.

            SPDLOG_ERROR("TextToSpeech node name: {} invalid speed bounds in graph {}: speed_min ({}) must be <= speed_max ({}).",

[Copilot]

Pull request overview

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

Suppressed comments (5)

src/audio/text_to_speech/t2s_calculator.cc:93

• In Open(), invalid calculator options (inverted/NaN speed bounds) are a caller/configuration error; returning absl::InternalError can incorrectly classify it as a server failure. Use absl::InvalidArgumentError to match other calculators’ option validation behavior.

        const auto& calcOptions = cc->Options<T2sCalculatorOptions>();
        const float speedMin = calcOptions.speed_min();
        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/audio/text_to_speech/t2s_calculator.cc:159

• The new request-time bounds check should be covered by a unit/integration test for custom speed_min/speed_max (not just the default range). For example: configure speed_min: 0.5 and verify that a request with speed: 0.25 is rejected and speed: 0.5 is accepted.

                if (!(speedMin <= speed && speed <= speedMax)) {
                    return absl::InvalidArgumentError(
                        absl::StrCat("speed must be between speed_min (", speedMin, ") and speed_max (", speedMax, ")"));
                }

src/test/audio/text2speech_test.cpp:436

• This test should assert the specific error code for inverted speed bounds (like the other config-validation tests do), rather than only asserting that it’s not OK. Otherwise unrelated failures could satisfy the assertion and mask regressions.

    ASSERT_NE(validateText2SpeechGraphConfig(manager, testPbtxt), StatusCode::OK);

src/audio/audio_utils.cpp:254

• If drwav_init_memory_write fails, the error message says "Failed to write all frames", which is misleading (no frames have been written yet). Use a message that reflects initialization failure to make debugging easier.

    auto status = drwav_init_memory_write(&wav, ppData, &pDataSize, &format, nullptr);
    if (status == DRWAV_FALSE) {
        throw std::runtime_error("Failed to write all frames");
    }

src/test/audio/audio_utils_test.cpp:510

• Avoid constructing a temporary std::string just to call .c_str()—it’s unnecessary and makes the call harder to read. Pass the std::string result directly.

    SetEnvironmentVar("OVMS_AUDIO_MAX_FILE_SIZE_BYTES", std::to_string(rawPcmBytes).c_str());

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/audio/audio_utils.cpp:234

• The thrown message is misleading when bitsPerSample < 8 (bytesPerSample == 0): this is invalid input, not an overflow. Splitting the checks improves diagnostics and matches the actual failure mode.

    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");
    }

src/test/audio/audio_utils_test.cpp:498

• prepareAudioOutput allocates its output via dr_wav; in the codebase the corresponding deallocator used is drwav_free (e.g., src/audio/audio_utils.cpp and src/audio/text_to_speech/t2s_calculator.cc). Using free() directly makes the test depend on dr_wav’s current allocator choice and can become incorrect if allocation callbacks change.

    EXPECT_NO_THROW(
        prepareAudioOutput(&ppData, pDataSize, sampleRate, bitsPerSample, speechSize, waveform.data()));
    if (ppData) {
        free(ppData);  // drwav allocates via DRWAV_MALLOC
    }

[Copilot]

Pull request overview

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

Suppressed comments (6)

src/test/audio/text2speech_test.cpp:191

• This acceptance test only asserts StatusCode::OK but does not validate that the response body is a valid WAV (unlike other successful TTS tests in this file). Adding a readWav() check helps ensure the boundary speed value still produces a valid audio payload.

    ASSERT_EQ(
        handler->dispatchToProcessor(endpoint, requestBody, &response, comp, responseComponents, writer, multiPartParser),
        ovms::StatusCode::OK);
}

src/audio/text_to_speech/t2s_calculator.cc:158

• The speed bounds validation error omits the actual provided speed value, which makes client-side debugging harder (especially when speed comes from request JSON). Consider including the rejected speed in the message.

                if (!(speedMin <= speed && speed <= speedMax)) {
                    return absl::InvalidArgumentError(
                        absl::StrCat("speed must be between speed_min (", speedMin, ") and speed_max (", speedMax, ")"));

src/test/audio/text2speech_test.cpp:176

• This acceptance test only asserts StatusCode::OK but does not validate that the response body is a valid WAV (unlike other successful TTS tests in this file). Adding a readWav() check helps ensure the boundary speed value still produces a valid audio payload.

This issue also appears on line 188 of the same file.

    ASSERT_EQ(
        handler->dispatchToProcessor(endpoint, requestBody, &response, comp, responseComponents, writer, multiPartParser),
        ovms::StatusCode::OK);
}

src/test/audio/audio_utils_test.cpp:497

• prepareAudioOutput() allocates the buffer using dr_wav, and production code frees it with drwav_free(). This test frees the returned buffer with free(), which can become incorrect if dr_wav is built with custom allocation callbacks/macros. Prefer freeing with the same allocator API used by the producer (or expose an OVMS helper to release the buffer).

    if (ppData) {
        free(ppData);  // drwav allocates via DRWAV_MALLOC
    }

src/test/audio/text2speech_test.cpp:245

• This success-path test does not validate that the generated payload is a valid WAV (only StatusCode::OK). Adding a readWav() check would better assert that the custom lower bound still produces a valid audio response.

    ASSERT_EQ(
        handler->dispatchToProcessor(endpoint, requestBody, &response, comp, responseComponents, writer, multiPartParser),
        ovms::StatusCode::OK);
}

src/test/audio/text2speech_test.cpp:221

• The custom-bounds suite validates the lower bound, but there are no tests for the configured upper bound (e.g., speed == speed_max should be accepted and speed > speed_max rejected). Since this PR adds configurable min/max, it would be good to cover both sides of the range for the custom config too.

TEST_F(Text2SpeechHttpCustomBoundsTest, speedBelowCustomMinRejected) {
    std::string requestBody = R"(
        {
            "model": ")" + modelName +
                              R"(",