lockscreen fade-out animation is < 60fps

VERIFIED FIXED

Status

Firefox OS
Gaia::System::Lockscreen
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gal, Assigned: timdream)

Tracking

unspecified
x86
Mac OS X

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 verified)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Currently its around 2fps. The animation does the following:

  transition:
    transform 0.5s ease,
    opacity 0.5s ease,
    visibility 0.5s ease;

This is supposed to run on the compositor thread.

STR:
1. disable the code pad (PIN)
2. lock the phone
3. turn screen back on
4. move slider to right to unlock the phone
5. animation is choppy (~2FPS)

By slowing down the animation above 10x we can observe that the slider moves back right to left during the fade-out, which causes reflows. Apply bug 937023 to stop that. With that applied, we still animate with 2fps.

Things to investigate:
1. Are we running this animation on the compositor?
2. If so, is the busy MT somehow stalling the compositor? If so, should the compositor thread get higher (realtime?) priority to avoid that?
3. Are we still repainting frequently still during the fade-out?

In parallel I observed high pmem use which causes pmem exhaustion. This might be related to bug 937023. Not sure. Keep an eye on this too.
(Reporter)

Comment 1

5 years ago
Assigned to Milan for re-assignment.
Assignee: nobody → milan
(Reporter)

Comment 2

