Skip to content

perf(discord): stream voice PCM to ffmpeg instead of a temp file - #68157

Closed
FixItFoundry wants to merge 1 commit into
NousResearch:mainfrom
FixItFoundry:perf/discord-voice-pcm-no-disk
Closed

perf(discord): stream voice PCM to ffmpeg instead of a temp file#68157
FixItFoundry wants to merge 1 commit into
NousResearch:mainfrom
FixItFoundry:perf/discord-voice-pcm-no-disk

Conversation

@FixItFoundry

Copy link
Copy Markdown
Contributor

What

VoiceReceiver.pcm_to_wav staged every captured utterance in a NamedTemporaryFile purely to give ffmpeg an input path, then unlinked it. This pipes the PCM to ffmpeg's stdin instead.

Per voice utterance that removes one file created, written, read back and deleted — on a path that runs for every spoken message (adapter.py calls it via asyncio.to_thread on each utterance). The try/finally cleanup goes away with it, so the function gets shorter as well as faster.

Why the WAV still goes to a file, not stdout

The obvious next step — capture the WAV from stdout too — is a trap, and worth recording here.

ffmpeg cannot seek on a pipe, so it cannot go back and patch the RIFF/data chunk sizes after writing. A WAV produced with -f wav pipe:1 carries placeholder 0xFFFFFFFF sizes. Measured on a 1s 48kHz stereo clip:

output RIFF size field data size field wave.getnframes()
pipe:1 4294967295 4294967295 2147483647
output_path 32070 (correct) 32000 (correct) 16000 (correct)

Any reader that trusts the header — which is the whole point of writing a WAV rather than raw PCM for STT — misjudges the length. So the input is piped and the output keeps its real, seekable path.

Tests

  • test_pcm_is_piped_to_stdin_not_staged_on_disk — asserts the PCM goes over input=, that pipe:0 is used, that the final arg is the real output path, and that no .pcm temp file appears in the command.
  • test_output_wav_header_reports_true_length — skipped when ffmpeg isn't installed; converts a real 1s tone and asserts the header reports 16000 frames rather than the piped-stdout placeholder.

Full tests/gateway/test_voice_command.py + test_discord_voice_mixer.py: 208 passed.

pcm_to_wav staged every captured utterance in a NamedTemporaryFile just to
hand ffmpeg an input path, then unlinked it. Feed the PCM to ffmpeg's stdin
instead: one fewer file created, written, read back and removed per voice
utterance, and the try/finally cleanup goes away with it.

The WAV output deliberately still goes to output_path rather than being
captured from stdout. ffmpeg cannot seek on a pipe, so a piped WAV is
written with placeholder 0xFFFFFFFF RIFF/data chunk sizes -- Python's wave
module then reports 2147483647 frames for a 1s clip, and strict readers
misjudge the length. Writing to the real path lets ffmpeg seek back and
patch the header.

Tests cover both halves: that the PCM goes over stdin with no temp file,
and (when ffmpeg is installed) that the resulting header reports the true
frame count.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/perf Performance improvement or optimization comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter tool/tts Text-to-speech and transcription P3 Low — cosmetic, nice to have labels Jul 20, 2026

@teknium1 teknium1 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.

Thanks for the focused per-utterance I/O reduction. The current-main premise is still present: pcm_to_wav stages PCM at plugins/platforms/discord/adapter.py:780, and it is invoked once per processed utterance at plugins/platforms/discord/adapter.py:4381.

Problems

  • The new hard-coded "ffmpeg" executable (plugins/platforms/discord/adapter.py:723 in this diff) regresses current main's resolve_ffmpeg_executable() call at plugins/platforms/discord/adapter.py:788. That resolver was added in 9b89da23fb8d76a7c208168c57c3eb5bec891d60 to support FFMPEG_PATH and Windows winget installs outside PATH; current coverage asserts this conversion path at tests/gateway/test_voice_command.py:776.

Suggested changes

  • Preserve resolve_ffmpeg_executable() as argv[0] while changing only the PCM input from a path to pipe:0 plus input=pcm_data.
  • Update the pipe-input test to exercise the resolved executable as well.

Automated hermes-sweeper review.

strict readers misreport the length.
"""
from hermes_cli._subprocess_compat import windows_hide_flags

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.

Please retain resolve_ffmpeg_executable() here rather than restoring the literal "ffmpeg". Current main uses the resolver for this conversion so Windows winget installs and FFMPEG_PATH work when ffmpeg is absent from PATH (9b89da23fb8d76a7c208168c57c3eb5bec891d60; tests/gateway/test_voice_command.py:776).

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/streaming Streaming responses: gateway delivery, provider wire labels Jul 30, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Thanks @FixItFoundry — the stdin-piping optimization is exactly right (no temp-file lifecycle per utterance, WAV correctly still file-backed to avoid placeholder RIFF sizes) and your tests verify the real ffmpeg header. Salvaged into #76970 with your authorship preserved via cherry-pick; during conflict resolution the bare "ffmpeg" from your stale base was swapped back to main's resolve_ffmpeg_executable() (Windows winget discovery + FFMPEG_PATH override), and main's 3 newer tests were retained alongside yours. 158 voice-suite tests green. Closing in favor of the salvage.

kshitijk4poor added a commit to kshitijk4poor/hermes-agent that referenced this pull request Aug 2, 2026
Simplify-pass follow-up on the NousResearch#68157 salvage (regression-neutral \u2014 the
old temp-file version was equally bare): capture ffmpeg's -loglevel
error output so a CalledProcessError carries the real message, and log
it at the voice-input catch site. Parity with transcription_tools'
ffmpeg call sites. Live-verified: forced ffmpeg failure produces the
captured message in the exception.
kshitijk4poor added a commit that referenced this pull request Aug 2, 2026
Simplify-pass follow-up on the #68157 salvage (regression-neutral \u2014 the
old temp-file version was equally bare): capture ffmpeg's -loglevel
error output so a CalledProcessError carries the real message, and log
it at the voice-input catch site. Parity with transcription_tools'
ffmpeg call sites. Live-verified: forced ffmpeg failure produces the
captured message in the exception.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
Simplify-pass follow-up on the NousResearch#68157 salvage (regression-neutral \u2014 the
old temp-file version was equally bare): capture ffmpeg's -loglevel
error output so a CalledProcessError carries the real message, and log
it at the voice-input catch site. Parity with transcription_tools'
ffmpeg call sites. Live-verified: forced ffmpeg failure produces the
captured message in the exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/discord Discord bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/tts Text-to-speech and transcription type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants