Closed Bug 1211654 Opened 4 years ago Closed 4 years ago

Adjusting "left" on an abspos element inside of "opacity:0" containing block invalidates the whole containing block

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
e10s ? ---
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: dholbert, Assigned: mattwoodrow)

References

Details

Attachments

(3 files)

STR:
 1. Turn on paint flashing.
 2. See how much gets repainted every second.

EXPECTED RESULTS:
Nothing should get repainted, because the only thing changing (every second) is the 'left' position of something that's inside of an "opacity:0" parent.

ACTUAL RESULTS:
The whole opacity:0 parent gets repainted every second.

I think this is because setting "opacity" turns the parent into a layer, and when we adjust the child, we have to update the layer. But really, since the layer has opacity:0, I'd expect we shouldn't have to repaint it at all...  (Or we could do some repainting offscreen if needed, but we don't need to recomposite the updated layer on-screen.)
(Forgot to mention STR step 1.5: "load attached testcase". :) )

mattwoodrow, any chance you could take a look here? (or suggest someone w/ layers knowledge who you think might be able to look into it).  I think this is causing bug 1210899, a perf issue with a new video-controls overlay (hidden w/ opacity:0 except when hovered) at twitch.tv.
Blocks: 1210899
Flags: needinfo?(matt.woodrow)
Attachment #8669919 - Attachment description: testcase 1 → testcase 1 (view with about:config pref nglayout.debug.paint_flashing = true)
This second testcase tweaks "background" (which is clearly a repaint-only change) on the child, instead of "left".

This causes has a smaller region of paint-flashing, as compared to the previous testcase -- only the child's rect gets invalidated, not the full container. But even this small amount of repainting seems like something we should optimize away, given that the child is inside of an "opacity:0" thing.

(I believe this is a simplified version of the "Smallish issues" mentioned in bug 1210899 comment 7.  And testcase 1 here is the "Largish issue".)
Attachment #8669919 - Attachment description: testcase 1 (view with about:config pref nglayout.debug.paint_flashing = true) → testcase 1: tweaking 'left' (view with about:config pref nglayout.debug.paint_flashing = true)
Attachment #8669944 - Attachment description: testcase 2: paint-only change (view with about:config pref nglayout.debug.paint_flashing = true) → testcase 2: tweaking 'background' (view with about:config pref nglayout.debug.paint_flashing = true)
I can reproduce this only nightly (where we have e10s enabled), but not on release.

Adjusting the 'left' value of the element makes us create an active layer for that content, which in turn promotes any ancestor layers (the opacity:0 one) to be active too.

We have a timeout (250ms iirc) for this activity, and if no further changes have been made, we'll trigger another paint to flatten everything back together again.

So each time we change the value of 'left' we're getting causing two paints to be triggered, and each of these paints the entire area of the opacity (either removing it from the background when it moves into its own layer, or adding it back in).

We'd normally skip all of this for opacity:0, but with e10s enabled we keep building all of these things so that the compositor has the correct hit-testing information for content within the opacity.
Flags: needinfo?(matt.woodrow)
I think ideally we'd just not create display items when within an opacity:0 subtree (or discard them when we're done), and just accumulate event regions instead.

That doesn't work if any of the child frames establishes an AGR though (which is what happens here), since then we'd get a new event regions item in the display item set we want to discard. We could probably fix this case by detecting that we're in opacity:0 and overriding the activity, but transforms would be much harder (since they are a reference frame too).

I guess we could manually discard display items except for event regions items (and container items containing event regions), but that sounds overly complex.

We could instead just force nsDisplayOpacity::GetLayerState to return LAYER_INACTIVE (and override the requests of its children) in the case where it exists purely to handle event regions. We'd still get these layers, but not the invalidations.

What do you think Markus?
Flags: needinfo?(mstange)
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> We could instead just force nsDisplayOpacity::GetLayerState to return
> LAYER_INACTIVE (and override the requests of its children) in the case where
> it exists purely to handle event regions. We'd still get these layers, but
> not the invalidations.

That sounds good to me.
I agree, sounds good.
Flags: needinfo?(mstange)
Attached patch inactive-opacitySplinter Review
Assignee: nobody → matt.woodrow
Attachment #8670590 - Flags: review?(mstange)
Attachment #8670590 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/9935002f3e69
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Matt, is this fix safe enough to uplift to Aurora 43? This fix may improve Twitch.tv's Flash video frame rate, which regressed when Twitch launched their new Flash video player (with an HTML control overlay).
Flags: needinfo?(matt.woodrow)
Comment on attachment 8670590 [details] [diff] [review]
inactive-opacity

Approval Request Comment
[Feature/regressing bug #]: Not a regression.
[User impact if declined]: Poor framerate on twitch.tv
[Describe test coverage new/current, TreeHerder]: Manually tested.
[Risks and why]: Simple over-invalidation fix, should be very low risk.
[String/UUID change made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8670590 - Flags: approval-mozilla-aurora?
Marking 43 as affected.   
Does this only affect 43 if e10s is enabled? 
If so, we can still uplift this, but it will only help in 43 for another couple of weeks. 
Also nominating this bug for e10s tracking/triage.
tracking-e10s: --- → ?
Flags: needinfo?(matt.woodrow)
Yes, it's e10s specific.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8670590 [details] [diff] [review]
inactive-opacity

OK to uplift, let's see if this improves performance for affected sites
Attachment #8670590 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.