New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support arbitrary rasterization coordinate roots. #3030

Merged
merged 5 commits into from Sep 11, 2018

Conversation

Projects
None yet
5 participants
@gw3583
Contributor

gw3583 commented Sep 6, 2018

This allows us to choose to rasterize any picture in an
arbitrary coordinate space.

This fixes a large number of real world pages that use 3d
transforms. It also simplifies fixing any remaining 3d
transform related issues.

The initial patch uses this to promote any picture that has
perspective into a rasterization root.

There are a number of benefits to this:

  • It simplifies a lot of the math involving plane splitting,
    since we don't need to deal with splits drawn in screen-space.
  • It makes caching perspective rendered pictures much easier,
    since we can cache them between frames even when the
    root coordinate system is animating the picture (this isn't
    implemented yet, but is a simple-ish follow up).

We can make use of this to render text shadows in local space,
as a performance optimization (which can then be cached between
frames and display lists).

Part of this change involves simplifying the plane splitting
code to work in a similar way to traditional brush images. It's
likely that in the future we can completely remove ps_split_composite
and draw plane splits with brush_image for better batching.

Introducing a new coordinate space allows for some more optimal
calculations of required raster rectangles, which reduces the
number of pixels that we draw for clip masks and transformed
pictures, in some cases.

Follow ups:

  • Support arbitrary rasterization scales for raster roots, for
    better quality / performance.
  • Support caching of pictures as discussed above.

This change is Reviewable

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 6, 2018

Contributor

r? @kvark

This needs some more code documentation - but I thought I'd open the PR now anyway, and work on the remaining bits tomorrow. This fixes a lot of real world pages that have 3d transforms on them.

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

There are 2 FAILs in R6 that I need to look into, and 1 new PASS in Wr1.

