Closed Bug 1728258 Opened 2 months ago Closed 1 month ago

Avoid needing AnimatedGeometryRoot for RDL

Categories

(Core :: Web Painting, task)

task

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox94 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

RDL is currently the only consumer of AnimatedGeometryRoot, so it would be nice to be able to remove this, and save space within nsDisplayItem.

It uses the 'async' subset of AGRs to identify frames that might be moved on the compositor, and thus we can't reason about intersections beyond those boundaries.

During processing of the mutated frames, it uses nsDisplayListBuilder::FindAnimatedGeometryRootFor, which walks up the frame tree to the nearest AGR. We could replace this with a version that just returns the frame matching the AGR criteria.

During processing of the old display list, it grabs the AGR from the display item directly. We could possibly do the frame tree walk instead here, but the display list is assumed to be large, so this might be noticeably slower than what we currently have.

We do have ASRs on the display item, and during preprocessing we should have recursed through the display items for any transforms or position:sticky, which covers all the async-AGR cases.

If we can identify which of the ASR/nearest-'AGR'-container is the innermost, then I think we can use that to find the AGR-frame that we need.

Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16ae9f5b4aa0
Compute AnimatedGeometryRoot frames in RetainedDisplayListBuilder without needing the existing object stored on nsDisplayItem. r=miko,mstange

Backed out 7 changesets (Bug 1728050, Bug 1728251, Bug 1728232, Bug 1542929, Bug 1714138, Bug 1728258) for causing reftest failures.
Backout link
Push with failures R8
Also on multiple platforms - Rs6 R5
Failure Log

Flags: needinfo?(matt.woodrow)
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82b98d2f0dcf
Compute AnimatedGeometryRoot frames in RetainedDisplayListBuilder without needing the existing object stored on nsDisplayItem. r=miko,mstange
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 94 Branch → ---

This change made the assumption that recursing into the display list also always recursed into descendant ASRs for the item's GetActiveScrolledRoot.

That fails in this case, as we recurse through an nsDisplayStickyPosition that has a valid ASR, and then a child item (also nsDisplayStickyPosition) has the null/root ASR.

This doesn't happen with a full display list build, only when we do a partial update to a build using RDL and then recompute the container ASRS. As far as I am aware, the assumption is still valid for full builds, and this is just an existing RDL bug.

It seems like there's quite a lot of complex logic for picking the container ASR within BuildDisplayListForStackingContext (with different logic for each type of container item), and often we store multiple ASRs for each item.

The RDL logic for recomputing container ASRs after changes within the child list is a lot more simple, and it seems very likely that we're missing a lot of cases here :( The logic needs to be somewhat different, since we're working using a set of items, rather than recursing through frames. I'm worried that getting the equivalent logic fully correct, and tested is going to be tough.

Adding a null check to avoid crashing here and letting this patch land seems simple, but doesn't help with the underlying issue. Given that we're not aware of any user-visible behaviour issues with this bug, maybe that's ok for now.

Markus, any ideas or thoughts on what we should do here?

Miko, will your proposed changes to retain items instead of merging let us avoid this issue?

Flags: needinfo?(mstange.moz)
Flags: needinfo?(mikokm)
Flags: needinfo?(matt.woodrow)

Given that we're not aware of any breakage, I'm fine with adding a null check and filing a follow-up bug to simplify ASR selection for container display items.

With FLB gone, we probably need to take another close look at the guarantees we want from ASRs on container items. The goal of the current code was that calling GetBounds on a container item always returns a meaningful rectangle with respect to that item's ASR. And that this rectangle remains correct even if the contained items are scrolled asynchronously.
But maybe we won't have any GetBounds callers anymore, or at least no callers that need to anticipate async scrolling. And maybe WR does not need this guarantee either... specifically, the guarantee that stacking contexts never have contents which refer to ancestor spatial nodes, unless those contents are clipped to a rectangle that's attached to the stacking context's spatial node.

Flags: needinfo?(mstange.moz)
See Also: → 1730826
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14f05ffa9f78
Compute AnimatedGeometryRoot frames in RetainedDisplayListBuilder without needing the existing object stored on nsDisplayItem. r=miko,mstange
Status: REOPENED → RESOLVED
Closed: 1 month ago1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)

Miko, will your proposed changes to retain items instead of merging let us avoid this issue?

It might. As long as the reused container items and the items inside of them have the right ASRs set, it should be a matter of updating the current container ASR during display list building so that the new items (especially the container item wrapping the reused items) will get the correct ASR.

Flags: needinfo?(mikokm)
You need to log in before you can comment on or make changes to this bug.