[Fix] Prevent AVAudioEngine crash when enabling recording - #103
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe audio engine now tracks input/output availability, exposes native and Objective-C availability APIs, gates engine state queries, and logs availability. Voice-processing configuration now propagates failures through bounded engine recreation retries, rollback, and recovery. An iOS XCTest covers repeated recording transitions while playout and voice processing remain active. Audio engine availability and public bindings
Voice-processing recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RTCAudioDeviceModule
participant WorkerThread
participant AudioEngineDevice
participant AVAudioEngine
RTCAudioDeviceModule->>WorkerThread: setEngineAvailability(input, output)
WorkerThread->>AudioEngineDevice: SetEngineAvailability(input, output)
AudioEngineDevice-->>WorkerThread: availability result
AudioEngineDevice->>AVAudioEngine: configure voice processing
AVAudioEngine-->>AudioEngineDevice: success or native failure
AudioEngineDevice->>AVAudioEngine: release and recreate for bounded retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ipavlidakis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
modules/audio_device/audio_engine_device.h (1)
421-425: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
input_topologyalso fires on everyMuteMode::RestartEnginemute toggle.
IsInputEnabled()returns false whenmute_mode == RestartEngine && input_muted, so a mute/unmute in that mode flipsprev.IsInputEnabled() != next.IsInputEnabled()while output stays enabled. Those transitions previously took the cheaper restart path; they now tear down and rebuild the whole graph (plus, on failure, the new retry/recovery path). If only genuine record enable/disable should recreate, consider keying oninput_enabled/input_enabled_persistent_moderather than the mute-aware helper.♻️ Possible narrowing
- bool input_topology = prev.IsOutputEnabled() && next.IsOutputEnabled() && - prev.IsInputEnabled() != next.IsInputEnabled(); + bool input_topology = prev.IsOutputEnabled() && next.IsOutputEnabled() && + (prev.input_enabled != next.input_enabled || + prev.input_available != next.input_available);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/audio_device/audio_engine_device.h` around lines 421 - 425, Update the input_topology condition to detect only genuine input enablement changes, using the persistent input configuration fields such as input_enabled or input_enabled_persistent_mode instead of mute-aware IsInputEnabled(). Preserve recreation for record enable/disable while keeping RestartEngine mute/unmute transitions on the existing cheaper restart path.sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm (2)
199-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoop does not assert the regression's distinguishing signal.
The iteration only checks recording/playing/VP flags, which would also pass if VP recovery silently fell back. Consider asserting
audioDeviceModule.isEngineRunningand, since VP is mandatory per the PR objectives, thatinitAndStartRecordingnever returns the VP error code, so a degraded-but-running graph still fails the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm` around lines 199 - 208, Strengthen the loop in the audio engine regression test by asserting audioDeviceModule.isEngineRunning after initAndStartRecording, and verify that initAndStartRecording never returns the VP error code. Keep the existing recording, voice-processing, stop, and playing assertions so degraded VP fallback is detected while preserving the current lifecycle checks.
178-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest mutates the shared
AVAudioSessionwithout restoring it, and re-asserts a stalesessionError.The category/mode change to
PlayAndRecord/voiceChatpersists after this test and can affect the other tests in this class (execution order is not guaranteed). Also,sessionErroris never reset between calls; Cocoa only guarantees the error out-param is meaningful when the call returnsNO, so assertingXCTAssertNil(sessionError)after a successful call is not contract-guaranteed. Capture the previous category/mode insetUp/restore intearDown(oraddTeardownBlock:) and resetsessionError = nilbefore each call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm` around lines 178 - 216, Update the test setup and cleanup around the shared AVAudioSession to capture its original category and mode in setUp (or an addTeardownBlock) and restore them after the test, including deactivating the session as needed. In the test’s AVAudioSession configuration calls, reset sessionError to nil immediately before each setCategory, setActive, and restoration call, then assert the result and error for that specific call.modules/audio_device/audio_engine_device.mm (1)
2154-2155: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff100 ms
usleepblocks the ADM control thread.Each retry blocks
thread_synchronously (up to 200 ms here, more if the recovery path recurses, and macOS start already sleeps another 100 ms per start attempt). SinceModifyEngineStateis reached viaBlockingCallfrom the Objective-C layer, this stalls the caller too. Consider whether the delay can be shortened or the retry rescheduled viaPostDelayedTask.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/audio_device/audio_engine_device.mm` around lines 2154 - 2155, Replace the synchronous usleep delay in the AVAudioEngine recovery/retry path with a non-blocking delayed task, such as PostDelayedTask, so ModifyEngineState and its BlockingCall caller are not stalled. Preserve the existing retry ordering and delay semantics while ensuring subsequent engine-state work runs asynchronously on the appropriate thread; if rescheduling is not viable, reduce the blocking delay to the minimum required interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/audio_device/audio_engine_device.mm`:
- Around line 2180-2201: Update the VP recovery path around recovery_state and
ApplyDeviceEngineState so recovery cannot re-enter the same voice-processing
retry block. Use an explicit recovery guard flag or construct recovery_state
with input/voice processing disabled while retaining the required speaker-only
state, then preserve the existing release and error-result handling.
In `@sdk/objc/api/peerconnection/RTCAudioDeviceModule.mm`:
- Around line 586-605: Update the five RTCAudioEngineAvailability constructions
in engineAvailability to use braced aggregate initialization, replacing
parenthesized boolean arguments with {false, false} or {input_available,
output_available} as appropriate. Preserve the existing availability and
error-handling branches.
---
Nitpick comments:
In `@modules/audio_device/audio_engine_device.h`:
- Around line 421-425: Update the input_topology condition to detect only
genuine input enablement changes, using the persistent input configuration
fields such as input_enabled or input_enabled_persistent_mode instead of
mute-aware IsInputEnabled(). Preserve recreation for record enable/disable while
keeping RestartEngine mute/unmute transitions on the existing cheaper restart
path.
In `@modules/audio_device/audio_engine_device.mm`:
- Around line 2154-2155: Replace the synchronous usleep delay in the
AVAudioEngine recovery/retry path with a non-blocking delayed task, such as
PostDelayedTask, so ModifyEngineState and its BlockingCall caller are not
stalled. Preserve the existing retry ordering and delay semantics while ensuring
subsequent engine-state work runs asynchronously on the appropriate thread; if
rescheduling is not viable, reduce the blocking delay to the minimum required
interval.
In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm`:
- Around line 199-208: Strengthen the loop in the audio engine regression test
by asserting audioDeviceModule.isEngineRunning after initAndStartRecording, and
verify that initAndStartRecording never returns the VP error code. Keep the
existing recording, voice-processing, stop, and playing assertions so degraded
VP fallback is detected while preserving the current lifecycle checks.
- Around line 178-216: Update the test setup and cleanup around the shared
AVAudioSession to capture its original category and mode in setUp (or an
addTeardownBlock) and restore them after the test, including deactivating the
session as needed. In the test’s AVAudioSession configuration calls, reset
sessionError to nil immediately before each setCategory, setActive, and
restoration call, then assert the result and error for that specific call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdd417bf-c9cd-48e8-9047-9dbee05ea03c
📒 Files selected for processing (5)
modules/audio_device/audio_engine_device.hmodules/audio_device/audio_engine_device.mmsdk/objc/api/peerconnection/RTCAudioDeviceModule.hsdk/objc/api/peerconnection/RTCAudioDeviceModule.mmsdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (4)
modules/audio_device/audio_engine_device.h (1)
421-425: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
input_topologyalso fires on everyMuteMode::RestartEnginemute toggle.
IsInputEnabled()returns false whenmute_mode == RestartEngine && input_muted, so a mute/unmute in that mode flipsprev.IsInputEnabled() != next.IsInputEnabled()while output stays enabled. Those transitions previously took the cheaper restart path; they now tear down and rebuild the whole graph (plus, on failure, the new retry/recovery path). If only genuine record enable/disable should recreate, consider keying oninput_enabled/input_enabled_persistent_moderather than the mute-aware helper.♻️ Possible narrowing
- bool input_topology = prev.IsOutputEnabled() && next.IsOutputEnabled() && - prev.IsInputEnabled() != next.IsInputEnabled(); + bool input_topology = prev.IsOutputEnabled() && next.IsOutputEnabled() && + (prev.input_enabled != next.input_enabled || + prev.input_available != next.input_available);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/audio_device/audio_engine_device.h` around lines 421 - 425, Update the input_topology condition to detect only genuine input enablement changes, using the persistent input configuration fields such as input_enabled or input_enabled_persistent_mode instead of mute-aware IsInputEnabled(). Preserve recreation for record enable/disable while keeping RestartEngine mute/unmute transitions on the existing cheaper restart path.sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm (2)
199-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLoop does not assert the regression's distinguishing signal.
The iteration only checks recording/playing/VP flags, which would also pass if VP recovery silently fell back. Consider asserting
audioDeviceModule.isEngineRunningand, since VP is mandatory per the PR objectives, thatinitAndStartRecordingnever returns the VP error code, so a degraded-but-running graph still fails the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm` around lines 199 - 208, Strengthen the loop in the audio engine regression test by asserting audioDeviceModule.isEngineRunning after initAndStartRecording, and verify that initAndStartRecording never returns the VP error code. Keep the existing recording, voice-processing, stop, and playing assertions so degraded VP fallback is detected while preserving the current lifecycle checks.
178-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest mutates the shared
AVAudioSessionwithout restoring it, and re-asserts a stalesessionError.The category/mode change to
PlayAndRecord/voiceChatpersists after this test and can affect the other tests in this class (execution order is not guaranteed). Also,sessionErroris never reset between calls; Cocoa only guarantees the error out-param is meaningful when the call returnsNO, so assertingXCTAssertNil(sessionError)after a successful call is not contract-guaranteed. Capture the previous category/mode insetUp/restore intearDown(oraddTeardownBlock:) and resetsessionError = nilbefore each call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm` around lines 178 - 216, Update the test setup and cleanup around the shared AVAudioSession to capture its original category and mode in setUp (or an addTeardownBlock) and restore them after the test, including deactivating the session as needed. In the test’s AVAudioSession configuration calls, reset sessionError to nil immediately before each setCategory, setActive, and restoration call, then assert the result and error for that specific call.modules/audio_device/audio_engine_device.mm (1)
2154-2155: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff100 ms
usleepblocks the ADM control thread.Each retry blocks
thread_synchronously (up to 200 ms here, more if the recovery path recurses, and macOS start already sleeps another 100 ms per start attempt). SinceModifyEngineStateis reached viaBlockingCallfrom the Objective-C layer, this stalls the caller too. Consider whether the delay can be shortened or the retry rescheduled viaPostDelayedTask.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/audio_device/audio_engine_device.mm` around lines 2154 - 2155, Replace the synchronous usleep delay in the AVAudioEngine recovery/retry path with a non-blocking delayed task, such as PostDelayedTask, so ModifyEngineState and its BlockingCall caller are not stalled. Preserve the existing retry ordering and delay semantics while ensuring subsequent engine-state work runs asynchronously on the appropriate thread; if rescheduling is not viable, reduce the blocking delay to the minimum required interval.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/audio_device/audio_engine_device.mm`:
- Around line 2180-2201: Update the VP recovery path around recovery_state and
ApplyDeviceEngineState so recovery cannot re-enter the same voice-processing
retry block. Use an explicit recovery guard flag or construct recovery_state
with input/voice processing disabled while retaining the required speaker-only
state, then preserve the existing release and error-result handling.
In `@sdk/objc/api/peerconnection/RTCAudioDeviceModule.mm`:
- Around line 586-605: Update the five RTCAudioEngineAvailability constructions
in engineAvailability to use braced aggregate initialization, replacing
parenthesized boolean arguments with {false, false} or {input_available,
output_available} as appropriate. Preserve the existing availability and
error-handling branches.
---
Nitpick comments:
In `@modules/audio_device/audio_engine_device.h`:
- Around line 421-425: Update the input_topology condition to detect only
genuine input enablement changes, using the persistent input configuration
fields such as input_enabled or input_enabled_persistent_mode instead of
mute-aware IsInputEnabled(). Preserve recreation for record enable/disable while
keeping RestartEngine mute/unmute transitions on the existing cheaper restart
path.
In `@modules/audio_device/audio_engine_device.mm`:
- Around line 2154-2155: Replace the synchronous usleep delay in the
AVAudioEngine recovery/retry path with a non-blocking delayed task, such as
PostDelayedTask, so ModifyEngineState and its BlockingCall caller are not
stalled. Preserve the existing retry ordering and delay semantics while ensuring
subsequent engine-state work runs asynchronously on the appropriate thread; if
rescheduling is not viable, reduce the blocking delay to the minimum required
interval.
In `@sdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm`:
- Around line 199-208: Strengthen the loop in the audio engine regression test
by asserting audioDeviceModule.isEngineRunning after initAndStartRecording, and
verify that initAndStartRecording never returns the VP error code. Keep the
existing recording, voice-processing, stop, and playing assertions so degraded
VP fallback is detected while preserving the current lifecycle checks.
- Around line 178-216: Update the test setup and cleanup around the shared
AVAudioSession to capture its original category and mode in setUp (or an
addTeardownBlock) and restore them after the test, including deactivating the
session as needed. In the test’s AVAudioSession configuration calls, reset
sessionError to nil immediately before each setCategory, setActive, and
restoration call, then assert the result and error for that specific call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdd417bf-c9cd-48e8-9047-9dbee05ea03c
📒 Files selected for processing (5)
modules/audio_device/audio_engine_device.hmodules/audio_device/audio_engine_device.mmsdk/objc/api/peerconnection/RTCAudioDeviceModule.hsdk/objc/api/peerconnection/RTCAudioDeviceModule.mmsdk/objc/unittests/RTCPeerConnectionFactoryAudioEngine_xctest.mm
🛑 Comments failed to post (1)
sdk/objc/api/peerconnection/RTCAudioDeviceModule.mm (1)
586-605: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Determine the C++ standard used for the ObjC++ SDK targets. rg -n --hidden -g '!*.md' -e 'cxx_std|-std=c\+\+|cflags_cc|std:c\+\+' BUILD.gn build_overrides/ build/config/compiler/BUILD.gn 2>/dev/null | head -50 fd -H -t f 'BUILD.gn' sdk/objc | headRepository: GetStream/webrtc
Length of output: 217
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate relevant source/header/build files ==" fd -H -t f 'RTCAudioEngineAvailability|RTCAudioEngine' sdk/objc . 2>/dev/null | head -100 fd -H -t f 'BUILD.gn' sdk/objc . 2>/dev/null | head -100 echo echo "== inspect SDK source/imports and availability definitions ==" rg -n --hidden -g '!*.md' -e 'RTCAudioEngineAvailability|AudioEngineAvailability|class RTCAudioEngineAvailability|struct RTCAudioEngineAvailability|typedef .*RTCAudioEngineAvailability|using RTCAudioEngineAvailability' . 2>/dev/null | head -200 echo echo "== candidate files with line counts ==" for f in $(fd -H -t f 'RTCAudioEngineAvailability|RTCAudioEngine.*\.h|RTCAudioEngine.*\.mm' sdk/objc . 2>/dev/null); do echo "--- $f" wc -l "$f" done echo echo "== inspect BUILD.gn cxx settings near matches ==" grep -n -C 2 'cflags_cc\|cxx_std\|-std=c++\|std:c++\|objc' BUILD.gn sdk -R 2>/dev/null | head -200Repository: GetStream/webrtc
Length of output: 12863
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== inspect peerconnection header/implementation around availability ==" sed -n '45,90p' sdk/objc/api/peerconnection/RTCAudioDeviceModule.h sed -n '540,615p' sdk/objc/api/peerconnection/RTCAudioDeviceModule.mm echo echo "== inspect sdk objc common config ==" sed -n '45,95p' sdk/BUILD.gn echo echo "== inspect root config cflags/cc around lines 300-435 ==" sed -n '295,435p' BUILD.gn echo echo "== search for cxx_default / cxx17/14/20 in root and sdk build ==" rg -n --hidden -g '!*.md' -e 'cxx17|cxx14|cxx20|cxx_default|standard_library|target_cpu|cxx_std|c++17|c++14|c++20|RTCaudioEngineAvailability|RTCAudioEngineState' BUILD.gn sdk -R 2>/dev/null | head -200 echo echo "== locate all aggregate T(...) style direct init for ObjC availability/flags structs ==" python3 - <<'PY' import pathlib, re for p in list(pathlib.Path("sdk/objc/api/peerconnection").glob("RTCAudioDeviceModule.h"))+list(pathlib.Path("sdk/objc/api/peerconnection").glob("RTCAudioDeviceModule.mm")): text=p.read_text() for m in re.finditer(r'\bRTC_OBJC_TYPE\((\w+)\)\(\s*\W?\s*\w+\s*,\s*\w+\s*\)\s*;?\s*', text): print(f"{p}:{text[:m.start()].count(chr(10))+1}: {m.group(0).replace(chr(10),' ')}") PYRepository: GetStream/webrtc
Length of output: 9933
Use braced aggregate initialization for
RTCAudioEngineAvailability.
RTCAudioEngineAvailabilityis a plain aggregate withboolfields, soT(a7, b)is only valid direct initialization in C++20; pre-C++20 ObjC++ targets can reject it. Replace the five occurrences with{false, false}or{input_available, output_available}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/objc/api/peerconnection/RTCAudioDeviceModule.mm` around lines 586 - 605, Update the five RTCAudioEngineAvailability constructions in engineAvailability to use braced aggregate initialization, replacing parenthesized boolean arguments with {false, false} or {input_available, output_available} as appropriate. Preserve the existing availability and error-handling branches.
3 participants
Summary
AVAudioEnginewhen the input topology or voice-processing state changes.setVoiceProcessingEnablederrors and Objective-C exceptions into a recoverable audio-device error.Impact
Validation
git diff --checkninja -C src/out/verify_ios_stream_on obj/modules/audio_device/audio_device_impl/audio_engine_device.oninja -C src/out/ios_1922_check obj/modules/audio_device/audio_device_impl/audio_engine_device.oninja -C src/out/ios_1922_tests obj/sdk/sdk_unittests_sources/RTCPeerConnectionFactoryAudioEngine_xctest.oSummary by CodeRabbit