Switch box-shadows to use brush masks (and add basic brush prim). #1841

glennw · 2017-10-10T22:52:39Z

This introduces a new brush primitive type. Right now, this is
extremely basic and doesn't provide much new functionality. As
more items are ported to use it, the functionality of the brush
shader type will expand (mostly the VS parts).

The major change in this patch is to remove the old box-shadow
shader and switch box shadows to be drawn via a brush mask +
gaussian blur approach.

This is a performance regression for small / simple box shadows
compared to previously, but this is only temporary. The goal of
this (large) patch is to switch over to a different box-shadow
technique and address correctness issues first, and then we can
apply a number of optimizations on top of that. It is a performance
win on very large box shadows, which was the performance worst
case for the previous box shadow shader.

The changes included:

Extend blur shader to support both A8 and RGBA8 blurs.
Basic brush shader and primitive code.
Brush mask shader.
Remove old box shadow shader.
Move some common clip functions to ellipse.glsl.
Extend cache image shader to support both A8 and RGBA8 sources.
Support clip masks in cache image shader.
Remove box shadow primitive type.
Add reftests for spread, offset, blur, border radius box-shadow variations.
Support adjusting the border radii of box shadows as defined by spec.

Next steps:

Extend the mask shader to support arbitrary border radii per corner.
Apply various optimizations to the brush and blur code.

This change is

glennw · 2017-10-10T22:59:58Z

r? @kvark

There's a couple of blockers before this can be merged - but it can probably be reviewed now.

The main issue is that the clip mask shader used for fast path box shadows (with zero blur) introduces a faint line of AA in some cases where it shouldn't. I'm a little unsure if I should leave this here until @nical finishes up his work on the AA changes, which may just resolve that problem. That causes a few test failures in the Servo WPT suite. @nical Do you think your AA work is likely to fix that, and how soon do you think it will be ready?

kvark · 2017-10-12T00:35:53Z

Reviewed 30 of 30 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

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

    // will be expanded to support transform picture types (the common kind).
    vec2 pos = pic_task.target_rect.p0 + aPosition.xy * pic_task.target_rect.size;
    vLocalPos = aPosition.xy * pic_task.target_rect.size / uDevicePixelRatio;

I'm concerned that this makes vLocalPos to live in a separate space from vLocalRect, which is confusing

webrender/res/cs_text_run.glsl, line 23 at r1 (raw file):

    int picture_address = prim.user_data2;

    // Fetch the parent text-shadow for this primitive. This allows the code

nit: comment still implies the shadow

webrender/res/ellipse.glsl, line 70 at r1 (raw file):

float clip_against_ellipse_if_needed(vec2 pos,
                                     float current_distance,

nit: would be nicer with the new formatting style

webrender/res/ps_cache_image.glsl, line 68 at r1 (raw file):

        // Apply the clip mask
        color.a = min(color.a, do_clip());

I guess this will now become easier as @mstange convinced us to use multiplication ;)

webrender/res/ps_line.glsl, line 102 at r1 (raw file):

#ifdef WR_FEATURE_CACHE
    int picture_address = prim.user_data0;
    PrimitiveGeometry shadow_geom = fetch_primitive_geometry(picture_address);

should shadow_geom be renamed to picture_geom?

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

                        shadow_rect,
                        BorderRadius::uniform(shadow_radius),
                        ClipMode::ClipOut

Shouldn't this be ClipIn?

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

                    );

                    self.add_primitive(

add_primitive could possibly go outside the match

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

                    ));

                    self.add_primitive(

similarly, this could possibly be moved out of the match

Comments from Reviewable

glennw · 2017-10-12T06:29:25Z

Review status: 26 of 30 files reviewed at latest revision, 8 unresolved discussions.

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