[wip] Fixed flutter not rendering in inactive state by Kavantix · Pull Request #17396 · flutter-team-archive/engine

Kavantix · 2020-03-30T14:09:56Z

Moved to #25196

Flutter currently does not render on ios in the inactive state
This is because it is using the wrong lifecycle events to turn rendering on/off

Relates to flutter/flutter#50732 and flutter/flutter#33236

I'm not sure what other impact this has on the engine though so need some help.
I noticed that in the application delegate there is also an setIsGpuDisabled method which is also called on the wrong events.

Kavantix · 2020-04-13T11:12:52Z

I've looked into this some more and it seems like this behaviour was changed in an attempt to fix some bug by this pr: #7450
However, like can be seen by the issues I listed before this breaks the behaviour an app should have.
Also pr #13908 seems to fix the issue of opengl calls while backgrounded

Looking at the apple documentation linked in #7450 it seems that it might be a little outdated, also it contradicts itself somewhat by saying you should again enable rendering in applicationWillEnterForeground.
However, it would be important to make sure that we flush all gl commands in didEnterBackground.

@xster @gaaclarke @cbracken do you have any additional comments and/or thoughts on this?

cbracken · 2020-04-13T16:38:01Z

As background, #7450 fixed a very real crasher that affected a ton of devices. I should probably have been more explicit in my commit message as to which devices were affected, but IIRC it was iPhone 6, iPhone 6S devices in particular. If I'm not mistaken, this was a fix to a previous change that had paused animation in applicationWillEnterBackground:.

Looking at the apple documentation linked in #7450 it seems that it might be a little outdated

Can you give a bit of detail as to what part of it is incorrect? If you've got a link to up-to-date documentation, that would be very useful. It's important that this cover all iOS devices back to the iPhone 4S and all OS levels back to iOS 8.4.1.

also it contradicts itself somewhat by saying you should again enable rendering in applicationWillEnterForeground.

Can you provide details on what this contradicts? Is your suggestion that it's inconsistent, and animation should be re-enabled in applicationDidBecomeActive: instead?

Kavantix · 2020-04-13T17:35:29Z

As background, #7450 fixed a very real crasher that affected a ton of devices. I should probably have been more explicit in my commit message as to which devices were affected, but IIRC it was iPhone 6, iPhone 6S devices in particular. If I'm not mistaken, this was a fix to a previous change that had paused animation in applicationWillEnterBackground:.

Interesting, weird that it would only affect older devices, especially the 6s since that isn't even that old.
However, stopping in applicationWillEnterBackground should be fine, I do have an iPhone 5 at the office that I could pick up to test with, and seeing if I can reproduce this issue when moving the gpu disabling to willEnterBackground.
Let me know if that would help.

Btw @cbracken, does surfaceUpdated trigger a flush of the gl commands?
Because if not that might explain why some people still report seeing this crash (since it would just be a race condition)

Looking at the apple documentation linked in #7450 it seems that it might be a little outdated

Can you give a bit of detail as to what part of it is incorrect? If you've got a link to up-to-date documentation, that would be very useful. It's important that this cover all iOS devices back to the iPhone 4S and all OS levels back to iOS 8.4.1.

Well there is a big banner saying that it's deprecated and you should use metal (which has been out since iOS 8 I believe)

also it contradicts itself somewhat by saying you should again enable rendering in applicationWillEnterForeground.

Can you provide details on what this contradicts? Is your suggestion that it's inconsistent, and animation should be re-enabled in applicationDidBecomeActive: instead?

Yes I would expect it to be consistent between the enable and disable because if you follow apple's guidelines then (like you explain in the PR) it would pause the ui on inactive and never enable it again since it never got backgrounded and thus doesnt enter the foreground again.

This is also kind of what's happening now, the ui pauses when it goes to the inactive state (which happens for instance when a native popup shows for permissions or you go to the app switcher).
Any native app will keep rendering in this case.

So either disable in inactivate and re-enable in becomeActive or inactivate in didEnterBackground and re-enable in didEnterForeground.

Also, here Apple explicitly states:

You should use this method to pause ongoing tasks, disable timers, and throttle down OpenGL ES frame rates. Games should use this method to pause the game. An app in the inactive state should do minimal work while it waits to transition to either the active or background state.

Which is also in contradiction with the other documentation that suggests to complete stop the OpenGL rendering.

Kavantix · 2020-04-22T21:09:28Z

