Make AdmProxy worker-thread-affine#1212
Open
hiroshihorie wants to merge 7 commits into
Open
Conversation
Platform ADM implementations (Windows CoreAudio2, iOS, the upcoming Apple AudioEngine device) bind a sequence checker to their construction thread and expect every control call on it. Previously AdmProxy called the platform ADM directly on whatever thread the caller was on, guarded only by a mutex, which violates that contract and races with WebRTC's own worker-thread calls. AdmProxy now owns all of its state on the worker thread. Every public method marshals once at the boundary via RunOnWorker (inline when the caller is already the worker, which covers all WebRTC-internal calls) and mode transitions execute as plain sequential worker code. This removes the mutex entirely along with the races it couldn't prevent: stale mode snapshots interleaving transitions, and the synthetic ADM being driven from caller threads. The proxy must be constructed on the worker thread (the factory already does this) so the eagerly created platform ADM binds to it. Destruction may happen on any thread and hops to the worker for teardown.
On Android the platform ADM is created lazily on first acquire, after WebRTC has already registered its audio transport with the proxy and after the app may have selected devices. Pass the stored transport and selection along to the newly created ADM. Previously the transport was never registered, so platform recording delivered no audio to the pipeline.
Rust fetches the controller per call, so a controller copy can briefly be the last reference to the AdmProxy on a Rust thread during factory teardown. The proxy marshals onto the runtime's worker thread, so the runtime must outlive every reference to the proxy that Rust can reach. Follows the existing pattern used by AudioTrack and VideoTrack.
The doc described a lazy create/terminate-at-ref-0 lifecycle and an in-proxy stub playout task that no longer exist. Document the actual design: eager platform ADM creation kept until proxy destruction, SyntheticAudioDevice as a separate sub ADM, and the worker-thread-affine threading model.
The selected device GUIDs were captured on every SetPlayoutDevice and SetRecordingDevice call but never read anywhere. They were meant for restoring selection across ADM re-creation, which no longer happens, and the one re-creation path left (Android lazy creation) re-applies by index. Keep only the indices and document the index 0 caveat.
Contributor
ChangesetThe following package versions will be affected by this PR:
|
Demonstrates the PlatformAudio API and doubles as an offline regression check for the worker-thread-affine AdmProxy: lifecycle and full runtime teardown/reacquire cycles, device enumeration/selection/hot-swap, recording on real hardware, audio processing configuration, and heavy concurrent access from many threads. Runs without a LiveKit server, needs audio hardware and mic permission so it lives as an example rather than a CI test.
d6c7b0a to
58e946b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors AdmProxy (in webrtc-sys) to be worker-thread-affine so all platform ADM access and proxy state mutations occur on the WebRTC worker thread, eliminating previous mutex-based cross-thread access and aligning with platform ADM sequence-checker expectations.
Changes:
- Make
AdmProxymarshal all public calls onto the WebRTC worker thread (RunOnWorker/BlockingCall) and remove the internal mutexes. - Fix Android’s lazily-created ADM not receiving the previously-registered
AudioTransport(and re-apply pre-create device selection). - Keep
RtcRuntimealive as long as Rust can reach the audio controller, and add aplatform_audioexample plus updated design docs and changeset.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| webrtc-sys/src/peer_connection_factory.cpp | Pass RtcRuntime into AudioDeviceController to extend runtime/thread lifetime across Rust reachability. |
| webrtc-sys/src/audio_device_controller.cpp | Store a shared_ptr<RtcRuntime> alongside AdmProxy to prevent worker-thread teardown races. |
| webrtc-sys/src/adm_proxy.cpp | Rework all ADM operations to run on the worker thread; remove mutex-based protection; add Android transport registration on lazy creation. |
| webrtc-sys/include/livekit/audio_device_controller.h | Update controller constructor and retain runtime reference for safe worker-thread marshaling lifetime. |
| webrtc-sys/include/livekit/adm_proxy.h | Document the threading model and add RunOnWorker/WithPlatformAdm helpers + worker-thread guarded state annotations. |
| livekit/examples/platform_audio.rs | Add an offline exerciser example for PlatformAudio/AdmProxy lifecycle, concurrency, and device churn. |
| docs/ADM_PROXY_DESIGN.md | Update the design doc to reflect eager/idle platform ADM lifecycle and the worker-thread-affine model. |
| .changeset/adm-proxy-worker-thread.md | Add a changeset documenting the behavioral fixes and example addition. |
Comments suppressed due to low confidence (2)
webrtc-sys/src/adm_proxy.cpp:282
- SwitchPlayoutMode() calls InitPlayout()/StartPlayout() without checking their return codes. If either fails, synthetic playout has already been stopped and the proxy can end up in a "playing_ = true" state with no active playout, leading to silent audio loss. Consider checking the return values, logging failures, and falling back to synthetic playout when platform startup fails.
if (IsPlatformPlayoutActive()) {
// Switch to platform mode - stop synthetic, start platform ADM
if (synthetic_adm_) {
synthetic_adm_->StopPlayout();
}
if (platform_adm_) {
platform_adm_->InitPlayout();
platform_adm_->StartPlayout();
}
webrtc-sys/src/adm_proxy.cpp:307
- SwitchRecordingAdm() ignores the return values from InitRecording()/StartRecording(). If these calls fail, recording_ remains true and callers will observe "recording" even though the platform ADM failed to start, which can cause confusing behavior and make failures hard to diagnose. Consider checking the return codes, logging failures, and clearing recording_ on failure.
// Start if new ADM supports recording
auto* adm = RecordingAdm();
if (adm) {
adm->InitRecording();
adm->StartRecording();
} else {
recording_ = false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
What
Rewrites
AdmProxy(webrtc-sys) so that every touch of the platform ADM, the synthetic ADM, and all proxy state happens on the WebRTC worker thread. Every public method marshals once at the boundary (RunOnWorker, aBlockingCallthat runs inline when the caller is already the worker), all mutable state is worker-owned and annotatedRTC_GUARDED_BY(worker_thread_), and both mutexes are removed. Mode transitions are now plain sequential worker code.Why
Platform ADM implementations (Windows CoreAudio2, iOS, and the upcoming Apple AudioEngine device) bind a sequence checker to their construction thread and
RTC_DCHECK_RUN_ONevery control method. Previously the proxy called the platform ADM directly on whatever thread the Rust FFI caller was on, guarded only by a mutex — violating that contract and racing with WebRTC's own worker-thread calls. This is a prerequisite for switching the Apple platform ADM to the AudioEngine device (follow-up: #1215, building on the now-merged webrtc-sdk/webrtc#260).Making the proxy thread-affine removes by construction the races a mutex couldn't prevent: stale mode snapshots interleaving transitions, the synthetic ADM being driven from caller threads, and
Terminate()racing mode switches.This mirrors upstream WebRTC's own pattern:
RTCPeerConnectionFactoryconstructs the ADM inside_workerThread->BlockingCall(...), andWebRtcVoiceEngine/AudioStateguard every ADM call with worker-thread sequence checkers.Also fixed along the way
AudioTransportat factory init, before the Android ADM exists;EnsurePlatformAdmCreatednow passes it (and any pre-acquire device selection) to the fresh ADM. Previously platform recording delivered no audio to the pipeline.AudioDeviceControllernow holds ashared_ptr<RtcRuntime>(same pattern as AudioTrack/VideoTrack) so the worker thread outlives every proxy reference reachable from Rust.playout_initialized_/recording_initialized_flags and never-read device GUID tracking.docs/ADM_PROXY_DESIGN.mdupdated to match the actual lifecycle (eager idle ADM, release does not terminate) and to document the threading model.Threading contract
BlockingCall); the eagerly created platform ADM binds its sequence checker there.AudioTransportdata) are NOT marshaled — they flow directly through the sub-ADMs' own I/O threads, matching the platform ADMs' design (separate io/render thread checkers).Behavior parity
Apart from the marshaling and the fixes listed above, behavior is unchanged: all no-platform-ADM defaults, state-update ordering, and gate semantics match the previous implementation method-for-method.
Testing
cargo check -p webrtc-sysandcargo build -p livekitpass.BlockingCallboundary (factory teardown ordering, dtor-on-any-thread, SyntheticAudioDevice task queue). Threading contract verified against the ADM implementations in webrtc-sdk (Windows/iOS/AudioEngine sequence checkers, voice engine and AudioState worker guards).cargo run -p livekit --example platform_audio— runs offline, no LiveKit server. Passing on macOS (arm64):playing_ = trueswitch branch) and DCHECK-level thread validation (release prebuilt compilesRTC_DCHECKout; needs a debug webrtc viaLK_CUSTOM_WEBRTC).