Closed
Bug 937630
Opened 11 years ago
Closed 11 years ago
lockscreen fade-out animation is < 60fps
Categories
(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)
Tracking
(blocking-b2g:koi+, b2g-v1.2 verified)
People
(Reporter: gal, Assigned: timdream)
References
Details
Attachments
(3 files, 3 obsolete files)
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 2•11 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+
Comment 3•11 years ago
|
||
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)
Comment 4•11 years ago
|
||
I think this is the same underlying problem as bug 936864. I'll look into it.
Flags: needinfo?(bgirard)
Updated•11 years ago
|
Keywords: regression,
regressionwindow-wanted
Assignee | ||
Comment 5•11 years ago
|
||
May I ask if bug 840752 is a dup of this one?
Comment 6•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
I just confirmed that this is hitting vsync on hamachi (64 FPS).
Reporter | ||
Comment 11•11 years ago
|
||
Are automatically freeing the layer once it hits 0 opacity?
Reporter | ||
Comment 12•11 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?
Comment 13•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 16•11 years ago
|
||
(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.
Assignee | ||
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
(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.
Assignee | ||
Comment 19•11 years ago
|
||
(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•11 years ago
|
||
You need to apply the patch from bug 936864 first otherwise async animation don't work for the lockscreen.
Depends on: 936864
Comment 21•11 years ago
|
||
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)
Comment 22•11 years ago
|
||
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: → 938737
Reporter | ||
Comment 23•11 years ago
|
||
Once bug 938737 is done we can bring the scale back if UX thinks thats important.
Flags: needinfo?(padamczyk)
Assignee | ||
Comment 24•11 years ago
|
||
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+
Comment 25•11 years ago
|
||
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.
Assignee | ||
Comment 26•11 years ago
|
||
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+
Comment 28•11 years ago
|
||
(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)
Assignee | ||
Comment 29•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/937249eae2b9cd19220927d81856a5ed273e7663
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 30•11 years ago
|
||
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 → ---
Assignee | ||
Comment 31•11 years ago
|
||
(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...
Assignee | ||
Comment 33•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: bgirard → timdream
Status: REOPENED → ASSIGNED
Comment 34•11 years ago
|
||
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•11 years ago
|
||
Why can't you simply set visibility in an endtransition event instead of animating it?
Assignee | ||
Comment 36•11 years ago
|
||
(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.)
Assignee | ||
Comment 37•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Attachment #833339 -
Flags: review?(gal)
Attachment #833339 -
Flags: review+
Assignee | ||
Comment 39•11 years ago
|
||
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•11 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.
Assignee | ||
Updated•11 years ago
|
Attachment #832920 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #832254 -
Attachment is obsolete: true
Comment 41•11 years ago
|
||
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+
Assignee | ||
Comment 42•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/8b3338df8cb0144f2c2826b4b2d5c1841acdd038
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 44•11 years ago
|
||
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 → ---
Assignee | ||
Comment 46•11 years ago
|
||
Will merge after test passes, sorry about that.
Assignee | ||
Comment 47•11 years ago
|
||
master: https://github.com/mozilla-b2g/gaia/commit/0c4522589d4dd5244b79e218d0b18a4bba5f8f62 Unit test passed.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 48•11 years ago
|
||
I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765 ?
Flags: needinfo?(cbook)
Assignee | ||
Comment 49•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #48) > I think Tomcat reverted it again in 9f72bf98cb7c256d80c8cb2e5cbbeaa39fd15765 > ? Yeah, reopen...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 50•11 years ago
|
||
(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)
Assignee | ||
Comment 52•11 years ago
|
||
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!
Assignee | ||
Comment 53•11 years ago
|
||
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
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 54•11 years ago
|
||
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=df768d0e8127
Comment 56•11 years ago
|
||
Moving to verifyme as this is a post landing verification.
Comment 57•11 years ago
|
||
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)
Assignee | ||
Comment 58•11 years ago
|
||
v1.2: a9f25572bb1886458d9e11efd6c111bff746c5e0
status-b2g-v1.2:
--- → fixed
Flags: needinfo?(timdream)
Assignee | ||
Updated•11 years ago
|
Attachment #833339 -
Flags: review?(gal)
Comment 59•11 years ago
|
||
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•11 years ago
|
Comment 60•11 years ago
|
||
(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•11 years ago
|
||
I tested with a debug setting 'Show frames per second' that shows fps when the sliding to unlock is animated.
Comment 62•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•