(I'll do a full try run on all platforms once I resolve those 2 FAILs tomorrow).

🎉

Contributor

gw3583 commented Sep 6, 2018

r? @kvark

This needs some more code documentation - but I thought I'd open the PR now anyway, and work on the remaining bits tomorrow. This fixes a lot of real world pages that have 3d transforms on them.

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

There are 2 FAILs in R6 that I need to look into, and 1 new PASS in Wr1.

(I'll do a full try run on all platforms once I resolve those 2 FAILs tomorrow).

🎉

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 6, 2018

Contributor

(includes the fix in #3029)

Contributor

gw3583 commented Sep 6, 2018

(includes the fix in #3029)

@kvark

My overall impression is a bit of worry about complexity. We are getting another space (yes, I asked for this back in the day, nobody else to blame here :) ) and an extra indirection to think about (primitive -> surface/picture -> raster -> world) instead of (primitive -> picture -> world). The mutability of transform palette passed around in a lot of places is not making the code simpler either.

This fixes a large number of real world pages that use 3d transforms.

I'm trying to understand why exactly this change fixes a large number of real world pages. I thought that the previous approach, conceptually, would only have troubles on supporting 3D -> 2D -> 3D nested chains. Are those used often in practice? If you are thinking of a wider (and simpler) set of cases, do you know why the PR makes them better, i.e. what was wrong with the previous approach?

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


webrender/res/brush_blend.glsl, line 37 at r1 (raw file):

    vec2 texture_size = vec2(textureSize(sColor0, 0).xy);
    vec2 f = (vi.local_pos - local_rect.p0) / local_rect.size;
    ImageResourceExtra extra_data = fetch_image_resource_extra(user_data.x);

where did the snapping go?


webrender/res/brush_blend.glsl, line 42 at r1 (raw file):

    f = mix(x, y, f.y);
    vec2 uv = mix(uv0, uv1, f);
    vUv = vec3(uv / texture_size, res.layer);

we are missing the gl_Position.w component here. Is this intentional?


webrender/res/brush_image.glsl, line 156 at r1 (raw file):

    vUv.xy /= texture_size;
    vUv.xy *= repeat.xy;
    if ((brush_flags & BRUSH_FLAG_PERSPECTIVE_INTERPOLATION) == 0) {

why is non-perspective interpolation removed for brushes?


webrender/res/clip_shared.glsl, line 82 at r1 (raw file):

    vec2 world_pos = device_pos / uDevicePixelRatio;

    vec4 pos = prim_transform.m * vec4(world_pos, 0.0, 1.0);

could you clarify this part? We are snapping based on clip transform but then applying the primitive transform. Will it end up badly snapped in some cases then?


webrender/res/clip_shared.glsl, line 83 at r1 (raw file):

    vec4 pos = prim_transform.m * vec4(world_pos, 0.0, 1.0);
    pos.xyz /= pos.w;

we can't do this, unless you know that W is always positive


webrender/res/cs_clip_box_shadow.glsl, line 63 at r1 (raw file):

    vec2 texture_size = vec2(textureSize(sColor0, 0));
    vec2 local_pos = vLocalPos.xy / vLocalPos.z;

I know this is copied from the old code, but do we have a guarantee that Z is positive here?


webrender/res/ps_split_composite.glsl, line 78 at r1 (raw file):

    vec4 final_pos = vec4(
        dest_origin + world_pos.xy * uDevicePixelRatio,

we need to multiply the offset by world_pos.w here


webrender/res/ps_split_composite.glsl, line 82 at r1 (raw file):

#ifdef WR_FRAGMENT_SHADER
void main(void) {
    bvec4 inside = lessThanEqual(vec4(vUvTaskBounds.xy, vUv.xy),

what's the guarantee that we aren't going to be rendering outside of the task bounds?


webrender/res/ps_split_composite.glsl, line 99 at r1 (raw file):

    vec2 f = (local_pos - geometry.local_rect.p0) / geometry.local_rect.size;

    vec2 x = mix(extra_data.st_tl, extra_data.st_tr, f.x);

can we use bilerp here?


webrender/src/batch.rs, line 9 at r1 (raw file):

use api::{YuvColorSpace, YuvFormat, WorldPixel, WorldRect};
use clip::{ClipNodeFlags, ClipNodeRange, ClipItem, ClipStore};
use clip_scroll_tree::{ClipScrollTree, SpatialNodeIndex};

I find it a bit worrying that batching now depends on CST


webrender/src/batch.rs, line 1723 at r1 (raw file):

        clip_store: &ClipStore,
        clip_scroll_tree: &ClipScrollTree,
        transforms: &mut TransformPalette,

why do we need to mutate it here?


webrender/src/gpu_types.rs, line 432 at r1 (raw file):

    }

    pub fn set_world_transform(

reads a bit weird that it's called set_world_transform but actually gets LayoutToPictureTransform


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

            let transforms = &mut self.transforms;

            *self.map

now I see why the mutable state is needed


webrender/src/gpu_types.rs, line 599 at r1 (raw file):

    //           some investigation on if this ever happens?
    let inv_transform = transform.inverse()
                                 .unwrap_or(PictureToLayoutTransform::identity());

we can error! at least in this case


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

    pub surface: Option<PictureSurface>,

    pub raster_spatial_node_index: SpatialNodeIndex,

nit: not clear if raster_ is needed here given that we are in RasterConfig struct


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

        let xf = frame_context.clip_scroll_tree.get_relative_transform(
            raster_spatial_node_index,

uh, I'm getting a little lost here. If this primitive spawns a surface, wouldn't the rasterized space match the surface space?
I reads like there can be more surfaces down the road based on the same raster space if they aren't perspectively transformed. Is there a benefit to this versus just treating Surface=Raster at all times?


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

        let map_raster_to_world = SpaceMapper::new_with_target(
            SpatialNodeIndex(0),

of all the spatial node indices in scope, why do we choose 0 here? :)


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

                                               .unwrap_or(RasterRect::max_rect());

                let map_to_raster = SpaceMapper::new_with_target(

there is quite a bit of common code between this and take_context


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

    let unclipped_raster_rect = match map_to_raster.map(&pic_rect) {
        Some(rect) => rect,
        None => return None,

this function gives us a great opportunity to abuse ?, e.g.

let unclipped_raster_rect = map_to_raster(&pic_rect)?;

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

    );

    let transform = map_to_raster.get_transform();

In at least 2 cases the caller only cares about the first result. Can/should we avoid the extra computation in this case?


webrender/src/renderer.rs, line 446 at r1 (raw file):

                name: "aClipTransformId",
                count: 1,
                kind: VertexAttributeKind::I32,

it is rather tempting to reduce the number of attributes by having ivec4


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

         self.m24.abs() > NEARLY_ZERO ||
         self.m34.abs() > NEARLY_ZERO ||
         (self.m44.abs() - 1.0) > NEARLY_ZERO

what if m44 = 0.5?


webrender_api/src/units.rs, line 55 at r1 (raw file):

#[derive(Hash, Clone, Copy, Debug, Eq, PartialEq, Ord, PartialOrd)]
pub struct RasterPixel;

need to document what this space is about

@pcwalton

This comment has been minimized.

Show comment
Hide comment
@pcwalton

pcwalton Sep 6, 2018

Collaborator

Note that this will possibly be needed to optimize blur as well, because right now there's no way to render contents at a lower resolution without introducing more transforms, which necessitate stacking contexts and reference frames.

Collaborator

pcwalton commented Sep 6, 2018

Note that this will possibly be needed to optimize blur as well, because right now there's no way to render contents at a lower resolution without introducing more transforms, which necessitate stacking contexts and reference frames.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 7, 2018

Contributor

Current try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57331aff6e49b2ec1003a85abff4b4130dd0e3ff

Down to one failure - working through that now, then will follow up on and address review comments :)

Contributor

gw3583 commented Sep 7, 2018

Current try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57331aff6e49b2ec1003a85abff4b4130dd0e3ff

Down to one failure - working through that now, then will follow up on and address review comments :)

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Sep 7, 2018

Member

I confirm (by downloading the last try artifact) that the PR fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1487570
🎉 🎉 🎉

Member

kvark commented Sep 7, 2018

I confirm (by downloading the last try artifact) that the PR fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1487570
🎉 🎉 🎉

@gw3583

Reviewable status: 28 of 31 files reviewed, 23 unresolved discussions (waiting on @kvark and @gw3583)


webrender/res/brush_blend.glsl, line 37 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

where did the snapping go?

This was needed when we were deriving the screen UV from the render task device rect. However, we now use the UVs provided by calculate_uv_rect_kind(), which handles the snapping when calculating the UVs.


webrender/res/brush_blend.glsl, line 42 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we are missing the gl_Position.w component here. Is this intentional?

Yes - any pictures that have perspective now form a rasterization root in their local space, so we don't need to apply perspective correction when compositing them.


webrender/res/brush_image.glsl, line 156 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why is non-perspective interpolation removed for brushes?

Same as above - we only ever composite with source images that were not drawn with perspective now.


webrender/res/clip_shared.glsl, line 82 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could you clarify this part? We are snapping based on clip transform but then applying the primitive transform. Will it end up badly snapped in some cases then?

I think this is wrong, yes - I have left it as is, since it's the same behavior as current (previous code only looked at the clip transform here). But it does seem potentially incorrect to me too, and should be considered as a follow up.


webrender/res/clip_shared.glsl, line 83 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we can't do this, unless you know that W is always positive

I think this is (somewhat) related to the last failure I have in try run - working on a solution to that now.


webrender/res/cs_clip_box_shadow.glsl, line 63 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I know this is copied from the old code, but do we have a guarantee that Z is positive here?

I'm not totally sure - this might be related to the current reftest failure on try.


webrender/res/ps_split_composite.glsl, line 78 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we need to multiply the offset by world_pos.w here

dest_origin is already in device pixels. I ran into this because I found the existing code was failing on my hi-dpi screen - this is the fix for that.


webrender/res/ps_split_composite.glsl, line 82 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what's the guarantee that we aren't going to be rendering outside of the task bounds?

In theory the clip mask generated on the surface handles this, but I want to do some more testing - this might actually be the issue with the failing reftest too.


webrender/res/ps_split_composite.glsl, line 99 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we use bilerp here?

Yes, and your comment helped me notice I had the parameter order here wrong (compared to the bilerp above) - fixing this actually fixes most of the last failing reftest I have!


webrender/src/batch.rs, line 9 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I find it a bit worrying that batching now depends on CST

Yea - it's because of the way we generate relative transforms in the transform palette on demand. A tidier solution, as a follow up, is that the culling and clipping generation code should pre-register all the needed relative transforms. Then they will exist in the transform palette.


webrender/src/batch.rs, line 1723 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we need to mutate it here?

See above comment about CST - we can fix this as a follow up.


webrender/src/gpu_types.rs, line 432 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

reads a bit weird that it's called set_world_transform but actually gets LayoutToPictureTransform

True, fixed.


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

Previously, kvark (Dzmitry Malyshau) wrote…

now I see why the mutable state is needed

Yea - it's a bit of a hack to get this landed. We can pre-register them in the future as described above.


webrender/src/gpu_types.rs, line 599 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we can error! at least in this case

Fixed


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

Previously, kvark (Dzmitry Malyshau) wrote…

nit: not clear if raster_ is needed here given that we are in RasterConfig struct

Added a comment to clarify.


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

Previously, kvark (Dzmitry Malyshau) wrote…

uh, I'm getting a little lost here. If this primitive spawns a surface, wouldn't the rasterized space match the surface space?
I reads like there can be more surfaces down the road based on the same raster space if they aren't perspectively transformed. Is there a benefit to this versus just treating Surface=Raster at all times?

Not always - for example, if you have a rotated picture on the main frame, the surface space comes from the picture, but the raster space could be the surface space (if drawing in local space for caching / performance) or the root world space (if drawing in screen space for quality).


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

Previously, kvark (Dzmitry Malyshau) wrote…

of all the spatial node indices in scope, why do we choose 0 here? :)

We know that node index 0 is world space - I changed these references to use ROOT_SPATIAL_NODE_INDEX for clarity.


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

Previously, kvark (Dzmitry Malyshau) wrote…

there is quite a bit of common code between this and take_context

Agreed, I'll tidy this up a bit.


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

Previously, kvark (Dzmitry Malyshau) wrote…

this function gives us a great opportunity to abuse ?, e.g.

let unclipped_raster_rect = map_to_raster(&pic_rect)?;

Agreed, I'll tidy this up a bit.


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

Previously, kvark (Dzmitry Malyshau) wrote…

In at least 2 cases the caller only cares about the first result. Can/should we avoid the extra computation in this case?

It's not called that often, but I will tidy it up to avoid the extra work in those cases since it's the more common case.


webrender/src/renderer.rs, line 446 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

it is rather tempting to reduce the number of attributes by having ivec4

Yep, this could definitely be tidied up in a follow up.


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

Previously, kvark (Dzmitry Malyshau) wrote…

what if m44 = 0.5?

That still works? All we're trying to do here is see if it's 1 or very close to 1?


webrender_api/src/units.rs, line 55 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

need to document what this space is about

Done.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 7, 2018

Contributor

Addressed most of those review comments. Still working on one reftest failure, and then a couple of tidy up comments from above still need to be addressed.

Contributor

gw3583 commented Sep 7, 2018

Addressed most of those review comments. Still working on one reftest failure, and then a couple of tidy up comments from above still need to be addressed.

@kvark

Reviewed 1 of 3 files at r2, 9 of 9 files at r3.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @kvark)


webrender/res/brush_image.glsl, line 156 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Same as above - we only ever composite with source images that were not drawn with perspective now.

Hold on. Brush image is our general hammer of drawing image-ish nails, not just compositing. If there is a transformed stacking context with an image in it, we'll still need to draw the brush_image primitive (generated by this image) begin transformed. What am I missing?


webrender/res/clip_shared.glsl, line 82 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

I think this is wrong, yes - I have left it as is, since it's the same behavior as current (previous code only looked at the clip transform here). But it does seem potentially incorrect to me too, and should be considered as a follow up.

please add a TODO comment since we are at it


webrender/res/ps_split_composite.glsl, line 78 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

dest_origin is already in device pixels. I ran into this because I found the existing code was failing on my hi-dpi screen - this is the fix for that.

It's in device pixels, but it's not in the homogeneous space, so it will end up divided by world_pos.w by the hardware unless you premultiply it here


webrender/res/ps_split_composite.glsl, line 99 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Yes, and your comment helped me notice I had the parameter order here wrong (compared to the bilerp above) - fixing this actually fixes most of the last failing reftest I have!

yay! glad to help :D


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

Previously, gw3583 (Glenn Watson) wrote…

Not always - for example, if you have a rotated picture on the main frame, the surface space comes from the picture, but the raster space could be the surface space (if drawing in local space for caching / performance) or the root world space (if drawing in screen space for quality).

Thank you for clarification! So a picture may need to spawn a surface. And then a surface may need to spawn a raster space (if it has perspective component). Correct?


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

Previously, gw3583 (Glenn Watson) wrote…

Agreed, I'll tidy this up a bit.

this is still TODO, right?


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

All we're trying to do here is see if it's 1 or very close to 1?

That's what we should be doing, but that's not what the code does :)


webrender_api/src/units.rs, line 55 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Done.

I think "screen space" should be replaced by "world space" here to match our strongly typed spaces.
In general, we should be careful with interchanging "screen" and "device", preferring the latter for there being a type for it.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 8, 2018

Contributor

OK, I have a fix for the remaining reftest failure - the fix also resolves the concerns above about clipping outside the boundaries of a plane splitting polygon.

I'll tidy up the patch on Monday (the implementation is a bit of a hack, but the result is correct) and post it here.

Contributor

gw3583 commented Sep 8, 2018

OK, I have a fix for the remaining reftest failure - the fix also resolves the concerns above about clipping outside the boundaries of a plane splitting polygon.

I'll tidy up the patch on Monday (the implementation is a bit of a hack, but the result is correct) and post it here.

gw3583 added some commits Sep 6, 2018

Support arbitrary rasterization coordinate roots.
This allows us to choose to rasterize any picture in an
arbitrary coordinate space.

This fixes a large number of real world pages that use 3d
transforms. It also simplifies fixing any remaining 3d
transform related issues.

The initial patch uses this to promote any picture that has
perspective into a rasterization root.

There are a number of benefits to this:
 * It simplifies a lot of the math involving plane splitting,
   since we don't need to deal with splits drawn in screen-space.
 * It makes caching perspective rendered pictures much easier,
   since we can cache them between frames even when the
   root coordinate system is animating the picture (this isn't
   implemented yet, but is a simple-ish follow up).

We can make use of this to render text shadows in local space,
as a performance optimization (which can then be cached between
frames and display lists).

Part of this change involves simplifying the plane splitting
code to work in a similar way to traditional brush images. It's
likely that in the future we can completely remove ps_split_composite
and draw plane splits with brush_image for better batching.

Introducing a new coordinate space allows for some more optimal
calculations of required raster rectangles, which reduces the
number of pixels that we draw for clip masks and transformed
pictures, in some cases.

Follow ups:
 * Support arbitrary rasterization scales for raster roots, for
   better quality / performance.
 * Support caching of pictures as discussed above.
@gw3583

Reviewable status: 24 of 32 files reviewed, 12 unresolved discussions (waiting on @kvark and @gw3583)


webrender/res/brush_image.glsl, line 156 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Hold on. Brush image is our general hammer of drawing image-ish nails, not just compositing. If there is a transformed stacking context with an image in it, we'll still need to draw the brush_image primitive (generated by this image) begin transformed. What am I missing?

The transform is still applied, but we don't need to specifically correct for perspective interpolation, since the source images from render tasks are no longer drawn with a perspective component. So it's just like drawing a normal image source in perspective.


webrender/res/clip_shared.glsl, line 82 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

please add a TODO comment since we are at it

Added a comment.


webrender/res/ps_split_composite.glsl, line 78 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

It's in device pixels, but it's not in the homogeneous space, so it will end up divided by world_pos.w by the hardware unless you premultiply it here

Ah, indeed. Fixed!


webrender/res/ps_split_composite.glsl, line 99 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

yay! glad to help :D

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

Thank you for clarification! So a picture may need to spawn a surface. And then a surface may need to spawn a raster space (if it has perspective component). Correct?

Yep, exactly :)


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

Previously, kvark (Dzmitry Malyshau) wrote…

this is still TODO, right?

Yep, fixed in the most recent commit.


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

Previously, kvark (Dzmitry Malyshau) wrote…

All we're trying to do here is see if it's 1 or very close to 1?

That's what we should be doing, but that's not what the code does :)

Ooops! Fixed.


webrender_api/src/units.rs, line 55 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think "screen space" should be replaced by "world space" here to match our strongly typed spaces.
In general, we should be careful with interchanging "screen" and "device", preferring the latter for there being a type for it.

Agreed, fixed.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 9, 2018

Contributor

I addressed the remaining review comments, and rebased. I also added a (hopefully) final commit that resolves the remaining reftest failure and comments about clipping on split composite primitives. It's basically a hack / workaround for legacy API issues with the clipping APIs. However, I think it's a reasonable implementation detail for now, that we can tidy up at a later point. Commit note here for details:

The split composite shader is now quite similar to a normal
brush shader (uses prim header, supports normal clip masks
etc). The only real difference is that it draws a quad polygon
from local points instead of a rectangle. It should be easy
to unify this into the brush_image shader in the future.

Add a temporary "ClipNodeCollector" interface to correctly
handle clips that are ancestors of the current rasterization
root. Although it's not enforced by the API, I have verified
that any time a clip from a spatial node that is an ancestor
of the current rasterization root is applied on all primitives
that are part of that picture. This means we can safely apply
those clips during compositing of the rasterization root rather
than on each individual primitive. This fixes correctness
issues with clips across rasterization roots, and is also a
quite significant performance win in some cases, since the
clip mask is now applied once when the rasterization root
is drawn, rather than on each indidvidual primitive.

The end result of this is actually quite good - it will allow
us to simplify some of the clipping code in the future, and is
a performance win. The implementation leaves a lot to be
desired, but gets the required result without having to fix
up both gecko and wr clipping API mismatches. The long term
solution for this is to update gecko and WR to make better use
of clip chains on stacking contexts, rather than putting
clips from the stacking context on each individual primitive (we
have discussed doing this previously, but it's low-ish priority
for now given other bugs).

I'll kick off an updated try run shortly. Hopefully after that and review of the last two commits, this should be ready to go. Although not ideal, it fixes a heap of existing 3d transform bugs we have.

Contributor

gw3583 commented Sep 9, 2018

I addressed the remaining review comments, and rebased. I also added a (hopefully) final commit that resolves the remaining reftest failure and comments about clipping on split composite primitives. It's basically a hack / workaround for legacy API issues with the clipping APIs. However, I think it's a reasonable implementation detail for now, that we can tidy up at a later point. Commit note here for details:

The split composite shader is now quite similar to a normal
brush shader (uses prim header, supports normal clip masks
etc). The only real difference is that it draws a quad polygon
from local points instead of a rectangle. It should be easy
to unify this into the brush_image shader in the future.

Add a temporary "ClipNodeCollector" interface to correctly
handle clips that are ancestors of the current rasterization
root. Although it's not enforced by the API, I have verified
that any time a clip from a spatial node that is an ancestor
of the current rasterization root is applied on all primitives
that are part of that picture. This means we can safely apply
those clips during compositing of the rasterization root rather
than on each individual primitive. This fixes correctness
issues with clips across rasterization roots, and is also a
quite significant performance win in some cases, since the
clip mask is now applied once when the rasterization root
is drawn, rather than on each indidvidual primitive.

The end result of this is actually quite good - it will allow
us to simplify some of the clipping code in the future, and is
a performance win. The implementation leaves a lot to be
desired, but gets the required result without having to fix
up both gecko and wr clipping API mismatches. The long term
solution for this is to update gecko and WR to make better use
of clip chains on stacking contexts, rather than putting
clips from the stacking context on each individual primitive (we
have discussed doing this previously, but it's low-ish priority
for now given other bugs).

I'll kick off an updated try run shortly. Hopefully after that and review of the last two commits, this should be ready to go. Although not ideal, it fixes a heap of existing 3d transform bugs we have.

@gw3583
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 10, 2018

Contributor

Try run looks good. The same new PASS results on Windows, plus a couple unrelated intermittents. re-r? @kvark

Contributor

gw3583 commented Sep 10, 2018

Try run looks good. The same new PASS results on Windows, plus a couple unrelated intermittents. re-r? @kvark

@kvark

I got just a few concerns left. Code is looking great for the most part 👍

Reviewed 14 of 14 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kvark and @gw3583)


webrender/res/clip_shared.glsl, line 83 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

I think this is (somewhat) related to the last failure I have in try run - working on a solution to that now.

This concern doesn't appear addressed?


webrender/src/clip.rs, line 472 at r4 (raw file):

                let clip_node = &self.clip_nodes[clip_node_index.0 as usize];

                // Check if any this clip node index should actually be

"Check if any this clip node" - something is missing here :)


