feat: handle lifecycle on android video effects#34
Merged
santhoshvai merged 5 commits intomasterfrom Apr 28, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe changes add explicit lifecycle management for video effect processors: a new Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant TrackPrivate as TrackPrivate
participant VideoSource as VideoSource
participant VideoEffectProcessor as VideoEffectProcessor
participant ST_Helper as SurfaceTextureHelper
participant FrameProcessor as VideoFrameProcessor
rect rgba(200,230,255,0.5)
Caller->>TrackPrivate: setVideoEffects(newEffects)
TrackPrivate->>VideoSource: setVideoProcessor(newProcessor)
VideoSource->>VideoEffectProcessor: create/attach(newProcessor)
end
rect rgba(200,255,220,0.5)
Note over TrackPrivate,VideoEffectProcessor: swap active processor reference
TrackPrivate->>TrackPrivate: clear previous videoEffectProcessor ref
TrackPrivate->>VideoEffectProcessor: assign new videoEffectProcessor
end
rect rgba(255,230,200,0.5)
Note over VideoEffectProcessor,ST_Helper: dispose previous processor asynchronously
TrackPrivate->>ST_Helper: enqueue dispose(previousProcessor)
ST_Helper->>FrameProcessor: invoke dispose() on wrapped processors
FrameProcessor-->>ST_Helper: disposed
ST_Helper-->>TrackPrivate: dispose completed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java (1)
580-620:⚠️ Potential issue | 🟡 MinorMinor: clone +
setVideoEffectscan leak a processor on teardown.
setVideoEffectsonly gates onvideoCaptureController instanceof CameraCaptureController, which clones satisfy (they share the controller). If a caller invokessetVideoEffects(cloneId, …):
- The new processor is attached to the shared
videoSourceand stored onclone.videoEffectProcessor.- The original's
videoEffectProcessorfield still references the previously-installed processor — now detached from the source but correctly disposed when the original is torn down.- On clone teardown,
TrackPrivate.dispose()skips disposal because of!isClone, so the processor actually attached to the shared source is neverdispose()d.If this path is supported, consider either storing/disposing the processor on the parent track or short-circuiting
setVideoEffectson clones.🔧 Option A — route processor ownership to the parent (sketch)
void setVideoEffects(String trackId, ReadableArray names) { TrackPrivate track = tracks.get(trackId); if (track != null && track.videoCaptureController instanceof CameraCaptureController) { + TrackPrivate owner = track.isClone() ? track.getParent() : track; VideoSource videoSource = (VideoSource) track.mediaSource; SurfaceTextureHelper surfaceTextureHelper = track.surfaceTextureHelper; - VideoEffectProcessor previousProcessor = track.videoEffectProcessor; - track.videoEffectProcessor = null; + VideoEffectProcessor previousProcessor = owner.videoEffectProcessor; + owner.videoEffectProcessor = null; ... - track.videoEffectProcessor = videoEffectProcessor; + owner.videoEffectProcessor = videoEffectProcessor; ... } }(Requires exposing
getParent()onTrackPrivate.)🔧 Option B — reject on clones
if (track != null && track.videoCaptureController instanceof CameraCaptureController) { + if (track.isClone()) { + Log.w(TAG, "setVideoEffects called on a cloned track; ignoring."); + return; + }Also applies to: 700-714
🤖 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 580 - 620, setVideoEffects can leak processors when called on a clone because clones share the same CameraCaptureController/videoSource but keep separate videoEffectProcessor fields; to fix, early-return for clones in setVideoEffects: detect clone state on TrackPrivate (e.g., track.isClone() or track.getParent() != null) and if true log and return without attaching a new processor, so only the parent owns and disposes processors; ensure you reference TrackPrivate.videoEffectProcessor and VideoSource.setVideoProcessor when making the change so the shared videoSource isn't given a processor from a clone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.java`:
- Around line 580-620: setVideoEffects can leak processors when called on a
clone because clones share the same CameraCaptureController/videoSource but keep
separate videoEffectProcessor fields; to fix, early-return for clones in
setVideoEffects: detect clone state on TrackPrivate (e.g., track.isClone() or
track.getParent() != null) and if true log and return without attaching a new
processor, so only the parent owns and disposes processors; ensure you reference
TrackPrivate.videoEffectProcessor and VideoSource.setVideoProcessor when making
the change so the shared videoSource isn't given a processor from a clone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f535ce15-ee9a-46ea-bc5e-a4e800b832ed
📒 Files selected for processing (3)
android/src/main/java/com/oney/WebRTCModule/GetUserMediaImpl.javaandroid/src/main/java/com/oney/WebRTCModule/videoEffects/VideoEffectProcessor.javaandroid/src/main/java/com/oney/WebRTCModule/videoEffects/VideoFrameProcessor.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/oney/WebRTCModule/CameraCaptureController.java (1)
122-151:⚠️ Potential issue | 🟠 MajorMissing
facingMode/deviceIdnow forces a front-camera switch.
ReactBridgeUtil.getMapStrValuereturnsnullwhen the key is absent, and per the primary callers (WebRTCModule.mediaStreamTrackApplyConstraints→GetUserMediaImpl.applyConstraints) theconstraintsmap is forwarded from JS as-is — callers routinely send resolution/fps-only updates withoutdeviceIdorfacingMode. In that case,deviceId == nullandfacingMode == null, so the fallback at Line 143 (isFrontFacing = facingMode == null || facingMode.equals("user")) evaluates totrueand the code will pick the front camera, silently switching away from a currently-active back camera on any plain resolution change.This is also inconsistent with the matching iOS change in this PR:
VideoCaptureController.m(Lines 170, 171–177) seedstargetFront = self.usingFrontCameraand only flips it whenfacingModeis explicitly"user"/"environment", preserving current state otherwise.Suggest either short-circuiting when neither selector is present, or defaulting to the current device/facing:
🐛 Proposed fix: preserve current camera when selectors are absent
// Re-read from the incoming constraints so `MediaStreamTrack._switchCamera()` // can flip the camera via `applyConstraints({facingMode})` — the documented // W3C pattern that browsers also implement. final String deviceId = ReactBridgeUtil.getMapStrValue(constraints, "deviceId"); final String facingMode = ReactBridgeUtil.getMapStrValue(constraints, "facingMode"); int cameraIndex = -1; String cameraName = null; // If deviceId is specified, then it takes precedence over facingMode. if (deviceId != null) { try { cameraIndex = Integer.parseInt(deviceId); cameraName = deviceNames[cameraIndex]; } catch (Exception e) { Log.d(TAG, "failed to find device with id: " + deviceId); } } - // Otherwise, use facingMode (defaulting to front/user facing). - if (cameraName == null) { + // Otherwise, use facingMode, preserving the current facing when unspecified + // so resolution/fps-only updates don't force a camera switch. + if (cameraName == null && (deviceId != null || facingMode != null)) { cameraIndex = -1; - final boolean isFrontFacing = facingMode == null || facingMode.equals("user"); + final boolean isFrontFacing = + facingMode == null ? this.isFrontFacing : facingMode.equals("user"); for (String name : deviceNames) { cameraIndex++; if (cameraEnumerator.isFrontFacing(name) == isFrontFacing) { cameraName = name; break; } } } + + if (cameraName == null && deviceId == null && facingMode == null) { + // No device-selection change requested; keep the current camera. + try { + cameraIndex = Integer.parseInt(currentDeviceId); + cameraName = deviceNames[cameraIndex]; + } catch (Exception e) { + // Fall through to the OverconstrainedError path below. + } + }🤖 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/CameraCaptureController.java` around lines 122 - 151, The code in CameraCaptureController reads deviceId and facingMode from constraints and treats a null facingMode as "user", which causes an unintended switch to the front camera when callers send constraints that omit both selectors; update the logic in CameraCaptureController so that when both deviceId and facingMode are absent you preserve the current camera instead of selecting a default: detect the case where ReactBridgeUtil.getMapStrValue(constraints, "deviceId") == null and ReactBridgeUtil.getMapStrValue(constraints, "facingMode") == null and short-circuit so cameraIndex/cameraName remain set to the currently-active device (or skip the for-loop that picks based on cameraEnumerator.isFrontFacing), otherwise keep the existing precedence (deviceId wins, then explicit facingMode handling). Ensure you reference and preserve the existing variables cameraIndex, cameraName, deviceNames and cameraEnumerator when implementing the short-circuit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/src/main/java/com/oney/WebRTCModule/CameraCaptureController.java`:
- Around line 122-151: The code in CameraCaptureController reads deviceId and
facingMode from constraints and treats a null facingMode as "user", which causes
an unintended switch to the front camera when callers send constraints that omit
both selectors; update the logic in CameraCaptureController so that when both
deviceId and facingMode are absent you preserve the current camera instead of
selecting a default: detect the case where
ReactBridgeUtil.getMapStrValue(constraints, "deviceId") == null and
ReactBridgeUtil.getMapStrValue(constraints, "facingMode") == null and
short-circuit so cameraIndex/cameraName remain set to the currently-active
device (or skip the for-loop that picks based on
cameraEnumerator.isFrontFacing), otherwise keep the existing precedence
(deviceId wins, then explicit facingMode handling). Ensure you reference and
preserve the existing variables cameraIndex, cameraName, deviceNames and
cameraEnumerator when implementing the short-circuit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 286eb9ec-8324-4881-bba6-1a3e1238229a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
android/src/main/java/com/oney/WebRTCModule/CameraCaptureController.javaandroid/src/main/java/com/oney/WebRTCModule/videoEffects/VideoEffectProcessor.javaios/RCTWebRTC/VideoCaptureController.mpackage.json
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/java/com/oney/WebRTCModule/videoEffects/VideoEffectProcessor.java
santhoshvai
added a commit
to GetStream/stream-video-js
that referenced
this pull request
Apr 28, 2026
### 💡 Overview Correctness, perf, and cleanup fixes in the React Native background-filter pipeline. ### 📝 Implementation notes - **iOS**: fix blur background-drift (CIGaussianBlur extent); run Vision segmentation at ~540p (2–4× faster on mid-tier chips); close retain cycle in filter closures and `NotificationCenter` observer leak. - **iOS segmentation async**: run Vision off the capture thread on a dedicated serial queue; composite using the last completed mask (≤1-frame staleness, imperceptible). Mirrors Android's ML Kit pattern. Unblocks the capture thread so preview stays at 30 fps on older chips (e.g. iPhone XS) where Vision was previously eating most of the frame budget. - **Android**: per-filter `YuvFrame` instance (fixes camera-flip race); GL/libyuv resource cleanup via a new `VideoFrameProcessor.dispose()` lifecycle hook. - **Remote URL backgrounds**: loaded off-thread on both platforms with timeouts; host-only error logging. - **SDK provider**: reapply on track replacement (camera flip); staleness guard for in-flight apply calls; `unregisterAllFilters()` on unmount so native processors can deallocate. Requires the paired [react-native-webrtc PR](GetStream/react-native-webrtc#34) for the `dispose()` hook and a camera-flip regression fix. 🎫 Ticket: https://linear.app/stream/issue/XYZ-123 📑 Docs: https://github.com/GetStream/docs-content/pull/<id> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an "unregister all filters" control to release previously registered filters. * **Bug Fixes** * Prevents stale/overlapping filter changes and reapplies effects when media tracks update. * Improved teardown to reliably remove filters and clear cached resources. * **Performance Improvements** * Background images load asynchronously to avoid UI blocking. * Blur processing downsamples for more efficient rendering. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
Add lifecycle to video-effect processors on Android
VideoFrameProcessor implementations that hold native or GL resources had no way to release them: VideoEffectProcessor was only ever replaced or dropped, never torn down. Long-running sessions that switched filters leaked every previous processor's resources.
Changes
Why not wire it to VideoProcessor.onCapturerStopped? That also fires on temporary pauses (camera disable / backgrounding) where the same processor is reused on resume — we'd dispose state the processor still needs.
Test plan
Summary by CodeRabbit