Better inner and outer rect calculation for ClipChains #2062

mrobinson · 2017-11-20T09:22:59Z

We calculate the real inner and outer boundaries of clips in screen
space and use it to discard clips and generate masks that are a little
smaller in some cases. This results in a small performance improvement
on pages with complicated clipping.

This change is

mrobinson · 2017-11-20T09:27:18Z

Here is how this looks on the W3 testcase from #1817.

Before                                  After
CPU: 1.54                               CPU: 1.87
Compositor: 15.39                       Compositor: 15.33
GPU: 11.95                              GPU: 10.66

I'm still looking at the regression in CPU time, but I'd love to get general comments on this approach.

I did a Gecko try push as well and the only issues I see are some GTest failures, which I think are unrelated.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e470a85a5a66cf9e3ae4a97b70e85ba4b35da67

mrobinson · 2017-11-20T10:09:56Z

I updated the reference image for one test that had a small number of subpixel differences.

glennw · 2017-11-20T10:34:42Z

I'll look at this in detail in the morning.

One question though - do you have a sense of what the profile numbers would be like with the improved inner / outer rect calculation and filtering, but without the shader changes?

I wonder if the majority of the wins might be from the CPU-side filtering? (I thought that the clip masks were already intersected with the screen rect of the clip node - since we support the clip mask being a portion of the primitive rect - but maybe that's not working / slightly different).

mrobinson · 2017-11-20T17:26:00Z

@glennw So I took another look at this. I think we need to do this with the change to the shader. The case (and I suspect it might be the important case) is when we can avoid masking at all. This happens when the screen space rectangle of the ClipChain is sufficient to clip the primitive. in this case we can just skip creating the task entirely and rely on the changes to the fragment shader. Maybe there is a better way to do this, but I don't think we can use local geometry in this case.

mrobinson · 2017-11-20T17:28:54Z

I pushed a new version of this PR which moves outer rect calculation to the ClipScrollTree update. This moves a little bit of work from the per-primitive parts to the per-ClipScrollNode part. I'm not seeing a CPU speedup for the W3C case, but I think that this cleaner in general.

I am also thinking about extending this sort of thing to handling some situations where we can generate shorter ClipChains in general.

bors-servo · 2017-11-20T17:55:24Z

☔ The latest upstream changes (presumably #2052) made this pull request unmergeable. Please resolve the merge conflicts.

kvark · 2017-11-20T23:45:15Z

Thanks @mrobinson, impressive work!
I expressed the current questions/concerns. Will want to take another look after the first wave is addressed.

Reviewed 22 of 22 files at r1.
Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

webrender/res/prim_shared.glsl, line 615 at r1 (raw file):

    vScreenClipRect.xy += task_offset;
    if (task.size == vec2(0.0)) {
        vScreenClipRect.y = (uBufferHeight - vScreenClipRect.y) - vScreenClipRect.w;

why are we doing this?

webrender/res/prim_shared.glsl, line 889 at r1 (raw file):

}

float apply_rectangular_screen_space_clip(vec4 screenSpaceClipRect) {

any reason not to use vScreenClipRect directly here?

webrender/res/prim_shared.glsl, line 893 at r1 (raw file):

    // is inside the screen space clipping box.
    vec2 topLeft = screenSpaceClipRect.xy;
    vec2 bottomRight = screenSpaceClipRect.xy + vec2(screenSpaceClipRect.z, screenSpaceClipRect.w);

nit: could use swizzling screenSpaceClipRect.zw

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

        }

        let outer = match can_calculate_outer_rect {

isn't setting this to true equivalent to saying local_outer = None?

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

        };

        let inner = match can_calculate_inner_rect {

similarly, setting local_inner to zero() should do the same

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

        resource_cache: &mut ResourceCache,
    ) {
        if self.clips.is_empty() {

I don't think the early out is useful here since all the function does is iterating the list twice

webrender/src/clip_scroll_node.rs, line 309 at r1 (raw file):

                        self.reference_frame_relative_scroll_offset,
                    scroll_offset: self.scroll_offset(),
                    combined_clip_outer_bounds: LayerRect::new(

could just do self.combined_clip_outer_bounds.to_f32()

webrender/src/clip_scroll_node.rs, line 352 at r1 (raw file):

            clip_sources.get_screen_bounds(&self.world_viewport_transform, device_pixel_ratio);

        let combined_outer_screen_rect = match screen_outer_rect.as_ref() {

no need for as_ref() in a match

webrender/src/clip_scroll_node.rs, line 359 at r1 (raw file):

            None => state.combined_outer_clip_bounds,
        };
        state.combined_outer_clip_bounds = combined_outer_screen_rect;

hmm, the old code doesn't modify state.combined_outer_clip_bounds. Why are we doing it now?

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

        device_pixel_ratio: f32,
        scene_properties: &SceneProperties,
        screen_rect: &DeviceIntRect,

there is already self.screen_rect, can we use it?

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

                    screen_inner_rect,
                    combined_outer_screen_rect:
                        combined_outer_rect.unwrap_or_else(DeviceIntRect::zero),

I don't think zero() is appropriate replacement for the lack of the outer bound

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

        // If everything is clipped out, then we don't need to render this primitive.
        let combined_outer_rect = match combined_outer_rect {
            Some(rect) if !rect.is_empty() => {

leaving a note for myself that this is related to the zero() assignment somewhere above

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

        let combined_outer_rect = match combined_outer_rect {
            Some(rect) if !rect.is_empty() => {
                metadata.screen_rect = Some(rect);

nit: let's move this assignment after the match, so that the match becomes non-mutating

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

            .chain(ClipChainNodeIter { current: extra_clip })
            .filter_map(|node| {
                if !node.screen_inner_rect.is_empty() {

nit: could be combined_inner_rect = if ... {} else {..};

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

        projection: &Transform3D<f32>,
        mode: M,
        buffer_height: u32,

I'd find it easier to understand if it was named "target_height" instead.

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

        LayerRect::new(
            LayerPoint::new(f32::MIN / 2.0, f32::MIN / 2.0),
            LayerSize::new(f32::MAX, f32::MAX),

would this guarantee that origin + size is not going to end up at infinity?

Comments from Reviewable

glennw · 2017-11-21T01:44:25Z

OK, I think I have worked out in my head what my concern is with this approach. I get easily confused by the various permutations, so I'll draw up a diagram as reference and try to explain shortly - it's quite likely I am not thinking it all through properly :)

glennw · 2017-11-21T02:57:00Z

OK, my thoughts below. I pre-emptively apologize if the stuff below sounds a bit short - I'm basically just trying to list a series of statements, so that we can easily discuss which bits below are correct and which bits are wrong, or missing some context / information. Let me know if that makes any sense (or completely misses the point!) - we can also set up a Vidyo / Skype call to discuss in detail, if that's easier?

This is a simplified set of examples - but I think it extends to cover more complex cases (e.g. when there are multiple clips in a clip chain).

Red blocks are clips. Green blocks are primitives.

In each of the four scenarios, we have (1) primitive completely inside clip (2) primitive partially clipped (3) primitive not affected by clip.

In (A) both clip and primitive are axis-aligned - the coordinate systems are considered compatible.
In (B) the primitive has a rotation - the coordinate systems are considered incompatible.
In (C) the clip has a rotation - the coordinate systems are considered incompatible.
In (D) both clip and primitive are rotated by a parent node - the coordinate systems are considered compatible.

I think the following statements are true:
- This change helps with avoiding a clip mask in scenario (A) and (B).
- (C) still requires a mask, but only in the overlapping case. If we detect the primitive is inside/outside the clip region, we can skip the mask. We can do a cheap check first, with AABB checks. If the potential clip mask is large enough to warrant it, and AABB check fails, we can use SAT or similar.
- (D) doesn't require a mask with or without this change (since we can apply it as a local clip in the vertex shader).
- In both (A) and (B) we can detect that the primitive is inside or outside the region (using AABB / SAT), we can skip the clip mask for those on CPU.

If the above is correct, what we get from this PR is to skip the clip mask in (A) and (B) when we detect that the clip region of interest intersects with the primitive.

However, in the case of (A), we can apply the same logic as (D) - that is, the coordinate systems are compatible, and therefore we can skip the clip mask and apply this as a local clip in the vertex shader.

The point above is important, for two reasons:
(1) Scenario (A) is certainly the vast majority of cases we will encounter on real web pages (but it's still important to handle (B) efficiently).
(2) In terms of GPU time, we are always limited by the number of fragments we process. This solution still invokes the fragment shader for those extra pixels (when drawing the primitive). Although it does save drawing a clip mask, I think we can do better by skipping the clip mask and applying a local clip in the vertex shader in these cases - this means we don't invoke the FS at all for the clipped fragments.

So, it does seem like this is a potential win for the intersecting case in scenario (B). My guess is that we almost never actually see this in real world, and that the benefits we're seeing are because we're not filtering clip masks out (and into a local clip when compatible) as much as we could be. There's also a slight downside that we have extra instructions running for every fragment shader to apply that screen space clip rect.

Apologies for the wall of text. Thoughts?

mrobinson · 2017-11-21T10:15:37Z

Review status: all files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

webrender/res/prim_shared.glsl, line 615 at r1 (raw file):