Refactor `VideoPlayer` to be less exposed to `EventChannel` & related #6908

matanlurey · 2024-06-11T20:09:34Z

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.

These are some proposed refactors to limit how much work VideoPlayer is doing, so I can better understand what needs to be reset (or not) internally. Specifically, VideoPlayer no longer knows what an EventChannel or EventSink is, and does not need to manage the lifecycle (it stores a private final VideoPlayerCallbacks instead), and instead there is a VideoPlayerCallbacks interface that does all that.

I'm totally open to:

Landing this as-is (+/- nits) and making minor improvements in follow-up PRs
Making more significant changes to this PR and then landing it
Not landing this PR at all because it doesn't follow the approach the folks who maintain the plugin prefer

Also happy to chat in VC/person about any of the changes.

gaaclarke

lgtm modulo some nitpicks

gaaclarke · 2024-06-12T21:24:34Z

...layer_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerCallbacks.java

+ * order to assert results.
+ */
+interface VideoPlayerCallbacks {
+  void onInitialized(


nit: rotationCorrectionInDegrees = 0 and rotationCorrectionInDegrees = null have the same semantic meaning, right? If so, no need to make this nullable.

Good point! Done.

gaaclarke · 2024-06-12T21:27:01Z

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

+    return VideoPlayerEventCallbacks.withSink(eventSink);
+  }
+
+  static VideoPlayerEventCallbacks withSink(EventChannel.EventSink eventSink) {


@VisibleForTesting?

gaaclarke · 2024-06-12T21:28:45Z

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

+
+  @Override
+  public void onBufferingEnd() {
+    Map<String, Object> event = new HashMap<>();


nit: All of these events are allocating new HashMap with the same value. They could be private constants to avoid generating garbage. None of them seem that chatty though.

I'm going to leave it as-is for now, hopefully we can use Map.of soonish?

camsim99

LGTM! Just left some suggestions :)

camsim99 · 2024-06-13T15:58:43Z

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

-            } else if (eventSink != null) {
-              eventSink.error("VideoError", "Video player had error " + error, null);
+            } else {
+              events.onError("VideoPlayer", "Video player had error " + error, null);


Are you switching this because the VideoPlayer tag used elsewhere? Looks like VideoError is at least also used on iOS so we may want to stay consistent for clarity: https://github.com/search?q=repo%3Aflutter%2Fpackages+VideoError&type=code.

Oh, nope, that was an actual mistake - I'll also add a test before I resolve this comment.

Done and test added!

camsim99 · 2024-06-13T15:59:29Z

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

-  private QueuingEventSink eventSink;
-
-  private final EventChannel eventChannel;
+  private final VideoPlayerCallbacks events;


Extreme nit: can this be something like videoPlayerEvents or something to indicate the type of events?

camsim99 · 2024-06-13T16:01:36Z

...layer_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerCallbacks.java

+ * expected events to send back through the plugin channel. In tests methods can be overridden in
+ * order to assert results.
+ */
+interface VideoPlayerCallbacks {


Maybe link the Player.Listener docs to the callbacks that directly map to one of those events?

auto-submit · 2024-06-13T17:06:23Z

auto label is removed for flutter/packages/6908, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

…`. (#6922) Similar to #6908, as part of flutter/flutter#148417. 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. In this PR, I've removed the concept of the class being "initialized" or not - the only thing "initialized" means is "for a given instance of `ExoPlayer`, has received the `'initialized'` event. As a result I removed the quasi-public API that was used for testing only and replaced it with observing what the real production instance does (`Player.STATE_READY`). After this PR, I'll likely do one more pass around the constructors - the constructor that takes an `ExoPlayer` that is marked `@VisibleForTesting` _also_ doesn't make sense once we'll support suspending/resuming video players, so it will need to get reworked (probably into taking a factory method).

…flutter#6908) Part of flutter/flutter#148417. 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. These are some proposed refactors to limit how much work `VideoPlayer` is doing, so I can better understand what needs to be reset (or not) internally. Specifically, `VideoPlayer` no longer knows what an `EventChannel` or `EventSink` is, and does not need to manage the lifecycle (it stores a `private final VideoPlayerCallbacks` instead), and instead there is a `VideoPlayerCallbacks` interface that does all that. I'm totally open to: - Landing this as-is (+/- nits) and making minor improvements in follow-up PRs - Making more significant changes to this PR and then landing it - Not landing this PR at all because it doesn't follow the approach the folks who maintain the plugin prefer Also happy to chat in VC/person about any of the changes.

…`. (flutter#6922) Similar to flutter#6908, as part of flutter/flutter#148417. 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. In this PR, I've removed the concept of the class being "initialized" or not - the only thing "initialized" means is "for a given instance of `ExoPlayer`, has received the `'initialized'` event. As a result I removed the quasi-public API that was used for testing only and replaced it with observing what the real production instance does (`Player.STATE_READY`). After this PR, I'll likely do one more pass around the constructors - the constructor that takes an `ExoPlayer` that is marked `@VisibleForTesting` _also_ doesn't make sense once we'll support suspending/resuming video players, so it will need to get reworked (probably into taking a factory method).

… related (flutter/packages#6908)

flutter/packages@7805455...dd04ab1 2024-06-13 matanlurey@users.noreply.github.com Move `Player.Listener` impl, remove `@VisibleForTesting isInitialized`. (flutter/packages#6922) 2024-06-13 engine-flutter-autoroll@skia.org Roll Flutter from b1f9d71 to 01db23b (15 revisions) (flutter/packages#6921) 2024-06-13 matanlurey@users.noreply.github.com Refactor `VideoPlayer` to be less exposed to `EventChannel` & related (flutter/packages#6908) 2024-06-13 v.ditsyak@gmail.com [go_router] Added proper `redirect` handling for `ShellRoute.$route` and `StatefulShellRoute.$route` for proper redirection handling in case of code generation (flutter/packages#6841) 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

…allback`. (#6982) 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. - #6922 - #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.

…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.

Refactor VideoPlayer to be less reliant on channels/sinks deep within.

2627784

matanlurey requested a review from jonahwilliams June 11, 2024 20:09

matanlurey requested a review from camsim99 as a code owner June 11, 2024 20:09

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

matanlurey added 2 commits June 11, 2024 13:18

Format.

ee4f836

++

0334e53

matanlurey requested review from gaaclarke and removed request for jonahwilliams June 12, 2024 21:08

matanlurey added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Jun 12, 2024

++

4303968

gaaclarke approved these changes Jun 12, 2024

View reviewed changes

++

e27eace

camsim99 approved these changes Jun 13, 2024

View reviewed changes

Address feedback.

f1f7581

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

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2024

++

d97a4ad

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

auto-submit bot merged commit 1ad8d89 into main Jun 13, 2024
74 checks passed

auto-submit bot deleted the refactor-video-player-events branch June 13, 2024 17:41

matanlurey mentioned this pull request Jun 13, 2024

Move Player.Listener impl, remove @VisibleForTesting isInitialized. #6922

Merged

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

Roll Packages from 7805455bf627 to dd04ab1c75cd (4 revisions) flutter/flutter#150264

Merged

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

1ad8d89 Refactor VideoPlayer to be less exposed to EventChannel &…

f6e70f7

… related (flutter/packages#6908)

matanlurey mentioned this pull request Jun 25, 2024

Final refactor of video_player_android before SurfaceProducer#setCallback. #6982

Merged

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

Refactor `VideoPlayer` to be less exposed to `EventChannel` & related #6908

Refactor `VideoPlayer` to be less exposed to `EventChannel` & related #6908

matanlurey commented Jun 11, 2024

gaaclarke left a comment

gaaclarke Jun 12, 2024

matanlurey Jun 12, 2024

gaaclarke Jun 12, 2024

matanlurey Jun 12, 2024

gaaclarke Jun 12, 2024

matanlurey Jun 12, 2024

camsim99 left a comment

camsim99 Jun 13, 2024

matanlurey Jun 13, 2024

matanlurey Jun 13, 2024

camsim99 Jun 13, 2024

matanlurey Jun 13, 2024

camsim99 Jun 13, 2024

matanlurey Jun 13, 2024

auto-submit bot commented Jun 13, 2024

Refactor VideoPlayer to be less exposed to EventChannel & related #6908

Refactor VideoPlayer to be less exposed to EventChannel & related #6908

Conversation

matanlurey commented Jun 11, 2024

gaaclarke 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

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

auto-submit bot commented Jun 13, 2024

Refactor `VideoPlayer` to be less exposed to `EventChannel` & related #6908

Refactor `VideoPlayer` to be less exposed to `EventChannel` & related #6908