894773 - Don't force layers on single opacity change

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

I've been noticing while browsing with layer border that we hit a lot of opacity changes on the web but most are low frequency opacity changes on mouse over. Quite often we create and destroy a layer 0.2 sec apart so it appears to always be worse.

The worse offender is the tab strip favicon.

We've discussed only setting an active layer if the mutation is done from inside rAF but I think this is better since we can still get opacity animations from setTimeout is as a response to DOM events.

Comment 1

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

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

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

Patch looks good but I'm afraid it will wreak havoc with reftests that make an opacity change (or transform change) to force layers to be active...

Comment 2

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=429fac58050a

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f36094bf6f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This made layout/reftests/bugs/650228-1.html start passing on Android. Failing annotation removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d7a1d8f9e4

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93f36094bf6f
https://hg.mozilla.org/mozilla-central/rev/c5d7a1d8f9e4

Comment 7

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

I wonder if we should back this out on Aurora. It has an obvious bug, but even if we fix that on Aurora, this still hurts scripted animation performance a little bit by delaying layerization. The heuristics in bug 911889 will hopefully fix that, but it's not worth pulling bug 911889 onto Aurora wholesale.

Comment 8

[Benoit Girard (:BenWa)]

Do you want to pull this out of aurora because we create the layer on the second frame? Or because you think it's worse to delay opacity by an extra frame?

In either case I think we're still better with my patch then not from my experience having ran with layer borders but if we have better patches coming down the pipes in central then it's fine, I'm not a fan of flip-flopping behavior across releases.

Comment 9

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

(In reply to Benoit Girard (:BenWa) from comment #8)
> Do you want to pull this out of aurora because we create the layer on the
> second frame? Or because you think it's worse to delay opacity by an extra
> frame?

We create the active layer on the second frame instead of the first, which means we typically do 2 ThebesLayer repaints of the instead of 1 at the start of an animation.

> In either case I think we're still better with my patch then not from my
> experience having ran with layer borders but if we have better patches
> coming down the pipes in central then it's fine, I'm not a fan of
> flip-flopping behavior across releases.

OK then I think we should take this out of Aurora to reduce the number of behavior changes.

Comment 10

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

Attachment: backout on Aurora

Comment 11

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

Comment on attachment 799799 [details] [diff] [review]
backout on Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this bug
User impact if declined: sluggish start of jQuery animations
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): just a backout
String or IDL/UUID changes made by this patch: none

Comment 12

[Alex Keybl [:akeybl]]

Please nominate bug 911889 for tracking if you believe it is important enough to remember for 26.

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8cbb2d018c67

Comment 14

[Benoit Girard (:BenWa)]

This is still an issue. I see a backout changeset for aurora but I don't see anything for central. The code isn't in central either.

You claimed that bug 894773 would fix this but I still see the issue. STR is mouse over the tab favicon.

Comment 15

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

Attachment: testcase

I'm pretty sure it is fixed in simple cases. In this test, turn on paint flashing and click on the DIV. It changes color exactly once.

Comment 16

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

I also see this when I turn on chrome flashing and mouse over the favicon. It changes color when the mouse moves over it, and when the mouse moves off, and at no other time.

Please confirm, or tell me what you see instead.

Comment 17

[Benoit Girard (:BenWa)]

Looks fix to me now. Thanks!