[video_player] Simplify native iOS code #9800

stuartmorgan-g · 2025-08-13T16:00:47Z

Refactors and simplifies some of the native code in video_player_avfoundation:

Separates create into two different entry points for the two display types, instead of having one entry point and then branching several times based on the display type.
Moves dispose to the player instance, with the other instance-specific calls, by making plugin-level tracking cleanup part of onDispose.
Moves the URL->AVPlayerItem logic out of the two player classes and into a central helper in the plugin class, reducing duplication and simplifying the player initialization logic.
Addresses the TODO to separate texture IDs from player IDs.
Makes expectFrame a private implementation detail rather than making a client call it.
Removes an ARC check; we have used ARC everywhere in this repo for a long time.

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. ↩ ↩² ↩³

gemini-code-assist

Code Review

This pull request introduces a significant and well-executed refactoring of the native iOS/macOS code for the video_player_avfoundation plugin. The changes simplify the API by separating player creation based on view type, moving disposal logic to player instances, and centralizing AVPlayerItem creation. This improves code organization, reduces duplication, and enhances maintainability. The separation of player and texture IDs is also a welcome improvement. The accompanying test changes are thorough and correctly reflect the new API. I have a couple of minor suggestions to improve the changelog description and strengthen one of the tests.

packages/video_player/video_player_avfoundation/CHANGELOG.md

packages/video_player/video_player_avfoundation/test/avfoundation_video_player_test.dart

