Various improvements towards correct handling of 3d transforms. #2995

gw3583 · 2018-08-29T09:44:25Z

Pictures only calculate a local rect if they are not a pass-through primitive (i.e. if they have a surface).
Change batching code to use world rects. This avoids the need to do int conversions, and avoids the need to offset each bounding rect.
Change prim metadata to store world rects in floats, rather than integer screen rects. This integrates better with the plane splitting code.
Change clipping code to project to world space for complex clips.
Introduce SpaceMapper, a struct for efficiently converting between arbitrary coordinate systems.
Introduce Picture coordinate space, which is used to hold the local rect for a given Picture which has a render surface.
Prim metadata now only stores the clipped world rect. Most of the time the unclipped world rect isn't needed. It's only needed for Picture primitives that are drawing to a surface - so we calculate it on demand in Picture::prepare_for_render().

The major change here is that pictures now only calculate a local rect if they have a surface. Previously, all pictures tried to calculate a local rect. This caused problems because we were trying to map rects with partial transforms, and losing information as we then only considered the 2d projection within each picture. Now, we only calculate a local rect for pictures that actually have a surface. This fixes several reftests.

This change is

gw3583 · 2018-08-29T09:46:01Z

r? @kvark

This is, I hope, the last major change we need in order to be able to rasterize pictures in different coordinate spaces. This will make caching feasible for all picture surface types, and make it possible to fix the remaining 3d transform issues. Apologies for the size of the patch!

Try run is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d23a9348cbf9a08fadb700a34c85dbe4c28bec6e

There's a lot of orange items, some discussion below:

Unrelated intermittents:
ts_paint_heavy
tp6_google_heavy
nav2_test_document_open.html
update.https.html

FAIL -> PASS
mask-layer-3.html
1081185-1.html
transform3d-preserve3d-011.html
object-position-svg-001o.html (not sure if caused by the existing intermittent PASS, or now permanent)

PASS -> FAIL
img-novb-height-meet-1.html and img-novb-height-slice-1.html - these are already marked as fuzzy for WR and skiaContent. I think it's probably fine to increase the fuzz here.
long-chain.html - fail on Linux only - now passes on Windows/OSX. Already has fuzziness on WR and all other platforms. I think it's fine to increase the fuzziness here.
async-scrolling/perspective-scrolling-1.html and async-scrolling/perspective-scrolling-4.html - discussion below.

Most of the new failures are fine to fuzz, I think. I'm still not 100% sure what is causing the perspective-scrolling failure. I think it is actually a race condition in Gecko or the async scene code in WR, but I'm not certain yet. What leads me to think this is:

I changed the reference image to also include preserve-3d and the tests still fail.
That means the only difference between the tests is that one sets scrollTop via JS, and one sets async-scroll reftest property.
I logged out the state of the clip-scroll tree for each test when the screenshot is taken:

Update RF SpatialNodeIndex(0) [I]
Update other SpatialNodeIndex(1) (0.0,0.0)
Update RF SpatialNodeIndex(2) [I]
Update other SpatialNodeIndex(3) (0.0,0.0)
Update other SpatialNodeIndex(4) (0.0,0.0)
Update RF SpatialNodeIndex(5) [1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 24.00621, 0.0, 1.0]
Update other SpatialNodeIndex(6) (0.0,-100.0)
Update RF SpatialNodeIndex(7) [1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, -400.0, -400.0, 1.0, -1.0, 100.0, 100.0, 0.0, 1.0]

Update RF SpatialNodeIndex(0) [I]
Update other SpatialNodeIndex(1) (0.0,0.0)
Update RF SpatialNodeIndex(2) [I]
Update other SpatialNodeIndex(3) (0.0,0.0)
Update other SpatialNodeIndex(4) (0.0,0.0)
Update RF SpatialNodeIndex(5) [I]
Update other SpatialNodeIndex(6) (0.0,0.0)
Update RF SpatialNodeIndex(7) [1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, -400.0, -500.0, 1.0, -1.0, 100.0, 100.0, 0.0, 1.0]

As you can see from above, the incoming offset in spatial node 6 is different between the tests - one has an offset while one doesn't. I think this explains why the test fails, and suggests the problem lies earlier than the changes in this patch could cause. But I want to do a bit more investigation first to try and confirm what is going on here.

gw3583 · 2018-08-29T09:47:02Z

(TL;DR of the above is: apart from review comments / changes, I need to do a little bit more investigation of a couple of the try results, to either confirm that they are unrelated to this patch, or fix them first before landing).

gw3583 · 2018-08-30T01:01:52Z

OK, I think I have fixed the perspective-scrolling-1.html test. Kicking off a try run shortly.

kvark · 2018-08-30T02:22:37Z

OK, I think I have fixed the perspective-scrolling-1.html test. Kicking off a try run shortly.

wasn't a Gecko issue after all? what was it then?

gw3583 · 2018-08-30T02:28:15Z

It was an issue with not including the accumulated scroll offset when defining a new coordinate system.

Current try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf31254cd5f2e4e13d92c8a09a19dc5021df9db6&selectedJob=196555936

