Closed Bug 894773 Opened 6 years ago Closed 6 years ago

Don't force layers on single opacity change

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed

People

(Reporter: BenWa, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #776908 - Flags: review?(roc)
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...
Attachment #776908 - Flags: review?(roc) → review+
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Attachment #776908 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #777388 - Flags: review+
Attached patch patchSplinter Review
https://hg.mozilla.org/integration/mozilla-inbound/rev/93f36094bf6f
Attachment #777388 - Attachment is obsolete: true
Attachment #785065 - Flags: review+
Attachment #785065 - Flags: checkin+
This made layout/reftests/bugs/650228-1.html start passing on Android. Failing annotation removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d7a1d8f9e4
https://hg.mozilla.org/mozilla-central/rev/93f36094bf6f
https://hg.mozilla.org/mozilla-central/rev/c5d7a1d8f9e4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
(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.
Attachment #799799 - Flags: review?(bgirard) → review+
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
Attachment #799799 - Flags: approval-mozilla-aurora?
Attachment #799799 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please nominate bug 911889 for tracking if you believe it is important enough to remember for 26.
Target Milestone: mozilla26 → mozilla25
Depends on: 912769
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(roc)
Resolution: FIXED → ---
Attached file 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.
Flags: needinfo?(roc)
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.
Looks fix to me now. Thanks!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.