feat: cherry pick commits from upstream flutter-webrtc - #75
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR syncs with flutter-webrtc v1.5.2, upgrading Gradle/Kotlin/AGP toolchains, reworking libwebrtc's manifest-driven download/extraction and link paths, adding a fullScreenOnly screen-capture option end-to-end (Dart to Android), fixing SurfaceTextureRenderer/VideoFileRenderer concurrency and encoding issues, updating audio option handling, and correcting iOS speakerphone override. ChangesBuild tooling, versioning, and dependency updates
Estimated code review effort: 2 (Simple) | ~12 minutes libwebrtc download and linking rework
Estimated code review effort: 3 (Moderate) | ~20 minutes Full-screen-only screen capture option
Estimated code review effort: 3 (Moderate) | ~20 minutes Video/audio rendering and encoding fixes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dart as Helper/NativePeerConnectionFactory
participant Channel as MethodChannel
participant MCH as MethodCallHandlerImpl
participant GUM as GetUserMediaImpl
participant Fragment as ScreenRequestPermissionsFragment
participant Projection as MediaProjectionManager
Dart->>Channel: invokeMethod("requestCapturePermission", {fullScreenOnly})
Channel->>MCH: onMethodCall(call)
MCH->>GUM: requestCapturePermission(result, fullScreenOnly)
GUM->>Fragment: screenRequestPermissions(receiver, fullScreenOnly)
Fragment->>Fragment: store FULL_SCREEN_ONLY in arguments
Fragment->>Projection: requestStart(activity, requestCode, fullScreenOnly)
alt fullScreenOnly && Android 14+
Projection->>Projection: createConfigForDefaultDisplay()
else
Projection->>Projection: createScreenCaptureIntent()
end
Projection-->>Fragment: capture intent result
Fragment-->>MCH: ResultReceiver callback
MCH-->>Dart: result(success)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (3)
ios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/AudioUtils.m (1)
85-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable path still uses Core Audio constant and raw session.
Same inconsistency as the darwin implementation: line 85 uses
kAudioSessionOverrideAudioRoute_Noneandsession.sessionwhile the enable path usesAVAudioSessionPortOverrideSpeakerandsession. Align the disable path for consistency.🔧 Suggested fix for the disable path
- success = [session.session overrideOutputAudioPort:kAudioSessionOverrideAudioRoute_None - error:&error]; + success = [session overrideOutputAudioPort:AVAudioSessionPortOverrideNone + error:&error];🤖 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 `@ios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/AudioUtils.m` around lines 85 - 86, The disable path in AudioUtils.m is inconsistent with the enable path: it still calls session.session and uses the Core Audio constant kAudioSessionOverrideAudioRoute_None. Update the disable branch to use the same session object and AVAudioSessionPortOverride value style as the enable branch in the relevant AudioUtils method, so both paths are aligned and use the modern API consistently.common/darwin/Classes/AudioUtils.m (1)
85-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable path still uses Core Audio constant and raw session.
Line 85 uses
kAudioSessionOverrideAudioRoute_None(a Core Audio enum) andsession.session(rawAVAudioSession), while the enable path now correctly usesAVAudioSessionPortOverrideSpeakerandsession(theRTCAudioSessionwrapper). Since this PR is fixing the same type-mismatch bug in the enable path, the disable path should be aligned for consistency.kAudioSessionOverrideAudioRoute_Nonehappens to equalAVAudioSessionPortOverrideNone(both 0), but relying on that coincidence is fragile.🔧 Suggested fix for the disable path
- success = [session.session overrideOutputAudioPort:kAudioSessionOverrideAudioRoute_None - error:&error]; + success = [session overrideOutputAudioPort:AVAudioSessionPortOverrideNone + error:&error];🤖 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 `@common/darwin/Classes/AudioUtils.m` around lines 85 - 86, The disable path in AudioUtils.m is still using the old Core Audio enum and raw AVAudioSession access, so align it with the enable path fix. Update the `overrideOutputAudioPort:error:` call in the same audio session flow to use the `RTCAudioSession` wrapper (`session`) and the modern `AVAudioSessionPortOverrideNone` constant instead of `kAudioSessionOverrideAudioRoute_None` and `session.session`, keeping the speaker on/off handling consistent across both branches.lib/src/helper.dart (1)
211-221: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
factoryIdthrough the static helper
lib/src/helper.dart:211still callsrequestCapturePermissionwithout afactoryId, but the Android handler rejects null/empty ids before reaching thefullScreenOnlypath. The per-factory API already sendsfactoryId; this static entry point will keep failing for Android callers unless it is wired the same way or removed.🤖 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 `@lib/src/helper.dart` around lines 211 - 221, The static helper requestCapturePermission in helper.dart still omits factoryId, so Android rejects the call before handling fullScreenOnly. Update this helper to accept and forward factoryId through WebRTC.invokeMethod the same way the per-factory API does, or remove the static entry point if it is no longer valid. Keep the Android-only guard behavior intact and make sure the requestCapturePermission signature and call sites stay consistent with the factory-based flow.
🧹 Nitpick comments (2)
android/src/main/java/io/getstream/webrtc/flutter/record/VideoFileRenderer.java (1)
353-365: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffVerify the standalone-EGL fallback can reuse the already-attempted input surface.
When
setupEncoderSurface(sharedContext, "shared")fails aftereglBase.createSurface(surface)succeeded but a later step threw, the catch releases onlyeglBaseand leaves the encoder's inputsurfaceintact. The standalone retry then callsEglBase.createSurface(surface)again on the sameSurface. Depending on where the shared attempt failed, that surface may already be bound to the released EGL context, causing the fallback to fail withEGL_BAD_SURFACEand burn the whole config. Please confirm the fallback actually recovers rather than silently failing to the next config.🤖 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 `@android/src/main/java/io/getstream/webrtc/flutter/record/VideoFileRenderer.java` around lines 353 - 365, The standalone fallback in VideoFileRenderer.setupEncoderSurface should be able to reuse the same encoder input surface after a failed shared-context attempt. Update the error-handling path around the setupEncoderSurface(sharedContext, "shared") and setupEncoderSurface(null, "standalone") retries so that a failure after eglBase.createSurface(surface) does not leave the Surface in a state that makes the second attempt hit EGL_BAD_SURFACE. Verify the cleanup/release logic in setupEncoderSurface and the surrounding encoder-start flow preserves a valid retry path and that resetVideoEncoderState/releaseVideoEncoderResources fully recover before moving on.common/cpp/src/flutter_media_stream.cc (1)
224-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
audioConstraintspath. Incommon/cpp/src/flutter_media_stream.cc,audioConstraintsis created in both audio branches but never read, so theaddDefaultAudioConstraints(...)andParseMediaConstraints(...)calls here do nothing. Drop the local variable and these calls fromGetUserAudio.🤖 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 `@common/cpp/src/flutter_media_stream.cc` around lines 224 - 225, The GetUserAudio path in flutter_media_stream.cc is carrying an unused audioConstraints variable, so the addDefaultAudioConstraints(...) and ParseMediaConstraints(...) calls in the audio setup branch have no effect. Remove that local variable and the related constraint-building calls from GetUserAudio, and keep the audio source creation using the existing RTCAudioSource/RTCAudioOptions flow only.
🤖 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 `@third_party/CMakeLists.txt`:
- Around line 136-137: The notice in the libwebrtc extraction check is
misleading because it implies extraction already happened; update the message in
the CMake logic around the LIBWEBRTC_THIRD_PARTY_DIR/libwebrtc existence check
to accurately state that the libwebrtc directory is missing before extraction or
unpacking begins. Keep the condition and surrounding flow unchanged, but rewrite
the message text in the relevant message(NOTICE ...) call so it matches the
current state being validated.
- Around line 122-138: The CMake download/extraction flow in the third_party
logic creates generated artifacts that should not be tracked. Update the
repository ignore rules to exclude both the downloads cache and the extracted
libwebrtc tree, specifically the paths created by the file(DOWNLOAD) and
_libwebrtc_extract_and_normalize steps, so they are not accidentally committed.
- Around line 122-138: The file(DOWNLOAD) call in the third_party CMake logic is
missing explicit transfer hardening and safe path handling. Update the download
block around the existing file(DOWNLOAD) invocation to pass TLS_VERIFY ON and an
INACTIVITY_TIMEOUT value, and quote the ZIPFILE argument so the download target
works correctly when the path contains spaces. Keep the change localized to the
download/extract flow that uses download_status and download_log.
---
Outside diff comments:
In `@common/darwin/Classes/AudioUtils.m`:
- Around line 85-86: The disable path in AudioUtils.m is still using the old
Core Audio enum and raw AVAudioSession access, so align it with the enable path
fix. Update the `overrideOutputAudioPort:error:` call in the same audio session
flow to use the `RTCAudioSession` wrapper (`session`) and the modern
`AVAudioSessionPortOverrideNone` constant instead of
`kAudioSessionOverrideAudioRoute_None` and `session.session`, keeping the
speaker on/off handling consistent across both branches.
In `@ios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/AudioUtils.m`:
- Around line 85-86: The disable path in AudioUtils.m is inconsistent with the
enable path: it still calls session.session and uses the Core Audio constant
kAudioSessionOverrideAudioRoute_None. Update the disable branch to use the same
session object and AVAudioSessionPortOverride value style as the enable branch
in the relevant AudioUtils method, so both paths are aligned and use the modern
API consistently.
In `@lib/src/helper.dart`:
- Around line 211-221: The static helper requestCapturePermission in helper.dart
still omits factoryId, so Android rejects the call before handling
fullScreenOnly. Update this helper to accept and forward factoryId through
WebRTC.invokeMethod the same way the per-factory API does, or remove the static
entry point if it is no longer valid. Keep the Android-only guard behavior
intact and make sure the requestCapturePermission signature and call sites stay
consistent with the factory-based flow.
---
Nitpick comments:
In
`@android/src/main/java/io/getstream/webrtc/flutter/record/VideoFileRenderer.java`:
- Around line 353-365: The standalone fallback in
VideoFileRenderer.setupEncoderSurface should be able to reuse the same encoder
input surface after a failed shared-context attempt. Update the error-handling
path around the setupEncoderSurface(sharedContext, "shared") and
setupEncoderSurface(null, "standalone") retries so that a failure after
eglBase.createSurface(surface) does not leave the Surface in a state that makes
the second attempt hit EGL_BAD_SURFACE. Verify the cleanup/release logic in
setupEncoderSurface and the surrounding encoder-start flow preserves a valid
retry path and that resetVideoEncoderState/releaseVideoEncoderResources fully
recover before moving on.
In `@common/cpp/src/flutter_media_stream.cc`:
- Around line 224-225: The GetUserAudio path in flutter_media_stream.cc is
carrying an unused audioConstraints variable, so the
addDefaultAudioConstraints(...) and ParseMediaConstraints(...) calls in the
audio setup branch have no effect. Remove that local variable and the related
constraint-building calls from GetUserAudio, and keep the audio source creation
using the existing RTCAudioSource/RTCAudioOptions flow only.
🪄 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: 85bc37db-4530-4134-a85a-f86121cc4bef
📒 Files selected for processing (24)
.gitignoreCHANGELOG.mdandroid/build.gradleandroid/src/main/java/io/getstream/webrtc/flutter/GetUserMediaImpl.javaandroid/src/main/java/io/getstream/webrtc/flutter/MethodCallHandlerImpl.javaandroid/src/main/java/io/getstream/webrtc/flutter/SurfaceTextureRenderer.javaandroid/src/main/java/io/getstream/webrtc/flutter/record/VideoFileRenderer.javacommon/cpp/include/flutter_video_renderer.hcommon/cpp/src/flutter_media_stream.cccommon/darwin/Classes/AudioUtils.melinux/CMakeLists.txtexample/android/gradle.propertiesexample/android/gradle/wrapper/gradle-wrapper.propertiesexample/android/settings.gradleexample/windows/CMakeLists.txtios/stream_webrtc_flutter/Sources/stream_webrtc_flutter/AudioUtils.mlib/src/helper.dartlib/src/native/native_peer_connection_factory.dartlib/src/web/native_peer_connection_factory.dartlib/stream_webrtc_flutter.dartlinux/CMakeLists.txtthird_party/CMakeLists.txtthird_party/libwebrtc_version.iniwindows/CMakeLists.txt
…m/webrtc-flutter into feat/cherry-picks-from-upstream
2 participants
cherry picked PRs:
Fix audio session output port override for speaker toggle
Using delete[] instead of delete
fix(android): guard muxer writes against codec-config and invalid sample buffers
fix(android): make VideoFileRenderer more tolerant on Qualcomm/Hisi encoders
chore: Refactoring the CMake binary download process for Win/Linux.
chore(android): support AGP 9 built-in Kotlin
chore(deps): bump WebRTC-SDK to 144.7559.09 and libwebrtc to m144.7559.09 (partially)
fix: call SetRecordingDevice(0) when no sourceId is supplied
feat(android): add fullScreenOnly option to requestCapturePermission (force entire-screen on API 34+)
fix(android): recreate texture surface when frame size changes
Fix Windows libwebrtc extraction through plugin symlinks
fix(desktop): map echoCancellation/noiseSuppression/autoGainControl constraints to RTCAudioOptions
Summary by CodeRabbit
New Features
Bug Fixes
Chores