-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Release VideoPlayer resources when FlutterView is gone #739
Conversation
54bf55e
to
93d3f27
Compare
final MethodChannel channel = | ||
new MethodChannel(registrar.messenger(), "flutter.io/videoPlayer"); | ||
channel.setMethodCallHandler(new VideoPlayerPlugin(registrar)); | ||
channel.setMethodCallHandler(plugin); | ||
registrar.addViewDestroyListener( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not written java lambda's much but I think this can be shortened to:
registrar.addViewDestroyListener((FlutterNativeView view) {
plugin.onDestroy();
return false;
});
or some such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it that way, but the PR checker complained that this is not compatible with Java 7 (I don't know why it cares about Java 7 ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Did you test that this does what you intend it to do?
Yes, in conjunction with flutter/engine#6079 we do see FlutterView cleanly released: it doesn't crash and verbose log records that I temporarily added are produced in an expected order. I didn't find any unittests for VideoPlayer, so I didn't add test cases. |
Please don't hesitate to add unit tests even if we don't have any yet. |
void onDestroy() { | ||
// The whole FlutterView is being destroyed. Here we release resources | ||
// acquired for all instances of VideoPlayer. | ||
// TODO(https://github.com/flutter/flutter/issues/20989) Replace with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax for TODOs is TODO(username): description, url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrased this in another way, so that I'm not on the hook, but the bug owner :).
Kirill if you can think of a quick way to write a test for it, it would be great. Otherwise, please address Ian's comment about the TODO and you are good to go. Thanks for fixing it! |
Testing the Java code against FlutterView is a bit tough right now (b/111201965), so I'll keep it as-is for now. (FWIW, there are no Java tests in this repo now). |
…terView is being destroyed. Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance. This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358). flutter/flutter#20989 tracks ensuring that resources are released.
…terView is being destroyed. (flutter#739) Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance. This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358). flutter/flutter#20989 tracks ensuring that resources are released.
…ing FlutterView is being destroyed. (flutter#739)" This reverts commit e235935.
Otherwise the exoplayer MediaPlayer objects continue generating frames after FlutterNativeView is destroyed and onFrameAvailableListener for the video target SurfaceTexture tries to access a missing engine instance.
This is a workaround for widgets not receiving dispose signals when FlutterView is destroyed (flutter/flutter#19358).
flutter/flutter#20989 tracks ensuring that resources are released.