From bc6fd03a4be58e246980f87dde563532aba86557 Mon Sep 17 00:00:00 2001 From: Ettore Di Giacinto Date: Sun, 26 Jul 2026 16:51:07 +0000 Subject: [PATCH] backend(audio-cpp): fix the segment text a transcription response is built from Segment text is not decoration. core/http/endpoints/openai/transcription.go routes response_format text, srt, vtt and lrc through schema.TranscriptionResponse, which builds the entire body out of Segments[].Text and never reads the top-level text. So for those four formats the segment text IS the response. nemotron_asr emits one word_timestamp per SentencePiece token, and the word boundary is carried as a LEADING SPACE on the piece ("So", "me", " call"). join_words inserted a space unconditionally, so response_format=text returned "So me call me na ture ," while the correct sentence sat unread in the top-level field. The separator is now chosen from the words themselves: whole words are space-joined, subword pieces are concatenated, and one leading space anywhere selects the latter. Concatenating the real nemotron pieces reproduces text_output exactly, verified end to end. This does not touch the top-level text, which is still text_output verbatim. The rule that forbids deriving the transcript from the segments is about the direction segments -> text; segment text has no source other than its words. Two smaller corrections in the same area: timestamp_granularities ["word"] set only "word_timestamps", a key no family in the pinned upstream reads. It now sets "return_timestamps", which qwen3_asr does read and which both runs its forced aligner and shortens its chunk window, so asking for word granularity no longer silently returns nothing. The request-option comment claimed more than it delivered. prompt, translate and temperature are read by no ASR family, and are forwarded only so a family adopting them works unchanged; the comment now says so per key, and gives TranscriptRequest.diarize the same explicit treatment threads already had. Also: the shipping target now carries -Wall -Wextra -Wpedantic, which it never did, so "the build is clean" starts meaning something; and fill_transcript_result no longer swallows a null response pointer, since answering OK with an empty transcript is the one failure mode this unit exists to prevent. Assisted-by: Claude:claude-opus-5 [Claude Code] Signed-off-by: Ettore Di Giacinto --- backend/cpp/audio-cpp/CMakeLists.txt | 13 ++-- backend/cpp/audio-cpp/grpc-server.cpp | 57 +++++++++++++---- backend/cpp/audio-cpp/result_map.cpp | 10 +-- backend/cpp/audio-cpp/result_map.h | 3 +- backend/cpp/audio-cpp/transcript_assembly.cpp | 37 ++++++++++- backend/cpp/audio-cpp/transcript_assembly.h | 11 +++- .../audio-cpp/transcript_assembly_test.cpp | 64 +++++++++++++++++++ 7 files changed, 168 insertions(+), 27 deletions(-) diff --git a/backend/cpp/audio-cpp/CMakeLists.txt b/backend/cpp/audio-cpp/CMakeLists.txt index cf8221f06..d5da508a0 100644 --- a/backend/cpp/audio-cpp/CMakeLists.txt +++ b/backend/cpp/audio-cpp/CMakeLists.txt @@ -150,6 +150,14 @@ target_include_directories(${TARGET} PRIVATE "${AUDIO_CPP_DIR}/include" "${CMAKE_CURRENT_SOURCE_DIR}") +# The shipping binary is held to the same bar as the tests below. Upstream's own +# add_compile_options is a property of its directory and never reached this +# target, so until now "the build was clean" meant only that nothing was being +# checked. +if(NOT MSVC) + target_compile_options(${TARGET} PRIVATE -Wall -Wextra -Wpedantic) +endif() + target_link_libraries(${TARGET} PRIVATE hw_grpc_proto engine_runtime @@ -181,11 +189,6 @@ if(AUDIO_CPP_GRPC_BUILD_TESTS) # They are named *_ctest.cpp so the script's glob does not pick them up and # fail every backend's suite; everything that can be stdlib-only still is, # and still lives in a *_test.cpp beside its unit. - # - # -Wall -Wextra -Wpedantic here and not on ${TARGET}: upstream's own - # add_compile_options is a property of its subdirectory and does not reach - # ours, so without naming them the tests would build as quietly as - # everything else. add_executable(result_map_ctest result_map_ctest.cpp result_map.cpp diff --git a/backend/cpp/audio-cpp/grpc-server.cpp b/backend/cpp/audio-cpp/grpc-server.cpp index ef2f7f1dc..6133a3127 100644 --- a/backend/cpp/audio-cpp/grpc-server.cpp +++ b/backend/cpp/audio-cpp/grpc-server.cpp @@ -113,11 +113,14 @@ constexpr int kVadSampleRate = 16000; // one, which is the only reason the nanoseconds are right. // // The cost, stated plainly: vibevoice_asr resamples internally to 24 kHz, so a -// 48 kHz upload now travels 48 -> 16 -> 24 rather than 48 -> 24, losing the -// 8-12 kHz band it could have kept. That is accepted because a silently wrong -// timestamp is worse than a band-limited one, and because every other LocalAI -// ASR path already feeds 16 kHz. If the framework ever publishes a per-model -// preferred input rate, this constant is what should become that lookup. +// 48 kHz upload now travels 48 -> 16 -> 24 rather than 48 -> 24. That is not +// merely band limiting. resample_mono_linear is linear interpolation with no +// anti-alias filter (framework/audio/conversion.cpp), so the content above +// 8 kHz is ALIASED down on the way in, and the 16 -> 24 step aliases whatever +// the first step left. It is accepted because a silently wrong timestamp is +// worse than an aliased band, and because every other LocalAI ASR path already +// feeds 16 kHz. If the framework ever publishes a per-model preferred input +// rate, this constant is what should become that lookup. constexpr int kSpeechSampleRate = 16000; // Parses ModelOptions.MainGPU into a device index. @@ -260,21 +263,47 @@ build_transcription_request(const backend::TranscriptRequest &request, task_request.options["temperature"] = std::to_string(request.temperature()); } for (const auto &granularity : request.timestamp_granularities()) { - // Upstream families read this as a request for word-level timing. if (granularity == "word") { + // return_timestamps is the key upstream actually reads, and it is + // the one that does something: qwen3_asr defaults it to false + // (include/engine/models/qwen3_asr/types.h) and, when set, both + // runs its forced aligner and shortens its chunk window from 30 s + // to 15 s (src/models/qwen3_asr/session.cpp). Without it, asking + // for word granularity silently came back with no word timing. + // + // word_timestamps rides along as a forward-tolerant alias. NO + // family in the pinned upstream reads that key, a grep over src/ + // and include/ returns nothing, so it is sent only so that a family + // adopting the name later works with no change here. + task_request.options["return_timestamps"] = "true"; task_request.options["word_timestamps"] = "true"; } } - // Every option above is advisory. audio.cpp families look their request - // options up by name (runtime::find_option) and ignore the rest; the + // What lands and what does not. Families look their request options up by + // name (runtime::find_option) and ignore every key they do not know; the // unknown-key refusals upstream does have are on SESSION options, which - // arrive at load time, not here. So an option a family does not read costs - // nothing, and none of these can turn a valid request into an error. + // arrive at load time rather than here. So an unread key cannot turn a + // valid request into an error, but it is also not a feature, and the + // honest accounting against the pinned upstream is: // - // TranscriptRequest.threads is deliberately NOT forwarded: thread count is - // a SessionOptions field fixed when the session was created, so a - // per-request value has nowhere to go and pretending otherwise would be a - // knob that silently does nothing. + // language read, by nemotron_asr and vibevoice_asr among others. + // return_timestamps read, by qwen3_asr. + // prompt read by NO ASR family. Forwarded because it is the + // whisper meaning of the field and a family adopting + // it then works unchanged, not because it does + // anything today. + // translate read by nobody anywhere in upstream. + // temperature read only by TTS and voice conversion families, none + // of which this RPC can route to. + // + // Two request fields are deliberately not forwarded at all: + // + // threads thread count is a SessionOptions field fixed when the session + // was built, so a per-request value has nowhere to go. + // diarize this RPC routes to Asr or Alignment. A family that diarizes + // is reached through Diarize, which has its own handler and its + // own response shape, so forwarding this would imply that + // setting it turns speaker labels on here, and nothing would. return task_request; } diff --git a/backend/cpp/audio-cpp/result_map.cpp b/backend/cpp/audio-cpp/result_map.cpp index 3b4fb58a0..c9498d1ae 100644 --- a/backend/cpp/audio-cpp/result_map.cpp +++ b/backend/cpp/audio-cpp/result_map.cpp @@ -10,10 +10,12 @@ namespace audiocpp_backend { void fill_transcript_result(const engine::runtime::TaskResult &result, int sample_rate, float duration_seconds, backend::TranscriptResult *out) { - if (out == nullptr) { - return; - } - + // No null guard on `out`, deliberately. gRPC always hands a handler a + // response message, so a null here would be a programming error in a + // caller, and a guard that returned quietly would answer the client with an + // untouched, empty transcript and an OK status. That is the same + // indistinguishable-from-silence failure the rest of this unit exists to + // prevent; crashing on the developer's machine is the cheaper outcome. std::vector speech_segments; speech_segments.reserve(result.speech_segments.size()); for (const auto &segment : result.speech_segments) { diff --git a/backend/cpp/audio-cpp/result_map.h b/backend/cpp/audio-cpp/result_map.h index c3d1a4ba3..97965877c 100644 --- a/backend/cpp/audio-cpp/result_map.h +++ b/backend/cpp/audio-cpp/result_map.h @@ -28,7 +28,8 @@ namespace audiocpp_backend { // which is why the handler passes the buffer's rate rather than the file's. // // Segments are replaced, not appended to, so a message filled twice does not -// accumulate. +// accumulate. `out` must be non-null and is not checked; see the note at the +// top of the implementation for why that is not an oversight. void fill_transcript_result(const engine::runtime::TaskResult &result, int sample_rate, float duration_seconds, backend::TranscriptResult *out); diff --git a/backend/cpp/audio-cpp/transcript_assembly.cpp b/backend/cpp/audio-cpp/transcript_assembly.cpp index 887dad112..5349ec790 100644 --- a/backend/cpp/audio-cpp/transcript_assembly.cpp +++ b/backend/cpp/audio-cpp/transcript_assembly.cpp @@ -23,13 +23,48 @@ std::int64_t overlap(const Span &a, const Span &b) { return end > begin ? end - begin : 0; } +// Joins a segment's words into that segment's text. +// +// THE SEPARATOR IS NOT ALWAYS A SPACE, and getting it wrong is visible to every +// caller rather than cosmetic: core/http/endpoints/openai/transcription.go +// routes response_format text, srt, vtt and lrc through +// schema.TranscriptionResponse, which builds the entire body out of +// Segments[].Text and never reads the top-level text. For those four formats +// the segment text IS the response. +// +// Two producer conventions have to be told apart: +// +// whole words "Some", "call", "me" -> join with a space +// subword pieces "So", "me", " call" -> concatenate +// +// The second is SentencePiece, where a word boundary is carried as a LEADING +// SPACE on the piece; nemotron_asr emits one entry per token in exactly that +// form. Space-joining those produced "So me call me na ture ,", which is +// what response_format=text returned while the correct sentence sat unread in +// the top-level field. Concatenating them reproduces text_output exactly. +// +// The convention is read off the words themselves, because nothing else in the +// result declares it. One leading space anywhere is enough to decide: a +// whole-word producer has no reason to emit one, and a subword producer emits +// one at every word boundary, so the two populations do not overlap. A producer +// that mixed both conventions inside one segment could not be served correctly +// by any single separator; this picks concatenation for it. +// +// This does NOT touch the top-level text, which stays text_output verbatim. The +// rule that forbids deriving the transcript from the segments is about the +// direction segments -> text. Segment text has no source other than its words +// and is necessarily derived. std::string join_words(const std::vector &words) { + const bool subword_pieces = + std::any_of(words.begin(), words.end(), [](const OutWord &word) { + return !word.text.empty() && word.text.front() == ' '; + }); std::string out; for (const auto &word : words) { if (word.text.empty()) { continue; } - if (!out.empty()) { + if (!subword_pieces && !out.empty()) { out += " "; } out += word.text; diff --git a/backend/cpp/audio-cpp/transcript_assembly.h b/backend/cpp/audio-cpp/transcript_assembly.h index 4b3a3d088..e58fa2b72 100644 --- a/backend/cpp/audio-cpp/transcript_assembly.h +++ b/backend/cpp/audio-cpp/transcript_assembly.h @@ -62,8 +62,15 @@ struct AssembledTranscript { // // Words attach to the segment whose range contains their midpoint; a word // outside every segment attaches to the nearest one by midpoint distance so it -// is never silently dropped. A segment's text is its words joined by a single -// space, except that a lone segment with no words carries the full text. +// is never silently dropped. A lone segment with no words carries the full text. +// +// A segment's text is its words joined, and the separator depends on the +// producer's convention: whole words ("Some", "call") are joined with a space, +// while SentencePiece-style subword pieces, which carry the word boundary as a +// LEADING SPACE (" call"), are concatenated. One leading space anywhere in the +// segment selects concatenation. This matters beyond tidiness: response_format +// text, srt, vtt and lrc build their entire body out of the segment text and +// never read the top-level text. // // A segment's speaker is the speaker turn with the greatest overlap, except // when the segments came from the speaker turns themselves (source 2), where diff --git a/backend/cpp/audio-cpp/transcript_assembly_test.cpp b/backend/cpp/audio-cpp/transcript_assembly_test.cpp index 16fe9e2e9..a0e0ede49 100644 --- a/backend/cpp/audio-cpp/transcript_assembly_test.cpp +++ b/backend/cpp/audio-cpp/transcript_assembly_test.cpp @@ -81,6 +81,67 @@ static void test_words_only() { check(first.id == 0, "ids are zero based"); } +// Shape A-whole: the whole-word convention, stated explicitly rather than left +// implicit in the shape A tests. qwen3_forced_aligner emits one entry per WORD +// (processor.cpp parses per-word timestamp tokens), so its pieces carry no +// leading space and must be joined with one. +static void test_whole_words_are_space_joined() { + const std::vector words = { + {{0, 8000}, "Some"}, + {{8000, 16000}, "call"}, + {{16000, 24000}, "me"}, + }; + const auto out = assemble_transcript("Some call me", {}, {}, words, kRate); + + check(segment_at(out, 0, "whole words").text == "Some call me", + "whole words are joined with a single space"); +} + +// Shape A-subword: the SentencePiece convention, where the word boundary is a +// LEADING SPACE on the piece. These are the first eleven word_timestamps +// nemotron_asr actually returned for audio.cpp/assets/resources/sample_16k.wav +// with the q8_0 GGUF, copied verbatim rather than invented, including the lone +// " " piece at index 3. +// +// Space-joining these produced "So me call me na ture , other s call", +// which is not a cosmetic problem: response_format text, srt, vtt and lrc build +// their entire body from the segment text and never read the top-level text, so +// that string WAS the transcription response for those formats. +static void test_subword_pieces_are_concatenated() { + const std::vector words = { + {{15360, 16640}, "So"}, {{15360, 16640}, "me"}, + {{23040, 24320}, " call"}, {{28160, 29440}, " "}, + {{28160, 29440}, "me"}, {{30720, 32000}, " na"}, + {{33280, 34560}, "ture"}, {{35840, 37120}, ","}, + {{38400, 39680}, " other"}, {{40960, 42240}, "s"}, + {{43520, 44800}, " call"}, + }; + const auto out = assemble_transcript( + "Some call me nature, others call me mother nature.", {}, {}, words, kRate); + + check(out.text == "Some call me nature, others call me mother nature.", + "the top-level text is still text_output verbatim"); + check(segment_at(out, 0, "subword pieces").text == + "Some call me nature, others call", + "subword pieces are concatenated, reproducing text_output"); +} + +// One leading space anywhere decides for the whole segment. A subword producer +// emits a boundary space at every word start, so its first piece, which is +// sentence-initial, does not have one; keying off the first piece alone would +// therefore pick the wrong convention on every segment. +static void test_a_single_leading_space_selects_concatenation() { + const std::vector words = { + {{0, 8000}, "al"}, + {{8000, 16000}, "pha"}, + {{16000, 24000}, " beta"}, + }; + const auto out = assemble_transcript("alpha beta", {}, {}, words, kRate); + + check(segment_at(out, 0, "mixed").text == "alpha beta", + "a leading space on a later piece selects concatenation"); +} + // Shape A': the same producer, but text_output is punctuated and cased while // the word timestamps are not. qwen3_asr rebuilds text_output from its word // list only when timestamps are requested, so the two genuinely differ; this @@ -439,6 +500,9 @@ static void test_zero_sample_rate_is_safe() { int main() { test_words_only(); + test_whole_words_are_space_joined(); + test_subword_pieces_are_concatenated(); + test_a_single_leading_space_selects_concatenation(); test_words_only_with_punctuated_text_output(); test_covering_span_spans_every_word(); test_empty_word_contributes_no_separator();