Repository navigation
webrtc-sys: keep NativeAudioSource send streams out of AudioState - #1491
Conversation
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| - if (sending_) { | ||
| + // External sources never registered with AudioState (see Start()), and | ||
| + // must not be added here either: Stop() would not remove them again. | ||
| + if (sending_ && !config_.external_source) { |
There was a problem hiding this comment.
🔴 Existing WebRTC artifact retains audio crash
For default builds, StoreEncoderProperties() still runs from the unchanged prebuilt WebRTC artifact. WEBRTC_TAG selects that artifact, so publishing and removing native audio tracks during platform capture can still crash.
Learn more
The default build links a downloaded WebRTC static library rather than compiling this patch. WEBRTC_TAG still names the existing release, and download_url selects its unchanged archive. The new condition exists only in a WebRTC build made from the updated patch. Until a patched archive is published and selected, default SDK builds retain the dangling send-stream behavior under active platform capture.
Example: An application installs this SDK update without setting LK_CUSTOM_WEBRTC. It downloads the existing webrtc-89d790b archive, then repeatedly publishes and removes a stereo NativeAudioSource track while PlatformAudio records. The archive still registers the external stream in AudioState, so the capture thread can reach it after destruction.
Recommended fix: Build and publish patched WebRTC artifacts for supported targets, update WEBRTC_TAG to the new artifact release, and verify a default SDK build uses them.
Was this helpful? React with 👍 or 👎 to provide feedback.
The m150 port of external_audio_source.patch (livekit#1284) dropped `sending_ = false;` from AudioSendStream::Stop(), so a stopped stream stayed sending. StoreEncoderProperties() then adds it to AudioState whenever a new encoder is set up, and does not check config_.external_source, while Stop() only unregisters non-external streams. A NativeAudioSource stream added that way was never removed, and with PlatformAudio recording, the capture thread called into it from AudioTransportImpl::SendProcessedData after it was destroyed. Keep `sending_ = false;` in Stop() and leave external sources out of AudioState in StoreEncoderProperties(), as Start() already does. Fixes livekit#1490
2724c41 to
fa5e901
Compare
This change only edits the patch, so no released crate picks it up until a new WebRTC build is published and WEBRTC_TAG points at it. The changeset moves to that bump PR so the changelog lands with the fix.
alan-george-lk
left a comment
alan-george-lk
left a comment
There was a problem hiding this comment.
This fixed a crash in a platform audio unit test in the C++ SDK, appreciate it! Note that Devin.ai flagged a red-level warning
hiroshihorie
added
the
internal
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Audio source fix has no changeset
Deleting this changeset leaves the audio source patch without a release entry. The remaining changeset covers a different change, so knope cannot document this fix.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
4 participants
Fixes #1490. The m150 port of
external_audio_source.patch(#1284) droppedsending_ = false;fromAudioSendStream::Stop(), andStoreEncoderProperties()adds streams to AudioState without checkingconfig_.external_source. A NativeAudioSource stream added there is never removed, and the ADM capture thread later calls into it fromAudioTransportImpl::SendProcessedDataafter it is destroyed.This restores
sending_ = false;inStop()and skipsAddSendingStreaminStoreEncoderProperties()for external sources. The updated patch applies cleanly withgit applytoaudio/audio_send_stream.ccat blobe9e72b48e7, the file the current patch targets. Tested by recompiling that file into the webrtc-89d790blibwebrtc.a: a program that records with PlatformAudio and repeatedly publishes and removes a stereo NativeAudioSource track crashed 4 of 4 runs before and 0 of 4 after. Shipping it needs a new WebRTC build.