938737 - [Lockscreen] Paint background app *before* we unlock

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Benoit Girard (:BenWa)]

Currently we paint the locked application in the lock screen only when you unlock. You can see this by making the lockscreen div have opacity 0.5 (via app manager or rebuild). This introduce a delay when unlocking the screen and animating the lockscreen fade out.

We should paint the background app before the user complete the unlock. IMO it should happen shortly after the lockscreen has been painted the first time after turning the screen back on. It could also happen just when the user is interacting with the lockscreen or nearly about to unlock. If the screen is closed then we should rehide the background app.

Tim can your team look at this?

Comment 1

[Benoit Girard (:BenWa)]

Just to add, currently we wait 200ms for the app to paint as we unlock the screen so this could reduce the unlock delay by 100ms-200ms.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

That would trigger many undesired side effect, like music resume playing when the user "nearly about to unlock". We have not thought of passcode situation either.

Comment 3

[Benoit Girard (:BenWa)]

Perhaps we should have a way of painting the app without doing a full resume. Not sure how feasible this is. The passcode situation should not be different. We should paint the app when the lockscreen is visible (any panel).

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am duping this bug to bug 937630 because we ended up preserving the transition and paint the app before unlock animation.

Comment 5

[Benoit Girard (:BenWa)]

We still don't paint the app ahead of time which adds a noticeable delay. We need to prepare the background app ASAP.

We need to preserve the app contents while prevent undesirable effects.

Comment 6

[Benoit Girard (:BenWa)]

I add opacity to the lockscreen and we already overlay a screenshot while the screen is lock but we force a paint before starting to OMTA so there's no benefit from what we currently do.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: DOM Inspector screen

Currently we already have a screenshot overlay. Is this expected?

We can either:
1) use the screenshot that we're already computing anyways
2) Stop taking the screenshot and make sure we retain the layer tree instead of destroying it.

Tim can you provide more details about this screenshot?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Benoit Girard (:BenWa) from comment #7)
> Created attachment 8343388 [details]
> DOM Inspector screen
> 
> Currently we already have a screenshot overlay. Is this expected?
> 
> We can either:
> 1) use the screenshot that we're already computing anyways
> 2) Stop taking the screenshot and make sure we retain the layer tree instead
> of destroying it.
> 
> Tim can you provide more details about this screenshot?

Yes, that is the AppWindow feature I talked about in bug 945082 comment 4. It utilize mozbrowser.getScreenshot() feature to get a screenshot of a window.

I am not sure if screenshot gets taken already or not, what was the state of the phone when you inspect the DOM here? Did you played with task switcher/card view/card swiping already? Screenshot are very expensive so we usually don't capture it until it's needed.

Greg, could you work with Alive to quickly try out the idea here? (cover the foreground app with screenshot before unlock transition, only actually setVisible(true) after the app have unlocked) If this is proven to make the transition smoother, you could decide whether or not to push for a solid fix or not. I would imagine the state and transition control will be painful if we do this (that's why I asked this to be fixed in platform instead).

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Greg, please the last comment. I was meant to needinfo? you.

Comment 10

[Benoit Girard (:BenWa)]

We're already modifying the DOM to overlay a screenshot. You can see it if you get opacity on the lock screen. It's too fuzy to be the app and the DOM inspector reveals that it's a screenshot node that we're seeing.

Since we already have and take the screenshot we should just use it properly for now. The way the OMTAnimation is triggered it doesn't use the screenshot. We request the app to be painted before we start the OMTAnimation so we don't see the screenshot properly. We need to start the transition, wait for the requestAnimationFrame callback and then once that's called the OMTAnimation has started to face from the lockscreen to the app's screenshot. While the compositor is fading that then we can issue the request to resume the app like we currently do.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Alive, do we cover app window with screenshot every time the foreground app is being setVisible(false)'d now?

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

The idea seems good enough to be a workaround. But I think we should also try to use the 'will-animate' CSS property to pre-render the app, too. At least that's a 'formal' way to hint the render engine that it should do some pre-rendering things.

Beside that, there is one thing I've noticed is that the screenshot would take some time to generate, and the timing it get ready is not knowable. So if user lock the screen and unlock it very quickly, I don't know what would happen.

Comment 13

[Benoit Girard (:BenWa)]

Yes I plan on using will-animate when possible but even with will-animate we still need something behind.

Right now I'm not asking if we can use a screenshot. I'm *stating* the the lockscreen is using a screenshot for the app already. See my screenshot of the DOM that shows a screenshot-overlay node.

In it's current forms the screenshot is never seen when we unlock before of the sequence. We wait for the app to paint then we start the transition. The screenshot is currently useless.

I want to know what the purpose of the screenshot-overlay node is?

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: noscreenshot.png

I don't know when you take the screenshot. I launch an app normally and lock the screen, the DOM wasn't changed before or after I unlocked the lockscreen.

I've tested this on Nightly, but I don't think this would change if I test this on device.

Comment 15

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, I've found that. I focus on the homescreen showing delay issue but apparently the homescreen won't take screen, while other apps would do.

I can check the interested screenshot to see what it would be used, and why homescreen won't have one.

Comment 16

[Benoit Girard (:BenWa)]

I noticed that they aren't always there. Making the homescreen opacity: 0.5 makes this easier to debug. But like I said you can tell the sequence is such that we wait until the app has painted before we remove the lockscreen so the screenshot is not useful in its current form.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> Alive, do we cover app window with screenshot every time the foreground app
> is being setVisible(false)'d now?

Seems so.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1107

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Benoit Girard (:BenWa) from comment #16)
> I noticed that they aren't always there. Making the homescreen opacity: 0.5
> makes this easier to debug. But like I said you can tell the sequence is
> such that we wait until the app has painted before we remove the lockscreen
> so the screenshot is not useful in its current form.

Uh, it's not used for this case but it doesn't mean the feature is not useful. It's for edge gesture.
Now we could just remove all the 'wait-for-next-paint' code in the unlock function of lockscreen.

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

BTW we definitely shouldn't bring background app to foreground before lockscreen is unlocked. Please don't do this anyway by the fact this would affect process priority management and app UI(visibilitychange event).

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Greg Weng [:snowmantw] from comment #15)
> Oh, I've found that. I focus on the homescreen showing delay issue but
> apparently the homescreen won't take screen, while other apps would do.
> 
> I can check the interested screenshot to see what it would be used, and why
> homescreen won't have one.

You cannot take screenshot of homescreen before
https://bugzilla.mozilla.org/show_bug.cgi?id=878003 is fixed.

BTW homescreen app itself now has a bug that it shouldn't have background but it has now.
https://bugzilla.mozilla.org/show_bug.cgi?id=945686

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Alive Kuo [:alive][NEEDINFO][OOO:12/24-1/4] from comment #17)
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> window_manager.js#L1107
Updating URL:

https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/system/js/window_manager.js#L1107

I am assigning this bug to :snowmantw for him to try out a WIP patch that

0) (after bug 940842) set will-animate to the lock screen overlay when the user start swiping
1) when the lock screen unlocks, keep the foreground app in the screenshot state
2) only after transition is over, we replace the foreground app screenshot with actual app

