Do not pause rendering when android activity loses focus #4848
Conversation
|
What does this do to the app lifecycle events? The theory is that the framework is set up to be entirely in charge of when frames should be pumped; the engine should schedule frames every 16ms so long as the framework is requesting frames. |
|
We'll need to update them. I can actually move the logic to the "Interestingly" if I don't add Investigating |
|
I also found the code in |
|
I've found the cause of the assertion error (not a crash, caused by sending a message "platformChrome" when the native view doesn't exist). It seems that restarting the App, caused by the change in layout/density, caused RunBundleAndSnapshot to be re-invoked |
|
@Hixie I've updated the PR. This change doesn't require anything from the flutter side, though we should consolidate the life cycles later. |
|
Generally, LGTM. /cc @jason-simmons for Android expertise. Given that we're changing which events are fired and when, are there situations this will be a breaking change to users who rely on the current behaviour? i.e. is there any change in behaviour when not in splitscreen or using picture-in-picture mode? If pause/stop are fired very close in time in such cases, this feels fine (no need to warn flutter-dev@). |
| // the native view is created in a separate thread. This mostly happens when the app restarts in dev | ||
| // mode when switching into split-screen mode. Preventing app restarts on layout and density | ||
| // changes will prevent this, and afterwards this can be changed back to an assert. | ||
| if (!isAttached()) { |
There was a problem hiding this comment.
Does this still happen if you apply the pending engine refactoring by @chinmaygarde ? (https://github.com/chinmaygarde/flutter_engine/tree/shell)
The Dart application may fail if it sends a message to the host and the host drops the message with no response.
There was a problem hiding this comment.
So looking into this more - I can reproduce the issue on master, somewhat flaky though. Might only be a debug issue
| @@ -267,7 +267,7 @@ public void addActivityLifecycleListener(ActivityLifecycleListener listener) { | |||
| } | |||
|
|
|||
| public void onPause() { | |||
| mFlutterLifecycleChannel.send("AppLifecycleState.paused"); | |||
| mFlutterLifecycleChannel.send("AppLifecycleState.inactive"); | |||
There was a problem hiding this comment.
Might be worth testing, I might be wrong, but I think on most Android manufacturer OSes, the lock screen might put us in this state too. We should be sure about battery usage for common cases.
There was a problem hiding this comment.
I'll take a look at battery use. I'm fairly certain that Choreographer won't give use frames, so hopefully it looks okay.
There was a problem hiding this comment.
Did some research - we might be able to tell using the PowerManager API
There was a problem hiding this comment.
I checked a few devices (pixel 1, nexus 5, galaxy s5) - all will call onStop when exiting to the lock screen. I think this should be okay as is
|
Still needs to be confirmed that this is unblocked by #4837 when video is working |
|
Updates: These changes work better with the engine/shell refactor patched in. There are still some edge cases which happen if the app restarts when entering split screen. We seem to get one or two extra frames after the view has been detached, might be a race in the teardown. Also the video player plugin might have some unrelated issues on the android side. All these issues are fixed if we add density and screenLayout to config changes. This would be a major breaking change, everyone would have to update their androidManifest, though they should anyway so their apps don't restart. Thoughts @Hixie , @jason-simmons ? |
|
I'm fine with adding things to the android manifest. We should just make sure to document it in the changelog. Ideally we'd use @xster's upgrade idea to autoupgrade people's projects, but that can be a stretch goal. |
|
|
|
Looks like we already updated the manifest in flutter/flutter#15871, in fact. |
|
I've confirmed that this works on the latest flutter version locally, including video |
…)" This reverts commit c83d1ef.
Originally the
FlutterActivityusedActivity#onPause()andActivity#onResume()as a proxy for whether the Android activity was visible. This made perfect sense before splitscreen, picture in picture, and chromebooks. Currently, any time a Flutter activity loses focus, rendering will stop. This is more obvious when there is an animation playing. I haven't had time to check on a chromebook, but I imagine that losing focus there will stop rendering too.onPause()will instead trigger theinactivestate on Android, previously only used for iOS. This corresponds to foreground visible but not focused, which matches the current behavior. since thepausedlifecycle is also used by iOS, this will still be used - but onlyonStop()for android. This is called whenever an activity goes into the background or is completely invisible.Picture in Picture
Original on left, with fix on right.
Splitscreen
Original on left, with fix on right.
Follow up work
Currently Flutter applications will still restart when entering splitscreen mode. The fix for this is separate, just an addition to the
android:configChangesentry.Fixes flutter/flutter#11906, flutter/flutter#14619, flutter/flutter#14665