webrender/src/clip.rs, line 1120 at r4 (raw file):

    clip_node_index: ClipNodeIndex,
    spatial_node_index: SpatialNodeIndex,
    local_clip_rect: &mut LayoutRect,

It seems like this would be cleaner if instead of &mut clip rectangle we returned Option<LayoutRect>. This is a nit, not necessary for immediate addressing.


webrender/src/clip.rs, line 1140 at r4 (raw file):

        let xf = clip_scroll_tree.get_relative_transform(
            clip_node.spatial_node_index,
            ROOT_SPATIAL_NODE_INDEX,

shouldn't this be spatial_node_index, given that you are trying to find the relation between ref and clip?

@gw3583

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


webrender/res/clip_shared.glsl, line 83 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

This concern doesn't appear addressed?

It's addressed by the clip node collector change - we don't rasterize clips across perspective roots now. In fact this means we should be able to simplify this code significantly, but I left this as a follow up, since I want to get this stuff into gecko asap.


webrender/src/clip.rs, line 472 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

"Check if any this clip node" - something is missing here :)

Done.


webrender/src/clip.rs, line 1120 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

It seems like this would be cleaner if instead of &mut clip rectangle we returned Option<LayoutRect>. This is a nit, not necessary for immediate addressing.

The problem with that is that we also need to signal if the clip completely culled the primitive - I guess we could have Result<Option, ()> or something like that as a follow up?


