1524797 - http://impress.js.org/ text is in the wrong spots (3d transforms)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

https://impress.js.org/#/one-more-thing

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Somewhat reduced test case

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: More reduced test case

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Dzmitry, any guesses as to what's going wrong here?

Comment 4

[Dzmitry Malyshau [:kvark]]

I'll take a look

Comment 5

[Dzmitry Malyshau [:kvark]]

Attachment: scene-1-0-reduced.ron

Reduced scene RON. You can see how the preserve-3d gets mixed with Flat (with some filters) on the way to the root.

Comment 6

[Dzmitry Malyshau [:kvark]]

The problem here appears to be rather wide, and its related to flattening of stacking contexts. Flattening is when we drop the Z coordinate of the transformed primitive along the way to the root. It's supposed to be done at the root of each preserve-3D sub-hierarchy as well as for each stacking context outside of a preserve-3D hierarchy. We do the latter, and it's fairly straightforward, but we aren't doing the former properly. As a result, the transform from root to surface (that we compute for every surface!) is incorrect if there is any flatten root on the way. In fact, a transform would only make sense in either within the context of a flatten root (i.e. within preserve-3d hierarchy), or out of flatten roots if we compute it by evaluating the flattened transforms (as a whole) on the way to the actual root of the tree... We aren't doing this now, and implementing this appears to be a complex engineering task.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Hmm, why aren't we doing the former? I think this should work, Gecko wraps content with transform separators already when this happens:

https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/layout/generic/nsFrame.cpp#2566

I guess we're incorrectly avoiding creating a reference frame for the separators. Does changing this condition:

https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/layout/painting/nsDisplayList.cpp#7897

To:

  if (newTransformMatrix.IsIdentity() && !mIsTransformSeparator) {

Fix this?

Comment 8

[Dzmitry Malyshau [:kvark]]

Thanks for chiming in, Emilio!
That suggested change doesn't fix the issue. In general, I don't think inserting any transform separators into the tree would address this. Whenever WR code does this (and we do this in many places), we (theoretically) fail:

        let map_surface_to_world = SpaceMapper::new_with_target(
            ROOT_SPATIAL_NODE_INDEX,
            surface.surface_spatial_node_index,
            frame_context.screen_world_rect,
            frame_context.clip_scroll_tree,
        );

We fail because the clip-scroll tree doesn't know about those preserve-3d enclaves that need to be flattened. It appears to me that we'll have to change (read: re-design) the very concept of the clip-scroll tree in order to handle that.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ah, so this is the "clip-scroll-tree not being aware of flattening" issue I was discussing with Glenn not that long ago:

https://mozilla.logbot.info/gfx/20190121#c15861304

Seems like introducing a new coordinate space for the children of a node when we flatten something (that produces a different transform) and keeping track of that on the parent spatial node should be possible? But ICBW.

Comment 10

[Dzmitry Malyshau [:kvark]]

Attachment: WR: rework the relative transform query on a clip-scroll tree

This change rewords get_relative_transform and assotiated pieces of logic,
so that we flatten the transforms at preserve-3d context boundaries.

It addresses a problem found by 1524797 but doesn't resolve the bug yet (!).
There is another issue likely contributing here, and we can treat this PR
as WIP and not merge until the case is completely resolved.

Comment 11

[Dzmitry Malyshau [:kvark]]

Attachment: Enable non-screen rasterization of the plane splits

We used to hard-code the raster spatial node of plane splits to the root.
Now we are using the actual root during batching.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a856e1763485
Enable non-screen rasterization of the plane splits r=gw

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a856e1763485

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87dac4d84c6b
WR: rework the relative transform query on a clip-scroll tree r=gw

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/87dac4d84c6b