I hope this will defer the CPU time the app chew away at visibilitychange event after the animation has finished. Any maybe this is the only change necessary for bug 945082.

Comment 22

[Benoit Girard (:BenWa)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #21)
> 2) only after transition is over, we replace the foreground app screenshot
> with actual app

Actually you should start the transition and then call requestAnimationFrame at the same time. Once rAF is called then you know the previous frame has been sent to the compositor (assuming you kicked off a off-main-thread compatible animation like an opacity change) and that the animation is being interpolated in the compositor. This means the main thread is now free and can start resuming the app while the compositor is interpolating the animation.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

I think my patch would:

1. Listen `appcreated`, `appopen` and other events to maintain a current app information in LockScreen
2. When it goes to lock the device, take a screenshot and therefore hide the app itself (`setVisible(fase, true)`)
3. At the unlocking moment, `setVisible(true)` to show the app and hide the screenshot

The 3. would follow the Benoit's suggestion, to set a rAF and do the showing thing inside it's callback.

This would need to handle some special cases:

1. Homescreen, can't take screenshow as other apps (Comment #20).
2. The app got OOM killed while the screen is locked.

If I'm wrong please correct me.

Comment 24

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, Tim just replied me offline that OOM window would be killed with the screenshow as well. So I don't need to handle that.

Comment 25

[Greg Weng [:snowmantw][:gweng][:λ]]

Now the flow would be:

1. LockScreen (already) will trigger 'lock' event
2. VisibilityManager would receive this event, and it would order the AppWindow to create the screenshot
3. When unlock, it would wait 'unlock' event, but not 'will-unlock', to remove the screenshot

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

Jerry from our graphic team has discovered that the main thread of B2G process would do lots of things while unlocking, and I had tried to  suspend all 'will-unlock' and 'unlock' events handled in other components, but there was no luck: the animation still broken randomly. So I'm afraid that our process of drawing the app back would cost lots of resources, and that even can't be eased with Gaia works. Of course I would keep trying every possible solutions.

(I've found that issues like this usually can't be completely solved by Gaia works... And I really hate to guess reasons and try solutions in blind).

Comment 27

[Greg Weng [:snowmantw][:gweng][:λ]]

As I posted in Bug 945082, the animation it didn't be lagged, it is the cause of lagging. While there seems some works need to be done in Gecko, the Gaia can do few things.

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's get this through the usual central path, rather than rushing it into 1.3.  It seems like not all the questions are answered yet. It doesn't really block the 60fps bug.

Comment 29

[Greg Weng [:snowmantw][:gweng][:λ]]

Should we close this bug since this solution wouldn't help performance, and we're trying other possible solutions at Bug 945082?

Comment 30

[Benoit Girard (:BenWa)]

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #29)
> Should we close this bug since this solution wouldn't help performance

I'm sorry but I can't find evidence to support this conclusion in this thread (maybe I missed it while re-reading). It's hard to believe having the background app painting while waiting on the user to unlock the phone wouldn't improve performance. Possibly power if we retain the content while the screen is locked.

> and we're trying other possible solutions at Bug 945082?

There's useful performance optimizations in bug 945082 but we're still going to be left with this problem. These bugs don't overlap. If we don't have the content painted we're still going to take 50-100ms best case (optimal code paths) to re-rasterize a fullscreen buffer. I believe we need both fixed.

Comment 31

[BMO Automation]

Firefox OS is not being worked on