Refactor VideoPlayer to be less exposed to EventChannel & related (flutter#6908)

matanlurey · web-flow · commit 1ec3a9bfe23d · 2024-06-13T17:41:58.000Z
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.
diff --git a/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java b/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java
@@ -28,12 +28,7 @@
 import androidx.media3.datasource.DefaultHttpDataSource;
 import androidx.media3.exoplayer.ExoPlayer;
 import androidx.media3.exoplayer.source.DefaultMediaSourceFactory;
-import io.flutter.plugin.common.EventChannel;
 import io.flutter.view.TextureRegistry;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 
 final class VideoPlayer {
@@ -48,9 +43,7 @@ final class VideoPlayer {
 
   private final TextureRegistry.SurfaceTextureEntry textureEntry;
 
-  private QueuingEventSink eventSink;
-
-  private final EventChannel eventChannel;
+  private final VideoPlayerCallbacks videoPlayerEvents;
 
   private static final String USER_AGENT = "User-Agent";
 
@@ -62,13 +55,13 @@ final class VideoPlayer {
 
   VideoPlayer(
       Context context,
-      EventChannel eventChannel,
+      VideoPlayerCallbacks events,
       TextureRegistry.SurfaceTextureEntry textureEntry,
       String dataSource,
       String formatHint,
       @NonNull Map<String, String> httpHeaders,
       VideoPlayerOptions options) {
-    this.eventChannel = eventChannel;
+    this.videoPlayerEvents = events;
     this.textureEntry = textureEntry;
     this.options = options;
 
@@ -86,24 +79,23 @@ final class VideoPlayer {
     exoPlayer.setMediaItem(mediaItem);
     exoPlayer.prepare();
 
-    setUpVideoPlayer(exoPlayer, new QueuingEventSink());
+    setUpVideoPlayer(exoPlayer);
   }
 
   // Constructor used to directly test members of this class.
   @VisibleForTesting
   VideoPlayer(
       ExoPlayer exoPlayer,
-      EventChannel eventChannel,
+      VideoPlayerCallbacks events,
       TextureRegistry.SurfaceTextureEntry textureEntry,
       VideoPlayerOptions options,
-      QueuingEventSink eventSink,
       DefaultHttpDataSource.Factory httpDataSourceFactory) {
-    this.eventChannel = eventChannel;
+    this.videoPlayerEvents = events;
     this.textureEntry = textureEntry;
     this.options = options;
     this.httpDataSourceFactory = httpDataSourceFactory;
 
-    setUpVideoPlayer(exoPlayer, eventSink);
+    setUpVideoPlayer(exoPlayer);
   }
 
   @VisibleForTesting
@@ -118,37 +110,29 @@ public void configureHttpDataSourceFactory(@NonNull Map<String, String> httpHead
         httpDataSourceFactory, httpHeaders, userAgent, httpHeadersNotEmpty);
   }
 
-  private void setUpVideoPlayer(ExoPlayer exoPlayer, QueuingEventSink eventSink) {
+  private void setUpVideoPlayer(ExoPlayer exoPlayer) {
     this.exoPlayer = exoPlayer;
-    this.eventSink = eventSink;
-
-    eventChannel.setStreamHandler(
-        new EventChannel.StreamHandler() {
-          @Override
-          public void onListen(Object o, EventChannel.EventSink sink) {
-            eventSink.setDelegate(sink);
-          }
-
-          @Override
-          public void onCancel(Object o) {
-            eventSink.setDelegate(null);
-          }
-        });
 
     surface = new Surface(textureEntry.surfaceTexture());
     exoPlayer.setVideoSurface(surface);
     setAudioAttributes(exoPlayer, options.mixWithOthers);
 
+    // Avoids synthetic accessor.
+    VideoPlayerCallbacks events = this.videoPlayerEvents;
+
     exoPlayer.addListener(
         new Listener() {
           private boolean isBuffering = false;
 
           public void setBuffering(boolean buffering) {
-            if (isBuffering != buffering) {
-              isBuffering = buffering;
-              Map<String, Object> event = new HashMap<>();
-              event.put("event", isBuffering ? "bufferingStart" : "bufferingEnd");
-              eventSink.success(event);
+            if (isBuffering == buffering) {
+              return;
+            }
+            isBuffering = buffering;
+            if (buffering) {
+              events.onBufferingStart();
+            } else {
+              events.onBufferingEnd();
             }
           }
 
@@ -163,11 +147,8 @@ public void onPlaybackStateChanged(final int playbackState) {
                 sendInitialized();
               }
             } else if (playbackState == Player.STATE_ENDED) {
-              Map<String, Object> event = new HashMap<>();
-              event.put("event", "completed");
-              eventSink.success(event);
+              events.onCompleted();
             }
-
             if (playbackState != Player.STATE_BUFFERING) {
               setBuffering(false);
             }
@@ -180,30 +161,20 @@ public void onPlayerError(@NonNull final PlaybackException error) {
               // See https://exoplayer.dev/live-streaming.html#behindlivewindowexception-and-error_code_behind_live_window
               exoPlayer.seekToDefaultPosition();
               exoPlayer.prepare();
-            } else if (eventSink != null) {
-              eventSink.error("VideoError", "Video player had error " + error, null);
+            } else {
+              events.onError("VideoError", "Video player had error " + error, null);
             }
           }
 
           @Override
           public void onIsPlayingChanged(boolean isPlaying) {
-            if (eventSink != null) {
-              Map<String, Object> event = new HashMap<>();
-              event.put("event", "isPlayingStateUpdate");
-              event.put("isPlaying", isPlaying);
-              eventSink.success(event);
-            }
+            events.onIsPlayingStateUpdate(isPlaying);
           }
         });
   }
 
   void sendBufferingUpdate() {
-    Map<String, Object> event = new HashMap<>();
-    event.put("event", "bufferingUpdate");
-    List<? extends Number> range = Arrays.asList(0, exoPlayer.getBufferedPosition());
-    // iOS supports a list of buffered ranges, so here is a list with a single range.
-    event.put("values", Collections.singletonList(range));
-    eventSink.success(event);
+    videoPlayerEvents.onBufferingUpdate(exoPlayer.getBufferedPosition());
   }
 
   private static void setAudioAttributes(ExoPlayer exoPlayer, boolean isMixMode) {
@@ -248,43 +219,36 @@ long getPosition() {
   @SuppressWarnings("SuspiciousNameCombination")
   @VisibleForTesting
   void sendInitialized() {
-    if (isInitialized) {
-      Map<String, Object> event = new HashMap<>();
-      event.put("event", "initialized");
-      event.put("duration", exoPlayer.getDuration());
-
-      VideoSize videoSize = exoPlayer.getVideoSize();
-      int width = videoSize.width;
-      int height = videoSize.height;
-      if (width != 0 && height != 0) {
-        int rotationDegrees = videoSize.unappliedRotationDegrees;
-        // Switch the width/height if video was taken in portrait mode
-        if (rotationDegrees == 90 || rotationDegrees == 270) {
-          width = videoSize.height;
-          height = videoSize.width;
-        }
-        event.put("width", width);
-        event.put("height", height);
-
-        // Rotating the video with ExoPlayer does not seem to be possible with a Surface,
-        // so inform the Flutter code that the widget needs to be rotated to prevent
-        // upside-down playback for videos with rotationDegrees of 180 (other orientations work
-        // correctly without correction).
-        if (rotationDegrees == 180) {
-          event.put("rotationCorrection", rotationDegrees);
-        }
+    if (!isInitialized) {
+      return;
+    }
+    VideoSize videoSize = exoPlayer.getVideoSize();
+    int rotationCorrection = 0;
+    int width = videoSize.width;
+    int height = videoSize.height;
+    if (width != 0 && height != 0) {
+      int rotationDegrees = videoSize.unappliedRotationDegrees;
+      // Switch the width/height if video was taken in portrait mode
+      if (rotationDegrees == 90 || rotationDegrees == 270) {
+        width = videoSize.height;
+        height = videoSize.width;
+      }
+      // Rotating the video with ExoPlayer does not seem to be possible with a Surface,
+      // so inform the Flutter code that the widget needs to be rotated to prevent
+      // upside-down playback for videos with rotationDegrees of 180 (other orientations work
+      // correctly without correction).
+      if (rotationDegrees == 180) {
+        rotationCorrection = rotationDegrees;
       }
-
-      eventSink.success(event);
     }
+    videoPlayerEvents.onInitialized(width, height, exoPlayer.getDuration(), rotationCorrection);
   }
 
   void dispose() {
     if (isInitialized) {
       exoPlayer.stop();
     }
     textureEntry.release();
-    eventChannel.setStreamHandler(null);
     if (surface != null) {
       surface.release();
     }
diff --git a/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerCallbacks.java b/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerCallbacks.java
@@ -0,0 +1,33 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package io.flutter.plugins.videoplayer;
+
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+
+/**
+ * Callbacks representing events invoked by {@link VideoPlayer}.
+ *
+ * <p>In the actual plugin, this will always be {@link VideoPlayerEventCallbacks}, which creates the
+ * expected events to send back through the plugin channel. In tests methods can be overridden in
+ * order to assert results.
+ *
+ * <p>See {@link androidx.media3.common.Player.Listener} for details.
+ */
+interface VideoPlayerCallbacks {
+  void onInitialized(int width, int height, long durationInMs, int rotationCorrectionInDegrees);
+
+  void onBufferingStart();
+
+  void onBufferingUpdate(long bufferedPosition);
+
+  void onBufferingEnd();
+
+  void onCompleted();
+
+  void onError(@NonNull String code, @Nullable String message, @Nullable Object details);
+
+  void onIsPlayingStateUpdate(boolean isPlaying);
+}
diff --git a/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerEventCallbacks.java b/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerEventCallbacks.java
@@ -0,0 +1,98 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package io.flutter.plugins.videoplayer;
+
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
+import io.flutter.plugin.common.EventChannel;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+final class VideoPlayerEventCallbacks implements VideoPlayerCallbacks {
+  private final EventChannel.EventSink eventSink;
+
+  static VideoPlayerEventCallbacks bindTo(EventChannel eventChannel) {
+    QueuingEventSink eventSink = new QueuingEventSink();
+    eventChannel.setStreamHandler(
+        new EventChannel.StreamHandler() {
+          @Override
+          public void onListen(Object arguments, EventChannel.EventSink events) {
+            eventSink.setDelegate(events);
+          }
+
+          @Override
+          public void onCancel(Object arguments) {
+            eventSink.setDelegate(null);
+          }
+        });
+    return VideoPlayerEventCallbacks.withSink(eventSink);
+  }
+
+  @VisibleForTesting
+  static VideoPlayerEventCallbacks withSink(EventChannel.EventSink eventSink) {
+    return new VideoPlayerEventCallbacks(eventSink);
+  }
+
+  private VideoPlayerEventCallbacks(EventChannel.EventSink eventSink) {
+    this.eventSink = eventSink;
+  }
+
+  @Override
+  public void onInitialized(
+      int width, int height, long durationInMs, int rotationCorrectionInDegrees) {
+    Map<String, Object> event = new HashMap<>();
+    event.put("event", "initialized");
+    event.put("width", width);
+    event.put("height", height);
+    event.put("duration", durationInMs);
+    if (rotationCorrectionInDegrees != 0) {
+      event.put("rotationCorrection", rotationCorrectionInDegrees);
+    }
+    eventSink.success(event);
+  }
+
+  @Override
+  public void onBufferingStart() {
+    Map<String, Object> event = new HashMap<>();
+    event.put("event", "bufferingStart");
+    eventSink.success(event);
+  }
+
+  @Override
+  public void onBufferingUpdate(long bufferedPosition) {
+    // iOS supports a list of buffered ranges, so we send as a list with a single range.
+    Map<String, Object> event = new HashMap<>();
+    event.put("values", Collections.singletonList(bufferedPosition));
+    eventSink.success(event);
+  }
+
+  @Override
+  public void onBufferingEnd() {
+    Map<String, Object> event = new HashMap<>();
+    event.put("event", "bufferingEnd");
+    eventSink.success(event);
+  }
+
+  @Override
+  public void onCompleted() {
+    Map<String, Object> event = new HashMap<>();
+    event.put("event", "completed");
+    eventSink.success(event);
+  }
+
+  @Override
+  public void onError(@NonNull String code, @Nullable String message, @Nullable Object details) {
+    eventSink.error(code, message, details);
+  }
+
+  @Override
+  public void onIsPlayingStateUpdate(boolean isPlaying) {
+    Map<String, Object> event = new HashMap<>();
+    event.put("isPlaying", isPlaying);
+    eventSink.success(event);
+  }
+}
diff --git a/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java b/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayerPlugin.java
@@ -125,7 +125,7 @@ public void initialize() {
       player =
           new VideoPlayer(
               flutterState.applicationContext,
-              eventChannel,
+              VideoPlayerEventCallbacks.bindTo(eventChannel),
               handle,
               "asset:///" + assetLookupKey,
               null,
@@ -136,7 +136,7 @@ public void initialize() {
       player =
           new VideoPlayer(
               flutterState.applicationContext,
-              eventChannel,
+              VideoPlayerEventCallbacks.bindTo(eventChannel),
               handle,
               arg.getUri(),
               arg.getFormatHint(),
diff --git a/packages/video_player/video_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoPlayerTest.java b/packages/video_player/video_player_android/android/src/test/java/io/flutter/plugins/videoplayer/VideoPlayerTest.java
