fix: handle mute state management along with callkit#24
fix: handle mute state management along with callkit#24santhoshvai wants to merge 16 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis PR introduces an audio device module event observation system for iOS/macOS WebRTC. A new AudioDeviceModuleObserver class is created to listen for RTCAudioDeviceModule delegate callbacks and relay them via React Native's event bridge. The AudioDeviceModule is updated to accept and forward events to this observer. A TypeScript event emitter (AudioDeviceModuleEventEmitter) is added to provide type-safe JavaScript listeners for these events. Additionally, new event constants for frame cryption and audio device module lifecycle events are defined, with cryptor-related storage properties added to WebRTCModule. Changes
Sequence DiagramsequenceDiagram
participant Audio as RTCAudioDeviceModule
participant Observer as AudioDeviceModuleObserver
participant Module as AudioDeviceModule
participant RTC as WebRTCModule
participant Bridge as React Native Bridge
participant JS as JavaScript Code
Audio->>Observer: onSpeechActivity(isSpeaking)
Observer->>RTC: sendEvent(kEventAudioDeviceModuleSpeechActivity, data)
RTC->>Bridge: emit event
Bridge->>JS: audioDeviceModuleEvents.onSpeechActivity()
Audio->>Observer: onEngineCreated()
Observer->>RTC: sendEvent(kEventAudioDeviceModuleEngineCreated)
RTC->>Bridge: emit event
Bridge->>JS: audioDeviceModuleEvents.onEngineCreated()
Audio->>Module: configure/state queries
Module->>Observer: forward delegate calls
Observer->>RTC: sendEvent(kEventAudioDeviceModule*)
RTC->>Bridge: emit event
Bridge->>JS: listener callback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
santhoshvai
changed the title
# Conflicts: # ios/RCTWebRTC/WebRTCModule.h # ios/RCTWebRTC/WebRTCModule.m # src/index.ts
# Conflicts: # ios/RCTWebRTC/WebRTCModule.m
# Conflicts: # package.json
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ios/RCTWebRTC/WebRTCModule.h`:
- Around line 51-54: The three properties frameCryptors, keyProviders, and
dataPacketCryptors on WebRTCModule (RTCFrameCryptor, RTCFrameCryptorKeyProvider,
RTCDataPacketCryptor) are declared but never used; either remove these unused
NSMutableDictionary properties from WebRTCModule.h and any corresponding
initialization in WebRTCModule.m, or retain them but add a clear TODO/FIXME
comment above each declaration referencing a short issue ID/URL that tracks the
planned implementation and intended usage so future readers know why they
remain; update any related Obj-C .m initialization code to match the change
(remove initialization if deleting properties, or keep/init and reference the
TODO if retaining).
🧹 Nitpick comments (8)
ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift (2)
354-357: Debug log prefix"Callingx"looks like a leftover from testing.Consider using a consistent log tag (e.g.,
"AudioDeviceModule") or removing theseNSLogcalls if they're not intended for production.🔧 Suggested fix
- NSLog("[Callingx | AudioDeviceModule] speechActivityStarted") + NSLog("[AudioDeviceModule] speechActivityStarted") subject.send(.speechActivityStarted) case .ended: - NSLog("[Callingx | AudioDeviceModule] speechActivityEnded") + NSLog("[AudioDeviceModule] speechActivityEnded")
263-263: Commented-out code should be removed or documented.Line 263 contains a commented-out mute mode call. The surrounding doc comment (lines 257-262) explains the trade-offs of different modes, but the actual commented-out call is dead code. Either remove it or add a brief note explaining why it's retained for reference.
ios/RCTWebRTC/WebRTCModule.m (1)
122-124: Frame cryptor dictionaries are initialized but not cleaned up indealloc.Existing dictionaries
_localTracksand_localStreamsare explicitly cleaned up indealloc(lines 38-41). Consider whether_frameCryptors,_keyProviders, and_dataPacketCryptorsneed similar cleanup — particularly if their values hold references to native WebRTC objects that should be explicitly released or closed.ios/RCTWebRTC/AudioDeviceModuleObserver.m (2)
25-37: Inconsistent log ordering relative to event dispatch.In this method the log comes after the event dispatch, but in
didCreateEngine(line 40) the log comes before the dispatch. This inconsistency is harmless but makes log-based debugging harder — ifsendEventWithName:were to crash, some events would leave a log trail and others wouldn't.Consider placing all
RCTLogcalls before the bridge dispatch across all delegate methods for consistent traceability.
135-153: Configuration hooks silently succeed — consider documenting intent.These two methods log the format and return 0 (success) without performing any configuration or forwarding events to JS. If this is intentional (letting the default pipeline handle configuration), a brief comment clarifying that would help future maintainers understand these aren't accidental stubs.
Suggested inline comment
- (NSInteger)audioDeviceModule:(RTCAudioDeviceModule *)audioDeviceModule engine:(AVAudioEngine *)engine configureInputFromSource:(nullable AVAudioNode *)source toDestination:(AVAudioNode *)destination withFormat:(AVAudioFormat *)format context:(NSDictionary *)context { + // No custom input configuration needed; returning 0 lets the default pipeline proceed. RCTLog(@"[AudioDeviceModuleObserver] Configure input - format: %@", format); return 0; }src/AudioDeviceModuleEvents.ts (3)
36-45:setupListeners()doesn't set up listeners — consider renaming for clarity.The method initializes the
NativeEventEmitterbut doesn't register any listeners. Callers might be confused by the name. Something likeinitialize()orsetup()would better convey the actual behavior.Also, the platform guard
Platform.OS !== 'android'is broader than "iOS/macOS only" (it would pass on'web','windows', etc.). This is safe because the&& WebRTCModulecheck prevents initialization when the native module isn't present, but a comment or a more precise check (e.g.,['ios', 'macos'].includes(Platform.OS)) would match the stated intent.
50-144: Consider a private helper to reduce repetition across all listener methods.All nine
addXxxListenermethods repeat the same null-check-and-throw guard. A small private method would eliminate this duplication.♻️ Example DRY refactor
+ private getEmitter(): NativeEventEmitter { + if (!this.eventEmitter) { + throw new Error('AudioDeviceModuleEvents is only available on iOS/macOS. Did you call setupListeners()?'); + } + return this.eventEmitter; + } + addSpeechActivityListener(listener: (data: SpeechActivityEventData) => void) { - if (!this.eventEmitter) { - throw new Error('AudioDeviceModuleEvents is only available on iOS/macOS'); - } - - return this.eventEmitter.addListener('audioDeviceModuleSpeechActivity', listener); + return this.getEmitter().addListener('audioDeviceModuleSpeechActivity', listener); }Apply the same pattern to all other
addXxxListenermethods.
50-56: Throwing on Android may be surprising — consumers must guard calls.Every
addXxxListenermethod throws if the emitter is null (i.e., on Android or ifsetupListeners()wasn't called). Since the PR description says these events are "currently unused by Stream SDK," this may bite early adopters who calladdSpeechActivityListenerwithout a platform check and crash on Android.Returning a no-op subscription instead of throwing would make cross-platform consumption safer:
// no-op pattern return { remove: () => {} } as EmitterSubscription;Alternatively, the error message could mention calling
setupListeners()first, since that's also a failure path.
| @property(nonatomic, strong) NSMutableDictionary<NSString *, RTCFrameCryptor *> *frameCryptors; | ||
| @property(nonatomic, strong) NSMutableDictionary<NSString *, RTCFrameCryptorKeyProvider *> *keyProviders; | ||
| @property(nonatomic, strong) NSMutableDictionary<NSString *, RTCDataPacketCryptor *> *dataPacketCryptors; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of the new dictionary properties
rg -n --type=objc 'frameCryptors|keyProviders|dataPacketCryptors' -g '!WebRTCModule.h' -g '!WebRTCModule.m'Repository: GetStream/react-native-webrtc
Length of output: 55
🏁 Script executed:
# Also check WebRTCModule.m to see if these are used there
rg -n --type=objc 'frameCryptors|keyProviders|dataPacketCryptors' ios/RCTWebRTC/WebRTCModule.mRepository: GetStream/react-native-webrtc
Length of output: 244
🏁 Script executed:
# Get an overview of the file structure to understand the scope
git ls-files ios/RCTWebRTC/ | head -20Repository: GetStream/react-native-webrtc
Length of output: 824
🏁 Script executed:
# Check context around initialization in WebRTCModule.m (lines 115-135)
sed -n '115,135p' ios/RCTWebRTC/WebRTCModule.mRepository: GetStream/react-native-webrtc
Length of output: 976
🏁 Script executed:
# Look for any method names that might suggest usage (frameCryptor, keyProvider, dataPacketCryptor methods)
rg -n --type=objc 'frameCryptor|keyProvider|dataPacketCryptor' ios/RCTWebRTC/WebRTCModule.m | head -30Repository: GetStream/react-native-webrtc
Length of output: 244
🏁 Script executed:
# Check if there are any TODOs or comments in WebRTCModule.m related to encryption
rg -n -i 'todo|fixme|future|upcoming|planned' ios/RCTWebRTC/WebRTCModule.m | grep -i 'crypt\|encrypt'Repository: GetStream/react-native-webrtc
Length of output: 55
Remove unused cryptor dictionaries or add tracking for planned implementation.
The three dictionaries (frameCryptors, keyProviders, dataPacketCryptors) are initialized but never accessed anywhere in the codebase, and there are no TODO/FIXME comments indicating planned usage. Either remove these unused properties or add an issue/comment to track the intended implementation.
🤖 Prompt for AI Agents
In `@ios/RCTWebRTC/WebRTCModule.h` around lines 51 - 54, The three properties
frameCryptors, keyProviders, and dataPacketCryptors on WebRTCModule
(RTCFrameCryptor, RTCFrameCryptorKeyProvider, RTCDataPacketCryptor) are declared
but never used; either remove these unused NSMutableDictionary properties from
WebRTCModule.h and any corresponding initialization in WebRTCModule.m, or retain
them but add a clear TODO/FIXME comment above each declaration referencing a
short issue ID/URL that tracks the planned implementation and intended usage so
future readers know why they remain; update any related Obj-C .m initialization
code to match the change (remove initialization if deleting properties, or
keep/init and reference the TODO if retaining).
3 participants
Overview
Change muteMode to inputMixer, this plays well along with callkit..
Here is the explanation of the modes
Why?
voiceProcessing in our test, doesnt work will callkit well.
When we do a track.release() or track.diable() now:
It automatically triggers callkit setMuted to true and then soon it reverts to false..
Additional
I added RN event senders for AudioEngine, but its currently unused in Stream SDK. its for future, where we can listen to speaking while muted and audio levels from native level
Future
We would likely revert the change in the future. When we get the Audio LLC from Stream-Swift-Video-SDK
Summary by CodeRabbit
New Features
Chores