[video_player] Convert top-level classes to Swift by stuartmorgan-g · Pull Request #11658 · flutter/packages

stuartmorgan-g · 2026-05-06T17:56:09Z

Converts the top-level plugin class, along with the native view factory that is used for platform view management, from Objective-C to Swift. The Pigeon interface for the plugin class has been split out into a separate definition file so that it can use the Swift Pigeon generator while the player-instance-level API can continue to use Obj-C for now.

Since this is the first use of Swift in the plugin, this includes separating the Swift Package into more modules; now instead of:
video_player_avfoundation (Obj-C) -> video_player_avfoundation_ios and _macos (both Obj-C)
it's
video_player_avfoundation (Swift) -> video_player_avfoundation_objc (Obj-C) -> video_player_avfoundation_ios and _macos (both Obj-C)

The Swift conversions were done via Gemini as a starting point, but I did a side-by-side comparison to the Obj-C to ensure that everything was actually preserved, and fixed issues in its conversion manually.

The PR also includes adding some NONNULL region annotations to Obj-C headers that were accidentally missing it, since I ran into one of the omissions once I was calling the API from Swift, and then I audited all of the headers.

Part of flutter/flutter#119105

Pre-Review Checklist

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 converts the core native implementation of the video_player_avfoundation package from Objective-C to Swift, including the main plugin class and platform view factory. It also refactors the Pigeon API definitions into separate instance and plugin-level files. Feedback identifies a critical runtime crash risk in VideoPlayerPlugin.swift due to mutating the playersByIdentifier dictionary during iteration. Additionally, a safety concern was raised regarding the force-unwrapping of player lookups in NativeVideoViewFactory.swift, which could lead to crashes if an invalid ID is provided.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/VideoPlayerPlugin.swift (159)

Iterating directly over playersByIdentifier.values while calling disposeWithError is unsafe because disposeWithError triggers the onDisposed callback, which removes the player from the dictionary. Mutating a dictionary during iteration causes a runtime crash in Swift. You should iterate over a copy of the values, as was done in the original Objective-C implementation.

    for player in Array(playersByIdentifier.values) {

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/NativeVideoViewFactory.swift (63-64)

Force unwrapping the result of playerByIdentifierProvider will cause a crash if the playerId is not found in the registry. While the Dart side is expected to provide valid IDs, it's safer to handle a missing player gracefully to avoid crashing the host application. Returning a view with a default AVPlayer is a safer fallback than a runtime crash.

    guard let player = playerByIdentifierProvider(args.playerId) else {
      return FVPNativeVideoView(player: AVPlayer())
    }
    return FVPNativeVideoView(player: player.player)

stuartmorgan-g · 2026-05-06T18:14:03Z

Iterating directly over playersByIdentifier.values while calling disposeWithError is unsafe because disposeWithError triggers the onDisposed callback, which removes the player from the dictionary. Mutating a dictionary during iteration causes a runtime crash in Swift.

I researched this when I was locally reviewing the Gemini-created migration, because I was surprised that it had removed the copy, and from what I found it is safe. The Gemini comment here is misleading; based on what I found, the code here is not actually mutating the dictionary during iteration of the dictionary, but instead during iteration of an independent view.

hellohuanlin

LGTM

hellohuanlin · 2026-05-07T17:10:49Z

  }