LongCatIsLooong · 2025-08-14T18:06:44Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -186,55 +165,64 @@ - (void)initialize:(FlutterError *__autoreleasing *)error {
  upgradeAudioSessionCategory(AVAudioSessionCategoryPlayback, 0, 0);
 #endif

-  [self.playersByIdentifier.allValues makeObjectsPerformSelector:@selector(dispose)];
+  FlutterError *disposeError;


This local variable seems write-only?

Yeah, Pigeon generates its methods with non-null error pointers since it should always be calling them with a value, so they don't accidentally get dropped.

The alternative here is that I could make a public dispose and make disposeWithError: call that, and then have these call points call dispose. I couldn't decide at the time if that was worth the extra method, so whichever you prefer.

Ah thanks for the explanation. I'm onboard with keeping the local variable then.

LongCatIsLooong · 2025-08-14T18:14:30Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

-  if (textureBasedPlayer) {
-    playerIdentifier = [self.registrar.textures registerTexture:textureBasedPlayer];
-    [textureBasedPlayer setTextureIdentifier:playerIdentifier];
-  } else {


Out of curiosity, is there a reason for generating playerIdentifier differently for texture based and platform view based players? Looks like it's trying to use texture identifier as player identifier?

Historically, there was only the texture ID, and that was used throughout the plugin as the identifier for the player instance. When the platform view variant was added recently, that broke down; we still needed an identifier for each player, but not all players had texture IDs.

The PRs that added platform view support renamed all the platform interface parameters from textureId to playerId since that was simple, but to minimize churn in already-large PRs they didn't disentangle the texture ID from the player ID in the platform implementation internals. This is paying off that tech debt by switching to having a single way to manage player IDs, and reporting texture IDs separately in the texture case.

)

flutter/packages@09533b7...5c52c55 2025-08-15 stuartmorgan@google.com [video_player] Move Android buffer updates to Dart (flutter/packages#9771) 2025-08-15 sfprhythnn@gmail.com [webview_flutter] Add support for payment requests on Android (flutter/packages#9679) 2025-08-15 jason-simmons@users.noreply.github.com [vector_graphics_compiler] Set the m4_10 (Z scale) value to 1 when constructing an AffineMatrix from an SVG matrix (flutter/packages#9813) 2025-08-15 magder@google.com [url_launcher_ios] Fix test button text to work on iOS 26 (flutter/packages#9766) 2025-08-15 stuartmorgan@google.com [video_player] Simplify native iOS code (flutter/packages#9800) 2025-08-14 stuartmorgan@google.com [image_picker] Add the ability to pick multiple videos - platform_interface (flutter/packages#9804) 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 flutter-ecosystem@google.com 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

…r#173854) flutter/packages@09533b7...5c52c55 2025-08-15 stuartmorgan@google.com [video_player] Move Android buffer updates to Dart (flutter/packages#9771) 2025-08-15 sfprhythnn@gmail.com [webview_flutter] Add support for payment requests on Android (flutter/packages#9679) 2025-08-15 jason-simmons@users.noreply.github.com [vector_graphics_compiler] Set the m4_10 (Z scale) value to 1 when constructing an AffineMatrix from an SVG matrix (flutter/packages#9813) 2025-08-15 magder@google.com [url_launcher_ios] Fix test button text to work on iOS 26 (flutter/packages#9766) 2025-08-15 stuartmorgan@google.com [video_player] Simplify native iOS code (flutter/packages#9800) 2025-08-14 stuartmorgan@google.com [image_picker] Add the ability to pick multiple videos - platform_interface (flutter/packages#9804) 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 flutter-ecosystem@google.com 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

Refactors and simplifies some of the native code in `video_player_avfoundation`: - Separates `create` into two different entry points for the two display types, instead of having one entry point and then branching several times based on the display type. - Moves `dispose` to the player instance, with the other instance-specific calls, by making plugin-level tracking cleanup part of `onDispose`. - Moves the URL->`AVPlayerItem` logic out of the two player classes and into a central helper in the plugin class, reducing duplication and simplifying the player initialization logic. - Addresses the TODO to separate texture IDs from player IDs. - Makes `expectFrame` a private implementation detail rather than making a client call it. - Removes an ARC check; we have used ARC everywhere in this repo for a long time. Part of flutter/flutter#172763 ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). 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. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

…r#173854) flutter/packages@09533b7...5c52c55 2025-08-15 stuartmorgan@google.com [video_player] Move Android buffer updates to Dart (flutter/packages#9771) 2025-08-15 sfprhythnn@gmail.com [webview_flutter] Add support for payment requests on Android (flutter/packages#9679) 2025-08-15 jason-simmons@users.noreply.github.com [vector_graphics_compiler] Set the m4_10 (Z scale) value to 1 when constructing an AffineMatrix from an SVG matrix (flutter/packages#9813) 2025-08-15 magder@google.com [url_launcher_ios] Fix test button text to work on iOS 26 (flutter/packages#9766) 2025-08-15 stuartmorgan@google.com [video_player] Simplify native iOS code (flutter/packages#9800) 2025-08-14 stuartmorgan@google.com [image_picker] Add the ability to pick multiple videos - platform_interface (flutter/packages#9804) 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 flutter-ecosystem@google.com 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 added 7 commits August 13, 2025 11:47

Make expectFrame private to the texture player

436fe4b

De-dup AVPlayerItem construction

b3237fa

Move dispose to instances

ecdaf2b

Separate player ID from texture ID

e1751bb

Fix native test refactor

a88e47d

Remove ARC check

aada448

Version bump

1aa6cd4

stuartmorgan-g requested review from LongCatIsLooong and hellohuanlin as code owners August 13, 2025 16:00

github-actions bot added platform-ios platform-macos p: video_player labels Aug 13, 2025

gemini-code-assist bot reviewed Aug 13, 2025

View reviewed changes

packages/video_player/video_player_avfoundation/CHANGELOG.md Show resolved Hide resolved

packages/video_player/video_player_avfoundation/test/avfoundation_video_player_test.dart Show resolved Hide resolved

Fix implementation signature

5604a09

LongCatIsLooong approved these changes Aug 14, 2025

View reviewed changes

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

auto-submit bot merged commit 201ec5a into flutter:main Aug 15, 2025
80 checks passed

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

201ec5a38 [video_player] Simplify native iOS code (flutter/packages#9800

3af348f

)

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

Roll Packages from 09533b7d5d66 to f53df16dc595 (5 revisions) flutter/flutter#173853

Closed

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

201ec5a38 [video_player] Simplify native iOS code (flutter/packages#9800

349abee

)

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

Roll Packages from 09533b7d5d66 to 5c52c5545f54 (6 revisions) flutter/flutter#173854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[video_player] Simplify native iOS code #9800

[video_player] Simplify native iOS code #9800

stuartmorgan-g commented Aug 13, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong Aug 14, 2025

Uh oh!

stuartmorgan-g Aug 14, 2025 •

edited

Loading

Uh oh!

LongCatIsLooong Aug 14, 2025

Uh oh!

LongCatIsLooong Aug 14, 2025

Uh oh!

stuartmorgan-g Aug 14, 2025

Uh oh!

Uh oh!

Uh oh!

[video_player] Simplify native iOS code #9800

[video_player] Simplify native iOS code #9800

Conversation

stuartmorgan-g commented Aug 13, 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!

LongCatIsLooong Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g Aug 14, 2025 •

edited

Loading