fix(tts): synthesize mp3 then transcode to opus for .ogg targets - #55278
alaamohanad169-ship-it wants to merge 1 commit into
Conversation
Competing with the earlier open PR #54597 for the same bug (#54589): #54597 tries opus first then falls back to mp3+transcode, while this PR always requests mp3 then transcodes for Heads-up on scope: this branch appears to be stacked on |
|
Acknowledged: PR #54597 exists and takes a different approach. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: COMMENT — Mixed scope
This PR bundles three unrelated concerns under a single TTS fix title:
-
TTS opus compatibility (tools/tts_tool.py): Always request mp3, transcode to opus locally. This is the stated fix and is clean.
-
Gateway config secret management (gateway/config.py): Adds get_secret() fallback for 12 platform tokens (Telegram, Discord, Slack, etc.). This is a separate concern from TTS.
-
API server compaction preservation (gateway/platforms/api_server.py): Preserves leading [CONTEXT COMPACTION] messages during truncation. This is another separate concern.
Suggestion
Split into 3 focused PRs:
- TTS opus fix (title PR)
- Gateway config get_secret() integration
- API server compaction truncation fix
Each is independently valuable and reviewable, but bundling them makes it harder to bisect regressions and review each change in isolation.
c8066a4 to
62577c2
Compare
|
Updated: PR now contains only the TTS opus transcode fix (1 file: tools/tts_tool.py). The mixed scope issue has been resolved. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the TTS compatibility path.
Problems
- The fallback returned by the added
return synth_pathpath is not consumed: current main dispatches OpenAI TTS attools/tts_tool.py:2295without assigning_generate_openai_tts()'s return. If_convert_to_opus()returnsNone, the later file check still examines the requested.oggpath, so the advertised MP3 fallback fails. - The unconditional MP3 request changes direct OpenAI too. Current docs state OpenAI produces Opus natively (
website/docs/user-guide/features/tts.md:172); forcing MP3 makes an existing native-Opus route depend on ffmpeg.
Suggested changes
- Propagate the generated return path into
file_strbefore the existence check. - Preserve Opus-first behavior and retry as MP3 only for endpoints that reject Opus, then transcode that fallback.
- Add regression coverage for native Opus, unsupported-Opus fallback, and failed conversion.
Automated hermes-sweeper review.
| # for .ogg targets. This avoids breaking non-opus-compatible backends | ||
| # (e.g. Speaches/Kokoro) that reject response_format="opus". | ||
| wants_opus = output_path.endswith(".ogg") | ||
| response_format = "mp3" |
There was a problem hiding this comment.
This changes direct OpenAI too: current documentation states OpenAI produces native Opus (website/docs/user-guide/features/tts.md:172). Please preserve an Opus-first path and use MP3 plus local conversion only after an endpoint rejects Opus; otherwise ffmpeg becomes a new dependency for a route that currently needs none.
| except OSError: | ||
| pass | ||
| return converted | ||
| # Transcoding failed — return the mp3 as best-effort |
There was a problem hiding this comment.
This return path is currently discarded by the caller: text_to_speech_tool() invokes _generate_openai_tts(...) at tools/tts_tool.py:2295 without assigning its result, then validates the original .ogg path. On conversion failure the promised MP3 fallback will still report no output. Propagate the returned path into the dispatcher state.
_generate_openai_tts() hardcoded response_format='opus' for .ogg output paths. This breaks OpenAI-compatible TTS backends (e.g. Speaches/Kokoro) that don't support opus encoding. Always request mp3 from the API, then transcode to OGG/Opus locally via the existing _convert_to_opus() helper when an .ogg target is requested. This mirrors the Edge TTS provider path and works with any backend. Fixes #54589
62577c2 to
1cee6e3
Compare
|
Resolved at the class level by PR #73072. Rather than a per-provider transcode, |
Summary
_generate_openai_tts()hardcodedresponse_format="opus"for.oggoutput paths. This breaks OpenAI-compatible TTS backends (e.g. Speaches/Kokoro) that do not support opus encoding.Always request mp3 from the API, then transcode to OGG/Opus locally via the existing
_convert_to_opus()helper when an.oggtarget is requested. This mirrors the Edge TTS provider path and works with any backend.Changes
tools/tts_tool.py_generate_openai_tts(): always requestresponse_format="mp3", transcode to opus for.oggtargets using_convert_to_opus()Testing
Related
Fixes #54589