Conversation
📝 WalkthroughWalkthroughThis PR adds screen-share audio mixing: Android fetches and mixes screen PCM into the mic buffer and exposes media-projection permission data; iOS adds in-app ReplayKit-based screen capture, audio conversion/ring-buffer/mixer utilities, and ties in-app capture into the existing track creation and audio pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant RPS as ReplayKit<br/>RPScreenRecorder
participant InApp as InAppScreenCapturer
participant Converter as ScreenShareAudioConverter
participant Ring as AudioRingBuffer
participant Mixer as ScreenShareAudioMixer
participant WebRTC as WebRTC Audio<br/>Processing
RPS->>InApp: deliver video & audioApp samples
InApp->>Converter: audioApp CMSampleBuffer
Converter->>Ring: write converted PCM (float frames)
WebRTC->>Mixer: audioProcessingProcess(rtcAudioBuffer)
Mixer->>Ring: read(availableFrames)
Mixer->>Mixer: mix (additive, clamp) into rtcAudioBuffer
Mixer->>WebRTC: return mixed audioBuffer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java (1)
42-42: Visibility change exposes MediaProjection permission data to external packages.Making
GetUserMediaImplpublic and adding thegetMediaProjectionPermissionResultData()getter enables external code to access the MediaProjection permission Intent. This is required for the audio capture feature, but consider:
- Documenting this class/method as internal API to discourage misuse
- The returned Intent could be used to create additional MediaProjections without user consent (though Android should enforce single-use)
Also applies to: 65-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java` at line 42, The class GetUserMediaImpl and its getter getMediaProjectionPermissionResultData() expose the MediaProjection permission Intent publicly; restrict access by making the class and method package-private (remove public) or annotate them as internal (e.g., `@RestrictTo`(RestrictTo.Scope.LIBRARY)) and add a Javadoc comment marking them as internal API; additionally, avoid returning the raw Intent instance - document that callers must not reuse it and consider returning a safe copy or nulling/clearing the stored Intent after it has been consumed in GetUserMediaImpl to prevent external code from creating additional MediaProjections without consent.ios/RCTWebRTC/InAppScreenCapturer.m (1)
125-135: Observer removal may execute on a different thread.Observers are added on the main queue but
unregisterAppStateObserversmay be called from any thread. WhileNSNotificationCenteris thread-safe, there's a potential issue: if removal happens before the asyncregisterAppStateObserversdispatch executes, the flag will be cleared but observers will still be added afterward.Consider dispatching removal to main queue as well, or using the fix suggested for
registerAppStateObserverswhich handles this case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWebRTC/InAppScreenCapturer.m` around lines 125 - 135, unregisterAppStateObservers can be called off-main and races with the async registerAppStateObservers dispatch; ensure removal runs on the main queue and mirrors the register guard: dispatch the removal block to the main queue, check and set the _observingAppState flag inside that main-queue block, and then call [[NSNotificationCenter defaultCenter] removeObserver:... ] for UIApplicationDidBecomeActiveNotification and UIApplicationWillResignActiveNotification; alternatively, adopt the same post-dispatch re-check pattern used in registerAppStateObservers so observers aren’t added after you clear _observingAppState.ios/RCTWebRTC/InAppScreenCaptureController.m (1)
33-39: Minor: Redundant deviceId fallback.The
deviceIdis set to@"in-app-screen-capture"ininitWithCapturer:(Line 16), so the nil-coalescing operator on Line 35 is unnecessary sinceself.deviceIdwill never be nil after initialization.♻️ Suggested simplification
- (NSDictionary *)getSettings { return @{ - @"deviceId": self.deviceId ?: @"in-app-screen-capture", + @"deviceId": self.deviceId, @"groupId": @"", @"frameRate": @(30) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWebRTC/InAppScreenCaptureController.m` around lines 33 - 39, The getSettings method uses a redundant nil-coalescing fallback for deviceId even though initWithCapturer: already sets self.deviceId to @"in-app-screen-capture"; update -[InAppScreenCaptureController getSettings] to use self.deviceId directly (remove the ?: @"in-app-screen-capture" fallback) so the dictionary simply assigns @"deviceId": self.deviceId, keeping the rest of the keys unchanged.ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swift (1)
108-137: Consider extracting format comparison to avoid duplication.Lines 119-122 duplicate the format comparison logic from
ScreenShareAudioConverter.formatsMatch(). Consider exposing that method or usingconvertIfRequireddirectly (it already has the identity optimization).Simplified approach
- // 3. Convert to graph format (e.g. 48 kHz / 1 ch / float32) - let buffer: AVAudioPCMBuffer - if pcm.format.sampleRate != targetFormat.sampleRate - || pcm.format.channelCount != targetFormat.channelCount - || pcm.format.commonFormat != targetFormat.commonFormat - || pcm.format.isInterleaved != targetFormat.isInterleaved { - guard let converted = audioConverter.convertIfRequired(pcm, to: targetFormat) else { return } - buffer = converted - } else { - buffer = pcm - } + // 3. Convert to graph format if needed (converter handles identity case) + guard let buffer = audioConverter.convertIfRequired(pcm, to: targetFormat) else { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swift` around lines 108 - 137, The format comparison in enqueue(_:) duplicates logic in ScreenShareAudioConverter.formatsMatch(); change enqueue to avoid manual field-by-field checks by either calling ScreenShareAudioConverter.formatsMatch(pcm.format, targetFormat) or simply always calling audioConverter.convertIfRequired(pcm, to: targetFormat) and using the returned buffer (it already returns the original when no conversion is needed), then proceed to schedule and play the buffer; update references in enqueue to use audioConverter.pcmBuffer(from:), audioConverter.convertIfRequired(...), and playerNode.scheduleBuffer(...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java`:
- Around line 367-370: Retainment of mediaProjectionPermissionResultData can
leak a stale Intent; update GetUserMediaImpl to clear
mediaProjectionPermissionResultData when the MediaProjection is revoked or when
ScreenCapturerAndroid.onStop() is called and also null out displayMediaPromise
there; implement this by hooking into the MediaProjection callback/stop path
(where the MediaProjection is released) and calling
mediaProjectionPermissionResultData = null (and displayMediaPromise = null if
applicable), and add a freshness check inside
getMediaProjectionPermissionResultData() to validate the Intent (e.g., ensure a
live MediaProjection or timestamp/flag) and return null or throw if it is stale
so external callers cannot reuse an invalid Intent.
In `@android/src/main/java/com/oney/WebRTCModule/WebRTCModule.java`:
- Around line 166-185: The mixing loop can write past the mic buffer and perform
pointless writes; change the logic so it never writes beyond micShorts' valid
range by bounding the loop to the mic buffer capacity: compute micSamples =
Math.min(bytesRead / 2, micShorts.remaining()), keep screenSamples =
screenShorts.remaining(), then iterate i from 0 to micSamples (not totalSamples)
and for each i if i >= screenSamples copy screenShorts.get(i) into micShorts
only when i < micShorts.remaining(); otherwise add micShorts.get(i) +
screenShorts.get(i) when both exist, clamp to Short range, and write with
micShorts.put(i). Ensure no breaks that skip processing and avoid writing when
micShorts has no capacity.
In `@ios/RCTWebRTC/InAppScreenCapturer.m`:
- Around line 109-123: The race occurs because _observingAppState is set to YES
before observers are actually added on the main queue in
registerAppStateObservers, allowing unregisterAppStateObservers to run and skip
removal; fix by moving the state change and the addObserver calls together on
the main queue: in registerAppStateObservers dispatch to the main queue and
inside that block check _observingAppState, set _observingAppState = YES, then
call addObserver for appDidBecomeActive and appWillResignActive (or
alternatively use dispatch_sync to ensure immediate registration), and ensure
unregisterAppStateObservers likewise removes observers on the main queue and
sets _observingAppState = NO to keep the flag consistent with actual observer
registration.
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioConverter.swift`:
- Around line 90-95: In ScreenShareAudioConverter.swift the int16 branch that
checks pcmBuffer.int16ChannelData always performs a single interleaved memcpy
which breaks non-interleaved input when avFormat.isInterleaved is false; update
the int16 path to mirror the float handling: when avFormat.isInterleaved is true
do the single memcpy using bytesPerFrame and frameCount, otherwise loop
per-channel (using avFormat.formatDescription.mChannelsPerFrame / channelCount),
compute per-channel byte stride (bytesPerFrame / channelCount), and memcpy each
pcmBuffer.int16ChannelData[channel] from the proper offset in dataPointer for
frameCount bytes (respecting totalLength), similar to how the float
non-interleaved case is handled.
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swift`:
- Around line 85-93: startMixing() currently only sets isMixing and does not
perform the ADM reconfiguration described in the comment; update startMixing()
to, after setting isMixing = true, either (A) request an ADM reconfigure so
onConfigureInputFromSource runs again (call the ADM reconfiguration/reset
method) or (B) immediately wire the mixer nodes using the ADM's cached context:
grab AudioDeviceModule.lastInputSource, lastInputDestination and lastInputFormat
and run the same node-attachment/configuration logic used in
onConfigureInputFromSource to attach your mixer nodes; reference startMixing(),
onConfigureInputFromSource, and
AudioDeviceModule.lastInputSource/lastInputDestination/lastInputFormat when
making the change.
In `@ios/RCTWebRTC/WebRTCModule`+RTCMediaStream.m:
- Around line 217-234: The screen-share audio path is not wired: when
options.useInAppScreenCapture is true you must set
InAppScreenCapturer.audioBufferHandler and, if options.includeScreenShareAudio
is enabled, create/retain a ScreenShareAudioMixer and call [mixer startMixing]
before starting capture; attach the handler to forward audio into the mixer and
store the mixer reference (e.g., on the module or options) so you can call
[mixer stopMixing] from mediaStreamTrackRelease; update the block that creates
InAppScreenCapturer (and options.activeInAppScreenCapturer) to set
audioBufferHandler, check options.includeScreenShareAudio, instantiate and start
the mixer accordingly, and ensure mediaStreamTrackRelease stops and releases the
mixer.
---
Nitpick comments:
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java`:
- Line 42: The class GetUserMediaImpl and its getter
getMediaProjectionPermissionResultData() expose the MediaProjection permission
Intent publicly; restrict access by making the class and method package-private
(remove public) or annotate them as internal (e.g.,
`@RestrictTo`(RestrictTo.Scope.LIBRARY)) and add a Javadoc comment marking them as
internal API; additionally, avoid returning the raw Intent instance - document
that callers must not reuse it and consider returning a safe copy or
nulling/clearing the stored Intent after it has been consumed in
GetUserMediaImpl to prevent external code from creating additional
MediaProjections without consent.
In `@ios/RCTWebRTC/InAppScreenCaptureController.m`:
- Around line 33-39: The getSettings method uses a redundant nil-coalescing
fallback for deviceId even though initWithCapturer: already sets self.deviceId
to @"in-app-screen-capture"; update -[InAppScreenCaptureController getSettings]
to use self.deviceId directly (remove the ?: @"in-app-screen-capture" fallback)
so the dictionary simply assigns @"deviceId": self.deviceId, keeping the rest of
the keys unchanged.
In `@ios/RCTWebRTC/InAppScreenCapturer.m`:
- Around line 125-135: unregisterAppStateObservers can be called off-main and
races with the async registerAppStateObservers dispatch; ensure removal runs on
the main queue and mirrors the register guard: dispatch the removal block to the
main queue, check and set the _observingAppState flag inside that main-queue
block, and then call [[NSNotificationCenter defaultCenter] removeObserver:... ]
for UIApplicationDidBecomeActiveNotification and
UIApplicationWillResignActiveNotification; alternatively, adopt the same
post-dispatch re-check pattern used in registerAppStateObservers so observers
aren’t added after you clear _observingAppState.
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swift`:
- Around line 108-137: The format comparison in enqueue(_:) duplicates logic in
ScreenShareAudioConverter.formatsMatch(); change enqueue to avoid manual
field-by-field checks by either calling
ScreenShareAudioConverter.formatsMatch(pcm.format, targetFormat) or simply
always calling audioConverter.convertIfRequired(pcm, to: targetFormat) and using
the returned buffer (it already returns the original when no conversion is
needed), then proceed to schedule and play the buffer; update references in
enqueue to use audioConverter.pcmBuffer(from:),
audioConverter.convertIfRequired(...), and playerNode.scheduleBuffer(...)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c683d9e-b75e-43f1-baac-1cd400cea64f
📒 Files selected for processing (14)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaandroid/src/main/java/com/oney/WebRTCModule/WebRTCModule.javaandroid/src/main/java/com/oney/WebRTCModule/WebRTCModuleOptions.javaios/RCTWebRTC/InAppScreenCaptureController.hios/RCTWebRTC/InAppScreenCaptureController.mios/RCTWebRTC/InAppScreenCapturer.hios/RCTWebRTC/InAppScreenCapturer.mios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftios/RCTWebRTC/Utils/AudioDeviceModule/AudioGraphConfigurationDelegate.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioConverter.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swiftios/RCTWebRTC/WebRTCModule+RTCMediaStream.mios/RCTWebRTC/WebRTCModule.mios/RCTWebRTC/WebRTCModuleOptions.h
Comment on lines
+367
to
370
| // Note: mediaProjectionPermissionResultData is intentionally NOT nulled here. | ||
| // It is retained so it can be reused to create a MediaProjection for | ||
| // screen share audio capture (AudioPlaybackCaptureConfiguration). | ||
| displayMediaPromise = null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how/where the MediaProjection permission data is used for audio capture
# and whether there's lifecycle management
rg -n "getMediaProjectionPermissionResultData|AudioPlaybackCapture" --type javaRepository: GetStream/react-native-webrtc
Length of output: 462
🏁 Script executed:
# First, get the full GetUserMediaImpl.java file to understand the context
wc -l android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaRepository: GetStream/react-native-webrtc
Length of output: 142
🏁 Script executed:
# Get the relevant section and understand how mediaProjectionPermissionResultData is stored and used
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '60,80p'Repository: GetStream/react-native-webrtc
Length of output: 1117
🏁 Script executed:
# Check if there's any cleanup/reset of mediaProjectionPermissionResultData
rg -n "mediaProjectionPermissionResultData\s*=" android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaRepository: GetStream/react-native-webrtc
Length of output: 140
🏁 Script executed:
# Find all usages of getMediaProjectionPermissionResultData() across the codebase
rg -n "getMediaProjectionPermissionResultData\(\)" --type java -A 3 -B 1Repository: GetStream/react-native-webrtc
Length of output: 545
🏁 Script executed:
# Check for any lifecycle-related cleanup (e.g., onDestroy, cleanup, clear methods)
rg -n "onDestroy|cleanup|clear.*Intent|reset.*MediaProjection" android/src/main/java/com/oney/WebRTCModule/ --type javaRepository: GetStream/react-native-webrtc
Length of output: 55
🏁 Script executed:
# Get more context around where mediaProjectionPermissionResultData is set (line 105)
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '95,120p'Repository: GetStream/react-native-webrtc
Length of output: 1315
🏁 Script executed:
# Check the entire createScreenStream method and related methods for cleanup
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '350,380p'Repository: GetStream/react-native-webrtc
Length of output: 1627
🏁 Script executed:
# Look for stop/end/cleanup methods related to screen sharing
rg -n "stopScreenCapture|endScreenShare|stopDisplayMedia" --type javaRepository: GetStream/react-native-webrtc
Length of output: 55
🏁 Script executed:
# Check where screen sharing ends and if there's any cleanup
rg -n "displayMediaPromise.*null|mediaProjection.*null|unbind.*Service" android/src/main/java/com/oney/WebRTCModule/ --type java -B 2 -A 2Repository: GetStream/react-native-webrtc
Length of output: 3201
🏁 Script executed:
# Check how ScreenCaptureController uses the mediaProjectionPermissionResultData
fd -e java -x grep -l "ScreenCaptureController" {} \;Repository: GetStream/react-native-webrtc
Length of output: 215
🏁 Script executed:
# Search for actual calls to getMediaProjectionPermissionResultData() outside of the class
rg -n "getMediaProjectionPermissionResultData" --type java -B 3 -A 3Repository: GetStream/react-native-webrtc
Length of output: 806
🏁 Script executed:
# Check ScreenCaptureController.java to see if it uses mediaProjectionPermissionResultData
cat -n android/src/main/java/com/oney/WebRTCModule/ScreenCaptureController.java | head -100Repository: GetStream/react-native-webrtc
Length of output: 3934
🏁 Script executed:
# Look for where ScreenCaptureController gets the Intent/data
rg -n "new ScreenCaptureController|getMediaProjectionPermissionResultData" android/src/main/java/com/oney/WebRTCModule/ --type java -B 2 -A 2Repository: GetStream/react-native-webrtc
Length of output: 1290
🏁 Script executed:
# Check if there's any method to reset or clear the Intent when screen sharing stops
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '1,50p'Repository: GetStream/react-native-webrtc
Length of output: 2163
🏁 Script executed:
# Search for AudioPlaybackCaptureConfiguration usage in the codebase
rg -n "AudioPlaybackCaptureConfiguration" --type java -B 3 -A 3Repository: GetStream/react-native-webrtc
Length of output: 1668
🏁 Script executed:
# Check if getMediaProjectionPermissionResultData is actually called from any native modules or bindings
rg -n "getMediaProjectionPermissionResultData" . --type java --type js --type ts -B 2 -A 2Repository: GetStream/react-native-webrtc
Length of output: 600
🏁 Script executed:
# Look for any MediaProjection creation or reuse patterns
rg -n "createMediaProjection|getMediaProjection" --type java -B 2 -A 2Repository: GetStream/react-native-webrtc
Length of output: 590
🏁 Script executed:
# Check if there's any lifecycle management or cleanup when screen sharing ends
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '400,450p'Repository: GetStream/react-native-webrtc
Length of output: 2582
🏁 Script executed:
# Check if there are any issues or tests that mention stale Intent or MediaProjection problems
rg -n "stale|lifecycle|MediaProjection.*reuse|AudioPlaybackCapture" --type java -B 2 -A 2Repository: GetStream/react-native-webrtc
Length of output: 1310
🏁 Script executed:
# Check the complete flow of when/how getMediaProjectionPermissionResultData is intended to be called
rg -n "public|protected" android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | head -30Repository: GetStream/react-native-webrtc
Length of output: 1128
🏁 Script executed:
# Look for any documentation or comments about the intended use of getMediaProjectionPermissionResultData()
cat -n android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java | sed -n '65,112p'Repository: GetStream/react-native-webrtc
Length of output: 2344
🏁 Script executed:
# Check if ScreenCapturerAndroid or MediaProjection has any caching/reuse pattern that would support audio capture
rg -n "ScreenCapturerAndroid|createMediaProjection" . --type java -B 3 -A 3 | head -60Repository: GetStream/react-native-webrtc
Length of output: 1744
🏁 Script executed:
# Check if there are any JavaScript/TypeScript files that might use getMediaProjectionPermissionResultData
find . -type f \( -name "*.js" -o -name "*.ts" -o -name "*.tsx" \) -exec grep -l "getMediaProjectionPermissionResultData\|AudioPlaybackCapture" {} \;Repository: GetStream/react-native-webrtc
Length of output: 55
🏁 Script executed:
# Check if MediaProjectionService has any cleanup or lifecycle management
cat -n android/src/main/java/com/oney/WebRTCModule/MediaProjectionService.java 2>/dev/null | head -100Repository: GetStream/react-native-webrtc
Length of output: 4173
🏁 Script executed:
# Find the MediaProjectionService file
find . -name "MediaProjectionService.java" -type fRepository: GetStream/react-native-webrtc
Length of output: 146
Add lifecycle management to clear the retained Intent when screen sharing ends.
The mediaProjectionPermissionResultData Intent is retained for reuse in AudioPlaybackCaptureConfiguration, but there's no mechanism to clear it when:
- The MediaProjection is revoked (e.g., user stops screen sharing)
- The
ScreenCapturerAndroid.onStop()callback fires
Since getMediaProjectionPermissionResultData() is a public API, external consumers could retrieve and use a stale Intent for audio capture, which would fail. Either clear the Intent explicitly when the MediaProjection ends, or add validation to ensure freshness before use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java` around
lines 367 - 370, Retainment of mediaProjectionPermissionResultData can leak a
stale Intent; update GetUserMediaImpl to clear
mediaProjectionPermissionResultData when the MediaProjection is revoked or when
ScreenCapturerAndroid.onStop() is called and also null out displayMediaPromise
there; implement this by hooking into the MediaProjection callback/stop path
(where the MediaProjection is released) and calling
mediaProjectionPermissionResultData = null (and displayMediaPromise = null if
applicable), and add a freshness check inside
getMediaProjectionPermissionResultData() to validate the Intent (e.g., ensure a
live MediaProjection or timestamp/flag) and return null or throw if it is stale
so external callers cannot reuse an invalid Intent.
ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioConverter.swift
Outdated
Show resolved
Hide resolved
ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShareAudioMixer.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift (1)
182-191: Potential retain cycle with strongaudioGraphDelegatereference.The
audioGraphDelegateproperty is a strong reference. If the conforming delegate (e.g.,ScreenShareAudioMixer) holds a strong reference back to thisAudioDeviceModule, a retain cycle will occur. Consider making this a weak reference:- `@objc` public var audioGraphDelegate: AudioGraphConfigurationDelegate? + `@objc` public weak var audioGraphDelegate: AudioGraphConfigurationDelegate?The cached input context using weak references for
lastInputSourceandlastInputDestinationis appropriate since theAVAudioEngineowns these nodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift` around lines 182 - 191, The audioGraphDelegate property is a strong reference and can cause a retain cycle if the delegate (e.g., ScreenShareAudioMixer) holds a reference back to AudioDeviceModule; change the property audioGraphDelegate to a weak reference and ensure the AudioGraphConfigurationDelegate protocol is class-bound (e.g., inherits from AnyObject) so it can be weak; update the declaration of audioGraphDelegate (and any related `@objc` exposure) to use weak and adjust any callers/implementations if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift`:
- Around line 282-289: The setVoiceProcessingBypassed(_:) method updates
source.isVoiceProcessingBypassed but fails to update
isVoiceProcessingBypassedSubject, causing observers of
isVoiceProcessingBypassedPublisher to see stale state; modify
setVoiceProcessingBypassed(_ isBypassed: Bool) to also immediately send the new
value to isVoiceProcessingBypassedSubject (matching the pattern used in
setMuted() and setRecording()), while keeping the existing
audioDeviceModule(_:didUpdateAudioProcessingState:) callback for eventual
consistency.
---
Nitpick comments:
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift`:
- Around line 182-191: The audioGraphDelegate property is a strong reference and
can cause a retain cycle if the delegate (e.g., ScreenShareAudioMixer) holds a
reference back to AudioDeviceModule; change the property audioGraphDelegate to a
weak reference and ensure the AudioGraphConfigurationDelegate protocol is
class-bound (e.g., inherits from AnyObject) so it can be weak; update the
declaration of audioGraphDelegate (and any related `@objc` exposure) to use weak
and adjust any callers/implementations if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d0b71e8-ef28-4427-821a-cdd08403ae49
📒 Files selected for processing (3)
ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftios/RCTWebRTC/WebRTCModule.mios/RCTWebRTC/WebRTCModuleOptions.h
✅ Files skipped from review due to trivial changes (1)
- ios/RCTWebRTC/WebRTCModule.m
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/RCTWebRTC/WebRTCModuleOptions.h
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift`:
- Around line 182-185: The screenShareAudioMixer property is never assigned to
the audio processing module so its RTCAudioCustomProcessingDelegate callbacks
never run; update the code that creates/initializes the
RTCDefaultAudioProcessingModule (referencing WebRTCModule.m where
capturePostProcessingDelegate is currently nil) to set its
capturePostProcessingDelegate to AudioDeviceModule.screenShareAudioMixer (the
ScreenShareAudioMixer instance) when the module is created or when screen
sharing starts, and ensure you clear/unset it when screen sharing stops to avoid
dangling delegates; locate references to RTCDefaultAudioProcessingModule,
capturePostProcessingDelegate, AudioDeviceModule.screenShareAudioMixer and
ScreenShareAudioMixer to make the assignment and lifecycle cleanup.
In
`@ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift`:
- Around line 27-31: The current implementation treats audio as mono by only
enqueuing channelData[0] into ringBuffer and reusing that single stream in
mixFromRingBuffer(...), which collapses stereo/right-only content; update the
data path so the ringBuffer and read/mix logic handle frames * channels when
targetFormat indicates stereo (or preserve original channel count): modify
enqueue(_:) in ScreenShareAudioMixer to push interleaved (or per-channel)
samples for all channels rather than only channelData[0], extend AudioRingBuffer
capacity/semantics to store frames*channels, and update mixFromRingBuffer(...)
to read and distribute samples into each output channel (or perform an explicit
mono downmix before enqueue if mono is desired) and ensure silence detection
operates on the chosen mono mix instead of the left channel only.
- Around line 31-38: The mixer mutates and reads shared state (isMixing,
targetFormat, processingSampleRate, processingChannels and the
ScreenShareAudioConverter cache) from multiple threads; add a single
synchronization primitive (e.g. a private serial DispatchQueue or an NSLock)
inside ScreenShareAudioMixer and guard all reads/writes to those properties and
the converter with it, update methods that change state (start/stop,
route-change handlers, audioProcessingInitialize) to perform mutations on that
queue, and in enqueue(_:) snapshot the needed state (isMixing, a local copy of
targetFormat and a reference to the converter) while synchronized before
performing any conversion/mixing off-queue so conversion never races with reset;
ensure converter reset/replace also happens under the same lock/queue.
- Around line 40-43: Change the floatS16Scale constant from 32768.0 to 32767.0
and ensure the mixing/conversion path in ScreenShareAudioMixer clamps the final
summed/scaled sample to the Int16 range [-32768, 32767] (i.e., after adding
inputs and multiplying by floatS16Scale but before casting to Int16). Update the
constant floatS16Scale and modify the mixer/conversion routine that converts
normalized Float32 to FloatS16 (the mixing function in ScreenShareAudioMixer
where samples are summed and scaled) to perform clamping of the final value to
avoid producing +32768 or other out-of-range values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f37c1f3-8e47-4754-9e15-ed39e8fcc07c
📒 Files selected for processing (5)
ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/AudioRingBuffer.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioConverter.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swiftios/RCTWebRTC/WebRTCModule.m
✅ Files skipped from review due to trivial changes (1)
- ios/RCTWebRTC/WebRTCModule.m
ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
Show resolved
Hide resolved
ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
Show resolved
Hide resolved
ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/RCTWebRTC/InAppScreenCapturer.m`:
- Around line 139-156: The rapid FG/BG race can call startRPScreenRecorder while
stopCaptureWithHandler is still in-flight; add a boolean like _stopPending that
you set to YES in appWillResignActive before calling [[RPScreenRecorder
sharedRecorder] stopCaptureWithHandler:], clear it inside that stop completion
handler, and only call startRPScreenRecorder when _stopPending is NO (i.e. in
appDidBecomeActive check _stopPending before restarting and in the stop
completion handler if _shouldResumeOnForeground && _capturing && !_stopPending
then call startRPScreenRecorder); ensure you still clear
_shouldResumeOnForeground as appropriate.
🪄 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
Run ID: 9df2d525-162d-4517-887b-5f866c4cd26e
📒 Files selected for processing (3)
ios/RCTWebRTC/InAppScreenCapturer.mios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swiftios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- ios/RCTWebRTC/Utils/AudioDeviceModule/AudioDeviceModule.swift
- ios/RCTWebRTC/Utils/AudioDeviceModule/ScreenShare/ScreenShareAudioMixer.swift
Comment on lines
+139
to
+156
| - (void)appWillResignActive { | ||
| if (_capturing) { | ||
| _shouldResumeOnForeground = YES; | ||
| // Stop the RPScreenRecorder session — iOS suspends it in background anyway | ||
| [[RPScreenRecorder sharedRecorder] stopCaptureWithHandler:^(NSError * _Nullable error) { | ||
| if (error) { | ||
| NSLog(@"[InAppScreenCapturer] background stop error: %@", error.localizedDescription); | ||
| } | ||
| }]; | ||
| } | ||
| } | ||
|
|
||
| - (void)appDidBecomeActive { | ||
| if (_shouldResumeOnForeground && _capturing) { | ||
| _shouldResumeOnForeground = NO; | ||
| [self startRPScreenRecorder]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race during rapid foreground/background transitions.
If the app transitions quickly (background → foreground before stopCaptureWithHandler completes on line 143), startRPScreenRecorder will be called while a stop is still in progress. While RPScreenRecorder likely handles this gracefully by returning an error, consider adding a flag to track the pending stop operation and defer the restart until the completion handler fires.
🛡️ Proposed approach
`@implementation` InAppScreenCapturer {
BOOL _capturing;
BOOL _shouldResumeOnForeground;
BOOL _observingAppState;
+ BOOL _stoppingForBackground;
}
- (void)appWillResignActive {
if (_capturing) {
_shouldResumeOnForeground = YES;
+ _stoppingForBackground = YES;
// Stop the RPScreenRecorder session — iOS suspends it in background anyway
- [[RPScreenRecorder sharedRecorder] stopCaptureWithHandler:^(NSError * _Nullable error) {
+ __weak __typeof__(self) weakSelf = self;
+ [[RPScreenRecorder sharedRecorder] stopCaptureWithHandler:^(NSError * _Nullable error) {
+ __strong __typeof__(weakSelf) strongSelf = weakSelf;
+ if (!strongSelf) return;
+ strongSelf->_stoppingForBackground = NO;
if (error) {
NSLog(@"[InAppScreenCapturer] background stop error: %@", error.localizedDescription);
}
+ // Resume now if app became active while stopping
+ if (strongSelf->_shouldResumeOnForeground && strongSelf->_capturing) {
+ strongSelf->_shouldResumeOnForeground = NO;
+ [strongSelf startRPScreenRecorder];
+ }
}];
}
}
- (void)appDidBecomeActive {
- if (_shouldResumeOnForeground && _capturing) {
+ if (_shouldResumeOnForeground && _capturing && !_stoppingForBackground) {
_shouldResumeOnForeground = NO;
[self startRPScreenRecorder];
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/RCTWebRTC/InAppScreenCapturer.m` around lines 139 - 156, The rapid FG/BG
race can call startRPScreenRecorder while stopCaptureWithHandler is still
in-flight; add a boolean like _stopPending that you set to YES in
appWillResignActive before calling [[RPScreenRecorder sharedRecorder]
stopCaptureWithHandler:], clear it inside that stop completion handler, and only
call startRPScreenRecorder when _stopPending is NO (i.e. in appDidBecomeActive
check _stopPending before restarting and in the stop completion handler if
_shouldResumeOnForeground && _capturing && !_stopPending then call
startRPScreenRecorder); ensure you still clear _shouldResumeOnForeground as
appropriate.
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability