-
Notifications
You must be signed in to change notification settings - Fork 208
Fix buffered AudioSource.capture_frame hanging when the source drain stalls #1289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
9bc008d
096a065
83e5c3f
a5c788c
fdd552e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| libwebrtc: patch | ||
| livekit: patch | ||
| livekit-ffi: patch | ||
| webrtc-sys: patch | ||
| --- | ||
|
|
||
| Bound the buffered `AudioSource::capture_frame` completion wait and split the drain lock so a stalled or wedged source drain returns a recoverable error instead of hanging the producer (and the session) forever (#408, #420, #497) - #1289 (@sam-hark) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,14 @@ use webrtc_sys::audio_track as sys_at; | |
|
|
||
| use crate::{audio_frame::AudioFrame, audio_source::AudioSourceOptions, RtcError, RtcErrorType}; | ||
|
|
||
| /// Floor for the per-chunk completion wait, for very small queues. | ||
| const CAPTURE_COMPLETE_TIMEOUT_FLOOR: std::time::Duration = std::time::Duration::from_secs(1); | ||
|
|
||
| /// Headroom multiple applied to the queue duration when bounding the completion wait: a healthy | ||
| /// drain acknowledges a chunk within one queue-duration, so this leaves margin before the wait is | ||
| /// treated as a stall. | ||
| const CAPTURE_COMPLETE_TIMEOUT_QUEUE_MULTIPLE: u64 = 5; | ||
|
|
||
| #[derive(Clone)] | ||
| pub struct NativeAudioSource { | ||
| sys_handle: SharedPtr<sys_at::ffi::AudioTrackSource>, | ||
|
|
@@ -153,6 +161,20 @@ impl NativeAudioSource { | |
| let _ = tx.send(()); | ||
| } | ||
|
|
||
| // Bound the per-chunk completion wait on a multiple of the queue duration. The C++ | ||
| // drain acknowledges a chunk from a 10ms RepeatingTask; legitimate backpressure clears | ||
| // within one queue-duration. A wait beyond that means the drain stalled (CPU-starved, | ||
| // or a sink blocked in OnData), so surface a recoverable error rather than hang the | ||
| // caller — and the whole session — forever (see rust-sdks #408 / #420 / #497). | ||
| let queue_ms = (self.queue_size_samples as u64) | ||
| .saturating_mul(1000) | ||
| .checked_div((self.sample_rate as u64) * (self.num_channels as u64)) | ||
| .unwrap_or(0); | ||
| let capture_timeout = std::time::Duration::from_millis( | ||
| queue_ms.saturating_mul(CAPTURE_COMPLETE_TIMEOUT_QUEUE_MULTIPLE), | ||
| ) | ||
| .max(CAPTURE_COMPLETE_TIMEOUT_FLOOR); | ||
|
|
||
| // iterate over chunks of self._queue_size_samples | ||
| for chunk in frame.data.chunks(self.queue_size_samples as usize) { | ||
| let nb_frames = chunk.len() / self.num_channels as usize; | ||
|
|
@@ -161,7 +183,9 @@ impl NativeAudioSource { | |
| let ctx_ptr = Box::into_raw(ctx) as *const sys_at::SourceContext; | ||
|
|
||
| unsafe { | ||
| // In the fast path, C++ never store / invoke on_complete / ctx. | ||
| // C++ only takes ownership of `ctx` when capture_frame returns true; on a false | ||
| // return (buffer full, or a prior completion still pending) it has not, so reclaim | ||
| // the Box here to avoid leaking the Sender. | ||
| if !self.sys_handle.capture_frame( | ||
| chunk, | ||
| self.sample_rate, | ||
|
|
@@ -170,14 +194,26 @@ impl NativeAudioSource { | |
| ctx_ptr, | ||
| sys_at::CompleteCallback(lk_audio_source_complete), | ||
| ) { | ||
| drop(Box::from_raw(ctx_ptr as *mut oneshot::Sender<()>)); | ||
| return Err(RtcError { | ||
| error_type: RtcErrorType::InvalidState, | ||
| message: "failed to capture frame".to_owned(), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
devin-ai-integration[bot] marked this conversation as resolved.
Outdated
|
||
| Ok(_) => {} | ||
| Err(_) => { | ||
| self.clear_buffer(); | ||
| return Err(RtcError { | ||
| error_type: RtcErrorType::InvalidState, | ||
| message: "audio capture timed out: source drain stalled".to_owned(), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| // Copyright 2026 LiveKit, Inc. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| //! Regression tests for the buffered `NativeAudioSource::capture_frame` drain stall | ||
| //! (rust-sdks #408 / #420 / #497): the C++ `AudioTrackSource` drain fires the per-chunk completion | ||
| //! that `capture_frame` awaits. If the drain stops progressing — a sink blocked in `OnData`, or the | ||
| //! drain TaskQueue CPU-starved — an unbounded await would wedge the producer (and any session built | ||
| //! on it) forever. | ||
|
|
||
| use crate::audio_frame::AudioFrame; | ||
| use crate::audio_source::native::NativeAudioSource; | ||
| use crate::audio_source::AudioSourceOptions; | ||
|
|
||
| const SAMPLE_RATE: u32 = 48000; | ||
|
|
||
| fn silent_frame<'a>(samples: usize) -> AudioFrame<'a> { | ||
| AudioFrame { | ||
| data: vec![0i16; samples].into(), | ||
| sample_rate: SAMPLE_RATE, | ||
| num_channels: 1, | ||
| samples_per_channel: samples as u32, | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn capture_frame_completes_under_normal_backpressure() { | ||
| // The fix must not regress the healthy path. A large queue keeps the derived timeout far above | ||
| // the ~one-queue-duration a healthy deferral needs, so this asserts "no false-trip on legitimate | ||
| // backpressure" without coupling to the production floor (which would make it flaky on a loaded | ||
| // CI runner). No sink/track is created, so this is safe on every platform. | ||
| const QUEUE_MS: u32 = 1000; | ||
| let q = (SAMPLE_RATE / 1000 * QUEUE_MS) as usize; | ||
| let source = NativeAudioSource::new(AudioSourceOptions::default(), SAMPLE_RATE, 1, QUEUE_MS); | ||
|
|
||
| for i in 0..3u32 { | ||
| // q + q/2 forces a deferral (completion routed through the drain) every call. | ||
| source | ||
| .capture_frame(&silent_frame(q + q / 2)) | ||
| .await | ||
| .unwrap_or_else(|e| panic!("healthy capture {i} failed: {e:?}")); | ||
| } | ||
| } | ||
|
|
||
| // The stall reproducer needs a real audio track, which requires a `PeerConnectionFactory`. On | ||
| // macOS/Windows that brings up the platform AudioDeviceModule, whose real-time audio thread aborts | ||
| // the process when the test's blocking sink stalls it. The Linux CI webrtc build has no such device | ||
| // backend, so the reproducer runs there and is deterministic. The fix itself is platform-independent | ||
| // and the healthy-path test above runs everywhere. This test also creates a factory, so — like the | ||
| // sibling factory tests — it relies on serial execution (CI runs `--test-threads=1`). | ||
| #[cfg(target_os = "linux")] | ||
| mod stall { | ||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||
| use std::sync::Arc; | ||
| use std::time::Duration; | ||
|
|
||
| use tokio::sync::mpsc; | ||
| use webrtc_sys::audio_track as sys_at; | ||
|
|
||
| use super::{silent_frame, SAMPLE_RATE}; | ||
| use crate::audio_source::native::NativeAudioSource; | ||
| use crate::audio_source::AudioSourceOptions; | ||
| use crate::peer_connection_factory::native::PeerConnectionFactoryExt; | ||
| use crate::peer_connection_factory::PeerConnectionFactory; | ||
| use crate::RtcErrorType; | ||
|
|
||
| /// A sink whose `on_data` blocks the C++ 10ms drain task, modelling a wedged/starved consumer. | ||
| /// It signals `entered` on its first call (so the test awaits the stall instead of sleeping for | ||
| /// it) and unblocks when `released` is set. | ||
| struct BlockingSink { | ||
| entered: mpsc::UnboundedSender<()>, | ||
| released: Arc<AtomicBool>, | ||
| } | ||
|
|
||
| impl sys_at::AudioSink for BlockingSink { | ||
| fn on_data( | ||
| &self, | ||
| _data: &[i16], | ||
| _sample_rate: i32, | ||
| _num_channels: usize, | ||
| _num_frames: usize, | ||
| ) { | ||
| let _ = self.entered.send(()); | ||
| while !self.released.load(Ordering::Acquire) { | ||
| std::thread::sleep(Duration::from_millis(5)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // worker_threads = 2: one for the (on a revert, possibly-wedged) capture task, one for the timeout. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn capture_frame_recovers_when_drain_stalls() { | ||
| const QUEUE_MS: u32 = 200; | ||
| let q = (SAMPLE_RATE / 1000 * QUEUE_MS) as usize; | ||
|
|
||
| let factory = PeerConnectionFactory::default(); | ||
| let source = | ||
| NativeAudioSource::new(AudioSourceOptions::default(), SAMPLE_RATE, 1, QUEUE_MS); | ||
| let track = factory.create_audio_track("drain-stall", source.clone()); | ||
|
|
||
| let (entered_tx, mut entered_rx) = mpsc::unbounded_channel(); | ||
| let released = Arc::new(AtomicBool::new(false)); | ||
| let native_sink = sys_at::ffi::new_native_audio_sink( | ||
| Box::new(sys_at::AudioSinkWrapper::new(Arc::new(BlockingSink { | ||
| entered: entered_tx, | ||
| released: released.clone(), | ||
| }))), | ||
| SAMPLE_RATE as i32, | ||
| 1, | ||
| ); | ||
| let audio = unsafe { sys_at::ffi::media_to_audio(track.sys_handle()) }; | ||
|
sam-hark marked this conversation as resolved.
|
||
| audio.add_sink(&native_sink); | ||
|
|
||
| // Wait until the drain is actually stalled inside the sink (bounded, so a wiring regression | ||
| // fails the test cleanly instead of hanging the CI job). | ||
| tokio::time::timeout(Duration::from_secs(5), entered_rx.recv()) | ||
| .await | ||
| .expect("drain never entered the sink within 5s") | ||
| .expect("sink signal channel closed"); | ||
|
|
||
| // Feed 2x the queue so the second chunk must wait on the (now stalled) drain. | ||
| let src = source.clone(); | ||
| let mut handle = tokio::spawn(async move { src.capture_frame(&silent_frame(q * 2)).await }); | ||
|
|
||
| // Outer bound distinguishes "returned" from "hung", well above the fix's own timeout. | ||
| let res = tokio::time::timeout(Duration::from_secs(8), &mut handle).await; | ||
|
|
||
| // Release the drain regardless, so teardown and the recovery check below can proceed. | ||
| released.store(true, Ordering::Release); | ||
| if res.is_err() { | ||
| let _ = handle.await; | ||
| } | ||
|
|
||
| match res { | ||
| Ok(Ok(Err(e))) => { | ||
| assert_eq!(e.error_type, RtcErrorType::InvalidState); | ||
| assert!( | ||
| e.message.contains("source drain stalled"), | ||
| "unexpected error: {}", | ||
| e.message | ||
| ); | ||
| } | ||
| other => { | ||
| panic!("capture_frame should return an error on a stalled drain, got: {other:?}") | ||
| } | ||
| } | ||
|
|
||
| // The stall must leave the source usable: the timeout path releases the pending completion | ||
| // rather than poisoning the slot, so a fresh capture succeeds once the drain is unstuck. | ||
| tokio::time::timeout(Duration::from_secs(5), source.capture_frame(&silent_frame(q))) | ||
| .await | ||
| .expect("recovery capture hung") | ||
| .expect("source did not recover after the stall cleared"); | ||
|
|
||
| audio.remove_sink(&native_sink); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.