937630 - lockscreen fade-out animation is < 60fps

Status: VERIFIED FIXED

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

Description

[Andreas Gal :gal]

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.

Comment 1

[Andreas Gal :gal]

Assigned to Milan for re-assignment.

Comment 2

[Andreas Gal :gal]

I don't think we can ship this regression -> koi+. Please re-triage if you disagree and change as appropriate.

Comment 3

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

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.

Comment 4

[Benoit Girard (:BenWa)]

I think this is the same underlying problem as bug 936864. I'll look into it.

Comment 5

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

May I ask if bug 840752 is a dup of this one?

Comment 6

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

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.

Comment 7

[Benoit Girard (:BenWa)]

(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.

Comment 8

[Benoit Girard (:BenWa)]

Moving however to Lockscreen since we only need a Lockscreen change to fix this.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: 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.

Comment 10

[Benoit Girard (:BenWa)]

I just confirmed that this is hitting vsync on hamachi (64 FPS).

Comment 11

[Andreas Gal :gal]

Are automatically freeing the layer once it hits 0 opacity?

Comment 12

[Andreas Gal :gal]

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?

Comment 13

[Benoit Girard (:BenWa)]

(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.

Comment 14

[Benoit Girard (:BenWa)]

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

Comment 15

[Benoit Girard (:BenWa)]

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.

Comment 16

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

(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 17

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

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?

Comment 18

[Benoit Girard (:BenWa)]

(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.

Comment 19

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

(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

Comment 20

[Benoit Girard (:BenWa)]

You need to apply the patch from bug 936864 first otherwise async animation don't work for the lockscreen.

Comment 21

[Benoit Girard (:BenWa)]

Attachment: 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.

Comment 22

[Benoit Girard (:BenWa)]

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.

Comment 23

[Andreas Gal :gal]

Once bug 938737 is done we can bring the scale back if UX thinks thats important.

Comment 24

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

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.

Comment 25

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

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 26

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

Comment on attachment 832254 [details] [diff] [review]
patch

Let's land this for real and figure out what to do later in bug 937630.

Comment 27

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#13749

Comment 28

[Patryk Adamczyk [:patryk] UX]

(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.

Comment 29

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

https://github.com/mozilla-b2g/gaia/commit/937249eae2b9cd19220927d81856a5ed273e7663

Comment 30

[Mike Pennisi [:jugglinmike]]

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.

Comment 31

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

(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 32

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#13769

Comment 33

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

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.

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

Looks very smooth to me!

Comment 35

[Andreas Gal :gal]

Why can't you simply set visibility in an endtransition event instead of animating it?

Comment 36

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

(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.)

Comment 37

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

(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 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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...

Comment 39

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

Comment on attachment 833339 [details] [review]
mozilla-b2g:master PR#13769

I've attached another commit to use hidden attribute instead.

Comment 40

[Andreas Gal :gal]

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.

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 42

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

https://github.com/mozilla-b2g/gaia/commit/8b3338df8cb0144f2c2826b4b2d5c1841acdd038

Comment 44

[Julien Wajsberg [:julienw]]

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)

Comment 45

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#13795

Comment 46

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

Will merge after test passes, sorry about that.

Comment 47

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

master: https://github.com/mozilla-b2g/gaia/commit/0c4522589d4dd5244b79e218d0b18a4bba5f8f62

Unit test passed.

Comment 48

[Julien Wajsberg [:julienw]]

I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765 ?

Comment 49

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

(In reply to Julien Wajsberg [:julienw] from comment #48)
> I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765
> ?

Yeah, reopen...

Comment 50

[Carsten Book [:Tomcat]]

(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

Comment 51

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#13867

Comment 52

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

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!

Comment 53

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

Let's try this again!

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

https://travis-ci.org/mozilla-b2g/gaia/builds/14253381

Comment 54

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

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=df768d0e8127

Comment 55

[Preeti Raghunath(:Preeti)]

QA,

Please check the performance impact of the fix.

Comment 56

[Jason Smith [:jsmith]]

Moving to verifyme as this is a post landing verification.

Comment 57

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 58

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

v1.2: a9f25572bb1886458d9e11efd6c111bff746c5e0

Comment 59

[Josh Schmitt [Joshs]]

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

Comment 60

[Jason Smith [:jsmith]]

(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?

Comment 61

[Josh Schmitt [Joshs]]

I tested with a debug setting 'Show frames per second' that shows fps when the sliding to unlock is animated.

Comment 62

[Jason Smith [:jsmith]]

(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?

Comment 63

[Josh Schmitt [Joshs]]

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.

Comment 64

[Jason Smith [:jsmith]]

(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.