[video_player] Move iOS/macOS to per-player-instance Pigeon APIs #9529

stuartmorgan-g · 2025-06-30T17:39:53Z

Rather than having a single API to talk directly to the plugin, and having most of the methods in that API just do a map lookup and dispatch to a player instance, which was necessary when Pigeon didn't support instantiating multiple instances of an API, have each player instance set up an API instance that the Dart code can talk to directly. This follows the pattern used by plugins that migrated to Pigeon more recently (e.g., google_maps_flutter has a very similar pattern), reduces boilerplate native code, and moves closer to what an FFI-based implementation would look like if we go that route in the future.

Since the Dart unit tests needed to be significantly reworked anyway, this also moves to the pattern we are using in newer plugin code, where we use mockito to mock the Pigeon API surface. The "call log" approach it replaces dates back to pre-Pigeon, when only way to test that the right platform calls were made was to intercept and track method channel calls at the framework level.

Also updates to the latest version of Pigeon.

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

stuartmorgan-g · 2025-06-30T17:40:26Z

(c.f. #9511, which is the same structural change for the Android implementation.)

stuartmorgan-g · 2025-06-30T17:44:55Z

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

-      [self expectationForPredicate:[NSPredicate predicateWithFormat:@"position == 1234"]
-                evaluatedWithObject:player
-                            handler:nil];
-  [self waitForExpectations:@[ positionExpectation ] timeout:3.0];


This expectation was failing for me locally, and I believe it's been flaking occasionally. IIRC this check dates to before we waited for the seek to fully finish (the expectation fulfilled above, whose name I fixed). This test doesn't actually test the logic of seeking itself, only the frame handling around seeking, so expectation above should be all we need to order the test validations correctly.

Same with the change below.

stuartmorgan-g · 2025-06-30T17:47:04Z

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

@@ -590,59 +583,36 @@ - (void)testBufferingStateFromPlayer {
 }

 - (void)testVideoControls {
-  NSObject<FlutterPluginRegistrar> *registrar = OCMProtocolMock(@protocol(FlutterPluginRegistrar));


The rest of the changes in this file are just cleanup for cases that used to test via the plugin, but were really just testing player functionality so could be easily switched to creating a player instance directly instead of creating and driving the player via the plugin (which after my change was creating the player via the plugin, looking it up, and then driving it via the player anyway). See 1963bb3 for that cleanup, isolated as a separate commit.

stuartmorgan-g · 2025-06-30T17:48:02Z

...win/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m

 @end

 @implementation FVPTextureBasedVideoPlayer
 - (instancetype)initWithAsset:(NSString *)asset
                 frameUpdater:(FVPFrameUpdater *)frameUpdater
                  displayLink:(FVPDisplayLink *)displayLink
                    avFactory:(id<FVPAVFactory>)avFactory
-                 viewProvider:(NSObject<FVPViewProvider> *)viewProvider
-                   onDisposed:(void (^)(int64_t))onDisposed {


This changed to being a post-init property assignment because we need the player ID, which is only available just after creation.

stuartmorgan-g · 2025-06-30T17:48:28Z

...win/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m

-  [super dispose];
-
-  _onDisposed(self.frameUpdater.textureIdentifier);
-}


This moved to the superclass since all players have an onDisposed now.

stuartmorgan-g · 2025-06-30T17:49:20Z

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

+/// This method allows you to dispose without touching the event channel. This
+/// is useful for the case where the Engine is in the process of deconstruction
+/// so the channel is going to die or is already dead.
+- (void)disposeSansEventChannel {


This is moved from below without change, to create a #pragma mark - separated section for each protocol implemented.

stuartmorgan-g · 2025-06-30T17:49:43Z

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

+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+- (void)dispose {


This was also moved, for the same reason. The change is the addition of the _onDisposed call.

stuartmorgan-g · 2025-06-30T17:50:01Z

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

-  return FVPCMTimeToMillis([_player currentTime]);
-}
-
- (int64_t)duration {


This is moved below as it's not part of of the protocol.

stuartmorgan-g · 2025-06-30T17:50:41Z

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

- (void)pausePlayer:(NSInteger)playerIdentifier error:(FlutterError **)error {
-  FVPVideoPlayer *player = self.playersByIdentifier[@(playerIdentifier)];
-  [player pause];
-}


This is the core of the change: we no longer need boilerplate like this for every player method.

stuartmorgan-g · 2025-06-30T17:52:55Z

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

+  __weak typeof(self) weakSelf = self;
+  BOOL isTextureBased = textureBasedPlayer != nil;
+  player.onDisposed = ^() {
+    SetUpFVPVideoPlayerInstanceApiWithSuffix(messenger, nil, channelSuffix);


The addition of onDisposed to the base class is to allow easily cleaning up the communication channel, without having to push all the communication channel wire-up into the player class (which would be a bigger change, and is also undesirable for potential conversion to using FFI to talk directly to the player instance).

I'm curious why this would have FFI implications? In my mind the communication channel wire-up / tear-down part are implementation details within the platform, the dart side should be able to tell the plugin to create a player, or dispose an existing player without involving any communication channel concepts?

LongCatIsLooong

LGTM

LongCatIsLooong · 2025-07-01T23:32:38Z

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

+  /// exist.
+  @visibleForTesting
+  VideoPlayerInstanceApi ensureApiInitialized(int playerId) {
+    VideoPlayerInstanceApi? api = _playerApis[playerId];


nit: putIfAbsent?

LongCatIsLooong · 2025-07-02T17:54:21Z

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

@@ -219,6 +252,14 @@ class AVFoundationVideoPlayer extends VideoPlayerPlatform {
    return EventChannel('flutter.io/videoPlayer/videoEvents$playerId');
  }

+  VideoPlayerInstanceApi _apiFor(int playerId) {


Question: as a layman reader I find the "api" in these names slightly confusing. Is this a naming convention? I'd go with something like VideoPlayerInstance _playerWith(playerId) since these are just players?

LongCatIsLooong · 2025-07-02T17:55:31Z

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

@@ -219,6 +252,14 @@ class AVFoundationVideoPlayer extends VideoPlayerPlatform {
    return EventChannel('flutter.io/videoPlayer/videoEvents$playerId');
  }

+  VideoPlayerInstanceApi _apiFor(int playerId) {
+    final VideoPlayerInstanceApi? api = _playerApis[playerId];
+    if (api == null) {


uber nit:

return _playerApis[playerId] ?? (throw StateError('No active player with ID $playerId.'));

LongCatIsLooong · 2025-07-02T20:41:44Z

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

+  __weak typeof(self) weakSelf = self;
+  BOOL isTextureBased = textureBasedPlayer != nil;
+  player.onDisposed = ^() {
+    SetUpFVPVideoPlayerInstanceApiWithSuffix(messenger, nil, channelSuffix);


I'm curious why this would have FFI implications? In my mind the communication channel wire-up / tear-down part are implementation details within the platform, the dart side should be able to tell the plugin to create a player, or dispose an existing player without involving any communication channel concepts?

stuartmorgan-g added 4 commits June 30, 2025 12:33

Implement per-instance channels

e9604dd

Simplify tests

1963bb3

Version bump

3f7993a

Revert project automigrations

fc05c2d

stuartmorgan-g requested review from LongCatIsLooong and hellohuanlin as code owners June 30, 2025 17:39

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

stuartmorgan-g mentioned this pull request Jun 30, 2025

[video_player] Move Android to per-player-instance Pigeon APIs #9511

Open

11 tasks

stuartmorgan-g commented Jun 30, 2025

View reviewed changes

LongCatIsLooong approved these changes Jul 2, 2025

View reviewed changes

[video_player] Move iOS/macOS to per-player-instance Pigeon APIs #9529

Are you sure you want to change the base?

[video_player] Move iOS/macOS to per-player-instance Pigeon APIs #9529

Conversation

stuartmorgan-g commented Jun 30, 2025

Pre-Review Checklist

Footnotes

Uh oh!

stuartmorgan-g commented Jun 30, 2025

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!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jul 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LongCatIsLooong Jul 2, 2025 •

edited

Loading