[video_player] Move more Obj-C logic to Dart #9685

stuartmorgan-g · 2025-07-28T15:00:33Z

Moves some logic from native code to Dart:

Asset -> URL lookup.
Conversion from native buffer range format (start+duration) to Dart range format (start+end).

Also does some minor native cleanup:

Removes unnecessary state from FVPVideoPlayerPlugin; several registrar-vended objects were being stored, and later changes required storing the registrar itself, so now we can just get those from the registrar on demand.
Removes nil player handling; the player can never return nil now.
Removes formatHint from the Pigeon interface; it was only ever used on Android, so doesn't need to be sent across the language boundary.

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 moves asset-to-URL resolution logic from Objective-C to Dart, which improves the architecture. The native code refactoring, such as removing redundant state and simplifying player creation, is well done, and the tests are updated to cover the new logic. A missing break in a switch statement will cause asset loading to fail, and a comment with a suggested fix has been provided.

packages/video_player/video_player_avfoundation/lib/src/avfoundation_video_player.dart

LongCatIsLooong

LGTM

LongCatIsLooong · 2025-07-29T18:34:12Z

packages/video_player/video_player_avfoundation/lib/src/avfoundation_video_player.dart


-    String? asset;
-    String? packageName;
    String? uri;


nit: final?

That would make the code fail to compile if another type were added to DataSourceType, which is from a different package (and thus could be updated separately by clients). And preventing that would require a default case, which would mean we wouldn't get a warning about a non-exhaustive cases locally as a flag to update this code. So non-final has enough benefits to outweigh the downside.

Hmm I'm not sure I understand. The switch statement below is exhaustive and for each case in the switch the url variable is assigned exactly once so it could be final I think? I thought there's lint for this not sure why it is not complaining.

The switch statement below is exhaustive and for each case in the switch the url variable is assigned exactly once so it could be final I think?

Yes, right now, with video_player_platform_interface 6.4.0, it is exhaustive.

So imagine we had made it final, and published that as video_player_avfoundation 2.8.1. Then next week, we decide to add a new source type (e.g., in-memory bytes). So we make a series of PRs for the packages to add support for it, including:

video_player_platform_interface 6.5.0 adds a new DataSourceType.

video_player_avfoundation 2.9.0 adds support for the new DataSourceType, including updating this switch statement.

Now consider a client who updates video_player_platform_interface without updating video_player_avfoundation, which is perfectly valid. At this point the code no longer compiles.

The only way to prevent that outcome would be to make adding a new enum value be a major version change for the platform interface, and we a) try very hard to not make breaking changes to platform interface packages, and b) don't generally consider adding an enum value to be a breaking change.

So instead we code defensively in our switch statements that use cross-package enums. (I actually have a long-standing open feature request for a lint that would tell people not to rely on exhaustive switches for cross-platform enums, to use in libraries, for exactly this reason.)

…s#9685)

flutter/packages@ed235d1...d914120 2025-07-30 [email protected] Roll Flutter from 46b097a to c3279ca (18 revisions) (flutter/packages#9699) 2025-07-29 [email protected] [video_player] Move more Obj-C logic to Dart (flutter/packages#9685) 2025-07-29 [email protected] Update CODEOWNERS (flutter/packages#9692) 2025-07-29 [email protected] [video_player] Move more Java logic to Dart (flutter/packages#9672) 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

Moves some logic from native code to Dart: - Asset -> URL lookup. - Conversion from native buffer range format (start+duration) to Dart range format (start+end). Also does some minor native cleanup: - Removes unnecessary state from `FVPVideoPlayerPlugin`; several registrar-vended objects were being stored, and later changes required storing the registrar itself, so now we can just get those from the registrar on demand. - Removes nil player handling; the player can never return nil now. - Removes `formatHint` from the Pigeon interface; it was only ever used on Android, so doesn't need to be sent across the language boundary. 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.

flutter/packages@ed235d1...d914120 2025-07-30 [email protected] Roll Flutter from 46b097a to c3279ca (18 revisions) (flutter/packages#9699) 2025-07-29 [email protected] [video_player] Move more Obj-C logic to Dart (flutter/packages#9685) 2025-07-29 [email protected] Update CODEOWNERS (flutter/packages#9692) 2025-07-29 [email protected] [video_player] Move more Java logic to Dart (flutter/packages#9672) 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 added 10 commits July 28, 2025 10:39

Consolidate asset path logic; prep for moving to Dart

a691628

Move asset logic to Dart

b609ed0

Remove the unused format hint

e8bfe13

Consolidate switches

1d6bfa4

Remove some duplicate state

b5d07e5

Fix pre-existing race in example code

fba9c60

Version bump

cbf14f4

Update tests

b25e91e

Remove nil player handling; creating a player can't return nil

4f70486

macOS fix

ece0bb3

stuartmorgan-g requested review from LongCatIsLooong and hellohuanlin as code owners July 28, 2025 15:00

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

gemini-code-assist bot reviewed Jul 28, 2025

View reviewed changes

packages/video_player/video_player_avfoundation/lib/src/avfoundation_video_player.dart Show resolved Hide resolved

Move buffer duration transform logic

a2af01d

LongCatIsLooong approved these changes Jul 29, 2025

View reviewed changes

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

auto-submit bot merged commit 5d2a10f into flutter:main Jul 29, 2025
80 checks passed

stuartmorgan-g deleted the video-player-avfoundation-more-dart-logic branch July 30, 2025 10:06

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

5d2a10f [video_player] Move more Obj-C logic to Dart (flutter/package…

29d42e0

…s#9685)

engine-flutter-autoroll mentioned this pull request Jul 30, 2025

Roll Packages from ed235d11b7fd to 5d2a10fc1bb1 (3 revisions) flutter/flutter#172971

Closed

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

5d2a10f [video_player] Move more Obj-C logic to Dart (flutter/package…

ff9abe4

…s#9685)

engine-flutter-autoroll mentioned this pull request Jul 30, 2025

Roll Packages from ed235d11b7fd to d91412017a01 (4 revisions) flutter/flutter#172975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[video_player] Move more Obj-C logic to Dart #9685

[video_player] Move more Obj-C logic to Dart #9685

Uh oh!

stuartmorgan-g commented Jul 28, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

LongCatIsLooong left a comment

Uh oh!

LongCatIsLooong Jul 29, 2025

Uh oh!

stuartmorgan-g Jul 29, 2025

Uh oh!

LongCatIsLooong Jul 29, 2025

Uh oh!

stuartmorgan-g Jul 30, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[video_player] Move more Obj-C logic to Dart #9685

[video_player] Move more Obj-C logic to Dart #9685

Uh oh!

Conversation

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

Uh oh!

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!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jul 30, 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stuartmorgan-g commented Jul 28, 2025 •

edited

Loading

stuartmorgan-g Jul 30, 2025 •

edited

Loading