fix(ui): Skia flickering fix on Android #1147
Conversation
# Conflicts: # packages/stream_video_flutter/lib/src/call_participants/call_participant.dart
📝 WalkthroughWalkthroughThis PR implements Android Picture-in-Picture (PiP) support for video calls and fixes video flickering by introducing renderer scope prefixing. It bumps the Changes
Sequence Diagram(s)sequenceDiagram
participant CallContent as call_content.dart
participant APM as AndroidPipManager
participant Platform as Native Android
participant UI as UI Layer
CallContent->>APM: getInstance()
CallContent->>APM: addOnPictureInPictureModeChangedListener()
Note over Platform: User enters PiP mode
Platform->>APM: methodChannel.invokeMethod('onPictureInPictureModeChanged')
APM->>APM: _handleMethodCall()
APM->>APM: _notifyListeners()
APM->>CallContent: listener callback (isInPictureInPictureMode=true)
CallContent->>CallContent: _handlePictureInPictureModeChanged()
CallContent->>CallContent: setState()
CallContent->>UI: rebuild with _isInPictureInPictureMode flag
UI->>UI: conditionally hide participants widget if in PiP
Note over Platform: User exits PiP mode
Platform->>APM: methodChannel.invokeMethod('onPictureInPictureModeChanged')
APM->>CallContent: listener callback (isInPictureInPictureMode=false)
CallContent->>UI: rebuild and show participants widget
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 #1147 +/- ##
========================================
+ Coverage 6.44% 6.47% +0.03%
========================================
Files 600 601 +1
Lines 41978 42018 +40
========================================
+ Hits 2705 2721 +16
- Misses 39273 39297 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This reverts commit 73322ac.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video_flutter/lib/src/call_participants/livestream_hosts.dart (1)
99-106: MissingrendererScopePrefixin_buildScreenShareContent.The fallback
ScreenShareContentwidget doesn't passrendererScopePrefix, unlike similar usages elsewhere in this PR:
livestream_content.dartpassesrendererScopePrefix: 'livecontent'android_pip_overlay.dartpassesrendererScopePrefix: 'pipScreenShare'screen_share_call_participants_content.dartpassesrendererScopePrefix: 'screenshareContent'Given this PR's goal is to fix Skia flickering via renderer scope prefixing, the missing prefix here could leave this code path vulnerable to the same issue.
🐛 Suggested fix
Widget _buildScreenShareContent(CallParticipantState host) { return widget.screenShareContentBuilder?.call(context, widget.call, host) ?? ScreenShareContent( key: ValueKey('${host.uniqueParticipantKey}-livehost-screenshare'), + rendererScopePrefix: 'livehost', call: widget.call, participant: host, ); }
🤖 Fix all issues with AI agents
In
`@packages/stream_video_flutter/lib/src/call_screen/call_content/picture_in_picture/android_pip_manager.dart`:
- Around line 53-61: In _handleMethodCall, avoid mutating
_onPictureInPictureModeChangedListeners while iterating (which can throw
ConcurrentModificationError) by iterating over a shallow copy of the list before
invoking each listener; keep setting _isInPictureInPictureMode from
call.arguments as before and then iterate over something like
List.from(_onPictureInPictureModeChangedListeners) so listeners can safely
remove themselves during notification.
🧹 Nitpick comments (5)
packages/stream_video_flutter/lib/src/renderer/video_renderer.dart (1)
110-156: Add a delimiter between prefix and participant key to avoid ambiguous collisions.
Without a separator, certain prefix/participant combinations could collide (e.g., prefix “live” + key “host123” vs prefix “livehost” + key “123”).♻️ Proposed tweak (delimiter)
- return VisibilityDetector( - key: Key( - '${widget.rendererScopePrefix ?? ''}${widget.participant.uniqueParticipantKey}${widget.videoTrackType}-visibility', - ), + final scopePrefix = + widget.rendererScopePrefix == null ? '' : '${widget.rendererScopePrefix}-'; + return VisibilityDetector( + key: Key( + '$scopePrefix${widget.participant.uniqueParticipantKey}${widget.videoTrackType}-visibility', + ), onVisibilityChanged: (info) => _onVisibilityChanged(info, widget.participant.userId), child: child, );- return VideoTrackRenderer( - key: Key( - '${widget.rendererScopePrefix ?? ''}${widget.participant.uniqueParticipantKey}-${widget.videoTrackType}-renderer', - ), + final scopePrefix = + widget.rendererScopePrefix == null ? '' : '${widget.rendererScopePrefix}-'; + return VideoTrackRenderer( + key: Key( + '$scopePrefix${widget.participant.uniqueParticipantKey}-${widget.videoTrackType}-renderer', + ), videoFit: widget.videoFit, videoTrack: videoTrack, mirror: mirror, placeholderBuilder: widget.placeholderBuilder, );packages/stream_video_flutter/lib/src/call_screen/call_content/picture_in_picture/stream_picture_in_picture_android_view.dart (1)
100-102: Consider handling potential errors from the async call.
_setPictureInPictureAllowedreturns aFuture<void>but is called withoutawaitin multiple places (lines 60, 75, 96). While fire-and-forget may be intentional here, unhandled errors from the platform channel could go unnoticed.If errors are expected to be silently ignored, consider adding
.ignore()or a minimal error handler to make this explicit:_setPictureInPictureAllowed(shouldAllow).ignore();packages/stream_video_flutter/lib/src/call_screen/call_content/picture_in_picture/android_pip_overlay.dart (1)
97-113: Key format inconsistency with other files.The key separators here use
-(space-dash-space):
- Line 99:
'${pipParticipant.uniqueParticipantKey} - pipScreenShare'- Line 108:
'${pipParticipant.uniqueParticipantKey} - pipVideo'However, other files in this PR use
-(dash only):
screen_share_call_participants_content.dart:'-screenShareContent','-screenshare-video'livestream_content.dart:'-livecontent-video','-livecontent-screenshare'livestream_hosts.dart:'-livehost-video','-livehost-screenshare'Consider using a consistent separator format across all files for maintainability.
♻️ Suggested fix for consistency
if (shouldShowScreenShare) { pipBody = ScreenShareContent( key: ValueKey( - '${pipParticipant.uniqueParticipantKey} - pipScreenShare', + '${pipParticipant.uniqueParticipantKey}-pipScreenShare', ), rendererScopePrefix: 'pipScreenShare', call: widget.call, participant: pipParticipant, ); } else { pipBody = StreamCallParticipant( key: ValueKey( - '${pipParticipant.uniqueParticipantKey} - pipVideo', + '${pipParticipant.uniqueParticipantKey}-pipVideo', ), rendererScopePrefix: 'pipVideo', call: call, participant: pipParticipant, ); }packages/stream_video_flutter/lib/src/call_participants/screen_share_call_participants_content.dart (1)
181-185: Potential key format inconsistency when prefix is present.When
rendererScopePrefixis provided, the key becomes'{prefix}{participantKey}-screen-share'without a separator between the prefix and participant key. For example:'screenshareContentuser123-screen-share'.Consider adding a separator for consistency and readability:
♻️ Suggested improvement
child: StreamVideoRenderer( key: ValueKey( - '${widget.rendererScopePrefix ?? ''}${widget.participant.uniqueParticipantKey}-screen-share', + '${widget.rendererScopePrefix != null ? '${widget.rendererScopePrefix}-' : ''}${widget.participant.uniqueParticipantKey}-screen-share', ), rendererScopePrefix: widget.rendererScopePrefix,packages/stream_video_flutter/lib/src/call_screen/call_content/call_content.dart (1)
108-110: Consider guarding AndroidPipManager listener registration with a platform check.
AndroidPipManagerlisteners are registered and removed unconditionally (lines 123-125, 141-143) on all platforms. While the code functions correctly since the method channel won't deliver Android callbacks on non-Android platforms, it's cleaner to explicitly guard this Android-specific functionality with a platform check, consistent with the PiP view rendering logic (lines 195, 205).♻️ Suggested refactor
`@override` void initState() { super.initState(); _startListeningToCallState(); - _androidPipManager.addOnPictureInPictureModeChangedListener( - _handlePictureInPictureModeChanged, - ); + if (CurrentPlatform.isAndroid) { + _androidPipManager.addOnPictureInPictureModeChangedListener( + _handlePictureInPictureModeChanged, + ); + } }`@override` void dispose() { _callStateSubscription?.cancel(); - _androidPipManager.removeOnPictureInPictureModeChangedListener( - _handlePictureInPictureModeChanged, - ); + if (CurrentPlatform.isAndroid) { + _androidPipManager.removeOnPictureInPictureModeChangedListener( + _handlePictureInPictureModeChanged, + ); + } super.dispose(); }
...m_video_flutter/lib/src/call_screen/call_content/picture_in_picture/android_pip_manager.dart
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.