Closed Bug 938737 Opened 6 years ago Closed 2 years ago

[Lockscreen] Paint background app *before* we unlock

Categories

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

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: BenWa, Unassigned)

References

Details

Attachments

(2 files)

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?
Flags: needinfo?(timdream)
See Also: → 937630
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.
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.
Flags: needinfo?(timdream)
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).
I am duping this bug to bug 937630 because we ended up preserving the transition and paint the app before unlock animation.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 937630
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
Attached image 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?
Flags: needinfo?(timdream)
(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).
Flags: needinfo?(timdream)
Greg, please the last comment. I was meant to needinfo? you.
Flags: needinfo?(gweng)
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.
Alive, do we cover app window with screenshot every time the foreground app is being setVisible(false)'d now?
Flags: needinfo?(alive)
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.
Flags: needinfo?(gweng)
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?
Flags: needinfo?(gweng)
Attached image 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.
Flags: needinfo?(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.
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.
(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
(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.
Flags: needinfo?(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).
(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
(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.
Assignee: nobody → gweng
blocking-b2g: --- → 1.3?
(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.
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.
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.
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
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).
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.
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.
No longer blocks: 945082
blocking-b2g: 1.3? → ---
See Also: → 945082
Should we close this bug since this solution wouldn't help performance, and we're trying other possible solutions at Bug 945082?
Flags: needinfo?(timdream)
Flags: needinfo?(timdream) → needinfo?(bgirard)
(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.
Flags: needinfo?(bgirard)
Assignee: gweng → nobody
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.