fix(llc,ui): fixing screen share cancelation #1085
Conversation
WalkthroughAdds native WebRTC event plumbing for screen-share start/stop, iOS broadcast-extension handling, constraint-aware track enablement, subscription management for background services, a Darwin notification helper on iOS, and bumps stream_webrtc_flutter to ^1.0.13. Changes
Sequence Diagram(s)sequenceDiagram
participant System as System UI
participant Native as Native Platform
participant ScreenMgr as ScreenShareManager (iOS)
participant Plugin as StreamVideoFlutterPlugin
participant RtcNotifier as RtcMediaDeviceNotifier
participant Rtc as RtcManager
participant Call as Call
participant BG as BackgroundService
System->>Native: user stops/starts broadcast
Native->>ScreenMgr: post Darwin notification
ScreenMgr->>Plugin: onNotification(name)
Plugin->>RtcNotifier: postEvent("screenSharingStopped"/"screenSharingStarted")
RtcNotifier->>Rtc: nativeWebRtcEventsStream -> ScreenSharingStopped/Started
Rtc->>Call: emit NativeWebRtcEvent
Call->>Call: _onNativeWebRtcEvent -> disable/enable local screen-share track
Rtc->>BG: event also observed
BG->>BG: find matching callCid -> stop notification service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1085 +/- ##
========================================
+ Coverage 4.82% 4.84% +0.01%
========================================
Files 591 591
Lines 40402 40500 +98
========================================
+ Hits 1949 1961 +12
- Misses 38453 38539 +86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (3)
packages/stream_video_flutter/lib/src/call_background/background_service.dart (1)
244-276: Reduce notification churn: update only on meaningful changesThe service is updated on every
CallStateemission. This is noisy and can be expensive. Cache the last builtNotificationOptionsand update only if changed (deep equality or selective fields), or debounce.Example:
- final stateSubscription = call.state.listen((CallState value) async { + NotificationOptions? _last; + final stateSubscription = call.state.listen((CallState value) async { ... - try { - final updateOptions = optionsBuilder.call(call); + try { + final updateOptions = optionsBuilder.call(call); + if (_last != null && _last == updateOptions) return; ... - _logger.v(() => '<$callCid> [_startManagingCall] call service update result: $updateResult'); + if (updateResult) _last = updateOptions; + _logger.v(() => '<$callCid> [_startManagingCall] call service update result: $updateResult');If
NotificationOptionslacks ==, consider comparing key fields or hashing.packages/stream_video/lib/src/call/session/call_session.dart (1)
772-785: Local-unpublish handling: simplify and avoid redundant lookupYou already know the track is local (
track is! RtcRemoteTrack). Re‑querying the same track viartcManager?.getTrack(track.trackId)is redundant.Apply this diff:
- if (track is! RtcRemoteTrack) { - final localTrack = rtcManager?.getTrack(track.trackId); - if (localTrack != null && - localTrack.isScreenShareTrack && - localTrack.mediaTrack.enabled) { + if (track is! RtcRemoteTrack) { + final localTrack = track; + if (localTrack.isScreenShareTrack && localTrack.mediaTrack.enabled) { await setScreenShareEnabled(false); } return; }packages/stream_video_flutter/ios/Classes/ScreenShare/ScreenShareManager.swift (1)
77-93: LGTM! Correct callback handling with main queue dispatch.The callback implementation properly:
- Unwraps the unmanaged observer pointer
- Guards against nil values
- Dispatches user callbacks to the main queue (appropriate for Flutter/UI integration)
The static
CFNotificationCallbackcorrectly bridges C-style callbacks to Swift.Optional: Consider thread-safe callback access
The
onNotificationcallback is a mutablevarthat could theoretically be set tonilbetween the guard check and dispatch execution. If callback mutation during notification processing is a concern, consider:private var _onNotification: ((String) -> Void)? private let callbackLock = NSLock() var onNotification: ((String) -> Void)? { get { callbackLock.lock() defer { callbackLock.unlock() } return _onNotification } set { callbackLock.lock() defer { callbackLock.unlock() } _onNotification = newValue } }However, for typical usage patterns where the callback is set once during initialization, the current implementation is likely sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
melos.yaml(1 hunks)packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(5 hunks)packages/stream_video/lib/src/call/session/call_session.dart(1 hunks)packages/stream_video/lib/src/webrtc/rtc_manager.dart(5 hunks)packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart(2 hunks)packages/stream_video/lib/src/webrtc/rtc_track/rtc_track.dart(1 hunks)packages/stream_video/pubspec.yaml(1 hunks)packages/stream_video_flutter/CHANGELOG.md(1 hunks)packages/stream_video_flutter/example/pubspec.yaml(1 hunks)packages/stream_video_flutter/ios/Classes/ScreenShare/ScreenShareManager.swift(1 hunks)packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift(2 hunks)packages/stream_video_flutter/lib/src/call_background/background_service.dart(4 hunks)packages/stream_video_flutter/pubspec.yaml(1 hunks)packages/stream_video_noise_cancellation/pubspec.yaml(1 hunks)packages/stream_video_push_notification/pubspec.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift (1)
packages/stream_video_flutter/ios/Classes/ScreenShare/ScreenShareManager.swift (1)
observeNotification(24-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: analyze
- GitHub Check: build
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video
🔇 Additional comments (16)
packages/stream_video/CHANGELOG.md (1)
1-6: Changelog entries are clear and well-documented.The new "Unreleased" section accurately captures the two bug fixes from this PR: screen sharing not stopping via system UI cancellation on Android/iOS, and iOS broadcast extension handling improvements. Entries are concise, follow the established changelog format with emoji prefixes and platform tags, and align with the PR objectives.
packages/stream_video_flutter/pubspec.yaml (2)
25-25: New dependency added: rxdartThe
rxdartpackage has been added as a new dependency. Based on the PR context, this is likely used for managing native WebRTC event streams and screen sharing lifecycle events.
27-27: Verify that stream_webrtc_flutter v1.0.13 includes the expected WebRTC event handling fixes.Version 1.0.13 exists on pub.dev and was published today, making it a valid dependency update. However, no public changelog or release notes are currently available to confirm it contains the specific fixes for WebRTC event handling and Android/iOS system UI stop events mentioned in the review comment. Ensure this version actually includes the fixes you expect for screen sharing cancellation before merging.
packages/stream_video_noise_cancellation/pubspec.yaml (1)
18-18: LGTM!Dependency version bump is consistent with the broader repository update to stream_webrtc_flutter ^1.0.13.
packages/stream_video_push_notification/pubspec.yaml (1)
26-26: LGTM!Dependency version bump is consistent with the broader repository update to stream_webrtc_flutter ^1.0.13.
melos.yaml (1)
25-25: LGTM!Bootstrap dependency version is correctly aligned with the version bumps across all package pubspec files.
packages/stream_video_flutter/CHANGELOG.md (1)
1-7: LGTM!The changelog entries accurately document the screen sharing fixes for Android/iOS system UI cancellation and the iOS broadcast extension improvements mentioned in the PR objectives.
packages/stream_video_flutter/example/pubspec.yaml (1)
33-33: LGTM!Example app dependency version correctly updated to match the main packages.
packages/stream_video/pubspec.yaml (1)
34-34: LGTM!Core package dependency version correctly updated to stream_webrtc_flutter ^1.0.13.
packages/stream_video/lib/src/webrtc/rtc_track/rtc_track.dart (1)
42-44: LGTM!The new
isScreenShareTrackgetter follows the same pattern as the existingisVideoTrackandisAudioTrackgetters. The logic correctly identifies both screen share video and audio tracks, which aligns with the screen sharing improvements in this PR.packages/stream_video/lib/src/call/call.dart (1)
2002-2015: Good: pass constraints when enabling provided local tracksForwarding the concrete constraints to
setMicrophoneEnabled,setCameraEnabled, andsetScreenShareEnabledprevents capability regressions and aligns with the new flow. LGTM.packages/stream_video_flutter/lib/src/call_background/background_service.dart (1)
39-41: No issues found—the review comment is incorrectThe
Subscriptionsclass is actually publicly exported from thestream_videopackage. The filepackages/stream_video/lib/stream_video.dartexplicitly exportssrc/utils/subscriptions.dart, makingSubscriptionspart of the public API. Sincebackground_service.dartimports frompackage:stream_video/stream_video.dart, the symbol resolves correctly and compilation will succeed. The suggested refactor is unnecessary.Likely an incorrect or invalid review comment.
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart (1)
18-28: LGTM! Clean event class hierarchy.The abstract base class
NativeWebRtcEventwith concrete implementations for screen sharing events is well-structured and follows a clear, extensible pattern.packages/stream_video_flutter/ios/Classes/ScreenShare/ScreenShareManager.swift (3)
1-17: LGTM! Solid singleton pattern with proper thread safety.The class structure is well-designed with appropriate use of
CFNotificationCenterfor Darwin notifications andNSLockfor thread-safe access to shared state.
19-21: LGTM! Proper resource cleanup.The
deinitcorrectly removes all observers to prevent leaks.
24-75: LGTM! Thread-safe observer management.The observer management methods are well-implemented with:
- Proper locking to protect shared state
- Guards against duplicate additions and non-existent removals
- Correct use of
Unmanagedpointers for CFNotificationCenter- Clean use of
deferfor guaranteed lock release
packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift
Show resolved
Hide resolved
packages/stream_video_flutter/lib/src/call_background/background_service.dart
Show resolved
Hide resolved
packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/stream_video_flutter/lib/src/call_background/background_service.dart (1)
179-208: Null-safety issue flagged in previous reviewThe past review identified that using
!oncall.state.value.localParticipantat line 192 is unsafe even after the null check at line 186, since the check and access happen on separate lines and state could theoretically change.packages/stream_video/lib/src/call/call.dart (1)
471-515: Null-safety issue flagged in previous reviewThe past review identified that using
!onstate.value.localParticipantat line 488 is unsafe even after the null check at line 486. The check and the force-unwrap happen on separate accesses tostate.value.localParticipant, which could theoretically change between them.
🧹 Nitpick comments (1)
packages/stream_video/lib/src/webrtc/rtc_manager.dart (1)
1181-1212: Consider timeout for iOS broadcast extension flowWhen using iOS broadcast extension, the code creates a track and starts listening for
ScreenSharingStartedEventbut returns immediately without a timeout. If the user cancels the broadcast picker or the event never arrives, the subscription remains active indefinitely.Consider adding a timeout to clean up if the event doesn't arrive:
if (CurrentPlatform.isIos && enabled && constraints is ScreenShareConstraints && constraints.useiOSBroadcastExtension) { final screenShareTrackResult = await createScreenShareTrack( constraints: constraints, ); if (screenShareTrackResult.isFailure) { _logger.e( () => '[ScreenSharingStartedEvent] failed to create screen share track: ${screenShareTrackResult.getErrorOrNull()}', ); return screenShareTrackResult; } _startListeningToScreenShareStarted( screenShareTrackResult.getDataOrNull()!, ); // Add timeout to clean up if broadcast never starts _screenSharingStartedOperation = CancelableOperation.fromFuture( awaitNativeWebRtcEvent<ScreenSharingStartedEvent>() .timeout( const Duration(seconds: 30), onTimeout: () { _logger.w(() => '[setScreenShareEnabled] broadcast start timeout'); _screenSharingStartedSubscription?.cancel(); throw TimeoutException('Broadcast picker timeout'); }, ), ); return Future.value( Result.success(screenShareTrackResult.getDataOrNull()!), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/stream_video/lib/src/call/call.dart(11 hunks)packages/stream_video/lib/src/call/call_connect_options.dart(2 hunks)packages/stream_video/lib/src/webrtc/rtc_manager.dart(9 hunks)packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart(2 hunks)packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift(2 hunks)packages/stream_video_flutter/lib/src/call_background/background_service.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/lib/src/webrtc/rtc_media_device/rtc_media_device_notifier.dart
🧰 Additional context used
🧬 Code graph analysis (1)
packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift (1)
packages/stream_video_flutter/ios/Classes/ScreenShare/ScreenShareManager.swift (1)
observeNotification(24-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: analyze_legacy_version
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video
- GitHub Check: build
- GitHub Check: analyze
🔇 Additional comments (10)
packages/stream_video_flutter/ios/Classes/StreamVideoFlutterPlugin.swift (2)
6-7: LGTM! Clear notification identifiers.The static constants follow proper reverse-domain naming conventions for Darwin notifications and will be used consistently throughout the notification handling code.
20-35: LGTM! Threading concern from previous review is now resolved.The notification observer setup is correct, and the callback handler properly dispatches to the main thread (line 23) before posting events to the Flutter plugin. This ensures thread-safe interaction with Flutter's method channel, addressing the concern raised in the previous review.
The use of optional chaining for
FlutterWebRTCPlugin.sharedSingleton()?.postEvent(...)provides safe nil handling, and the switch statement correctly maps Darwin notifications to corresponding Flutter events.packages/stream_video/lib/src/call/call_connect_options.dart (1)
114-151: LGTM! Constraint-aware TrackEnabled implementationThe changes correctly make
TrackEnabledconstraint-aware by:
- Adding an optional
constraintsparameter to the factory- Storing constraints as a field
- Including constraints in equality comparison via
props- Updating
toString()for better debuggingThis allows different track enablement states to carry their specific constraints, which aligns well with the enhanced screen sharing and track management flow described in the PR.
packages/stream_video/lib/src/webrtc/rtc_manager.dart (2)
1223-1229: LGTM! Screen share track recreation logicThe decision to always create a new screen share track when enabling is well-justified by the comment. This ensures:
- Correct constraint handling on iOS
- Support for selecting different screens on web/desktop platforms
1345-1367: LGTM! Clean event awaiter and listener implementationsBoth methods are well-implemented:
awaitNativeWebRtcEventuses the.take(1).firstpattern, which auto-cancels the subscription and avoids manual Completer management (addresses past review feedback)._startListeningToScreenShareStartedproperly cancels any previous subscription before creating a new one, preventing leaks.packages/stream_video/lib/src/call/call.dart (5)
498-510: LGTM! ScreenSharingStartedEvent handlingThe event handling correctly:
- Updates participant state to reflect screen sharing is enabled
- Updates connect options with iOS broadcast extension constraints
- Aligns with the native-event-driven flow for iOS broadcast extensions
1964-1979: LGTM! Consistent constraint extraction patternThe constraint extraction across all three apply methods follows a safe and consistent pattern:
- Check if constraints are of the expected type
- Cast if present, or use defaults
- Pass to the enable method
This properly integrates with the constraint-aware
TrackEnabledstate introduced incall_connect_options.dart.Also applies to: 1989-1992, 2003-2019
2036-2049: LGTM! Constraint propagation in track settingWhen setting a local track, the code correctly propagates the track's media constraints to the corresponding enable method (lines 2036-2048). This ensures the track's constraints are reflected in the call's connect options and participant state.
2829-2831: LGTM! TrackOption state updates with constraintsAll three enable methods (
setCameraEnabled,setMicrophoneEnabled,setScreenShareEnabled) correctly update_connectOptionswith constraint-awareTrackOptioninstances. This maintains consistency between the actual track configuration and the stored connect options.Also applies to: 2893-2895, 2946-2948
2932-2938: LGTM! iOS broadcast extension flowThe early return when using iOS broadcast extension is correct. Instead of immediately updating state, the code waits for the
ScreenSharingStartedEvent(handled at lines 498-510), which provides a more accurate representation of when the broadcast actually starts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stream_video/lib/src/webrtc/rtc_manager.dart (1)
1351-1366: Auto-cancel after first start event and handle publish failuresListener should cancel after first event to avoid duplicate publishes and should log failures.
Apply:
void _startListeningToScreenShareStarted( RtcLocalTrack<ScreenShareConstraints> track, ) { _screenSharingStartedSubscription?.cancel(); - _screenSharingStartedSubscription = RtcMediaDeviceNotifier.instance - .nativeWebRtcEventsStream() - .whereType<ScreenSharingStartedEvent>() - .listen((event) async { - _logger.i(() => '[ScreenSharingStartedEvent] received: $event'); - - await publishVideoTrack( - track: track, - ); - }); + _screenSharingStartedSubscription = RtcMediaDeviceNotifier.instance + .nativeWebRtcEventsStream() + .whereType<ScreenSharingStartedEvent>() + .take(1) + .listen((event) async { + _logger.i(() => '[ScreenSharingStartedEvent] received: $event'); + final res = await publishVideoTrack(track: track); + if (res is Failure) { + _logger.e(() => + '[ScreenSharingStartedEvent] publish failed: ${res.getErrorOrNull()}'); + } + }, onError: (e, stk) { + _logger.e(() => '[ScreenSharingStartedEvent] stream error: $e'); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_video/lib/src/webrtc/rtc_manager.dart(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_flutter
- GitHub Check: analyze
- GitHub Check: build
- GitHub Check: analyze_legacy_version
🔇 Additional comments (6)
packages/stream_video/lib/src/webrtc/rtc_manager.dart (6)
109-111: Good: dedicated subscription field for screen-share start eventsField addition looks correct and scoped. Lifecycle handled below in dispose.
434-439: Good: cancel subscription on disposeCancelling and nulling
_screenSharingStartedSubscriptionprevents leaks.
1221-1224: Re-enabling screen share creates a new track; verify stale entry/ID behaviorCreating a new track when one exists is fine for constraints/picker UX, but ensure the old entry in
tracksdoesn’t linger (trackId stability matters). If trackIds change per creation, consider removing the stale entry after successful publish to avoid duplicates.Suggested follow-up (post-publish cleanup):
- if (enabled && track is RtcLocalScreenShareTrack) { + if (enabled && track is RtcLocalScreenShareTrack) { // Always create a new screen share track when enabling screen sharing. // This ensures correct handling of constraints on iOS and supports selecting different screens on web and desktop platforms. - return _createAndPublishTrack( + final res = await _createAndPublishTrack( trackType: trackType, constraints: constraints, ); + // If a new trackId is used, remove the stale entry. + res.fold( + success: (it) { + if (it.data.trackId != track.trackId) { + tracks.remove(track.trackId); + } + }, + failure: (_) {}, + ); + return res; }
1343-1349: LGTM: minimal and leak-free awaiter
awaitNativeWebRtcEvent<T>()viawhereType<T>().take(1).firstis concise and auto-cancels.
1175-1210: Ensure cancellation/stop on system UI cancel (stopped event path)If the user cancels from the system UI, confirm there’s a listener for a native “stopped/cancelled” event that mutes/unpublishes the screen-share track (and cancels any pending “started” subscription).
Would you like me to add a mirrored handler for
ScreenSharingStoppedEventthat:
- cancels
_screenSharingStartedSubscription,- calls
muteTrack/unpublishTrackfor screen share (and audio),- and updates state/logs?
Also applies to: 1351-1366
1-7: Clean up unused imports; whereType is properly available
- Remove
CancelableOperationimport (line 3): not used in this file.- Remove
utils/future.dartimport (line 22): not used in this file.whereType<T>()is available from the current RxDart import and works correctly throughout the codebase.-import 'package:async/async.dart' show CancelableOperation;-import '../utils/future.dart';
resolves FLU-293
This PR fixes an issue where screen sharing was not stopped correctly when canceled using system UI on Android or iOS. It also improves the iOS broadcast extension handling, waiting for a broadcast picker selection, with actually starting to screen share.
Summary by CodeRabbit
Bug Fixes
New Features
Chores