webrender/src/clip.rs, line 1140 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't this be spatial_node_index, given that you are trying to find the relation between ref and clip?

In this case (the complex clip spanning a complex transform), we map both the prim and clip to world space in order to do our trivial clip accept / reject / partial tests. So this transform is used to get the clip inner / outer rects in world space.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 10, 2018

Contributor

@kvark Thanks! Responded to those comments / quesions and pushed an updated commit.

Contributor

gw3583 commented Sep 10, 2018

@kvark Thanks! Responded to those comments / quesions and pushed an updated commit.

@kvark

kvark approved these changes Sep 10, 2018

:lgtm:

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kvark)


webrender/res/cs_clip_box_shadow.glsl, line 63 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

I'm not totally sure - this might be related to the current reftest failure on try.

Does it mean we are still blocked?

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Sep 10, 2018

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 10, 2018

Contributor

🚀

Contributor

gw3583 commented Sep 10, 2018

🚀

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 10, 2018

Contributor

@bors-servo r=kvark

Contributor

gw3583 commented Sep 10, 2018

@bors-servo r=kvark

Add normal clip mask support to ps_split_composite.
The split composite shader is now quite similar to a normal
brush shader (uses prim header, supports normal clip masks
etc). The only real difference is that it draws a quad polygon
from local points instead of a rectangle. It should be easy
to unify this into the brush_image shader in the future.

