Fix buffered AudioSource.capture_frame hanging when the source drain stalls - #1289
Fix buffered AudioSource.capture_frame hanging when the source drain stalls#1289sam-hark wants to merge 5 commits into
Conversation
07b1507 to
2ee4547
Compare
…in can't wedge the session The buffered NativeAudioSource awaited the C++ drain's per-chunk completion with no timeout, and the drain held its mutex across sink OnData, so a slow, starved, or wedged drain hung capture_frame — and any session built on it — forever (rust-sdks livekit#408, livekit#420, livekit#497). Bound the wait (scaled to the queue duration) and split the drain lock so a wedged sink blocks only sink add/remove, never the producer, without letting a sink be freed mid-OnData. clear_buffer now releases the pending completion so the source stays usable, and the false-return path reclaims the context it previously leaked. Adds regression tests (a Linux-gated blocking-sink reproducer plus a cross-platform healthy-path check).
2ee4547 to
9bc008d
Compare
| let _ = rx.await; | ||
| // Bound the wait for the drain's completion (timeout rationale above). On a stall, | ||
| // clear_buffer() releases the pending completion so the source stays usable afterward. | ||
| match tokio::time::timeout(capture_timeout, rx).await { |
There was a problem hiding this comment.
Curiously, in what setup or workload did you actually hit this timeout? My read is that normal Python/agents usage should await capture_frame() and therefore use it as backpressure, so this would only happen if the native webrtc drain stops making progress. Not sure how that could happen.
Was your repro tied to long TTS/audio bursts, high process load, CPU starvation, shutdown, or other workload sensitive conditions?
There was a problem hiding this comment.
My repro was tied to high process load mostly during long TTS playout, but it has also been very intermittent and difficult to pin down. To be honest, I can't claim this is definitely the root cause of audio stalls I've seen, but it's my leading cause.
It appears that under load the drain task stops getting scheduled, so the queue stays full and the capture_frame await never returns.
| const SourceContext* complete_ctx = nullptr; | ||
| { | ||
| webrtc::MutexLock lock(&buffer_mutex_); | ||
| scratch_.resize(samples10ms); |
There was a problem hiding this comment.
should we move this scratch_.resize(samples10ms); to the constructor ? I would think we don't need to call resize() for every tick.
| // context is freed rather than leaked. Serialized with the drain via buffer_mutex_, so the | ||
| // completion fires at most once. | ||
| if (on_complete_) { | ||
| on_complete_(capture_userdata_); |
There was a problem hiding this comment.
This is a known problem and not related to your change, but probably we shouldn't call on_complete_() under the buffer_mutex_ lock. Can you change the code to
void (complete)(const SourceContext) = nullptr;
const SourceContext* complete_ctx = nullptr;
{
webrtc::MutexLock lock(&buffer_mutex_);
buffer_.clear();
complete = on_complete_;
complete_ctx = capture_userdata_;
on_complete_ = nullptr;
capture_userdata_ = nullptr;
}
if (complete)
complete(complete_ctx);
}
| for (auto sink : sinks_) | ||
| sink->OnData(silence_buffer_.data(), kBitsPerSample, sample_rate_, | ||
| num_channels_, samples10ms / num_channels_); | ||
| sink->OnData(scratch_.data(), kBitsPerSample, sample_rate_, num_channels_, |
There was a problem hiding this comment.
Is sink->OnData() the main problem that could block forever ?
WebRTC’s send-side OnData is intended to be fast, this OnData just copies the frame and posts encoding work to an encoder task queue. It shouldn't block for long
There was a problem hiding this comment.
Likely not, I could remove this from the PR if preferred. This is just trying to enforce that the producer shouldn't be able to block the sink, and it enables adding a deterministic unit test so that the timeout fix can be directly tested.
- Size scratch_ once in the constructor instead of resize()-ing it on every 10ms drain tick. - clear_buffer(): snapshot the pending completion under buffer_mutex_ and fire it with no lock held, matching the drain path (no callback under the lock). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
tokio::time::timeout panics when awaited off a Tokio runtime, so calling the public capture_frame from another executor (async-std/smol/block_on) would crash instead of sending audio. Use livekit_runtime::timeout — the runtime- neutral abstraction already used elsewhere in libwebrtc (video_source) — and drop the now-unused tokio "time" feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Before you submit your PR
PR description
Summary
NativeAudioSource::capture_framecompletion wait (scaled to the queue duration) so a stalled source drain returns a recoverableRtcErrorinstead of hanging forever.AudioTrackSource::InternalSourcelock so the drain'ssink->OnData()no longer runs under the same mutex as the producer — a wedged/slow sink can no longer blockcapture_frame, while a sink still cannot be freed mid-OnData.clear_buffer, release the pending completion so the source stays usable; reclaim the completion context on thefalsereturn path (previously leaked).Problem
A buffered native audio source (
queue_size_ms > 0, the default used bylivekit-agents'_ParticipantAudioOutput) can hangcapture_frameforever, silently killing outbound audio while the room still reportsConnected. Reported repeatedly and worked around locally, never fixed upstream:Root cause
The buffered source is a producer/consumer pair across the FFI:
webrtc-sys/src/audio_track.cpp,AudioTrackSource::InternalSource: a 10 msRepeatingTaskdrains the buffer, pushes 10 ms to each sink viasink->OnData(...), and — only when it runs — fires the deferred completion for the chunk that filled the buffer past the notify threshold.libwebrtc/src/native/audio_source.rs,NativeAudioSource::capture_frame: for each chunk it hands C++ aoneshot::Senderandrx.awaits that completion with no timeout.The drain held
mutex_across the entireOnDataloop. So when the drain is slow or CPU-starved, it is almost always holding the lock, and the producer's synchronous FFIcapture_frameblocks acquiring that same mutex — often before it even reaches the awaited completion. Either way,capture_framenever returns:_wait_for_playoutnever completes, generation tasks pile up, and the session goes permanently mute while ICE stays connected.Fix
audio_track.cpp/audio_track.h— split the lock.sink_mutex_guardssinks_and is held across theOnDataloop;buffer_mutex_guards the buffer and completion state. The producer (capture_frame) only ever takesbuffer_mutex_, so a wedged/slow sink can no longer block it — whileRemoveSinkstill serializes against an in-flightOnData, so a sink cannot be freed mid-call. The drain copies the 10 ms frame into a reused scratch buffer (no per-tick allocation).audio_source.rs— bound the wait.rx.await→tokio::time::timeout(...), scaled to the queue duration (5 × queue_ms, floored at 1 s) so legitimate backpressure never trips it. On timeout,clear_buffer()and returnRtcError { InvalidState }.clear_buffer()now also releases any pending completion (freeing its context and clearing the slot), so a freshcapture_framesucceeds after a stall clears rather than erroring forever. Thefalse-return path now reclaims the boxedSenderit previously leaked.The timeout and the lock split are coupled: with the drain holding the mutex across
OnData, the producer blocks in the FFI before the await, so bounding the await alone would be dead code in exactly the scenario it targets.Breaking changes
None.
capture_framegains a recoverableRtcErrorfor the stall case, replacing an unobservable permanent hang; callers already handlecapture_frameerrors.MSRV
Unchanged.
Testing
libwebrtc/src/native/drain_stall_tests.rs, exercised by the standardcargo test:capture_frame_recovers_when_drain_stalls— attaches a sink whoseOnDatablocks (a deterministic stalled consumer) to a realAudioTrackSource, feedscapture_frame, asserts it returns the drain-stallRtcError(bounded so a wiring regression fails cleanly rather than hanging CI), then — after the stall clears — asserts a freshcapture_framesucceeds (the source is not poisoned). Gated to Linux: it needs a real audio track, and on macOS/Windows that brings up the platform AudioDeviceModule, whose real-time thread aborts the process when the blocking sink stalls it. The fix is platform-independent.capture_frame_completes_under_normal_backpressure— runs on every platform (no track/sink); with the drain running, deferred captures completeOk, confirming the timeout does not false-trip on legitimate backpressure. Uses a large queue so the assertion is not coupled to the production floor.Verified on a real webrtc build: stock hangs; timeout-without-lock-split still hangs (producer wedged on the mutex); the combined fix returns an error in ~1 s and the source recovers; the full
libwebrtclib suite passes.Async
The bounded wait is the fix itself; the test also uses
tokio::time::timeoutto detect a hang, mirroring the existingrenegotiation_does_not_deadlocktest, and subscribes to a channel signalled the instant the drain enters the sink rather than sleeping for it.Out of scope (deferred)
capture_frameremains a synchronous FFI call that can block its caller's task while the drain catches up; a fuller change might make the enqueue non-blocking (or run it off the async runtime). This PR keeps that shape and only ensures the wait is bounded and the lock no longer couples the producer to a wedged sink.