Encode streamed audio with one encoder per stream - #13820
Draft
hysts wants to merge 12 commits into
Draft
Conversation
gr.Audio(streaming=True) encoded every chunk into its own AAC file. Each of those files opens with a priming frame that has no overlap-add partner, so a player heard a short silence at every chunk boundary. Decoding the stream of an 8.000 s, 16 kHz signal fed in 4000-sample chunks: 33 gaps, 67.9 ms on average, 21.9% of the stream silent. Those 33 extra frames are also extra audio, so the stream decodes to 10.240 s of media for an 8.000 s source. Keep one ffmpeg encoder alive per stream instead, and cut the HLS segments out of its continuous output. The same signal then decodes to 8.064 s with one gap, at t=0, and 0.8% silence, and the output is 55% smaller. Browser playback in Chromium confirms the per-chunk gaps are gone. The encoder is created on the first chunk, fed the decoded PCM of each later chunk, and released from end_stream(), with a weakref.finalize backstop for the client-disconnect path that never reaches it. Example caching drives stream_output too, so it now uses a unique stream id and releases the encoder in a finally. Segment durations come from the frame count rather than from the source chunk, and #EXTINF carries six decimals: a player derives each segment's start by accumulating them, and three decimals cannot place a 1024-sample frame closely enough over a long stream.
Collaborator
🪼 branch checks and previews
Install Gradio from this PR pip install https://huggingface.co/buckets/gradio/pypi-previews/resolve/a309a386394f182e862d73bd2317651c31d00db0/gradio-6.26.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@a309a386394f182e862d73bd2317651c31d00db0#subdirectory=client/python"Import Gradio JS Client from this PR via CDN import { Client } from "https://huggingface.co/buckets/gradio/npm-previews/resolve/a309a386394f182e862d73bd2317651c31d00db0/browser.js"; |
Collaborator
🦄 change detectedThis Pull Request includes changes to the following packages.
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed encoder teardown/resource-lifecycle issue in flush_stream_output(), plus a changeset file that conflicts with documented repo PR rules.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes choppy/discontinuous gr.Audio(streaming=True) output by keeping a single AAC encoder alive for the lifetime of a stream, emitting continuous ADTS output across segments, and improving playlist timing precision so HLS players can place segments accurately.
Changes:
- Maintain per-stream AAC encoder state across streamed chunks and cut HLS segments from the continuous encoder output.
- Increase playlist
#EXTINFprecision to 6 decimals and add backend tests asserting continuity and playlist/decoded-duration agreement. - Add explicit stream-finalization hooks (
flush_stream_output/end_stream_output) and ensure cached streaming examples use unique stream IDs.
File summaries
| File | Description |
|---|---|
test/test_routes.py |
Asserts higher-precision #EXTINF and no discontinuity tags for AAC playlists. |
test/components/test_audio.py |
Adds an end-to-end continuity test validating a single priming frame across a stream. |
gradio/routes.py |
Emits #EXTINF with 6 decimal places to reduce accumulated timing drift. |
gradio/route_utils.py |
Adds MediaStream.on_end cleanup callbacks invoked from end_stream(). |
gradio/helpers.py |
Uses a unique stream_id for cached streaming outputs and ensures teardown in finally. |
gradio/components/base.py |
Adds flush_stream_output and end_stream_output hooks (no-op by default). |
gradio/components/audio.py |
Implements per-stream AAC encoding via a shared encoder registry and stream lifecycle hooks. |
gradio/blocks.py |
Wires stream lifecycle: per-stream IDs, flush-on-final, and end-of-stream cleanup callbacks/finalizer. |
gradio/audio_stream_encoder.py |
Introduces ffmpeg-backed continuous AAC stream encoder and ADTS framing utilities. |
.changeset/slick-cycles-send.md |
Adds a changeset entry (repo policy indicates these should not be hand-authored). |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+63
to
+78
| def _ffmpeg_decode(data: bytes, sample_rate: int, channels: int) -> bytes: | ||
| result = subprocess.run( | ||
| [ | ||
| "ffmpeg", "-v", "quiet", "-nostdin", | ||
| "-i", "pipe:0", | ||
| "-f", "s16le", "-acodec", "pcm_s16le", | ||
| "-ar", str(sample_rate), "-ac", str(channels), | ||
| "pipe:1", | ||
| ], | ||
| input=data, | ||
| capture_output=True, | ||
| check=False, | ||
| ) # fmt: skip | ||
| if not result.stdout: | ||
| raise RuntimeError("Could not decode the streamed audio chunk.") | ||
| return result.stdout |
Collaborator
Author
There was a problem hiding this comment.
This path is not reachable without the clearer error firing first. _ffmpeg_decode is only called from the branch of decode_to_pcm where both sample_rate and channels are given, and that only happens for the second and later chunks of a stream, after AacStreamEncoder.__init__ has already run shutil.which("ffmpeg") and raised the explicit RuntimeError if it is missing. The first chunk goes through _read_wav_pcm or pydub, neither of which reaches this function.
So a missing ffmpeg surfaces as "Streaming audio output requires ffmpeg to be installed and on PATH." before any chunk is decoded here. Leaving it as is rather than adding a second check for a state the stream cannot be in.
Collaborator
Author
There was a problem hiding this comment.
Follow-up on the same subject, since round two raised a nearby path (decode_to_pcm at the pydub fallback) rather than this one.
That fallback only runs for the first chunk of a stream, and only when the bytes are not 16-bit PCM wav, so it is reached before AacStreamEncoder exists and a missing ffmpeg would indeed surface as pydub's own error. It is unchanged behaviour, though: main calls AudioSegment.from_file plus export(format="adts") on every chunk with no ffmpeg check at all, so this PR narrows that path rather than widening it, and adds an explicit message for the common case. Adding a second shutil.which guard in front of pydub would be a change to error reporting on a path this PR is not about, so I would rather leave it out here.
The other point from that round was worth taking: cleanup callbacks that raise in end_stream() are now logged at debug level with exc_info, matching what this module already does for the history writer, in c018837.
flush() closed stdin and reaped the process but left the stdout pipe to the garbage collector, so a caller that flushes and then drops the encoder, which is what flush_stream_output does, released the descriptor only on collection. flush() is the terminal call, so it now hands off to close() for that.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a hand-authored .changeset/*.md file (against repo guidelines) and has a couple of cleanup/error-reporting gaps that should be addressed before merging.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
gradio/audio_stream_encoder.py:97
- For the first chunk of a stream, decode_to_pcm() falls back to pydub/ffmpeg when the bytes aren’t a simple 16-bit PCM WAV. If ffmpeg isn’t on PATH, this path can fail with a low-level exception before AacStreamEncoder’s clearer "requires ffmpeg" error is reached. Consider checking for ffmpeg availability before calling AudioSegment.from_file() and raising the same explicit RuntimeError.
gradio/route_utils.py:1211 - Exceptions raised by stream cleanup callbacks are currently swallowed with no logging, which can make encoder/process leaks very hard to diagnose in production. Since failures are intentionally non-fatal here, consider at least logging at debug level with exception info.
.changeset/slick-cycles-send.md:5
- This repo’s contribution guidelines say not to hand-author .changeset/*.md files because a GitHub Action generates them from the PR title, and a manual changeset can silently override the changelog entry. This file should be removed from the PR.
---
"gradio": patch
---
fix:Encode streamed audio with one encoder per stream
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
end_stream() has to swallow these so a teardown failure cannot replace the error being propagated, but swallowing it silently hides a leaked encoder process. Same debug-level trace this module already uses for the history writer.
The comment density in the diff ran well above what these files carry, and some of it narrated the change rather than a non-obvious why. Kept the reasons a reader cannot get from the code: why six decimals, why the cleanup hangs off the stream, why the encoder registry is module level, and the warning not to make take() wait on a predicted frame count. The continuity test was three times the length of its neighbours. The signal's content does not matter to a frame count, so it is silence now.
Experiment, to be kept or reverted once CI has spoken. The backend job has failed on every run of this PR while main and the other open PRs are green, always one timing-sensitive test in test_history or test_queueing, never anything this change touches. Collection order is deterministic, so adding a test this early in the tree shifts every later test's xdist worker assignment; marking it serial restores the parallel step to main's exact 1326 tests and also takes the ffmpeg subprocess out of it.
This reverts commit f4e9db4.
Four findings from review, all measured rather than reasoned about.
#EXTINF was computed from the input sample rate, but the encoder only
accepts the rates an ADTS stream can declare and silently resamples
anything else, so a frame is 1024 samples at a rate the playlist knew
nothing about: a 20 kHz input claimed 1.1776 s of audio per 23 frames
where the media carried 1.0681 s, a 10.3% error in exactly the
accounting the six decimals were added for. The resample is now the
encoder's explicit choice and the frame duration follows it. Verified
against the sampling-frequency index in the emitted frames.
A wav that declares a `data` size of 0, which is the usual placeholder
when the length is not known yet, decoded to no audio at all and the
chunk was dropped without a word. pydub sent such a file through ffmpeg,
which reads it to EOF, so this was new. It falls through to ffmpeg now.
Nothing checked the encoder's exit code, so an ffmpeg that died partway
left a stream that quietly stopped growing and a playlist that got its
#EXT-X-ENDLIST as if all were well, where the old per-chunk path raised.
take() paid its whole 100 ms timeout on every chunk too short to
complete a frame. That timeout is for encoder startup, so it is paid
once now. Per stream_output call at 16 kHz mono, 25 chunks:
main before after
10 ms 162.9 93.2 9.3 ms
20 ms 160.4 77.3 8.3
32 ms 160.9 57.3 7.3
64 ms 160.6 21.3 5.0
250 ms 159.1 2.8 2.9
Which moves 20 ms chunks from a quarter of real time to 2.4x, so a
real-time generator at small chunk sizes is no longer starved by the
server. Gap counts and sizes are unchanged.
stream_output was handed `first_chunk` and ignored it, so the registry lookup alone decided whether a stream was new. The key carries `id(iterator)`, which CPython can hand to a later run (#13809), and a first chunk arriving under a reused key would have fed the new stream's audio to the previous stream's encoder: resampled to that stream's parameters, or an outright failure if the process had already gone. A first chunk now closes whatever it finds and starts its own.
A generator called straight through /gradio_api/run/{api_name} yields
once and is dropped: only the queue continues one, and it always carries
an event id, so without one `restore_session_state` hands back no
iterator at all. The run therefore reaches neither a final chunk nor the
exception path, and its streams stayed open.
That cost segment bytes before this branch. It costs a live ffmpeg
process now, one per abandoned run, held for as long as the session is:
twenty such calls measured at twenty processes and about 800 MiB. Ending
the streams there releases each encoder and gives the playlist the
#EXT-X-ENDLIST it is owed, since the audio that exists is all there will
ever be.
This does not touch the MediaStream itself, which stays in
pending_streams on this path exactly as it does on main. That retention
was looked at in the review of #13811 and left alone; nothing here
re-opens it.
To be reverted once CI has answered. The backend job has failed on every run of this PR, always on one of test_history's recording tests, while main and the other backend PRs are green. Locally the same tests fail on main's own sources, so this machine cannot tell the two apart; these prints are here to read the failure off a CI runner instead. They report whether the task is created, whether it starts, the thread inventory of that xdist worker at that moment, the write limiter's tokens, and which side of the to_thread offload it stops on. Note for whoever reads the result: a green run proves nothing here. The outcome flips locally on nothing more than whether stdout is a pipe or a file, so the probe is inside the race it measures.
This reverts commit 9c7cc45.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
gr.Audio(streaming=True)encoded every yielded chunk into its own AAC file. Each of those files opens with an encoder priming frame that has no overlap-add partner, so the player rendered it as a short silence and the stream broke up at every chunk boundary.Decoding what the server sends for an 8.000 s, 16 kHz signal fed in 4000-sample chunks, before and after:
mainThis keeps one ffmpeg encoder alive for the length of a stream and cuts the HLS segments out of its continuous output, so consecutive segments share encoder state and join up. The one remaining gap is the stream's own priming frame, which has nothing before it to join to. Replaying the segments in Chromium, the per-chunk gaps are gone.
The same thing shows up as a length: 33 gaps of 67.9 ms is 2.24 s, which is exactly how much longer than its 8.000 s source the stream decodes. The playlist does not account for it either, because
#EXTINFcarries each source chunk's duration, so it sums to 8.000 s while the media it describes is 10.240 s. After this change the two agree, 8.064 s against 8.064 s, which is what the new backend test asserts.Things worth knowing about the implementation:
The encoder command carries
-probesize 32 -analyzeduration 0. Without those, ffmpeg buffers before it emits anything and the first segment arrives about two seconds late, which would trade the gaps for a startup delay. Output is byte-identical either way.The encoder cannot live on the component, which is shared across sessions, or on the
MediaStream, which is created afterstream_outputreturns, so it lives in a module-level dict keyed by the stream's playlist path.Teardown runs from
end_stream(), and the paths that reach it are normal completion, the exception handler incall_process_api,/cancel, and the heartbeat's session cleanup. One more was missing and is added here: a generator called straight through/gradio_api/run/{api_name}yields once and is then dropped, because only the queue continues a generator and it always carries an event id. That run used to leave its streams open, which cost segment bytes before this change and would cost a live ffmpeg process after it, one per abandoned run. Twenty such calls measured at twenty processes and about 800 MiB before, none after.The teardown is registered as the handle from
weakref.finalize, which runs once and disarms itself, soend_stream()releases the encoder and leaves nothing armed to fire later against a key a newer run may own by then. What the unarmed handle covers is interpreter exit, not a dropped event stream: the session cleanup that discards aMediaStreamends it first, so the finalizer never gets there.There is still no idle timeout, so a slow generator is never killed mid-stream.
A first chunk closes any encoder still parked under its key rather than adopting it. The key carries
run, which isid(iterator): that is what Streaming run key isid(iterator), which CPython can reuse #13809 is about, andpending_streamsalready keys on it, so this change does not introduce the collision, but adopting a stale encoder would resample the new stream's audio to the old stream's parameters or fail outright on a process that had already gone. The keys become unique once Streaming run key isid(iterator), which CPython can reuse #13809 is fixed; until then the close is what makes a reused id harmless here.merge_generated_values_into_output(example caching) is a second, easily missed driver ofstream_output, and it passedoutput_id="". That was harmless while the id was only a URL, but as an encoder key it would have made every cached streaming example share one encoder and leak the process for the app's lifetime. It now uses a unique id per example and releases the encoder in afinally. It also flushes the tail, because that driver never finishes the stream and the frames still inside the encoder would be dropped. For the three yields oftest_caching_with_generators_and_streamed_output, the cached example comes out at 3.181x its input wheremaingives 3.435x, the difference being the three priming framesmainadds.Segment durations come from the emitted frame count rather than from the source chunk, and
#EXTINFcarries six decimals. Packed audio has no timestamps of its own, so a player derives each segment's start by accumulating those values, and three decimals cannot place a 1024-sample frame (0.0232199 s at 44.1 kHz) closely enough over a long stream.The encoder asks for its own resample rather than leaving one to ffmpeg. An AAC stream can only declare the rates in the ADTS table, and a frame is 1024 samples at whatever rate comes out, so a rate outside that table made every
#EXTINFwrong: 20 kHz in claimed 1.1776 s per 23 frames where the media carried 1.0681 s, 10.3% out. Checked against the sampling-frequency index in the emitted frames for 8000, 20000, 40000, 44100 and 6000 Hz.A dead encoder now raises instead of going quiet. Nothing checked ffmpeg's exit code, so a process that died partway left a stream that simply stopped growing and a playlist that still got its
#EXT-X-ENDLIST; the old per-chunk path raised out of pydub, so this would have been a step back.A wav chunk that declares a
datasize of 0, the usual placeholder when the length is not yet known, falls through to ffmpeg rather than being read as no audio at all. The stdlibwavemodule trusts the declared size, so such a chunk decoded to nothing and was dropped in silence; pydub sent it through ffmpeg, which reads to EOF.StreamingOutputgainsflush_stream_outputandend_stream_output. Both are concrete no-ops on the base class, so Video and custom streaming components are unaffected, but it is a little more public surface than a bug fix usually adds and I am happy to reshape it.Audio.covert_to_adtsgoes away with the per-chunk path; nothing in the tree called it and it is not in the docs, but the name had no underscore.What this does not cover, since the title could be read as broader than it is: the continuity guarantee is for wav and PCM input, which is what a numpy yield and
format="wav"produce. A generator yielding mp3 or opus still has each chunk decoded on its own, so any decoder-side delay and non-frame-aligned split survives into the PCM the encoder then joins up. That is unchanged frommainrather than a new gap, and the added test covers the wav path only.Two smaller things this does not change, in case they come up in review:
MediaStream.playlistandMediaStream.segment_indexwere dead fields and are already gone frommain, and the float#EXT-X-TARGETDURATIONwas already fixed there too.One part of the reported choppiness is outside this change. On a generator that produces audio in real time, hls.js also stalls, because the player is configured
maxBufferLength: 1, lowLatencyMode: true(js/audio/player/AudioPlayer.svelte) and has no slack when playback outruns delivery. That is pre-existing and independent of the encoder; holdingplay()until roughly a second is buffered removes it in my measurements, at the cost of the startup latency that config exists to minimise, so it is a product call rather than something to slip in here.That the stall is the player's rather than the server's is worth showing, because it was not true of
mainand would not have been true of a first draft of this change either. Wall time perstream_outputcall at 16 kHz mono, and the supply rate it implies for a generator that produces instantly:mainmaincosts a flat 160 ms per chunk whatever its size, being two pydub ffmpeg spawns, so below 250 ms chunks it cannot keep up with a real-time generator at all and no player config could hide that. This encoder is above real time at every size.Closes: #11733
Testing
Backend tests cover the continuity property directly: eight chunks through
stream_outputproduce one ADTS stream with a single priming frame, and the playlist's#EXTINFsum matches what the segments decode to.What I could and could not verify by hand, since the design leans on browser behaviour:
AudioContextnever leavessuspended. Covered indirectly instead, by checking decoder state across segments with WebCodecs and by checking append contiguity through MSE, both cross-validated against Chromium.maindoes not emit#EXT-X-DISCONTINUITYfor.aac, and this PR does not change that, so native-HLS players see the same playlist structure as before.os.readon a raw fd.AI Disclosure
We encourage the use of AI tooling in creating PRs, but the any non-trivial use of AI needs be disclosed. E.g. if you used Claude to write a first draft, you should mention that. Trivial tab-completion doesn't need to be disclosed. You should self-review all PRs, especially if they were generated with AI.
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Testing and Formatting Your Code
PRs will only be merged if tests pass on CI. We recommend at least running the backend tests locally, please set up your Gradio environment locally and run the backed tests:
bash scripts/run_backend_tests.shPlease run these bash scripts to automatically format your code:
bash scripts/format_backend.sh, and (if you made any changes to non-Python files)bash scripts/format_frontend.sh