Add a temporary "ClipNodeCollector" interface to correctly
handle clips that are ancestors of the current rasterization
root. Although it's not enforced by the API, I have verified
that any time a clip from a spatial node that is an ancestor
of the current rasterization root is applied on all primitives
that are part of that picture. This means we can safely apply
those clips during compositing of the rasterization root rather
than on each individual primitive. This fixes correctness
issues with clips across rasterization roots, and is also a
quite significant performance win in some cases, since the
clip mask is now applied once when the rasterization root
is drawn, rather than on each indidvidual primitive.

The end result of this is actually quite good - it will allow
us to simplify some of the clipping code in the future, and is
a performance win. The implementation leaves a lot to be
desired, but gets the required result without having to fix
up both gecko and wr clipping API mismatches. The long term
solution for this is to update gecko and WR to make better use
of clip chains on stacking contexts, rather than putting
clips from the stacking context on each individual primitive (we
have discussed doing this previously, but it's low-ish priority
for now given other bugs).
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 11, 2018

Contributor

@bors-servo r=kvark

Contributor

gw3583 commented Sep 11, 2018

@bors-servo r=kvark

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 11, 2018

Contributor

Just pushed an amended commit to try and get bors to respond, but no luck so far 😢

Contributor

gw3583 commented Sep 11, 2018