Here is also som new documentation (for metal)
It seems that when using metal and calling in the background will never kill the app.

So would it be a good approach to keep the current behaviour when using openGL and move to the disabling in didEnterBackground only when running in metal?

jimwan · 2021-01-07T07:12:45Z

@Kavantix When will this be merged, i face the same issue now. And do you have any work around ways?

Kavantix · 2021-03-20T10:08:49Z

@cbracken @chinmaygarde do you have any update whether this is anywhere on the current roadmap and/or how I could help get it there

With the idea from my last comment it seems possible to fix this without breaking on older devices that cannot use metal.

This fix would greatly improve the ‘native’ feel of flutter apps on iOS

gaaclarke · 2021-03-23T00:12:16Z

  [center addObserver:self
-             selector:@selector(applicationBecameActive:)
-                 name:UIApplicationDidBecomeActiveNotification
+             selector:@selector(applicationWillEnterForeground:)


I'm not sure how these notifications match access to the GPU. The problem is the wording "Will Enter Foreground", which means it isn't currently in the foreground, which means it's in the background. If it's in the background does it have access to the GPU? Typically no, but the wording on the notification might just be awkward.

Unfortunately it's hard to prove this is correct because even with setIsGpuDisabled removed, the crash doesn't manifest 100% of the time. It's a race condition.

If we could provide instructions that could reproduce the crash 100% of the time with the setIsGpuDisabled removed, we could have more confidence in the change. I'll try playing around with it.

From apples docs on didEnterBackground:

Use this method to release shared resources, invalidate timers, and store enough app state information to restore your app to its current state in case it's terminated later. Disable updates to your app’s user interface, and avoid using some types of shared system resources (such as the user’s contacts database). Don’t use OpenGL ES in the background.

From willEnterForeground:

In iOS 4.0 and later, UIKit calls this method as part of the transition from the background to the active state. You can use this method to undo many of the changes you made to your app upon entering the background.

In the background you should not access the gpu indeed but before didEnterBackground is called we’re not in the background. And when willEnterForeground we are also no longer in the background.

Also the metal documentation I linked before has this:

When UIKit calls your app delegate's applicationDidEnterBackground(_:) method, make sure Metal has scheduled all command buffers you've already committed before your app returns control to the system. On each command queue, if the last command buffer you queued isn't already scheduled or complete, call waitUntilScheduled() to force it to be scheduled.

Also the crash is only when using openGL ES
When using metal any commands in the background will simply return an error instead of crashing the app:

After your app moves into the background, if Metal sees a new command buffer from your app, it returns an error, rather than scheduling the command buffer. You can test for this error by adding a completion handler (addCompletedHandler(_:)). Inside your completion handler, check the status and error properties.

The problem is the wording "Will Enter Foreground", which means it isn't currently in the foreground, which means it's in the background.

This is not correct, the app has more than 2 states.
When willEnterForeground is called it’s no longer in the background but in the inactive state, it might not be visible to the user yet though but that’s independent of the state

Finally from willResignActive which we’re currently using to stop rendering:

Use this method to pause ongoing tasks, disable timers, and throttle down OpenGL ES frame rates. Games should use this method to pause the game. An app in the inactive state should do minimal work while it waits to transition to either the active or background state.

So in this method the app should not stop rendering just try to use less resources

Hopefully these points help you figure this out 🙂
I’m glad it’s finally being looked at again!

Kavantix · 2021-03-23T06:51:49Z

Btw as a general note, feel free to push to this PR I don’t mind

gaaclarke · 2021-03-23T16:38:09Z

Ugh, I accidentally closed this with a bungled rebase. @Kavantix can you please force push your version or a rebased version, here's what it looks like rebased:

commit 52fdfe6617bde17a7f633cd30530ed365add74cd
Author: Pieter van Loon <[email protected]>
Date:   Mon Mar 30 15:57:31 2020 +0200

    Fixed flutter not rendering in inactive state
    
    Move setIsGpuDisabled to the correct notifications

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
index 23e56fdc4..cf23c1f0a 100644
--- a/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
+++ b/shell/platform/darwin/ios/framework/Source/FlutterEngine.mm
@@ -155,18 +155,13 @@ static constexpr int kNumProfilerSamplesPerSec = 5;
                object:nil];
 
   [center addObserver:self
-             selector:@selector(applicationDidEnterBackground:)
-                 name:UIApplicationDidEnterBackgroundNotification
-               object:nil];
-
-  [center addObserver:self
-             selector:@selector(applicationBecameActive:)
-                 name:UIApplicationDidBecomeActiveNotification
+             selector:@selector(applicationWillEnterForeground:)
+                 name:UIApplicationWillEnterForegroundNotification
                object:nil];
 
   [center addObserver:self
-             selector:@selector(applicationWillResignActive:)
-                 name:UIApplicationWillResignActiveNotification
+             selector:@selector(applicationDidEnterBackground:)
+                 name:UIApplicationDidEnterBackgroundNotification
                object:nil];
 
   [center addObserver:self
@@ -887,11 +882,11 @@ static void SetEntryPoint(flutter::Settings* settings, NSString* entrypoint, NSS
 
 #pragma mark - Notifications
 
-- (void)applicationBecameActive:(NSNotification*)notification {
+- (void)applicationWillEnterForeground:(NSNotification*)notification {
   [self setIsGpuDisabled:NO];
 }
 
-- (void)applicationWillResignActive:(NSNotification*)notification {
+- (void)applicationDidEnterBackground:(NSNotification*)notification {
   [self setIsGpuDisabled:YES];
 }
 
diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
index 91c73b0b3..49733a54b 100644
--- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
+++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
@@ -736,24 +736,24 @@ static void sendFakeTouchEvent(FlutterEngine* engine,
 
 - (void)applicationBecameActive:(NSNotification*)notification {
   TRACE_EVENT0("flutter", "applicationBecameActive");
-  if (_viewportMetrics.physical_width)
-    [self surfaceUpdated:YES];
   [self goToApplicationLifecycle:@"AppLifecycleState.resumed"];
 }
 
 - (void)applicationWillResignActive:(NSNotification*)notification {
   TRACE_EVENT0("flutter", "applicationWillResignActive");
-  [self surfaceUpdated:NO];
   [self goToApplicationLifecycle:@"AppLifecycleState.inactive"];
 }
 
 - (void)applicationDidEnterBackground:(NSNotification*)notification {
   TRACE_EVENT0("flutter", "applicationDidEnterBackground");
+  [self surfaceUpdated:NO];
   [self goToApplicationLifecycle:@"AppLifecycleState.paused"];
 }
 
 - (void)applicationWillEnterForeground:(NSNotification*)notification {
   TRACE_EVENT0("flutter", "applicationWillEnterForeground");
+  if (_viewportMetrics.physical_width)
+    [self surfaceUpdated:YES];
   [self goToApplicationLifecycle:@"AppLifecycleState.inactive"];
 }

gaaclarke · 2021-03-23T16:42:47Z

(in the future, it's easier to work with branches if they aren't named master or main)

Kavantix · 2021-03-23T17:37:22Z

(in the future, it's easier to work with branches if they aren't named master or main)

Yeah I know, did that by accident

Kavantix · 2021-03-23T17:52:27Z

I opened a new PR which is also rebased on master #25196

auto-assign Bot requested a review from gw280 March 30, 2020 14:09

googlebot added cla: yes labels Mar 30, 2020

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Sep 3, 2020

gaaclarke mentioned this pull request Mar 23, 2021

Extend drawing and rendering in lifecycles in iOS flutter/flutter#71051

Closed

gaaclarke reviewed Mar 23, 2021

View reviewed changes

gaaclarke closed this Mar 23, 2021

gaaclarke force-pushed the master branch from 85bb65c to 6527a68 Compare March 23, 2021 16:32

Kavantix mentioned this pull request Mar 23, 2021

Keep rendering to screen in iOS inactive state #25196

Merged

7 of 9 tasks

Conversation

Kavantix commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kavantix commented Apr 13, 2020

Uh oh!

cbracken commented Apr 13, 2020

Uh oh!

Kavantix commented Apr 13, 2020

Uh oh!

Kavantix commented Apr 22, 2020

Uh oh!

jimwan commented Jan 7, 2021

Uh oh!

Kavantix commented Mar 20, 2021

Uh oh!

gaaclarke Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kavantix Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kavantix Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kavantix Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Kavantix commented Mar 23, 2021

Uh oh!

gaaclarke commented Mar 23, 2021

Uh oh!

gaaclarke commented Mar 23, 2021

Uh oh!

Kavantix commented Mar 23, 2021

Uh oh!

Kavantix commented Mar 23, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

Kavantix commented Mar 30, 2020 •

edited

Loading