fix: streaming TTS in non pulseaudio systems #298
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes focus on enhancing the audio playback mechanism in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/tts.py (1)
1122-1131: Excellent architectural improvements for audio system compatibility.The changes successfully address the PR objective by:
- Adding support for multiple audio servers (PipeWire, PulseAudio, FFmpeg)
- Implementing graceful fallbacks
- Providing clear error messaging when no suitable player is found
This makes the TTS system more robust and flexible, especially in environments without PulseAudio.
Also applies to: 1141-1141, 1154-1157, 1168-1168
🧰 Tools
🪛 Ruff (0.8.2)
1124-1124: Undefined name
shutil(F821)
1124-1124: Undefined name
shutil(F821)
1124-1124: Undefined name
shutil(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/tts.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
ovos_plugin_manager/templates/tts.py
1124-1124: Undefined name shutil
(F821)
1124-1124: Undefined name shutil
(F821)
1124-1124: Undefined name shutil
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit_tests (3.11)
- GitHub Check: unit_tests (3.10)
🔇 Additional comments (2)
ovos_plugin_manager/templates/tts.py (2)
1141-1141: LGTM! Improved message handling readability.The message handling has been simplified while maintaining the same functionality.
Also applies to: 1168-1168
1154-1157: LGTM! Clear and efficient chunk streaming implementation.The method correctly handles streaming audio chunks with appropriate debug logging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/tts.py (1)
1123-1134: LGTM! Dynamic player selection improves compatibility.The implementation intelligently selects an appropriate audio player based on the system's available audio servers. The order of preference (ffplay → pw-play → paplay → aplay) is sensible, with ffplay being the preferred choice as it's the most versatile player.
Consider adding a debug log to show which player was selected:
if not player: raise RuntimeError("No audio player found (please install 'ffmpeg', 'pulseaudio-utils' or 'alsa-utils').") + LOG.debug(f"Selected audio player: {player}") self.play_args = [player]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/tts.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit_tests (3.11)
🔇 Additional comments (5)
ovos_plugin_manager/templates/tts.py (5)
8-8: LGTM! Import added for shutil module.The shutil module is correctly imported for the
whichfunction used in player detection.
1144-1144: LGTM! Message handling simplified.The message handling is simplified while maintaining the same functionality with a sensible default.
1157-1160: LGTM! Improved logging for stream chunks.The log message now includes helpful information (class name and chunk size) for debugging streaming issues.
1171-1171: LGTM! Message handling simplified.The message handling is simplified while maintaining the same functionality with a sensible default.
1123-1134: Verify audio player selection in the codebase.Let's verify that the audio player selection is used consistently across the codebase.
✅ Verification successful
Audio player selection is correctly implemented
The dynamic audio player selection is properly isolated in the TTS template and doesn't conflict with the rest of the audio system, which uses higher-level abstractions (OCP, MPRIS, Audio Service). No other parts of the codebase directly use these command-line players.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded audio player references that might need updating. # Search for hardcoded player references echo "Searching for hardcoded player references..." rg -i "paplay|aplay|ffplay|pw-play" --type py # Search for potential audio playback implementations echo "Searching for potential audio playback implementations..." ast-grep --pattern 'class $name implements StreamingTTSCallbacks { $$$ }'Length of output: 795
Script:
#!/bin/bash # Search for audio playback related code echo "Searching for audio playback methods..." rg -i "play.*audio|audio.*play" --type py -A 2 echo "Searching for subprocess calls that might be related to audio..." ast-grep --pattern 'subprocess.$_([$$$])'Length of output: 10379
replaces #238
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation