feat: stereo audio output support #23
Conversation
santhoshvai
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces observable audio device state management on iOS via a new Combine-based AudioDeviceModule wrapper, while Android gains a helper method for configuring hardware-accelerated audio processing. Configuration updates include pod dependency bumps, Swift source inclusion, and removal of specific AVAudioSession options. Changes
Sequence Diagram(s)sequenceDiagram
participant WRC as WebRTCModule
participant RTCM as RTCAudioDeviceModule
participant ADM as AudioDeviceModule
participant Sub as Subscriber
WRC->>RTCM: initialize with AudioEngine
RTCM->>ADM: set as delegate
RTCM->>ADM: didCreateEngine(engine)
ADM->>ADM: update engine reference
ADM->>Sub: emit didCreateAudioEngine event
RTCM->>ADM: willEnableEngine(...)
ADM->>Sub: emit willEnableAudioEngine event
RTCM->>ADM: willStartEngine(...)
ADM->>ADM: installAudioLevelTap()
ADM->>Sub: emit willStartAudioEngine event
RTCM->>ADM: didReceiveSpeechActivityEvent(...)
ADM->>Sub: emit speechActivityEvent
RTCM->>ADM: didUpdateAudioProcessingState(...)
ADM->>ADM: update voice processing state
ADM->>Sub: publish isVoiceProcessingEnabled, etc.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/RCTWebRTC/WebRTCModule.m (1)
85-110: GuardAudioDeviceModuleinit against nilpeerConnectionFactory.audioDeviceModule.When a custom
audioDeviceis supplied (lines 96–101), some WebRTC builds do not populatepeerConnectionFactory.audioDeviceModule. Passingnilto the non-optionalRTCAudioDeviceModuleparameter at line 110 will crash when the initializer immediately accesses properties likesource.isPlaying(line 204 in AudioDeviceModule.swift). Add a nil check before initialization.Proposed fix
- _audioDeviceModule = [[AudioDeviceModule alloc] initWithSource:_peerConnectionFactory.audioDeviceModule]; + RTCAudioDeviceModule *adm = _peerConnectionFactory.audioDeviceModule; + if (adm != nil) { + _audioDeviceModule = [[AudioDeviceModule alloc] initWithSource:adm]; + } else { + _audioDeviceModule = nil; + }
🤖 Fix all issues with AI agents
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift`:
- Around line 242-257: The setStereoPlayoutPreference method currently discards
the Int return values from source.setMuteMode and
source.setRecordingAlwaysPreparedMode; change these calls to use the existing
throwingExecution pattern (or check the Int result and call the ADM error
logging/throw helper used by setPlayout/setMicrophoneMuted) so failures surface
instead of being ignored; update setStereoPlayoutPreference to call
throwingExecution around setMuteMode and setRecordingAlwaysPreparedMode (or
inspect their return and invoke the same error/log path) before setting
source.prefersStereoPlayout to ensure errors are logged or propagated.
- Around line 197-226: The init(_ source: any RTCAudioDeviceModuleControlling,
...) seeds isVoiceProcessingBypassedSubject with
source.isVoiceProcessingBypassed but then sets source.isVoiceProcessingBypassed
= true without updating the subject, causing a stale published state; after
assigning source.isVoiceProcessingBypassed = true in init, immediately update
isVoiceProcessingBypassedSubject (e.g., call
isVoiceProcessingBypassedSubject.send(true) or set its value) so the publisher
reflects the new state before WebRTC's didUpdateAudioProcessingState callback,
and if forcing bypass globally is not intended, wrap the assignment in a guard
(or document the global behavior) to only set it under the required condition
(e.g., stereo playout).
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioEngineLevelNodeAdapter.swift`:
- Around line 65-68: The uninstall method currently hardcodes removeTap(onBus:
0) causing taps on non-zero buses to remain; update
AudioEngineLevelNodeAdapter.uninstall to call mixer.removeTap(onBus: bus)
(matching the bus used by installInputTap) so the tap installed on the given bus
is removed; keep the existing guard on inputTap and engine checks and pass
through the bus parameter when removing the tap.
🧹 Nitpick comments (2)
ios/RCTWebRTC/Utils/AudioDeviceModule/AudioEngineLevelNodeAdapter.swift (1)
88-115: Consider multi‑channel RMS and reuseConstant.silenceDB.Lines 105–115 only sample
channelData[0]and clamp with a magic-160. For multi‑channel input (stereo), this can under‑report levels. Consider averaging across channels and usingConstant.silenceDBfor consistency.♻️ Suggested adjustment
- vDSP_rmsqv(channelData[0], 1, &rms, frameCount) + let channelCount = Int(buffer.format.channelCount) + guard channelCount > 0 else { return } + var rmsSum: Float = 0 + for channel in 0..<channelCount { + var channelRms: Float = 0 + vDSP_rmsqv(channelData[channel], 1, &channelRms, frameCount) + rmsSum += channelRms + } + rms = rmsSum / Float(channelCount) ... - let clampedRMS = max(-160.0, min(0.0, Float(rmsDB))) + let clampedRMS = max(Constant.silenceDB, min(0.0, Float(rmsDB)))ios/RCTWebRTC/WebRTCModule.h (1)
26-39: Consider markingaudioDeviceModulenullable to match runtime availability.When a custom
audioDeviceis used, the wrapper may not be created (see.m). Exposing this as nullable makes the API contract explicit.♻️ Suggested tweak
-@property(nonatomic, strong) AudioDeviceModule *audioDeviceModule; +@property(nonatomic, strong, nullable) AudioDeviceModule *audioDeviceModule;
| /// Switches between stereo and mono playout while keeping the recording | ||
| /// state consistent across reinitializations. | ||
| /// - Parameter isPreferred: `true` when stereo output should be used. | ||
| @objc public func setStereoPlayoutPreference(_ isPreferred: Bool) { | ||
| /// - Important: `.voiceProcessing` requires VP to be enabled in order to mute and | ||
| /// `.restartEngine` rebuilds the whole graph. Each of them has different issues: | ||
| /// - `.voiceProcessing`: as it requires VP to be enabled in order to mute/unmute that | ||
| /// means that for outputs where VP is disabled (e.g. stereo) we cannot mute/unmute. | ||
| /// - `.restartEngine`: rebuilds the whole graph and requires explicit calling of | ||
| /// `initAndStartRecording` . | ||
| _ = source.setMuteMode(isPreferred ? .inputMixer : .voiceProcessing) | ||
| /// - Important: We can probably set this one to false when the user doesn't have | ||
| /// sendAudio capability. | ||
| _ = source.setRecordingAlwaysPreparedMode(false) | ||
| source.prefersStereoPlayout = isPreferred | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the AudioDeviceModule.swift file and examine its structure
find . -name "AudioDeviceModule.swift" -type fRepository: GetStream/react-native-webrtc
Length of output: 136
🏁 Script executed:
# Look at the specific lines mentioned in the review
git ls-files | grep -i audiodevicemoduleRepository: GetStream/react-native-webrtc
Length of output: 282
🏁 Script executed:
# Search for the file using fd
fd AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 134
🏁 Script executed:
# Read the AudioDeviceModule.swift file to examine the method in question
wc -l ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 138
🏁 Script executed:
# Read the file around the mentioned lines
sed -n '230,270p' ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 2075
🏁 Script executed:
# Find the source object declaration and type
head -100 ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 4920
🏁 Script executed:
# Read the RTCAudioDeviceModuleControlling protocol
cat -n ios/RCTWebRTC/Utils/AudioDeviceModule/RTCAudioDeviceModuleControlling.swiftRepository: GetStream/react-native-webrtc
Length of output: 2063
🏁 Script executed:
# Search for throwingExecution helper method
grep -n "throwingExecution" ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 627
🏁 Script executed:
# Search for all usages of setMuteMode in the codebase
rg "setMuteMode" -A 2 -B 2Repository: GetStream/react-native-webrtc
Length of output: 1305
🏁 Script executed:
# Read the throwingExecution helper implementation
sed -n '557,572p' ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 500
🏁 Script executed:
# Check if there are any comments around setStereoPlayoutPreference about error handling
sed -n '240,260p' ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftRepository: GetStream/react-native-webrtc
Length of output: 1319
🏁 Script executed:
# Search for setRecordingAlwaysPreparedMode usages to see if there's a pattern
rg "setRecordingAlwaysPreparedMode" -A 2 -B 2Repository: GetStream/react-native-webrtc
Length of output: 1182
🏁 Script executed:
# Check reset() method context
grep -n "func reset" ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift -A 3Repository: GetStream/react-native-webrtc
Length of output: 154
Add error handling to stereo preference setter.
setMuteMode and setRecordingAlwaysPreparedMode return status codes (Int), but their return values are discarded. This is inconsistent with how other similar ADM operations are handled elsewhere in the code (e.g., setPlayout, setMicrophoneMuted use the throwingExecution pattern to surface errors). Consider logging failures or propagating errors so stereo preference changes don't fail silently.
🤖 Prompt for AI Agents
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift` around lines
242 - 257, The setStereoPlayoutPreference method currently discards the Int
return values from source.setMuteMode and source.setRecordingAlwaysPreparedMode;
change these calls to use the existing throwingExecution pattern (or check the
Int result and call the ADM error logging/throw helper used by
setPlayout/setMicrophoneMuted) so failures surface instead of being ignored;
update setStereoPlayoutPreference to call throwingExecution around setMuteMode
and setRecordingAlwaysPreparedMode (or inspect their return and invoke the same
error/log path) before setting source.prefersStereoPlayout to ensure errors are
logged or propagated.
| func uninstall(on bus: Int = 0) { | ||
| if let mixer = inputTap, mixer.engine != nil { | ||
| mixer.removeTap(onBus: 0) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "AudioEngineLevelNodeAdapter.swift" -type fRepository: GetStream/react-native-webrtc
Length of output: 146
🏁 Script executed:
cat -n ./ios/RCTWebRTC/Utils/AudioDeviceModule/AudioEngineLevelNodeAdapter.swiftRepository: GetStream/react-native-webrtc
Length of output: 5446
Use the provided bus parameter when removing the tap.
Line 67 hardcodes onBus: 0 instead of using the bus parameter, while installInputTap correctly uses it (line 52). This causes taps installed on non-zero buses to persist after uninstall is called, leaking taps and keeping callbacks running.
🐛 Proposed fix
func uninstall(on bus: Int = 0) {
if let mixer = inputTap, mixer.engine != nil {
- mixer.removeTap(onBus: 0)
+ mixer.removeTap(onBus: bus)
}🤖 Prompt for AI Agents
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioEngineLevelNodeAdapter.swift`
around lines 65 - 68, The uninstall method currently hardcodes removeTap(onBus:
0) causing taps on non-zero buses to remain; update
AudioEngineLevelNodeAdapter.uninstall to call mixer.removeTap(onBus: bus)
(matching the bus used by installInputTap) so the tap installed on the given bus
is removed; keep the existing guard on inputTap and engine checks and pass
through the bus parameter when removing the tap.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
### 💡 Overview Apart from stereo audio support, we also ensure that from video client we make sure that the audio session always configured before joining a call and then audio session configuration is deactivated after leaving the call depends on: GetStream/react-native-webrtc#23 ### 📝 Implementation notes 🎫 Ticket: https://linear.app/stream/issue/RN-332 📑 Docs: GetStream/docs-content#899 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Stereo audio output option for listener role * In-call manager lifecycle hooks: setup, start, stop * Runtime-registered global in-call manager stub * **Improvements** * Centralized and unified audio setup across platforms * Debounced audio route-change handling and richer audio logging * Safer activity-aware teardown and thread-safe audio controls * **Chores** * Removed legacy audio-state utility * Sample apps updated to opt into new lifecycle APIs (no auto-start) <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.