Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. #6982

matanlurey · 2024-06-25T04:42:00Z

I'm working on re-landing #6456, this time without using the ActivityAware interface (see flutter/flutter#148417). As part of that work, I'll need to better control the ExoPlayer lifecycle and save/restore internal state.

Follows the patterns of some of the previous PRs, i.e.

The changes in this PR are mostly tests, it was extremely difficult to just add more tests to the already very leaky VideoPlayer abstraction which had lots of @VisibleForTesting methods and other "holes" to observe state. This PR removes all of that, and adds test coverage where it was missing.

Namely it:

Adds a new class, VideoAsset, that builds and configures the media that ExoPlayer uses.
Removes all "testing" state from VidePlayer, keeping it nearly immutable.
Added tests for most of the classes I've added since, which were mostly missing.

That being said, this is a large change. I'm happy to sit down with either of you and walk through it.

Opening as a draft for the moment, since there is a pubspec change needing I want to handle first.

…further-refactor

camsim99

Looks good overall! Left some questions and a nit.

camsim99 · 2024-06-25T17:44:01Z

...er/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoAsset.java

+  abstract MediaSource.Factory getMediaSourceFactory(Context context);
+
+  /** Streaming formats that can be provided to the video player as a hint. */
+  enum StreamingFormat {


Nit: I think the enum values generally are all caps in Java

Thank you! Done.

camsim99 · 2024-06-25T17:51:51Z

..._android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerEventCallbacks.java

@@ -97,6 +97,7 @@ public void onError(@NonNull String code, @Nullable String message, @Nullable Ob
  @Override
  public void onIsPlayingStateUpdate(boolean isPlaying) {
    Map<String, Object> event = new HashMap<>();
+    event.put("event", "isPlayingStateUpdate");


Why do we need this event on top of isPlaying?

This was actually a regression in a previous refactor, we just haven't made another release since they have been internal refactors only. In other words, all events contain an event key-value pair and I made a mistake!

camsim99 · 2024-06-25T18:00:03Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+
+  @Test
+  public void remoteVideoByDefaultSetsUserAgentAndCrossProtocolRedirects() {
+    RemoteVideoAsset asset =


Should the tests constructing RemoteVideoAssets be using VideoAsset.fromRemoteUrl?

Good question. It's intentionally using this constructor so that we have access to the @VisibleForTesting method, but I think it could be made as or more clear by casting. Let me make that change and add a comment.

(Great suggestion btw, consistency is king)

camsim99 · 2024-06-25T18:01:52Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+
+    assert mediaItem.localConfiguration != null;
+    assertEquals(mediaItem.localConfiguration.uri, Uri.parse("asset:///asset-key"));
+  }


Do you think it's helpful to test that the type of asset. getMediaSourceFactory is what we expect?

Because of the wrapping, I don't think it is possible.

camsim99 · 2024-06-25T18:02:55Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+            "https://flutter.dev/video.mp4", VideoAsset.StreamingFormat.Unknown, headers);
+
+    DefaultHttpDataSource.Factory mockFactory = mockHttpFactory();
+    asset.getMediaSourceFactory(ApplicationProvider.getApplicationContext(), mockFactory);


Do you think it'd be helpful to test the overridden behavior of getMediaSourceFactory for RemoteVideoAssets?

So in production, there is a single method, getMediaSourceFactory(Context), which always uses a new DefaultHttpDataSource.Factory (the same behavior before this patch).

For testing, we added a way to not call new DefaultHttpDataSource.Factory, as we'd otherwise we'd not be able to assert that the HTTP factory is configured as expected.

I suspect I could assert that the factory makes a real HTTP connection by default? I will leave a comment for the next person, and otherwise at least assert the returned object is real.

Ah I see makes sense!

camsim99 · 2024-06-25T18:05:59Z

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

+ *
+ * <p>It's hypothetically possible to write better tests using {@link
+ * androidx.media3.test.utils.FakeMediaSource}, but you really need a PhD in the Android media APIs
+ * in order to figure out how to set everything up so the player "works".


Your disclaimers are the best lol!

matanlurey

Thanks for the thoughtful review!

matanlurey · 2024-06-25T18:26:11Z

...er/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoAsset.java

+  abstract MediaSource.Factory getMediaSourceFactory(Context context);
+
+  /** Streaming formats that can be provided to the video player as a hint. */
+  enum StreamingFormat {


Thank you! Done.

matanlurey · 2024-06-25T18:27:48Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+
+    assert mediaItem.localConfiguration != null;
+    assertEquals(mediaItem.localConfiguration.uri, Uri.parse("asset:///asset-key"));
+  }


Because of the wrapping, I don't think it is possible.

matanlurey · 2024-06-25T18:29:54Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+
+  @Test
+  public void remoteVideoByDefaultSetsUserAgentAndCrossProtocolRedirects() {
+    RemoteVideoAsset asset =


Good question. It's intentionally using this constructor so that we have access to the @VisibleForTesting method, but I think it could be made as or more clear by casting. Let me make that change and add a comment.

(Great suggestion btw, consistency is king)

matanlurey · 2024-06-25T18:37:22Z

...ideo_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoAssetTest.java

+            "https://flutter.dev/video.mp4", VideoAsset.StreamingFormat.Unknown, headers);
+
+    DefaultHttpDataSource.Factory mockFactory = mockHttpFactory();
+    asset.getMediaSourceFactory(ApplicationProvider.getApplicationContext(), mockFactory);


So in production, there is a single method, getMediaSourceFactory(Context), which always uses a new DefaultHttpDataSource.Factory (the same behavior before this patch).

For testing, we added a way to not call new DefaultHttpDataSource.Factory, as we'd otherwise we'd not be able to assert that the HTTP factory is configured as expected.

I suspect I could assert that the factory makes a real HTTP connection by default? I will leave a comment for the next person, and otherwise at least assert the returned object is real.

gmackall

LGTM

gmackall · 2024-06-25T20:42:13Z

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

-
-    unstableUpdateDataSourceFactory(
-        httpDataSourceFactory, httpHeaders, userAgent, httpHeadersNotEmpty);
+    setUpVideoPlayer(exoPlayer);


not worth blocking anything but if you agree and want to change:

Seems a bit odd to me to do half the setup of exoPlayer out of setUpVideoPlayer and half of it in.

Agreed. This will change (for the better) in the next PR.

…allback`. (flutter#6982) I'm working on re-landing flutter#6456, this time without using the `ActivityAware` interface (see flutter/flutter#148417). As part of that work, I'll need to better control the `ExoPlayer` lifecycle and save/restore internal state. Follows the patterns of some of the previous PRs, i.e. - flutter#6922 - flutter#6908 The changes in this PR are _mostly_ tests, it was extremely difficult to just add more tests to the already very leaky `VideoPlayer` abstraction which had lots of `@VisibleForTesting` methods and other "holes" to observe state. This PR removes all of that, and adds test coverage where it was missing. Namely it: - Adds a new class, `VideoAsset`, that builds and configures the media that `ExoPlayer` uses. - Removes all "testing" state from `VidePlayer`, keeping it nearly immutable. - Added tests for most of the classes I've added since, which were mostly missing. That being said, this is a large change. I'm happy to sit down with either of you and walk through it. --- Opening as a draft for the moment, since there is a pubspec change needing I want to handle first.

…cer#setCallback`. (flutter/packages#6982)

flutter/packages@03f5f6d...412ec46 2024-06-28 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.6 to 3.25.10 (flutter/packages#6924) 2024-06-27 ditman@gmail.com [video_player] Exposes VideoPlayerWebOptions. (flutter/packages#6990) 2024-06-27 ditman@gmail.com [ci] Add Wasm compilation to all_packages web app. (flutter/packages#6981) 2024-06-27 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Updates `README` with a usage section and fix some interface docs (flutter/packages#6988) 2024-06-27 stuartmorgan@google.com [tools] Fix vm test requirement (flutter/packages#6995) 2024-06-27 jacksongardner@google.com Update `web_benchmarks` package to properly support wasm. (flutter/packages#6970) 2024-06-27 stuartmorgan@google.com [pigeon] Implement equals for Java data classes (flutter/packages#6992) 2024-06-25 matanlurey@users.noreply.github.com Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. (flutter/packages#6982) 2024-06-25 stuartmorgan@google.com [pigeon] Update testing and docs (flutter/packages#6984) 2024-06-25 parlough@gmail.com [various] Update flutter.dev links to more reliable destinations (flutter/packages#6963) 2024-06-25 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds initial Android implementation (flutter/packages#6733) 2024-06-25 engine-flutter-autoroll@skia.org Roll Flutter from 6c06abb to e726eb4 (51 revisions) (flutter/packages#6987) 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,rmistry@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

matanlurey added 4 commits June 24, 2024 21:27

Further refactor and add test coverage for video_player_android.

fea9087

Merge remote-tracking branch 'origin/main' into video-player-android-…

eb5be68

…further-refactor

++ for draft review.

892115e

Javafmt.

deb7d4b

matanlurey requested a review from gmackall June 25, 2024 04:42

matanlurey requested a review from camsim99 as a code owner June 25, 2024 04:42

github-actions bot added platform-android p: video_player labels Jun 25, 2024

Address android_lint.

e0f663c

matanlurey added the override: no versioning needed Override the check requiring version bumps for most changes label Jun 25, 2024

Add back missing gradle dependency thingy.

796fedc

camsim99 approved these changes Jun 25, 2024

View reviewed changes

Address feedback.

0516633

matanlurey commented Jun 25, 2024

View reviewed changes

matanlurey added override: no changelog needed Override the check requiring CHANGELOG updates for most changes autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jun 25, 2024

Revert pubspec override.

9bd2beb

gmackall reviewed Jun 25, 2024

View reviewed changes

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 25, 2024

matanlurey merged commit 1612774 into main Jun 25, 2024
74 checks passed

matanlurey deleted the video-player-android-further-refactor branch June 25, 2024 21:59

engine-flutter-autoroll mentioned this pull request Jun 26, 2024

Roll Packages from 03f5f6d5660c to 161277461ae3 (5 revisions) flutter/flutter#150856

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 26, 2024

1612774 Final refactor of video_player_android before `SurfaceProdu…

498cdb7

…cer#setCallback`. (flutter/packages#6982)

engine-flutter-autoroll mentioned this pull request Jun 26, 2024

Roll Packages from 03f5f6d5660c to 161277461ae3 (5 revisions) flutter/flutter#150863

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 26, 2024

1612774 Final refactor of video_player_android before `SurfaceProdu…

cbfd51e

…cer#setCallback`. (flutter/packages#6982)

engine-flutter-autoroll mentioned this pull request Jun 27, 2024

Roll Packages from 03f5f6d5660c to 69e7fc159064 (8 revisions) flutter/flutter#150927

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024

1612774 Final refactor of video_player_android before `SurfaceProdu…

6ebf342

…cer#setCallback`. (flutter/packages#6982)

engine-flutter-autoroll mentioned this pull request Jun 27, 2024

Roll Packages from 03f5f6d5660c to 62e1963cf7ec (9 revisions) flutter/flutter#150934

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024

1612774 Final refactor of video_player_android before `SurfaceProdu…

b658b2c

…cer#setCallback`. (flutter/packages#6982)

engine-flutter-autoroll mentioned this pull request Jun 27, 2024

Roll Packages from 03f5f6d5660c to 62e1963cf7ec (9 revisions) flutter/flutter#150949

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 27, 2024

1612774 Final refactor of video_player_android before `SurfaceProdu…

e19d019

…cer#setCallback`. (flutter/packages#6982)

engine-flutter-autoroll mentioned this pull request Jun 28, 2024

Roll Packages from 03f5f6d5660c to 412ec4615aa4 (12 revisions) flutter/flutter#150985

Merged

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

1612774 Final refactor of video_player_android before `SurfaceProdu…

cfa82c3

…cer#setCallback`. (flutter/packages#6982)

reidbaker mentioned this pull request Jul 2, 2024

i151188 update espresso dependencies v2 reidbaker/packages#6

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. #6982

Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. #6982

matanlurey commented Jun 25, 2024 •

edited

Loading

camsim99 left a comment

camsim99 Jun 25, 2024

matanlurey Jun 25, 2024

camsim99 Jun 25, 2024

matanlurey Jun 25, 2024

camsim99 Jun 25, 2024

matanlurey Jun 25, 2024

camsim99 Jun 25, 2024

matanlurey Jun 25, 2024

camsim99 Jun 25, 2024

matanlurey Jun 25, 2024

camsim99 Jun 25, 2024

camsim99 Jun 25, 2024

matanlurey left a comment

matanlurey Jun 25, 2024

matanlurey Jun 25, 2024

matanlurey Jun 25, 2024

matanlurey Jun 25, 2024

gmackall left a comment

gmackall Jun 25, 2024

matanlurey Jun 25, 2024

Final refactor of video_player_android before SurfaceProducer#setCallback. #6982

Final refactor of video_player_android before SurfaceProducer#setCallback. #6982

Conversation

matanlurey commented Jun 25, 2024 • edited Loading

camsim99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmackall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. #6982

Final refactor of `video_player_android` before `SurfaceProducer#setCallback`. #6982

matanlurey commented Jun 25, 2024 •

edited

Loading