Just pushed an amended commit to try and get bors to respond, but no luck so far 😢

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 11, 2018

Member

@bors-servo r=kvark

Member

jdm commented Sep 11, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

📌 Commit a99f733 has been approved by kvark

Contributor

bors-servo commented Sep 11, 2018

📌 Commit a99f733 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

⌛️ Testing commit a99f733 with merge dede39f...

Contributor

bors-servo commented Sep 11, 2018

⌛️ Testing commit a99f733 with merge dede39f...

bors-servo added a commit that referenced this pull request Sep 11, 2018

Auto merge of #3030 - gw3583:raster-17, r=kvark
Support arbitrary rasterization coordinate roots.

This allows us to choose to rasterize any picture in an
arbitrary coordinate space.

This fixes a large number of real world pages that use 3d
transforms. It also simplifies fixing any remaining 3d
transform related issues.

The initial patch uses this to promote any picture that has
perspective into a rasterization root.

There are a number of benefits to this:
 * It simplifies a lot of the math involving plane splitting,
   since we don't need to deal with splits drawn in screen-space.
 * It makes caching perspective rendered pictures much easier,
   since we can cache them between frames even when the
   root coordinate system is animating the picture (this isn't
   implemented yet, but is a simple-ish follow up).

We can make use of this to render text shadows in local space,
as a performance optimization (which can then be cached between
frames and display lists).

