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

stuartmorgan-g · 2025-06-27T15:05:47Z

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-27T15:07:58Z

...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java

+  public @NonNull Long getPosition() {
+    long position = exoPlayer.getCurrentPosition();
+    // TODO(stuartmorgan): Move this; this is relying on the fact that getPosition is called
+    //  frequently to drive buffering updates, which is a fragile hack.


This hack is pre-existing; it's just being moved from the VideoPlayerPlugin.java version of this method. I added the comment so that future people don't have to go through the same process I did of wondering why getting the position sends a buffer update as a side effect.

stuartmorgan-g · 2025-06-27T15:39:04Z

packages/video_player/video_player_android/test/android_video_player_test.dart

  TestWidgetsFlutterBinding.ensureInitialized();

+  (AndroidVideoPlayer, MockAndroidVideoPlayerApi, MockVideoPlayerInstanceApi)
+  setUpMockPlayer({required int playerId}) {


This, and the corresponding removal of the variables and setUp below, is me taking the opportunity to move away from stateful test fixtures (for the reasons described in https://abseil.io/tips/122) while I was reworking this anyway.

I could have made these state variables and configured them in setUp, following the old structure, but I'm trying to move the packages tests away from stateful fixtures over time.

stuartmorgan-g · 2025-06-27T15:42:14Z

packages/video_player/video_player_android/test/android_video_player_test.dart

+      when(api.create(any)).thenAnswer((_) async => newPlayerId);
+
+      const String asset = 'someAsset';
+      const String package = 'somePackage';


Another opportunistic change throughout the tests; I moved from repeated-magic-values to constants. The repeated magic values is something Reid had flagged a while ago as bad patter in flutter/packages tests, so we've been slowly moving toward using a constant when we are trying to assert that values are the same across steps of a test.

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

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

gmackall · 2025-07-15T21:14:03Z

From android triage: @ash2moon are you able to take a look at this, when you get a chance?

stuartmorgan-g · 2025-07-16T19:53:52Z

FYI I'm going to use this PR as a quick initial test of the new Gemini review bot we just turned on for this repo as an experiment, so you'll see some comment traffic from that.

stuartmorgan-g · 2025-07-16T19:53:59Z

/gemini review

gemini-code-assist

Code Review

The code changes introduce the ability to load multiple signed images to a registry using a bulk load feature. There are a couple of potential issues related to long to int casting that could lead to bugs with very long videos or when using platform views. There is also a minor suggestion to improve a test assertion.

gemini-code-assist · 2025-07-16T19:55:29Z

...o_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java

+    // Set up the instance-specific API handler, and make sure it is removed when the player is
+    // disposed.
+    BinaryMessenger messenger = flutterState.binaryMessenger;
+    final String channelSuffix = Integer.toString((int) id);


The player id is a long. For platform view players, this ID starts near Long.MAX_VALUE. Casting it to an int with (int) id will cause an overflow, resulting in an incorrect channel suffix. This will break the communication channel for that player instance.

You should use Long.toString(id) to safely convert the long ID to a string for the channel suffix.

Suggested change

final String channelSuffix = Integer.toString((int) id);

final String channelSuffix = Long.toString(id);

gemini-code-assist · 2025-07-16T19:55:30Z

...r/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java

+  public void seekTo(@NonNull Long position) {
+    exoPlayer.seekTo(position.intValue());
  }


The ExoPlayer.seekTo method expects a long value for the position in milliseconds. By casting position to an int using position.intValue(), you risk truncating the value if it exceeds Integer.MAX_VALUE (approximately 24.8 days). This can lead to incorrect seeking for very long videos.

You should pass the long value directly to seekTo.

Suggested change

public void seekTo(@NonNull Long position) {

exoPlayer.seekTo(position.intValue());

}

@Override

public void seekTo(@NonNull Long position) {

exoPlayer.seekTo(position);

}

gemini-code-assist · 2025-07-16T19:55:30Z

...deo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoPlayerTest.java


    when(mockExoPlayer.getCurrentPosition()).thenReturn(20L);
-    assertEquals(20L, videoPlayer.getPosition());
+    assertEquals(20L, videoPlayer.getPosition().doubleValue(), 0.001);


The getPosition() method now returns a Long. Comparing it as a double with a delta is less precise and can hide potential issues. It's better to compare the Long values directly for equality.

Suggested change

assertEquals(20L, videoPlayer.getPosition().doubleValue(), 0.001);

assertEquals(Long.valueOf(20L), videoPlayer.getPosition());

ash2moon

LGTM!

flutter/packages#9511)

flutter/packages@963550c...6b2e34e 2025-07-28 [email protected] [interactive_media_ads] Adds support to set general SDK settings (flutter/packages#9648) 2025-07-28 [email protected] Roll Flutter from 20788c0 to cfbaa18 (43 revisions) (flutter/packages#9688) 2025-07-25 [email protected] Roll Flutter (stable) from d7b523b to edada7c (7 revisions) (flutter/packages#9677) 2025-07-25 [email protected] Resolve `android_build_all_packages` Warnings (flutter/packages#9643) 2025-07-25 [email protected] [video_player] Move Android to per-player-instance Pigeon APIs (flutter/packages#9511) 2025-07-24 [email protected] [ci] Re-enable wasm and Windows ARM64 tests (flutter/packages#9667) 2025-07-24 [email protected] Roll Flutter from afba7d7 to 20788c0 (27 revisions) (flutter/packages#9670) 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

…er#9511) 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 [^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@963550c...6b2e34e 2025-07-28 [email protected] [interactive_media_ads] Adds support to set general SDK settings (flutter/packages#9648) 2025-07-28 [email protected] Roll Flutter from 20788c0 to cfbaa18 (43 revisions) (flutter/packages#9688) 2025-07-25 [email protected] Roll Flutter (stable) from d7b523b to edada7c (7 revisions) (flutter/packages#9677) 2025-07-25 [email protected] Resolve `android_build_all_packages` Warnings (flutter/packages#9643) 2025-07-25 [email protected] [video_player] Move Android to per-player-instance Pigeon APIs (flutter/packages#9511) 2025-07-24 [email protected] [ci] Re-enable wasm and Windows ARM64 tests (flutter/packages#9667) 2025-07-24 [email protected] Roll Flutter from afba7d7 to 20788c0 (27 revisions) (flutter/packages#9670) 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 4 commits June 27, 2025 10:42

Switch most methods to a per-instance API channel

71cad85

Switch remaining tests from Pigeon generated tests to mockito

e3684d1

Pigeon update

bc7452e

Version bump

18d5e4b

stuartmorgan-g requested a review from ash2moon as a code owner June 27, 2025 15:05

github-actions bot added platform-android p: video_player labels Jun 27, 2025

stuartmorgan-g commented Jun 27, 2025

View reviewed changes

Copypasta fix

a16cf7e

stuartmorgan-g commented Jun 27, 2025

View reviewed changes

stuartmorgan-g added the triage-android Should be looked at in Android triage label Jun 30, 2025

stuartmorgan-g changed the title ~~[video_player] Move to per-player-instance Pigeon APIs~~ [video_player] Move Android to per-player-instance Pigeon APIs Jun 30, 2025

stuartmorgan-g mentioned this pull request Jun 30, 2025

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

Merged

11 tasks

stuartmorgan-g added 2 commits July 8, 2025 15:28

Merge branch 'main' into video-player-android-dart-player-instance

b8e99a3

Apply iOS review changes for consistency

ec2ddc7

gemini-code-assist bot reviewed Jul 16, 2025

View reviewed changes

stuartmorgan-g added 2 commits July 16, 2025 15:58

Merge branch 'main' into video-player-android-dart-player-instance

3406ff6

Gemini review feedback

f41c1b1

ash2moon approved these changes Jul 24, 2025

View reviewed changes

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

auto-submit bot merged commit f2ff383 into flutter:main Jul 25, 2025
78 checks passed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

428496c

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to efefea612618 (4 revisions) flutter/flutter#172765

Closed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

9bd1263

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to efefea612618 (4 revisions) flutter/flutter#172777

Closed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

4e8fd13

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to bb9d65a168df (5 revisions) flutter/flutter#172778

Closed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

ccd1492

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to bb9d65a168df (5 revisions) flutter/flutter#172851

Closed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

9f22501

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to 10b3d180b345 (6 revisions) flutter/flutter#172863

Closed

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

f2ff383 [video_player] Move Android to per-player-instance Pigeon APIs (

d24a9ad

flutter/packages#9511)

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

Roll Packages from 963550c6c297 to 6b2e34edba3c (7 revisions) flutter/flutter#172869

Merged

	final String channelSuffix = Integer.toString((int) id);
	final String channelSuffix = Long.toString(id);

	assertEquals(20L, videoPlayer.getPosition().doubleValue(), 0.001);
	assertEquals(Long.valueOf(20L), videoPlayer.getPosition());

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

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

Uh oh!

Conversation

stuartmorgan-g commented Jun 27, 2025

Pre-Review Checklist

Footnotes

Uh oh!

stuartmorgan-g Jun 27, 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!

stuartmorgan-g Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jun 27, 2025

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jun 30, 2025

Uh oh!

gmackall commented Jul 15, 2025

Uh oh!

stuartmorgan-g commented Jul 16, 2025

Uh oh!

stuartmorgan-g commented Jul 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

ash2moon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stuartmorgan-g Jun 27, 2025 •

edited

Loading