Still a few jobs to go, but I think it looks good now, despite all the orange items (I'll follow up with some details in a comment shortly about the differences).

gw3583 · 2018-08-30T02:36:59Z

Linux:

[1] [2] [mda1] [h1] [h2] [tp6] [mda2] [wpt3] unrelated failures

[R1] New PASS in mask-layer-3.html
[R4] New PASS in blur-cap-large-radius-on-software.html
[R8] New PASS in 1081185-1.html
[Wr1] New PASS in transform3d-preserve3d-011.html
[R2] New FAIL in img-novb-height-meet-1.html and img-novb-height-slice-1.html [see below]
[R7] New FAIL in long-chain.html [see below]

OSX:

[R1] New PASS in 1081185-1.html
[R2] New PASS in long-chain.html and blur-cap-large-radius-on-software.html

Windows:
[not completed yet - I believe they will be the same differences as above, but will confirm]

Discussion:

So the failures are in img-novb-height-meet-1.html, img-novb-height-slice-1.html and long-chain.html (on Linux). In each of these cases the results are very close, and already have fuzziness on WR and/or other backends. With signoff from @jrmuizel, I think these are fine to fuzz, which just leaves new PASS results.

kvark

I find this to be an amazing set of changes. It simplifies a bunch of code (like content origin offsetting, some conversions to device space), and at the same time makes our type system stronger by introducing the picture coordinate space. Got a few notes below:

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @gw3583)

webrender/src/clip.rs, line 327 at r1 (raw file):

    pub has_non_root_coord_system: bool,
    pub has_non_local_clips: bool,
    pub pic_clip_rect: PictureRect,

not clear (from reading this only) why the struct has 2 clip rects and what is the difference for the user

webrender/src/clip.rs, line 418 at r1 (raw file):

        local_prim_clip_rect: LayoutRect,
        spatial_node_index: SpatialNodeIndex,
        prim_to_pic_mapper: &SpaceMapper<LayoutPixel, PicturePixel>,

I wonder how you resisted the temptation to call it SpaceWarp ;)

webrender/src/clip.rs, line 484 at r1 (raw file):

                            }
                            ClipSpaceConversion::Transform(..) => {
                                // TODO(gw): In the future, we can reduce the size

so this is a possible performance regression?

webrender/src/frame_builder.rs, line 75 at r1 (raw file):

    pub pipelines: &'a FastHashMap<PipelineId, Arc<ScenePipeline>>,
    pub screen_rect: DeviceIntRect,
    pub world_rect: WorldRect,

is screen_rect still useful, given that we have world_rect here and a ton of things is now expected to work with world coordinates?

webrender/src/frame_builder.rs, line 105 at r1 (raw file):

    pub local_rect_changed: bool,
    pub map_local_to_pic: SpaceMapper<LayoutPixel, PicturePixel>,
    pub map_pic_to_world: SpaceMapper<PicturePixel, WorldPixel>,

nit: this particular field doesn't seems like it should be a part of PictureState, given that it's not going to change upon traversing the items of the picture. Is there a more appropriate home for it to live?

webrender/src/picture.rs, line 282 at r1 (raw file):

        match local_rect {
            Some(local_rect) => {
                let local_content_rect = LayoutRect::from_untyped(&local_rect.to_untyped());

could we avoid going through untyped, e.g. by scaling with ScaleFactor? it's pretty much like mem::transmute in math terms :)

webrender/src/picture.rs, line 499 at r1 (raw file):

                let device_rect = clipped
                    .inflate(blur_range, blur_range)
                    .intersection(&unclipped.to_i32())

is it ever needed as non-i32?

webrender/src/prim_store.rs, line 170 at r1 (raw file):

            }
            CoordinateSpaceMapping::Offset(ref offset) => {
                Some(TypedRect::from_untyped(&rect.translate(offset).to_untyped()))

similarly, would be great to avoid going through "untyped"

webrender/src/prim_store.rs, line 527 at r1 (raw file):

    }

    pub fn may_need_clip_mask(&self) -> bool {

IIRC, we wanted to rename this one?

webrender/src/prim_store.rs, line 1737 at r1 (raw file):

            };

            let clipped_world_rect = match world_rect.intersection(&frame_context.world_rect) {

the pattern of those two matches is repeated in other places, we should make a helper with a descriptive name

webrender/src/prim_store.rs, line 2266 at r1 (raw file):

    }

    fn reset_clip_task(&mut self) -> bool {

please document what the return value means here

webrender/src/prim_store.rs, line 2744 at r1 (raw file):

        }
        // Reset clips from previous frames since we may clip differently each frame.
        if !self.reset_clip_task() {

so if there wasn't anything in the previous frame means we aren't going to update anything this frame? Wouldn't this cause us to miss some clips that activate only upon scrolling and such?

webrender/src/util.rs, line 465 at r1 (raw file):

        clipper.add_frustum(
            transform,
            None,

hmm, so this is no longer taking the screen bounds into account, which means produced polygons will be guaranteed to stretch to infinity if they intersect the near plane. Feeding that data to GPU then could cause us severe precision issues, and I'd prefer us to not torture it too much :)

jrmuizel · 2018-08-30T03:23:39Z

I'm fine with bumping the fuzz on those tests.

gw3583

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @gw3583 and @kvark)

webrender/src/clip.rs, line 327 at r1 (raw file):