-  @Test func playerForPlatformViewDoesNotRegisterTexture() {
+  @Test func playerForPlatformViewDoesNotRegisterTexture() throws {


hellohuanlin · 2026-05-07T17:11:43Z

+
+/// A factory class responsible for creating native video views that can be embedded in a
+/// Flutter app.
+class NativeVideoViewFactory: NSObject, FlutterPlatformViewFactory {


nit: final
is NSObject required?

nit: final

Done.

is NSObject required?

Yes; you can't have a Swift class that implements an Obj-C protocol without also inheriting from NSObject (it's a compiler error).

hellohuanlin · 2026-05-07T17:13:03Z

+
+  #if os(macOS)
+    func create(
+      withViewIdentifier viewId: Int64,


nit: viewID

should we be consistent with either using identifier or ID?

I changed it to viewIdentifier. Sadly this poor Obj-C variable naming was straight from our header.

hellohuanlin · 2026-05-07T17:14:20Z

+    }
+  #endif
+
+  func createArgsCodec() -> FlutterMessageCodec & NSObjectProtocol {


maybe just argsCodec as it doesn't actually create

I think many of the engine's Obj-C APIs are poorly named, but changing them is difficult. If the iOS team is willing to maintain multiple versions of a bunch of APIs for a long time horizon, so that we can eventually move toward better names, I'd definitely be on board with that, it's just not been something I've had a chance to prioritize.

stuartmorgan-g · 2026-05-06T18:02:56Z

-      with: FVPCreationOptions.make(withUri: "", httpHeaders: [:]), error: &error)
-    #expect(error == nil)
-    let player = videoPlayerPlugin.playersByIdentifier[identifiers!.playerId] as! FVPVideoPlayer
+    // Provide a URI that is a valid URI, but not a video, to trigger a failure inside AVPlayer.


Unlike the other changes in this file, this isn't just a rote structural change for the switch from testing a Obj-C-Pigeon-generated API to a Swift-Pigeon-generated API. The old test used "" as the URI, but that actually ends up creating a nil NSURL, which fails earlier than what this test needs now because we aren't internally passing along a nil NSURL on accident.

The goal of this test is to test an AVPlayer that fails to load a video, and for that we need a valid URI.

stuartmorgan-g · 2026-05-07T18:04:08Z

+    }
+  #endif
+
+  func createArgsCodec() -> FlutterMessageCodec & NSObjectProtocol {


I think many of the engine's Obj-C APIs are poorly named, but changing them is difficult. If the iOS team is willing to maintain multiple versions of a bunch of APIs for a long time horizon, so that we can eventually move toward better names, I'd definitely be on board with that, it's just not been something I've had a chance to prioritize.

stuartmorgan-g · 2026-05-07T20:15:58Z

+
+/// A factory class responsible for creating native video views that can be embedded in a
+/// Flutter app.
+class NativeVideoViewFactory: NSObject, FlutterPlatformViewFactory {


nit: final

Done.

is NSObject required?

Yes; you can't have a Swift class that implements an Obj-C protocol without also inheriting from NSObject (it's a compiler error).

stuartmorgan-g · 2026-05-07T20:33:39Z

+
+  #if os(macOS)
+    func create(
+      withViewIdentifier viewId: Int64,


I changed it to viewIdentifier. Sadly this poor Obj-C variable naming was straight from our header.

auto-submit · 2026-05-07T20:41:02Z

autosubmit label was removed for flutter/packages/11658, because - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

…#186256) flutter/packages@0411f1d...92552b1 2026-05-07 stuartmorgan@google.com [video_player] Convert top-level classes to Swift (flutter/packages#11658) 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 17 commits May 6, 2026 10:43

Restructure target layout

fa0d181

Split and regenerate Pigeon files

c0b50d0

Gemini conversion

63185b8

Update podspec

67d7b55

Manual audit of Gemini conversion, misc fixup

4968e4d

Convert native view factory

86ff314

More compilation fixes

e2d9cc9

autoformat

412a03b

Manual Swift style

2d73daa

Eliminate NSNumber

6e6cbbc

Replace force unwrap with if let idiom

b8d7dba

Minor class declaration cleanup

5cbd7a4

Test fixup WIP

702638b

Update unit tests

623c375

Fix macOS optionality handling

0c20da5

Version bump

a9d7f3d

Revert incidental project change from building

7207a9d

stuartmorgan-g requested review from LouiseHsu and hellohuanlin May 6, 2026 17:56

flutter-dashboard Bot added the CICD Run CI/CD label May 6, 2026

github-actions Bot added platform-ios platform-macos p: video_player labels May 6, 2026

Add the Swift podspec entries

0790989

github-actions Bot removed the CICD Run CI/CD label May 6, 2026

gemini-code-assist Bot reviewed May 6, 2026

View reviewed changes

Remove accidentally auto-added podspec entries

476d7a7

flutter-dashboard Bot added the CICD Run CI/CD label May 6, 2026

Pods :/

7ff22df

github-actions Bot removed the CICD Run CI/CD label May 6, 2026

stuartmorgan-g added the CICD Run CI/CD label May 6, 2026

hellohuanlin approved these changes May 7, 2026

View reviewed changes

Review feedback

d0516d7

stuartmorgan-g commented May 7, 2026

View reviewed changes

github-actions Bot removed the CICD Run CI/CD label May 7, 2026

stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD labels May 7, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2026

Add meta, for Pigeon

3d72e03

github-actions Bot removed the CICD Run CI/CD label May 7, 2026

stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD labels May 7, 2026

auto-submit Bot merged commit 92552b1 into flutter:main May 7, 2026
83 checks passed

engine-flutter-autoroll mentioned this pull request May 8, 2026

Roll Packages from 0411f1deabb4 to 92552b16bcc1 (1 revision) flutter/flutter#186256

Merged

Conversation

stuartmorgan-g commented May 6, 2026

Pre-Review Checklist

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/VideoPlayerPlugin.swift (159)

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/NativeVideoViewFactory.swift (63-64)

Uh oh!

stuartmorgan-g commented May 6, 2026

Uh oh!

hellohuanlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented May 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants