[video_player] Separate event stream from player on iOS #9700

stuartmorgan-g · 2025-07-30T16:02:19Z

Abstracts the details of the native->Dart communication system from the player instance by adding a listener protocol, and moving the event channel into an implementation of that protocol.

Also simplifies the logic to avoid messaging races during channel setup by adding a generic queue to the listener implementation instead of requiring the player to track whether or not it's allowed to send messages, and hard-coding specific messages to re-send later.

Part of flutter/flutter#172763

Pre-Review Checklist

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

Abstracts the details of the native->Dart communication system from the player instance by adding a listener protocol, and moving the event channel into an implementation of that protocol. Also simplifies the logic to avoid messaging races during channel setup by adding a generic queue to the listener implementation instead of requiring the player to track whether or not it's allowed to send messages, and hard-coding specific messages to re-send later. Part of flutter/flutter#172763

gemini-code-assist

Code Review

This pull request refactors the event handling mechanism on iOS to decouple the video player from the Flutter event channel implementation. It introduces a new FVPVideoEventListener protocol and an FVPEventBridge class that handles communication with Dart. This is a good architectural improvement that increases modularity and testability. The changes also fix a potential race condition during event channel setup by queueing events. The review identified a few issues: a high-severity bug where an event was being sent inside a loop incorrectly, and some medium-severity maintainability issues related to typos in test code and property attributes.

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m

packages/video_player/video_player_avfoundation/darwin/RunnerTests/VideoPlayerTests.m

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPEventBridge.m

stuartmorgan-g · 2025-07-30T16:17:00Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the native video player by introducing an FVPVideoEventListener protocol, decoupling it from the Flutter event channel communication. This improves the architecture and maintainability of the code. The addition of an event queue in FVPEventBridge to handle potential race conditions during initialization is also a great improvement.

The tests have been updated to reflect these changes, using a stub implementation of the new listener protocol, which makes them cleaner.

I have a couple of minor suggestions to improve code clarity and type safety.

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPEventBridge.m

.../Sources/video_player_avfoundation/include/video_player_avfoundation/FVPVideoEventListener.h

LongCatIsLooong · 2025-07-30T18:21:27Z

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPEventBridge.m

+  return nil;
+}
+
+- (FlutterError *_Nullable)onCancelWithArguments:(id _Nullable)arguments {


The documentation says:

Such request pairs may occur during Flutter hot restart.

Are plugins destroyed and recreated during hot restart, or they live through hot restart? If it's the latter then the event queue is always nil after a hot restart?

The native code's plugin instance survives hot restart, but video_player calls an init method when the Dart plugin instance (which doesn't survive) is created, which will call this code and destroy all the player instances, and thus their bridges.

(Even if it didn't, since these channels have ID-based names, and the IDs are assigned by the native code, after a hot restart it should never be possible for the Dart code to try re-listening to any previously-existing video player event channels anyway.)

LongCatIsLooong

Whoops forgot to LGTM

…ter/packages#9700)

flutter/packages@db6988d...f0645d8 2025-08-01 [email protected] Mark `FlutterIOOverrides` as `final` (flutter/packages#9719) 2025-07-31 [email protected] [in_app_purchase_storekit] Updated comment (flutter/packages#9445) 2025-07-31 [email protected] [video_player] Separate event stream from player on iOS (flutter/packages#9700) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

stuartmorgan-g requested review from LongCatIsLooong and hellohuanlin as code owners July 30, 2025 16:02

github-actions bot added platform-ios platform-macos p: video_player labels Jul 30, 2025

gemini-code-assist bot reviewed Jul 30, 2025

View reviewed changes

Gemini review fixes

e662b81

gemini-code-assist bot reviewed Jul 30, 2025

View reviewed changes

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPEventBridge.m Outdated Show resolved Hide resolved

.../Sources/video_player_avfoundation/include/video_player_avfoundation/FVPVideoEventListener.h Outdated Show resolved Hide resolved

Gemini nits

c8489e7

LongCatIsLooong reviewed Jul 30, 2025

View reviewed changes

LongCatIsLooong approved these changes Jul 31, 2025

View reviewed changes

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 31, 2025

auto-submit bot merged commit fe53da2 into flutter:main Jul 31, 2025
80 checks passed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 1, 2025

fe53da2 [video_player] Separate event stream from player on iOS (flut…

d1265b3

…ter/packages#9700)

engine-flutter-autoroll mentioned this pull request Aug 1, 2025

Roll Packages from db6988d785fa to f0645d8448a5 (3 revisions) flutter/flutter#173111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[video_player] Separate event stream from player on iOS #9700

[video_player] Separate event stream from player on iOS #9700

Uh oh!

stuartmorgan-g commented Jul 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Jul 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong Jul 30, 2025

Uh oh!

stuartmorgan-g Jul 30, 2025

Uh oh!

LongCatIsLooong left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[video_player] Separate event stream from player on iOS #9700

[video_player] Separate event stream from player on iOS #9700

Uh oh!

Conversation

stuartmorgan-g commented Jul 30, 2025

Pre-Review Checklist

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Jul 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants