Closed Bug 790508 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: dzbarsky)

References

Details

Attachments

(4 files, 2 obsolete files)

Press the big red button
369 bytes, patch
Details | Diff | Splinter Review
Stack trace for nsDisplayOpacity creation
21.43 KB, text/plain
Details
Avoid creating extra nsDisplayOpacity and nsDisplayTransform
1.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
Patch
1.18 KB, patch
cjones
: review+
Details | Diff | Splinter Review
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.
Is this being handle altogether with bug 788409?
That's most likely a separate bug. This is just the performance problem.
This animation is not being async-ified either, and the logging doesn't report that.
Could someone point me to the CSS for this animation?
(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
Hmmm, it may have to do with the :not pseudo in the CSS rule. I'll try to look at this tonight.
So this is something you think is escaping the animation logging?
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.
(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]
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.
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.
Attached patch Patch (obsolete) — — Splinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #663612 - Flags: review?(matt.woodrow)
Attached patch Patch (obsolete) — — Splinter Review
Attachment #663612 - Attachment is obsolete: true
Attachment #663612 - Flags: review?(matt.woodrow)
Attachment #663624 - Flags: review?(matt.woodrow)
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.
Attachment #663624 - Flags: review?(matt.woodrow) → review?(roc)
I tried this out to see if it helped, but doesn't. Are asyncifying perspective? Do its frames get counted as active+prerendered?
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 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.
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.
(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.)
(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?
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.
But what I was trying to ask is, irrespective of the work here, do we optimize 3d transforms appropriately wrt async animations?
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.
(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.
(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.)
(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.
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?
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?
No, but I will try reducing the testcase I have.
If you can get a stacktrace for the creation of the nsDisplayOpacity while painting, that'll probably be enough data.
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".
Yup, good catch. Don't we also need the same check on IsTransformed to avoid creating extra nsDisplayTransforms?
blocking-basecamp: --- → ?
(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!
blocking-basecamp: ? → +
Whiteboard: [leave open]
Attachment #663624 - Attachment is obsolete: true
Attachment #663624 - Flags: review?(roc)
Attachment #665039 - Flags: review?(roc)
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.
Attachment #665039 - Flags: review?(roc) → review+
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
And a bustage fix because I screwed up the merge: https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb4e8d008c0
Attached patch Patch — — Splinter Review
Attachment #666441 - Flags: review?(jones.chris.g)
Attachment #666441 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/mozilla-central/rev/02ba33fdf2d2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: