[video_player] Move [player dispose] to onUnregistered (flutter#2124)

Chris Yang · web-flow · commit d28c98c5e9ab · 2019-11-20T13:57:48.000-08:00
diff --git a/packages/video_player/CHANGELOG.md b/packages/video_player/CHANGELOG.md
@@ -1,3 +1,9 @@
+## 0.10.3+1
+
+* Dispose `FLTVideoPlayer` in `onTextureUnregistered` callback on iOS.
+* Add a temporary fix to dispose the `FLTVideoPlayer` with a delay to avoid race condition.
+* Updated the example app to include a new page that pop back after video is done playing.
+
 ## 0.10.3
 
 * Add support for the v2 Android embedding. This shouldn't impact existing
diff --git a/packages/video_player/example/lib/main.dart b/packages/video_player/example/lib/main.dart
@@ -8,6 +8,7 @@
 import 'package:flutter/cupertino.dart';
 import 'package:flutter/material.dart';
 import 'package:video_player/video_player.dart';
+import 'package:flutter/scheduler.dart';
 
 /// Controls play and pause of [controller].
 ///
@@ -28,7 +29,7 @@ class VideoPlayPause extends StatefulWidget {
 class _VideoPlayPauseState extends State<VideoPlayPause> {
   _VideoPlayPauseState() {
     listener = () {
-      setState(() {});
+      SchedulerBinding.instance.addPostFrameCallback((_) => setState(() {}));
     };
   }
 
@@ -48,8 +49,11 @@ class _VideoPlayPauseState extends State<VideoPlayPause> {
 
   @override
   void deactivate() {
-    controller.setVolume(0.0);
-    controller.removeListener(listener);
+    SchedulerBinding.instance.addPostFrameCallback((_) {
+      controller.setVolume(0.0);
+      controller.removeListener(listener);
+    });
+
     super.deactivate();
   }
 
@@ -360,73 +364,152 @@ class AspectRatioVideoState extends State<AspectRatioVideo> {
   }
 }
 
-void main() {
-  runApp(
-    MaterialApp(
-      home: DefaultTabController(
-        length: 3,
-        child: Scaffold(
-          appBar: AppBar(
-            title: const Text('Video player example'),
-            bottom: const TabBar(
-              isScrollable: true,
-              tabs: <Widget>[
-                Tab(
-                  icon: Icon(Icons.cloud),
-                  text: "Remote",
-                ),
-                Tab(icon: Icon(Icons.insert_drive_file), text: "Asset"),
-                Tab(icon: Icon(Icons.list), text: "List example"),
-              ],
-            ),
+class App extends StatelessWidget {
+  @override
+  Widget build(BuildContext context) {
+    return DefaultTabController(
+      length: 3,
+      child: Scaffold(
+        key: const ValueKey<String>('home_page'),
+        appBar: AppBar(
+          title: const Text('Video player example'),
+          actions: <Widget>[
+            IconButton(
+              key: const ValueKey<String>('push_tab'),
+              icon: const Icon(Icons.navigation),
+              onPressed: () {
+                Navigator.push<PlayerVideoAndPopPage>(
+                  context,
+                  MaterialPageRoute<PlayerVideoAndPopPage>(
+                      builder: (BuildContext context) =>
+                          PlayerVideoAndPopPage()),
+                );
+              },
+            )
+          ],
+          bottom: const TabBar(
+            isScrollable: true,
+            tabs: <Widget>[
+              Tab(
+                icon: Icon(Icons.cloud),
+                text: "Remote",
+              ),
+              Tab(icon: Icon(Icons.insert_drive_file), text: "Asset"),
+              Tab(icon: Icon(Icons.list), text: "List example"),
+            ],
           ),
-          body: TabBarView(
-            children: <Widget>[
-              SingleChildScrollView(
-                child: Column(
-                  children: <Widget>[
-                    Container(
-                      padding: const EdgeInsets.only(top: 20.0),
+        ),
+        body: TabBarView(
+          children: <Widget>[
+            SingleChildScrollView(
+              child: Column(
+                children: <Widget>[
+                  Container(
+                    padding: const EdgeInsets.only(top: 20.0),
+                  ),
+                  const Text('With remote mp4'),
+                  Container(
+                    padding: const EdgeInsets.all(20),
+                    child: NetworkPlayerLifeCycle(
+                      'https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4',
+                      (BuildContext context,
+                              VideoPlayerController controller) =>
+                          AspectRatioVideo(controller),
                     ),
-                    const Text('With remote mp4'),
-                    Container(
-                      padding: const EdgeInsets.all(20),
-                      child: NetworkPlayerLifeCycle(
-                        'https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4',
+                  ),
+                ],
+              ),
+            ),
+            SingleChildScrollView(
+              child: Column(
+                children: <Widget>[
+                  Container(
+                    padding: const EdgeInsets.only(top: 20.0),
+                  ),
+                  const Text('With assets mp4'),
+                  Container(
+                    padding: const EdgeInsets.all(20),
+                    child: AssetPlayerLifeCycle(
+                        'assets/Butterfly-209.mp4',
                         (BuildContext context,
                                 VideoPlayerController controller) =>
-                            AspectRatioVideo(controller),
-                      ),
-                    ),
-                  ],
-                ),
+                            AspectRatioVideo(controller)),
+                  ),
+                ],
               ),
-              SingleChildScrollView(
-                child: Column(
-                  children: <Widget>[
-                    Container(
-                      padding: const EdgeInsets.only(top: 20.0),
-                    ),
-                    const Text('With assets mp4'),
-                    Container(
-                      padding: const EdgeInsets.all(20),
-                      child: AssetPlayerLifeCycle(
-                          'assets/Butterfly-209.mp4',
-                          (BuildContext context,
-                                  VideoPlayerController controller) =>
-                              AspectRatioVideo(controller)),
-                    ),
-                  ],
-                ),
-              ),
-              AssetPlayerLifeCycle(
-                  'assets/Butterfly-209.mp4',
-                  (BuildContext context, VideoPlayerController controller) =>
-                      VideoInListOfCards(controller)),
-            ],
-          ),
+            ),
+            AssetPlayerLifeCycle(
+                'assets/Butterfly-209.mp4',
+                (BuildContext context, VideoPlayerController controller) =>
+                    VideoInListOfCards(controller)),
+          ],
         ),
       ),
+    );
+  }
+}
+
+void main() {
+  runApp(
+    MaterialApp(
+      home: App(),
     ),
   );
 }
+
+class PlayerVideoAndPopPage extends StatefulWidget {
+  @override
+  _PlayerVideoAndPopPageState createState() => _PlayerVideoAndPopPageState();
+}
+
+class _PlayerVideoAndPopPageState extends State<PlayerVideoAndPopPage> {
+  VideoPlayerController _videoPlayerController;
+  bool startedPlaying = false;
+
+  @override
+  void initState() {
+    super.initState();
+
+    _videoPlayerController =
+        VideoPlayerController.asset('assets/Butterfly-209.mp4');
+    _videoPlayerController.addListener(() {
+      if (startedPlaying && !_videoPlayerController.value.isPlaying) {
+        Navigator.pop(context);
+      }
+    });
+  }
+
+  @override
+  void dispose() {
+    _videoPlayerController.dispose();
+    super.dispose();
+  }
+
+  Future<bool> started() async {
+    await _videoPlayerController.initialize();
+    await _videoPlayerController.play();
+    startedPlaying = true;
+    return true;
+  }
+
+  @override
+  Widget build(BuildContext context) {
+    return Material(
+      elevation: 0,
+      child: Center(
+        child: FutureBuilder<bool>(
+          future: started(),
+          builder: (BuildContext context, AsyncSnapshot<bool> snapshot) {
+            if (snapshot.data == true) {
+              return AspectRatio(
+                  aspectRatio: _videoPlayerController.value.aspectRatio,
+                  child: VideoPlayer(_videoPlayerController));
+            } else {
+              return const Text('waiting for video to load');
+            }
+          },
+        ),
+      ),
+    );
+  }
+}
diff --git a/packages/video_player/example/pubspec.yaml b/packages/video_player/example/pubspec.yaml
@@ -4,15 +4,16 @@ description: Demonstrates how to use the video_player plugin.
 dependencies:
   flutter:
     sdk: flutter
+  video_player:
+    path: ../
 
 dev_dependencies:
   flutter_test:
     sdk: flutter
   flutter_driver:
     sdk: flutter
   e2e: "^0.2.0"
-  video_player:
-    path: ../
+  test: any
 
 flutter:
   uses-material-design: true
diff --git a/packages/video_player/example/test_driver/video_player.dart b/packages/video_player/example/test_driver/video_player.dart
@@ -0,0 +1,11 @@
+// Copyright 2019, the Chromium project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:flutter_driver/driver_extension.dart';
+import 'package:video_player_example/main.dart' as app;
+
+void main() {
+  enableFlutterDriverExtension();
+  app.main();
+}
diff --git a/packages/video_player/example/test_driver/video_player_e2e.dart b/packages/video_player/example/test_driver/video_player_e2e.dart
@@ -12,7 +12,6 @@ const Duration _playDuration = Duration(seconds: 1);
 void main() {
   E2EWidgetsFlutterBinding.ensureInitialized();
   VideoPlayerController _controller;
-
   tearDown(() async => _controller.dispose());
 
   group('asset videos', () {
diff --git a/packages/video_player/example/test_driver/video_player_e2e_test.dart b/packages/video_player/example/test_driver/video_player_e2e_test.dart
@@ -4,7 +4,6 @@
 
 import 'dart:async';
 import 'dart:io';
-
 import 'package:flutter_driver/flutter_driver.dart';
 
 Future<void> main() async {
diff --git a/packages/video_player/example/test_driver/video_player_test.dart b/packages/video_player/example/test_driver/video_player_test.dart
@@ -0,0 +1,27 @@
+// Copyright 2019, the Chromium project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:async';
+import 'package:flutter_driver/flutter_driver.dart';
+import 'package:test/test.dart';
+
+Future<void> main() async {
+  final FlutterDriver driver = await FlutterDriver.connect();
+  tearDownAll(() async {
+    driver.close();
+  });
+
+  //TODO(cyanglaz): Use TabBar tabs to navigate between pages after https://github.com/flutter/flutter/issues/16991 is fixed.
+  //TODO(cyanglaz): Un-skip the test after https://github.com/flutter/flutter/issues/43012 is fixed
+  test('Push a page contains video and pop back, do not crash.', () async {
+    final SerializableFinder pushTab = find.byValueKey('push_tab');
+    await driver.waitFor(pushTab);
+    await driver.tap(pushTab);
+    await driver.waitForAbsent(pushTab);
+    await driver.waitFor(find.byValueKey('home_page'));
+    await driver.waitUntilNoTransientCallbacks();
+    final Health health = await driver.checkHealth();
+    expect(health.status, HealthStatus.ok);
+  }, skip: 'Cirrus CI currently hangs while playing videos');
+}
diff --git a/packages/video_player/ios/Classes/VideoPlayerPlugin.m b/packages/video_player/ios/Classes/VideoPlayerPlugin.m
@@ -363,6 +363,12 @@ - (CVPixelBufferRef)copyPixelBuffer {
   }
 }
 
+- (void)onTextureUnregistered {
+  dispatch_async(dispatch_get_main_queue(), ^{
+    [self dispose];
+  });
+}
+
 - (FlutterError* _Nullable)onCancelWithArguments:(id _Nullable)arguments {
   _eventSink = nil;
   return nil;
@@ -487,7 +493,22 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
     if ([@"dispose" isEqualToString:call.method]) {
       [_registry unregisterTexture:textureId];
       [_players removeObjectForKey:@(textureId)];
-      [player dispose];
+      // If the Flutter contains https://github.com/flutter/engine/pull/12695,
+      // the `player` is disposed via `onTextureUnregistered` at the right time.
+      // Without https://github.com/flutter/engine/pull/12695, there is no guarantee that the
+      // texture has completed the un-reregistration. It may leads a crash if we dispose the
+      // `player` before the texture is unregistered. We add a dispatch_after hack to make sure the
+      // texture is unregistered before we dispose the `player`.
+      //
+      // TODO(cyanglaz): Remove this dispatch block when
+      // https://github.com/flutter/flutter/commit/8159a9906095efc9af8b223f5e232cb63542ad0b is in
+      // stable And update the min flutter version of the plugin to the stable version.
+      dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)),
+                     dispatch_get_main_queue(), ^{
+                       if (!player.disposed) {
+                         [player dispose];
+                       }
+                     });
       result(nil);
     } else if ([@"setLooping" isEqualToString:call.method]) {
       [player setIsLooping:[argsMap[@"looping"] boolValue]];
diff --git a/packages/video_player/pubspec.yaml b/packages/video_player/pubspec.yaml
@@ -2,7 +2,7 @@ name: video_player
 description: Flutter plugin for displaying inline video with other Flutter
   widgets on Android and iOS.
 author: Flutter Team <flutter-dev@googlegroups.com>
-version: 0.10.3
+version: 0.10.3+1
 homepage: https://github.com/flutter/plugins/tree/master/packages/video_player
 
 flutter:
