790508 - Transition from Gaia lock screen --> home screen is <5 fps

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

This is pretty much the first thing any new user of b2g will see, so we need to make sure we make a good impression.  A <5fps animation is not a good impression! :)

I haven't analyzed this in any detail yet, but it might be related to the slow composition path Kan-Ru found we're hitting for the lock screen in bug 788408.

Comment 1

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

Is this being handle altogether with bug 788409?

Comment 2

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

That's most likely a separate bug.  This is just the performance problem.

Comment 3

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

This animation is not being async-ified either, and the logging doesn't report that.

Comment 4

[David Zbarsky (:dzbarsky)]

Could someone point me to the CSS for this animation?

Comment 5

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

(In reply to David Zbarsky (:dzbarsky) from comment #4)
> Could someone point me to the CSS for this animation?

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/lockscreen/lockscreen.css#L25

Comment 6

[David Zbarsky (:dzbarsky)]

Hmmm, it may have to do with the :not pseudo in the CSS rule.  I'll try to look at this tonight.

Comment 7

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

So this is something you think is escaping the animation logging?

Comment 8

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

Aha!

I/Gecko   ( 7933): Performance warning: Async animation disabled because frame was not marked active for opacity animation [div with id 'lockscreen'][ANIMATIONS LOG]


Must have missed this somehow, before.

Comment 9

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

(In reply to David Zbarsky (:dzbarsky) from comment #6)
> Hmmm, it may have to do with the :not pseudo in the CSS rule.  I'll try to
> look at this tonight.

Removing that in favor of an "unlocked" class doesn't change anything, although I do see more warnings

I/Gecko   (23144): Performance warning: Async animation disabled because frame was not marked active for opacity animation [span][ANIMATIONS LOG]
I/Gecko   (23144): Performance warning: Async animation disabled because frame was not marked active for transform animation [div with id 'lockscreen-area-handle'][ANIMATIONS LOG]
I/Gecko   (23144): Performance warning: Async animation disabled because frame was not marked active for opacity animation [div with id 'lockscreen'][ANIMATIONS LOG]
I/Gecko   (23144): Performance warning: Async animation disabled because frame was not marked active for opacity animation [div with id 'lockscreen'][ANIMATIONS LOG]

Comment 10

[David Zbarsky (:dzbarsky)]

Ah! I bet I know what may be happening here...
The transition has a delay, so we "run" it on the main thread for the first 0.3 seconds, but then the main thread is blocked so we don't get a chance to offload it to the compositor.

Comment 11

[David Zbarsky (:dzbarsky)]

Nope, that's not the issue.
I'm not quite sure what's happening here.  I think some of the async animation warnings are due to opacity transitions on descendants of #lockscreen.

I did notice that if I disable the opacity animation, the scale transform is at 60 fps.

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 663624 [details] [diff] [review]
Patch

I don't know the style change -> frames code well enough to review this sorry.

I think this is correct, and we only call MarkLayersActive on the primary frame for the content, but roc (or someone else who knows this code) should confirm this.

Comment 15

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

Attachment: Press the big red button

I tried this out to see if it helped, but doesn't.  Are asyncifying perspective?  Do its frames get counted as active+prerendered?

Comment 16

[David Zbarsky (:dzbarsky)]

How would that help?  We don't do anything special for perspective (but we should probably count perspective and perspective-origin as geometric properties and disable transform animations when animating perspective).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 663624 [details] [diff] [review]
Patch

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

Can you explain why this is necessary? It's not obvious to me.

Comment 18

[David Zbarsky (:dzbarsky)]

When we call MarkLayersActive, we always call it on an element's primary frame.  I'm hitting a case where I set an element active for opacity and transform animations, but the nsDisplayOpacity ends up with a different underlying frame, so we can't perform the animation on the compositor.

Comment 19

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

(In reply to David Zbarsky (:dzbarsky) from comment #16)
> How would that help?  We don't do anything special for perspective (but we
> should probably count perspective and perspective-origin as geometric
> properties and disable transform animations when animating perspective).

3D transforms force the entire frame to be prerendered.  They should always count as prerendered wrt async animations.

(The fact that we do this doesn't make me very happy, but it's extremely hard to do anything else, and it matches webkit behavior.  Having an escape hatch for authors to make their animations fast is also a good thing.)

Comment 20

[David Zbarsky (:dzbarsky)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #19)
> (In reply to David Zbarsky (:dzbarsky) from comment #16)
> > How would that help?  We don't do anything special for perspective (but we
> > should probably count perspective and perspective-origin as geometric
> > properties and disable transform animations when animating perspective).
> 
> 3D transforms force the entire frame to be prerendered.  They should always
> count as prerendered wrt async animations.
> 
> (The fact that we do this doesn't make me very happy, but it's extremely
> hard to do anything else, and it matches webkit behavior.  Having an escape
> hatch for authors to make their animations fast is also a good thing.)

With the patch in this bug the frames already end up prerendered.  Is it possible that the the compositor isn't able to hit 60 fps?

Comment 21

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

Possible, but unlikely.  If we're creating a bajillion intermediate fbos that could hurt.  But like we discussed on IRC, I don't really see a difference on otoro with this patch vs. without.

Comment 22

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

But what I was trying to ask is, irrespective of the work here, do we optimize 3d transforms appropriately wrt async animations?

Comment 23

[David Zbarsky (:dzbarsky)]

Chris, what do you see on otoro if you comment out the opacity transition? 
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/lockscreen/lockscreen.css#L28

We don't do anything differently for 3d transforms than for 2d.  We already force own layer for async transform, which is the same thing we do for 3d transforms.  I'm not sure I understand what you're asking.

Comment 24

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

(In reply to David Zbarsky (:dzbarsky) from comment #23)
> Chris, what do you see on otoro if you comment out the opacity transition? 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/lockscreen/
> lockscreen.css#L28
> 

Will check later.

> We don't do anything differently for 3d transforms than for 2d.  We already
> force own layer for async transform, which is the same thing we do for 3d
> transforms.  I'm not sure I understand what you're asking.

My understanding from Matt is that we always fully pre-render frames that have 3d transforms.  However, ShouldPrerender() (the version I wrote) will return false for frames that are larger than the screen size.  I'm checking to see if we would still asynchronously animate a frame that's fully prerendered because it's 3d-transformed, even if ShouldPrerender() says "false".

I may have misunderstood Matt, though.

Comment 25

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

(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> (In reply to David Zbarsky (:dzbarsky) from comment #23)
> > Chris, what do you see on otoro if you comment out the opacity transition? 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/lockscreen/
> > lockscreen.css#L28
> > 
> 
> Will check later.
> 

~60 fps.  (Without your patch applied.)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Zbarsky (:dzbarsky) from comment #18)
> When we call MarkLayersActive, we always call it on an element's primary
> frame.  I'm hitting a case where I set an element active for opacity and
> transform animations, but the nsDisplayOpacity ends up with a different
> underlying frame, so we can't perform the animation on the compositor.

Why are we creating nsDisplayOpacity for a different underlying frame? Can you say more about this situation? It sounds weird/wrong.

Comment 27

[David Zbarsky (:dzbarsky)]

Setting a breakpoint at nsDisplayList.cpp:350, (where the CanUseAsyncAnimations check fails), I see the following:
(gdb) p aItem
$6 = (nsDisplayOpacity *) 0x10b1e9028
(gdb) p aItem->GetUnderlyingFrame()
$7 = (nsBlockFrame *) 0x10b746ea0
(gdb) p aItem->GetUnderlyingFrame()->GetContent()
$8 = (nsHTMLDivElement *) 0x12eb69cb0
(gdb) p aItem->GetUnderlyingFrame()->GetContent()->GetPrimaryFrame()
$9 = (nsHTMLScrollFrame *) 0x10b746be0

I'm not really sure why this happens - perhaps when we need a scrollframe we wrap the opacity around it instead of putting it inside?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I would expect the nsDisplayOpacity to be on the scrollframe, not the scrolled blockframe. Your debug output suggests it's on the blockframe.

Have you got a small testcase for this?

Comment 29

[David Zbarsky (:dzbarsky)]

No, but I will try reducing the testcase I have.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you can get a stacktrace for the creation of the nsDisplayOpacity while painting, that'll probably be enough data.

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: Stack trace for nsDisplayOpacity creation

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Aha, I think I know what the problem is! It seems nsIFrame::HasOpacity returns true for the scrolled blockframe. And that would be because "mContent && nsLayoutUtils::HasAnimationsForCompositor(mContent, eCSSProperty_opacity)" is true. Basically, *every* frame for a given element is going to return true for HasOpacity, but only the primary frame should.

So I think that check needs an additional "&& mContent->GetPrimaryFrame() == this".

Comment 33

[David Zbarsky (:dzbarsky)]

Yup, good catch.  Don't we also need the same check on IsTransformed to avoid creating extra nsDisplayTransforms?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Zbarsky (:dzbarsky) from comment #33)
> Yup, good catch.  Don't we also need the same check on IsTransformed to
> avoid creating extra nsDisplayTransforms?

Yes!

Comment 35

[David Zbarsky (:dzbarsky)]

Attachment: Avoid creating extra nsDisplayOpacity and nsDisplayTransform

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 665039 [details] [diff] [review]
Avoid creating extra nsDisplayOpacity and nsDisplayTransform

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

Put the GetPrimaryFrame checks last since they'll normally be true.

Comment 37

[David Zbarsky (:dzbarsky)]

I accidentally pushed this fix along with a warning fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/403ebae0552f

So I addressed roc's comment:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81e6cc2d297f

Comment 38

[David Zbarsky (:dzbarsky)]

And a bustage fix because I screwed up the merge:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb4e8d008c0

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81e6cc2d297f
https://hg.mozilla.org/mozilla-central/rev/ebb4e8d008c0

Comment 41

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 42

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02ba33fdf2d2

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02ba33fdf2d2