Prevent long flash when switching to Flutter app. (#47903) #16527
Conversation
mklim
left a comment
mklim
left a comment
There was a problem hiding this comment.
LGTM.
This seems like the type of thing that should both theoretically be unit testable, but in practice be very difficult to test because none of our engine classes use DI and a lot of these classes try to load C libraries at construction time. I haven't dug into it deeply but at a glance I can see issues with the internal construction of FlutterView and that blocking testing any of this. Maybe we should file some kind of bug to refactor these classes out to be more testable since it's a problem we keep hitting?
| * | ||
| * <p>This method creates a new {@link FlutterView}, adds a {@link FlutterUiDisplayListener} to | ||
| * it, and then returns it. | ||
| * <p> |
There was a problem hiding this comment.
nit: I expect the formatter is going to complain about this, ./ci/format.sh | patch -p0 should work to fix it. If you install google-java-format in your IDE of choice you should be able to get the changes applied within IntelliJ too. https://github.com/google/google-java-format#intellij-android-studio-and-other-jetbrains-ides
There was a problem hiding this comment.
Thanks. Formatter installed and run.
| flutterSplashView.displayFlutterViewWithSplash(flutterView, host.provideSplashScreen()); | ||
|
|
||
| Log.v(TAG, "Attaching FlutterEngine to FlutterView."); | ||
| flutterView.attachToFlutterEngine(flutterEngine); |
There was a problem hiding this comment.
The main potential negative implication of this I can think of is that this is going to be blocking rendering from the Android side. So if for some reason attachToFlutterEngine takes a long time the user is going to see a black screen and Android may want to kill the Flutter app.
As I understand it this is a very cheap method and shouldn't block for any noticeable amount of time though, so I'm not worried about it.
There was a problem hiding this comment.
Yeah, that was the tradeoff made here. We'll accept whatever this blocking call is in exchange for not having the very noticeable black screen at each app switch. I think this blocking call was the original impetus to post it on the main handler.
| Log.v(TAG, "onDestroyView()"); | ||
| ensureAlive(); | ||
|
|
||
| flutterView.detachFromFlutterEngine(); |
There was a problem hiding this comment.
The big potential drawback of the move to here is that this method isn't always actually called before the activity is killed, so there's a chance that FlutterView is never going to see this call in some cases now.
I'm not sure how much of a concern this is because I'm not sure how often this happens on Android, just that it does happen and it's not recommended to actually do critical work on onDestroy for that reason. As I understand it this typically happens if Android is about to kill the app for going OOM. The typical "fix" for this is to move actual critical resource releasing into onPause instead. I don't think that's really necessary here, but it may be something to revisit if we start seeing a lot of bugs around plugins not getting detached callbacks.
There was a problem hiding this comment.
Agreed. The reason this was initially done elsewhere is because of the non-guarantee that you mention. But my hope/expectation is that if onDestroy() is skipped it's because the app is being evicted, at which point nothing will survive.
* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558)
* f49a8b6 Roll src/third_party/skia c03e6982f96f..465864cad5d2 (14 commits) (flutter/engine#16524) * c477c06 Enable verbose logging for shell unittests on Fuchsia (flutter/engine#16526) * a662579 Clear frame references at the end of every CanvasKit frame (flutter/engine#16525) * 3f31ea3 Roll src/third_party/skia 465864cad5d2..21f382c19d76 (6 commits) (flutter/engine#16528) * 38fb6b1 Roll fuchsia/sdk/core/linux-amd64 from 8L7NY... to Bmq1m... (flutter/engine#16529) * 9c0168a Roll fuchsia/sdk/core/mac-amd64 from PMcw3... to 7JkB7... (flutter/engine#16530) * e8a888d Roll src/third_party/skia 21f382c19d76..f83d0346c06a (2 commits) (flutter/engine#16532) * 1e8b331 Roll src/third_party/dart 5244d99a5d4e..5fc031ebc1d7 (42 commits) (flutter/engine#16533) * c4e3ae6 Roll src/third_party/skia f83d0346c06a..88c3793a4eaa (1 commits) (flutter/engine#16534) * 6cdb14e Roll src/third_party/skia 88c3793a4eaa..abefc9c170c9 (1 commits) (flutter/engine#16535) * 975acd8 Roll src/third_party/skia abefc9c170c9..4fe89b4d871d (2 commits) (flutter/engine#16536) * b7424d0 Roll src/third_party/dart 5fc031ebc1d7..30151a654151 (2 commits) (flutter/engine#16537) * 25e8127 Roll src/third_party/skia 4fe89b4d871d..dc2782c380f6 (1 commits) (flutter/engine#16538) * 74fa10c Roll src/third_party/dart 30151a654151..76b18c455e2c (1 commits) (flutter/engine#16539) * 91b8e40 Roll src/third_party/skia dc2782c380f6..cdf2491afa04 (1 commits) (flutter/engine#16540) * 5acf9b1 Roll src/third_party/skia cdf2491afa04..50a490a1a4fb (2 commits) (flutter/engine#16541) * 9897777 Roll src/third_party/skia 50a490a1a4fb..c3b67eb988c8 (4 commits) (flutter/engine#16542) * 78a8909 Use os_log instead of syslog on Apple platforms (flutter/engine#13487) * ea56ad2 libtxt: use a fixture in the benchmarks (flutter/engine#16531) * a61dbf2 Revert "Use os_log instead of syslog on Apple platforms (#13487)" (flutter/engine#16546) * 539f64f [fuchsia] Disable retained layers (flutter/engine#16548) * c3b5072 Expose DPI helper functions for Runner apps to use (flutter/engine#16313) * 5041ff1 support endless trace buffer (flutter/engine#16520) * 6aacf5e Re-land: Use os_log instead of syslog on Apple platforms (flutter/engine#16549) * a5736b8 Roll src/third_party/skia c3b67eb988c8..b1525c721ea6 (4 commits) (flutter/engine#16543) * 49a370f Roll src/third_party/dart 76b18c455e2c..e4c39721c473 (6 commits) (flutter/engine#16544) * 270421c Fix ensureInitializationCompleteAsync callback when already initialized. (#39675) (flutter/engine#16503) * ca02b91 Prevent long flash when switching to Flutter app. (#47903) (flutter/engine#16527) * 44e80fd skiping tests in Safari. LUCI recipe for Mac is ready. this is the only step left for stopping us running unit tests in Safari (flutter/engine#16550) * 5fb0116 iOS platform view gesture blocking policy. (flutter/engine#15940) * e0ebaea Revert "Re-land: Use os_log instead of syslog on Apple platforms (#16549)" (flutter/engine#16558) * 8a6b949 [Fuchsia] Dump syslog output after tests have run (flutter/engine#16561) * bca879c Roll src/third_party/dart e4c39721c473..0299903f3e78 (31 commits) (flutter/engine#16553) * cd11d7a Roll fuchsia/sdk/core/mac-amd64 from 7JkB7... to t4kck... (flutter/engine#16555) * 99a265b [web] Fix edge cases in Paragraph.getPositionForOffset to match Flutter (flutter/engine#16557) * 8f8af1f Update felt documentation (flutter/engine#16559) * 13dce50 Roll src/third_party/skia b1525c721ea6..67da665c27ff (32 commits) (flutter/engine#16562) * 7c67573 Fix multiline Javadoc code blocks (flutter/engine#16565) * aece5ad Move log_listener call into the reboot trap (flutter/engine#16564) * 42f18d9 Roll src/third_party/skia 67da665c27ff..886e8500a9f2 (3 commits) (flutter/engine#16566) * c4c6ef6 Samsung keyboard duplication workaround: updateSelection (flutter/engine#16547) * 15062ca Revert "Re-arm timer as necessary in MessageLoopFuchsia" (flutter/engine#16568) * 8802a1d Roll src/third_party/skia 886e8500a9f2..9102c86a81ad (1 commits) (flutter/engine#16570) * dbdcae4 Roll src/third_party/skia 9102c86a81ad..6029cbd560b7 (2 commits) (flutter/engine#16575) * f39bc73 Exposes FlutterSurfaceView, and FlutterTextureView to FlutterActivity and FlutterFragment. (#41984, #47557) (flutter/engine#16552) * db030ec Roll src/third_party/skia 6029cbd560b7..1a733b5b760a (1 commits) (flutter/engine#16577) * 050d29d Roll src/third_party/skia 1a733b5b760a..1d1333fcedf8 (3 commits) (flutter/engine#16578) * 97fd898 Roll fuchsia/sdk/core/mac-amd64 from t4kck... to oHa-O... (flutter/engine#16581) * 2e67866 Roll src/third_party/skia 1d1333fcedf8..3bf3b92dfab0 (1 commits) (flutter/engine#16584)
) (flutter#16527)" This reverts commit 95f7aa3.
3 participants
Prevent long flash when switching to Flutter app. (#47903)
@Hixie discovered a long black flash when switching to a Flutter app using the v2 embedding. It turns out that there was always a slight flash when selecting a Flutter app from the app switcher, but the v2 embedding made it much longer.
The root cause of the flash appears to be attaching/detaching the FlutterEngine to/from the FlutterView during non-destruction lifecycle methods. This decision wasn't inherently wrong, but since it created this visual artifact, this PR pushes attachment/detachment behavior to the creation/destruction lifecycle methods.
Testing for this behavior is likely impossible. I spoke with @Hixie a bit about it and we couldn't come up with any good ideas. @jason-simmons if you can think of anything worthwhile, please feel free to comment. The flash happens at the interface between the OS and the Flutter UI, and it is highly timing/processor dependent (flaky).
@mklim @jason-simmons it would be great if you two could carefully read through the changes in this PR to help find any likely issues that I may have missed with this change. The change seems to run fine so far on an emulator and physical device, but lifecycle methods have many implications throughout an app's life, so a couple more eyes on this wouldn't hurt.