5 years ago
I don't think we can ship this regression -> koi+. Please re-triage if you disagree and change as appropriate.
blocking-b2g: --- → koi+
Benoit, can you do a profile (remember to pick up the patch from bug 937023) as a priority - once we know what it is, we can hopefully involve more people to get the fix.
Assignee: milan → bgirard
Flags: needinfo?(bgirard)
I think this is the same underlying problem as bug 936864. I'll look into it.
Flags: needinfo?(bgirard)
Keywords: regression, regressionwindow-wanted
May I ask if bug 840752 is a dup of this one?
Don't think so.  At least, the fix that BenWa has in mind is a simple one, where Nick suggests something risky and complicated for bug 840572.
(In reply to Milan Sreckovic [:milan] from comment #6)
> Don't think so.  At least, the fix that BenWa has in mind is a simple one,
> where Nick suggests something risky and complicated for bug 840572.

Exactly. To clarify nrc is trying to solve the problem of properly handly the animation if we're transitioning the 'visibility' property. I believe that just animating the opacity should be sufficient. I have a local patch where I have the fade and scale running smoothly. I will post something soon.
Moving however to Lockscreen since we only need a Lockscreen change to fix this.
Component: Graphics → Gaia::System::Lockscreen
Keywords: regression, regressionwindow-wanted
Product: Core → Firefox OS
Version: Trunk → unspecified
Created attachment 831858 [details] [diff] [review]
patch

Note that this adds a 1 second delay to the animation lock screen to allow us to pre-render the background. Otherwise by time we rendered the background the animation is done.
Attachment #831858 - Flags: review?(timdream)
I just confirmed that this is hitting vsync on hamachi (64 FPS).
(Reporter)

Comment 11

5 years ago
Are automatically freeing the layer once it hits 0 opacity?
(Reporter)

Comment 12

5 years ago
Also, the 1s delay seems bad. Why do we need to prerender this? Shouldn't this already be rendered? Is the problem that the layering changes? If so, shouldn't we use the compositor to composite the existing layers together instead of re-paint?
(In reply to Andreas Gal :gal from comment #12)
> Also, the 1s delay seems bad. Why do we need to prerender this? Shouldn't
> this already be rendered? Is the problem that the layering changes? If so,
> shouldn't we use the compositor to composite the existing layers together
> instead of re-paint?

The problem is when the lockscreen is showing the lockscreen isn't an active layer so everything is a single layer. Layout performs visibility checks and only renders the lockscreen. One the start the animation the lockscreen becomes an active layer so we have to render the homescreen in that instant. I'd imagine this requires us to sync decode the homescreen icon since we don't keep the image lock which takes us in the 100's of millisecond.

We could avoid this delay by forcing the homescreen into it's own active layer. Trading off memory to avoid that 1s delay.
This certainly doesn't help. We have a 200ms before we start the unlock:
http://mxr.mozilla.org/gaia/source/apps/system/js/lockscreen.js#823
Ok so the 200ms delay and the rest is related. Here's what I know:
- Firstly as-is the Homescreen wont render behind the Lockscreen because the lockscreen is opaque and doesn't need to be active relative to the homescreen. We could force it to be a layer.
- When locking, the lock screen sends a message that the apps use the clear their contents
- When unlock, the lock screen sends an unlock message that the apps use to repaint themsevles. Then lockscreen waits for 200ms (see my above link)
- After 200ms it hopes that the compositor received the update from the app. At this point it kicks off the animation.

I have a local patch to confirm this and work around this issue by:
- Disabled the lock/unlock event from being dispatch (This may have other consequence and regressions I'm not familiar with this event).
- Force the homescreen to be an active layer.

Then the animation no longer needs then 1s delay. But of course it will cause us to keep rendering the app under the lockscreen and take up more memory. This is a trade off.
(In reply to Benoit Girard (:BenWa) from comment #15)
> I have a local patch to confirm this and work around this issue by:
> - Disabled the lock/unlock event from being dispatch (This may have other
> consequence and regressions I'm not familiar with this event).

That would result many state control issues.

> - Force the homescreen to be an active layer.

Are you saying if we don't |iframe.setVisible(false)| the active app frame this problem will go away? We couldn't do that because many apps rely on hidden state to do their own state controls too.

> Then the animation no longer needs then 1s delay. But of course it will
> cause us to keep rendering the app under the lockscreen and take up more
> memory. This is a trade off.

I think we could live with a delay like .3s at most. 1 sec is way to long.
Comment on attachment 831858 [details] [diff] [review]
patch

Thanks for identifying the cause.

https://mxr.mozilla.org/gaia/source/apps/system/js/lockscreen.js#821

Right above the 200ms timeout you pointed, there is a nextpaint listener in place -- 200ms is just a fail safe in case the app takes more than 200ms to paint. Are you saying home screen hit's that? If so, if we move prolong that timeout to 500 or even 600ms, would it help?
Attachment #831858 - Flags: review?(timdream)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #17)
> Comment on attachment 831858 [details] [diff] [review]
> patch
> 
> Thanks for identifying the cause.
> 
> https://mxr.mozilla.org/gaia/source/apps/system/js/lockscreen.js#821
> 
> Right above the 200ms timeout you pointed, there is a nextpaint listener in
> place -- 200ms is just a fail safe in case the app takes more than 200ms to
> paint.

Ahh, ok. That's fine then.

> Are you saying home screen hit's that? If so, if we move prolong that
> timeout to 500 or even 600ms, would it help?

I'll have another look again tomorrow with this information and study it more carefully.

If our transition includes a scale then starting the animation late is very noticeable because suddenly the lock screen jumps to say 1.5x and animates to 2x. However I find that if we only transition opacity a jump to 0.5 isn't as noticeable. I think we can afford smaller delays if we only animate opacity. But we might not even need to if this code is working as indented.
(In reply to Benoit Girard (:BenWa) from comment #18)
> > Are you saying home screen hit's that? If so, if we move prolong that
> > timeout to 500 or even 600ms, would it help?
> 
> I'll have another look again tomorrow with this information and study it
> more carefully.

No worries. I've just tried and it doesn't work. It looks like, on a Unagi,

1. home screen next paint will only took place around ~250ms
2. it will always hit the timeout for the first unlock after boot
3. even if I lengthen the timeout from 200ms to 600ms, fly out transition will still not async. Apparently the only solution (workaround) we found so far is attachment 831858 [details] [diff] [review].

> If our transition includes a scale then starting the animation late is very
> noticeable because suddenly the lock screen jumps to say 1.5x and animates
> to 2x. However I find that if we only transition opacity a jump to 0.5 isn't
> as noticeable. I think we can afford smaller delays if we only animate
> opacity. But we might not even need to if this code is working as indented.

I'll try to see if we could do something to cheat this :P
You need to apply the patch from bug 936864 first otherwise async animation don't work for the lockscreen.
Depends on: 936864
Created attachment 832254 [details] [diff] [review]
patch

Ok so we need at the very least to disable the visibility change otherwise we wont get a async animation. I think disabling the transform for now is good because it makes the delay in starting the animation harder to perceive, but that part is optional.

With this patch I get a nice fade out screen.
Attachment #831858 - Attachment is obsolete: true
Attachment #832254 - Flags: review?(timdream)
I talked to :gal on irc and he agrees that the scale animation isn't that important.

Meanwhile I spin off improving the timing of when we paint the locked app as bug 938737 which should eliminate the jank at the start of the unlock animation.
See Also: → bug 938737
(Reporter)

Comment 23

5 years ago
Once bug 938737 is done we can bring the scale back if UX thinks thats important.
Flags: needinfo?(padamczyk)
Comment on attachment 832254 [details] [diff] [review]
patch

Your patch seems nice but I am asking Greg to do a final attempt to unload the "unlock" moment, see if that fix the transition (instead of removing some of the desired transition).

If we couldn't figure out a way in hours before weekend starts, I will r+ and land your patch.

Thanks.
Attachment #832254 - Flags: feedback+
The issue can't be solved this adjust 'unlock' events and unlock process in the lockscreen. We should do more tests to find out what Gaia can do, especially what would be done by every event listener.
Comment on attachment 832254 [details] [diff] [review]
patch

Let's land this for real and figure out what to do later in bug 937630.
Attachment #832254 - Flags: review?(timdream) → review+
(In reply to Andreas Gal :gal from comment #23)
> Once bug 938737 is done we can bring the scale back if UX thinks thats
> important.

I agree it needs to be performant, so we can remove the scaling animation for now. We're likely going to add improved transitions for v.1.3 / v.1.4 in and around the homescreen / app navigation, so we can revisit it then.
Flags: needinfo?(padamczyk)
https://github.com/mozilla-b2g/gaia/commit/937249eae2b9cd19220927d81856a5ed273e7663
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
reverted: https://github.com/mozilla-b2g/gaia/commit/bdf4ce57b3f6a3083954f10a438ebdeed3b97846

Hi guys,

I was working on some tests for Clock, and I noticed that this patch introduced a regression. Portions of the Lockscreen render on top of the application surface, and although these Lockscreen elements are not visible, they interfere with mouse/touch events.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Mike Pennisi [:jugglinmike] from comment #30)
> I was working on some tests for Clock, and I noticed that this patch
> introduced a regression. Portions of the Lockscreen render on top of the
> application surface, and although these Lockscreen elements are not visible,
> they interfere with mouse/touch events.

Interesting. Although the patch removed |visibility: hidden| from the CSS, |opacity: 0| and |pointer-events: none| should be equal to that...
Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

Given the fact we cannot remove visibility transition, so here is an alternative patch in attempt to fix this bug.

(I have no access to bug 840572 BTW)

* use a cubic bezier timing function to "hide" the initial delay.
* move all the calls away from nextPaint() function and run them ahead of time in the unlock() function itself; have anything that will triggered by the 'unlock' event runs after the transition have concluded.
* make the nextpaint timeout a bit longer, from 200ms to 400ms.

I can verify locally on Unagi that there will always be a smooth transition.
Attachment #833339 - Flags: review?(gal)
Attachment #833339 - Flags: review?(21)
Attachment #833339 - Flags: feedback?(bgirard)
Assignee: bgirard → timdream
Status: REOPENED → ASSIGNED
Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

Looks very smooth to me!
Attachment #833339 - Flags: feedback?(bgirard) → feedback+
(Reporter)

Comment 35

5 years ago
Why can't you simply set visibility in an endtransition event instead of animating it?
(In reply to Andreas Gal :gal from comment #35)
> Why can't you simply set visibility in an endtransition event instead of
> animating it?

I could, but transitionend event listener is an anti-pattern I tried to avoid. We always have trouble cleaning unfired callbacks, or detecting transition that is cancelled midway. IMHO using transitionend will make the patch unsafe for v1.2.

(Web Animations is suppose to fix all that on the platform side, I am told.)
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #36)
> (In reply to Andreas Gal :gal from comment #35)
> > Why can't you simply set visibility in an endtransition event instead of
> > animating it?
> 
> I could, but transitionend event listener is an anti-pattern I tried to
> avoid. We always have trouble cleaning unfired callbacks, or detecting
> transition that is cancelled midway. IMHO using transitionend will make the
> patch unsafe for v1.2.

On the other hand, I've overlook the listener that is already in place -- will submit a patch to use that.
Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

I'm a bit sad about the setTimeout with some values. I agree with you that transitionend is a wrong pattern that has exploded a lot of time on our hands...
Attachment #833339 - Flags: review?(21) → review+
Attachment #833339 - Flags: review?(gal)
Attachment #833339 - Flags: review+
Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

I've attached another commit to use hidden attribute instead.
Attachment #833339 - Flags: review?(gal)
Attachment #833339 - Flags: review?(21)
(Reporter)

Comment 40

5 years ago
I think the gfx team is supposed to fix the visibility transition for real. Please talk to Milan to get the bug # and then remove the hack once thats done.
Attachment #832920 - Attachment is obsolete: true
Attachment #832254 - Attachment is obsolete: true
Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

if you still have the animation, hidden is fine. People usually avoid it because it toggle from display:none to block and animations does not like that and are skipped.
Attachment #833339 - Flags: review?(21) → review+
https://github.com/mozilla-b2g/gaia/commit/8b3338df8cb0144f2c2826b4b2d5c1841acdd038
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 938737
Reverted master: cbf9ff66b9e80d7294d5eaa922af249c2f46481f

Unit test failure:
  ✖ 1 of 8 tests failed:

  1) [system] enable/disable homegesture when lockscreen is disabled:
     Error: expected false to equal true
      at chaiAssert (http://test-agent.gaiamobile.org:8080/common/test/helper.js:33)
      at equal (http://test-agent.gaiamobile.org:8080/common/vendor/chai/chai.js:1250)
      at (anonymous) (http://system.gaiamobile.org:8080/test/unit/home_gesture_test.js:123)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Will merge after test passes, sorry about that.
master: https://github.com/mozilla-b2g/gaia/commit/0c4522589d4dd5244b79e218d0b18a4bba5f8f62

Unit test passed.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765 ?
Flags: needinfo?(cbook)
(In reply to Julien Wajsberg [:julienw] from comment #48)
> I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765
> ?

Yeah, reopen...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #49)
> (In reply to Julien Wajsberg [:julienw] from comment #48)
> > I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765
> > ?
> 
> Yeah, reopen...

yep because the checkin from https://hg.mozilla.org/integration/b2g-inbound/rev/000913999cd2 broke the Gaia Unit Tests like  https://tbpl.mozilla.org/php/getParsedLog.php?id=30700269&tree=B2g-Inbound and was reverted then in https://hg.mozilla.org/integration/b2g-inbound/rev/24aff45402dc

sorry somehow my backout comment landed in Bug 939434
Flags: needinfo?(cbook)
Update: Zac have found a possible timing issue with unlocking the phone with the |lockscreen.locked| mozSettings value and re-lock it again with |LockScreen.lock()|. I have change my code for handling all that. If Travis CI passes I will try to re-land the patch.

Fingers crossed!
Let's try this again!

master: https://github.com/mozilla-b2g/gaia/commit/45f65f707ae1d724c48c4ebf439d1617c35bbe42

https://travis-ci.org/mozilla-b2g/gaia/builds/14253381
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
QA,

Please check the performance impact of the fix.
Keywords: qawanted
Moving to verifyme as this is a post landing verification.
Keywords: qawanted → verifyme
I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 45f65f707ae1d724c48c4ebf439d1617c35bbe42
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(timdream)
v1.2: a9f25572bb1886458d9e11efd6c111bff746c5e0
status-b2g-v1.2: --- → fixed
Flags: needinfo?(timdream)
Attachment #833339 - Flags: review?(gal)
The animation is at around 60fps when the sliding animation occurs on both Buri 1.2 and 1.3
There was no issue with mouse/touch after the animation.

Changing to verified.

Buri 1.2
Environmental Variables
Device: Buri v1.2 COM RIL
Build ID: 20131204004003
Gecko: 758f3fb32dda
Gaia: 8d762f3376318fd6be390432db750ae4904c9ab6
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: v1.2_20131115

Master:
Environmental Variables
Device: Buri 1.3 Moz
Build ID: 20131203040236
Gecko: 8648aa476eef
Gaia: 31808a29cfcffa584b6a88b4f1e515387f485a1b
Platform Version: 28.0a1
RIL Version: 01.02.00.019.102 
Firmware Version: v1.2_20131115
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

5 years ago
status-b2g-v1.2: fixed → verified
(In reply to jschmitt from comment #59)
> The animation is at around 60fps when the sliding animation occurs on both
> Buri 1.2 and 1.3
> There was no issue with mouse/touch after the animation.
> 
> Changing to verified.
> 
> Buri 1.2
> Environmental Variables
> Device: Buri v1.2 COM RIL
> Build ID: 20131204004003
> Gecko: 758f3fb32dda
> Gaia: 8d762f3376318fd6be390432db750ae4904c9ab6
> Platform Version: 26.0
> RIL Version: 01.02.00.019.102 
> Firmware Version: v1.2_20131115
> 
> Master:
> Environmental Variables
> Device: Buri 1.3 Moz
> Build ID: 20131203040236
> Gecko: 8648aa476eef
> Gaia: 31808a29cfcffa584b6a88b4f1e515387f485a1b
> Platform Version: 28.0a1
> RIL Version: 01.02.00.019.102 
> Firmware Version: v1.2_20131115

How exactly did you measure this?
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-b2g-v1.2: verified → fixed

Updated

5 years ago
Keywords: verifyme
I tested with a debug setting 'Show frames per second' that shows fps when the sliding to unlock is animated.
(In reply to jschmitt from comment #61)
> I tested with a debug setting 'Show frames per second' that shows fps when
> the sliding to unlock is animated.

How many times did you run that test?
I ran that test 15 times towards camera and 15 times towards unlocking the phone on Buri 1.2 and another set of 15 times on the 1.3 build.
(In reply to jschmitt from comment #63)
> I ran that test 15 times towards camera and 15 times towards unlocking the
> phone on Buri 1.2 and another set of 15 times on the 1.3 build.

Okay - that sounds satisfactory for the verification then.
Status: RESOLVED → VERIFIED
status-b2g-v1.2: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.