Part of this change involves simplifying the plane splitting
code to work in a similar way to traditional brush images. It's
likely that in the future we can completely remove ps_split_composite
and draw plane splits with brush_image for better batching.

Introducing a new coordinate space allows for some more optimal
calculations of required raster rectangles, which reduces the
number of pixels that we draw for clip masks and transformed
pictures, in some cases.

Follow ups:
 * Support arbitrary rasterization scales for raster roots, for
   better quality / performance.
 * Support caching of pictures as discussed above.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3030)
<!-- Reviewable:end -->
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 11, 2018

Contributor

@bors-servo retry

Contributor

gw3583 commented Sep 11, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

⌛️ Testing commit a99f733 with merge 04d63e7...

Contributor

bors-servo commented Sep 11, 2018

⌛️ Testing commit a99f733 with merge 04d63e7...

bors-servo added a commit that referenced this pull request Sep 11, 2018

Auto merge of #3030 - gw3583:raster-17, r=kvark
Support arbitrary rasterization coordinate roots.

This allows us to choose to rasterize any picture in an
arbitrary coordinate space.

This fixes a large number of real world pages that use 3d
transforms. It also simplifies fixing any remaining 3d
transform related issues.

The initial patch uses this to promote any picture that has
perspective into a rasterization root.

There are a number of benefits to this:
 * It simplifies a lot of the math involving plane splitting,
   since we don't need to deal with splits drawn in screen-space.
 * It makes caching perspective rendered pictures much easier,
   since we can cache them between frames even when the
   root coordinate system is animating the picture (this isn't
   implemented yet, but is a simple-ish follow up).

We can make use of this to render text shadows in local space,
as a performance optimization (which can then be cached between
frames and display lists).

Part of this change involves simplifying the plane splitting
code to work in a similar way to traditional brush images. It's
likely that in the future we can completely remove ps_split_composite
and draw plane splits with brush_image for better batching.

Introducing a new coordinate space allows for some more optimal
calculations of required raster rectangles, which reduces the
number of pixels that we draw for clip masks and transformed
pictures, in some cases.

Follow ups:
 * Support arbitrary rasterization scales for raster roots, for
   better quality / performance.
 * Support caching of pictures as discussed above.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3030)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 04d63e7 to master...

Contributor

bors-servo commented Sep 11, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 04d63e7 to master...

@bors-servo bors-servo merged commit a99f733 into servo:master Sep 11, 2018

3 of 4 checks passed

code-review/reviewable 1 discussion left (gw3583)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Sep 11, 2018

Closed

Remove flat varying #3012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment