840752 - Make gaia master unlock transition animate asynchronously

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Relatively recent regression.

Separately, this is causing us to exhaust pmem doing some upscale.

b? -> radar

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

STR
 (1) git checkout dfc2b0d9bc828270da10eccfd30ace0c88730fad in gaia
 (2) make reset-gaia
 (3) Unlock the lock screen

The change in bug 829075 has us use a canvas for the lock screen background.  That seems to be throwing us off the async path, and additionally is causing us to create huge temporary surfaces.

So morphing this bug into the work we need to async-ify it.

Comment 2

[Nick Cameron [:nrc]]

Not much joy with this so far, sorry. I've been trying to reproduce this on desktop with some simplified test cases of transitioning canvases, but they always get asyncified just fine.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The lockscreen is most likely animating a canvas and some other junk (text etc.).  The "other junk" should be the cause of the huge temporaries.

Comment 4

[Anthony Ricaud (:rik)]

If that helps, here are the properties we transition on unlock:
    transform 0.5s ease,
    opacity 0.5s ease,
    visibility 0.5s ease;

And we go from 
  transform: scale(1);
  opacity: 1;
  visibility: visible;

to:
  transform: scale(2);
  opacity: 0;
  visibility: hidden;

Comment 5

[Nick Cameron [:nrc]]

Also from Rik, the html: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L771
the css: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/lockscreen/lockscreen.css

and the offending change: https://github.com/Rik/gaia/commit/606515c9d2cc6068ce99a7543b246703c65b3499

Comment 6

[Nick Cameron [:nrc]]

I am not getting far with this. I am having a lot of difficulty reproducing. It would be really, really useful to have a minimal test case for this. Could one the Gaia folks do that please?

So far, I have found that transitioning visibility introduces a lot of flushes and generally messes things up. This is not unexpected because visibility cannot (currently) be asynced. I would suggest not transitioning on visibility, only on opacity, and setting visibility as a non-transitioning thing. But that has not affected the asyned-ness of the actual transition so I am not sure that will fix this, it would be good to know though.

I found a bug with the transitionEnd event when we transition on both opacity and transform. But that is about not transitioning, not forcing the animation to be sync. So I don't think that will affect this either. But maybe turning off the opacity transition and seeing if that makes a difference would be useful too.

Comment 7

[Anthony Ricaud (:rik)]

I'm trying to create a testcase. How can I detect if we get out of the async path on desktop (OS X 10.8)?

Comment 8

[Nick Cameron [:nrc]]

Thanks rik! You need to set the following prefs in about:config to true:
layers.offmainthreadcomposition.animate-opacity
layers.offmainthreadcomposition.animate-transform
layers.offmainthreadcomposition.enabled
layers.offmainthreadcomposition.log-animations
layers.offmainthreadcomposition.throttle-animations

Some of those may not exist at all and some will be set to false. You will need to restart after setting these (and not just the mac-pretend-restart). (Also, I assume you are using nightly, should work in Aurora too, but probably not beta).

You should more noticeable jitteryness when we fall off the off sync path (try animating anything other than opacity, transform, visibility to see what this looks like). Note that moving the mouse at all will effectively drop us off async.

rik: are you happy to patch and rebuild FF? If so I'll post a patch that will actually tell you  when you are sync/async animating.

Comment 9

[Anthony Ricaud (:rik)]

Thanks for the details, that's very helpful. I'm happy to apply a patch that might be more reliable than my impressions on a fast machine.

Comment 10

[Nick Cameron [:nrc]]

Attachment: primitive loggin

This is a very basic bit of logging, but it is fairly reliable. You will see a mixture of 'flush' and 'throttle' printed out. You will always see some 'flush' if you see all 'flush' then we have fallen back to sync animation. If you see mostly 'throttle' with occasional 'flush' we are async. Moving the mouse should give a lot more 'flush's. If you see mostly 'flush' and the occasional 'throttle' then something weird is going on.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can we add this to the existing animation logging?

Comment 12

[Nick Cameron [:nrc]]

I think we could do much better than this. I'll have a go when I tackle this bug.

Comment 13

[Anthony Ricaud (:rik)]

From my testing with the debug, it looks like removing the visibility property altogether has better results than just removing the transition of visibility.

Also, when I test with a maximized window on my laptop, I get this message:
Performance warning: Async animation disabled because frame size (1425, 522) is bigger than the viewport (1620, 959) [div with id 'lockscreen']

Given that we do a scale(2) transform, could that also be a problem on the device?

Comment 14

[Nick Cameron [:nrc]]

(In reply to Anthony Ricaud (:rik) from comment #13)
> From my testing with the debug, it looks like removing the visibility
> property altogether has better results than just removing the transition of
> visibility.
> 
OK, visibility is a bit weird in its interaction with OMTA, so I can believe that.

> Also, when I test with a maximized window on my laptop, I get this message:
> Performance warning: Async animation disabled because frame size (1425, 522)
> is bigger than the viewport (1620, 959) [div with id 'lockscreen']
> 
> Given that we do a scale(2) transform, could that also be a problem on the
> device?

Could be, I am not sure. It should be easy to test - change b2g.js to set layers.offmainthreadcomposition.log-animations = true and view the log on the phone, the async animation warning will fire if there is a problem (if the logging doesn't work for some reason I can try to fix it).

Comment 15

[David Zbarsky (:dzbarsky)]

(In reply to Nick Cameron [:nrc] from comment #14)
> (In reply to Anthony Ricaud (:rik) from comment #13)
> > From my testing with the debug, it looks like removing the visibility
> > property altogether has better results than just removing the transition of
> > visibility.
> > 
> OK, visibility is a bit weird in its interaction with OMTA, so I can believe
> that.
> 

I wrote a patch to animate visibility with OMTA a while ago.  It's probably a little rotted, but does it help?

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Visibility patch

Comment 17

[Nick Cameron [:nrc]]

Comment on attachment 716747 [details] [diff] [review]
Visibility patch

Review of attachment 716747 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM and we should definitely and this at some point. For the immediate b2g problem, I would be much happier fixing this at the Gaia level if all it takes is to not use visibility (given the amount of bug fixing that has accompanied past OMTA adventures). Of course, if we can't do that and visibility is the problem, then this would be the solution

::: layout/base/nsDisplayList.cpp
@@ -327,5 @@
>  static void
>  AddAnimationsAndTransitionsToLayer(Layer* aLayer, nsDisplayListBuilder* aBuilder,
>                                     nsDisplayItem* aItem, nsCSSProperty aProperty)
>  {
> -  aLayer->ClearAnimations();

Why do we no longer need to clear animations?

Comment 18

[David Zbarsky (:dzbarsky)]

(In reply to Nick Cameron [:nrc] from comment #17)

> ::: layout/base/nsDisplayList.cpp
> @@ -327,5 @@
> >  static void
> >  AddAnimationsAndTransitionsToLayer(Layer* aLayer, nsDisplayListBuilder* aBuilder,
> >                                     nsDisplayItem* aItem, nsCSSProperty aProperty)
> >  {
> > -  aLayer->ClearAnimations();
> 
> Why do we no longer need to clear animations?

We can't clear when adding a single property since visiblity and opacity are on the same layer.  Instead we clear when building the layer, before calling this function.

Comment 19

[Anthony Ricaud (:rik)]

Here's what I've tried on the device. I enabled slow animations logging and confirmed it is working (we log "Async transition of pseudoelements not supported.  See bug 771367 [span]" when closing a panel in the settings app).

First, I don't see any logging when unlocking the screen with Gaia master. Then I tried removing |visibility: hidden| and see if it behaves better. I can't really tell only with my eyes.

Nick: Is it possible to add some logs for this situation?
Chris: What tool do you use to note the memory issues? Maybe I could work with the same tools to try different solutions.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can you be a bit more specific about the memory issues?  With the original implementation, you would be able to see a memory spike by running

$ watch -n 1 'adb shell b2g-procrank'

Comment 21

[Nick Cameron [:nrc]]

(In reply to Anthony Ricaud (:rik) from comment #19)
> Here's what I've tried on the device. I enabled slow animations logging and
> confirmed it is working (we log "Async transition of pseudoelements not
> supported.  See bug 771367 [span]" when closing a panel in the settings app).
> 
> First, I don't see any logging when unlocking the screen with Gaia master.
> Then I tried removing |visibility: hidden| and see if it behaves better. I
> can't really tell only with my eyes.
> 
> Nick: Is it possible to add some logs for this situation?

Yes, but what exactly is this situation? Are we still using SMIL for the unlock screen (in which case there is no way it is getting OMTAed, actually)? Or what kind of animation/transition is happening? Thanks!

Comment 22

[Anthony Ricaud (:rik)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Can you be a bit more specific about the memory issues?
I was referring to bug 829075 comment 33 where you said we have around 10mb of temporary surfaces.

> With the original implementation, you would be able to see a memory spike by running
> 
> $ watch -n 1 'adb shell b2g-procrank'
With this tool, I don't see a 10mb spike at all. At most, I see a 500k spike in the Pss or Uss fields. I have a Gecko build from last Friday (Feb 22nd).

(In reply to Nick Cameron [:nrc] from comment #21)
> Yes, but what exactly is this situation? Are we still using SMIL for the
> unlock screen (in which case there is no way it is getting OMTAed,
> actually)? Or what kind of animation/transition is happening? Thanks!
I'm still talking about the existing codebase, the one we're talking in comment 4 and comment 5. So CSS transitions on CSS transforms.

My goal here is to have a metric on my machine so that I can witness the issue first. And then with this metric, I can try alternatives that improve it.

Comment 23

[Nick Cameron [:nrc]]

Probably the easiest way to get logging on b2g is to s/printf/printf_stderr in the patch, then do the same as before and watch for the ratio of of throttles to flushes.

I can also add some better logging which does this a bit more cleanly - something like "throttled x% of the last y flushes" every now and then. Is that the kind of thing you are after? I think the existing logging catches a lot of the cases where we fall off the OMTA path, so I'm not sure what more to add for now.

Comment 24

[Julien Wajsberg [:julienw]]

Nick, David, any update on this ?

I'd like to know if we should back out 829075 from gaia master or if this bug has a chance to land at one point.

Comment 25

[David Zbarsky (:dzbarsky)]

I think we want a gaia patch for this.  A Gecko patch would probably be too risky at this point.

Comment 26

[Nick Cameron [:nrc]]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Nick, David, any update on this ?
> 
> I'd like to know if we should back out 829075 from gaia master or if this
> bug has a chance to land at one point.

Sorry, been busy with the layers refactoring, not had a chance to look at this. Without a simpler test case I don't think I'll get anywhere.

Comment 27

[Julien Wajsberg [:julienw]]

I bet Rik could try do it at one point.

Rik, setting needinfo to you but no need to rush (I guess).

Comment 28

[Nicholas Nethercote [inactive]]

If this causes any memory consumption regressions (as we saw with bug 829075) please tag with [MemShrink].

Comment 29

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

Hi everyone!

We need to start this investigation again to see if it's possible to fix this on the Gecko side for v1.2 or we need to tweak a few things in Gaia (if it's even possible).

The lock screen does not animates smoothly asynchronously.

Comment 30

[Nick Cameron [:nrc]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #29)
> Hi everyone!
> 
> We need to start this investigation again to see if it's possible to fix
> this on the Gecko side for v1.2 or we need to tweak a few things in Gaia (if
> it's even possible).
> 
> The lock screen does not animates smoothly asynchronously.

As far as I can tell, the chance of getting a low risk Gecko fix for this on the 1.2 timeframe is minimal. In fact, I would say any Gecko fix is not going to be low risk, certainly not low enough that we would want to take it for 1.2. Therefore I would think a Gaia fix is the way forward here. In any case, I am too busy with other things at the moment to take this on, sorry. I'm of course happy to keep giving advice for how to make a Gaia fix work.

Comment 31

[Preeti Raghunath(:Preeti)]

Moving to 1.3 based on comment 30

Comment 32

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

I don't know how bug 937630 is related to this one but I don't think we need to block on both of them?

Comment 33

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

These is at a different level than bug 937630 which is a lockscreen bug.  Let's keep both open.