Convert rectangle from prim to brush shader and support segments. #2146

glennw · 2017-12-01T02:04:32Z

Add support for brushes to be drawn as a list of segments that
are part of a larger primitive. Initially, these segments are
nine-patches only, although they may be expanded in the future
to work with generic segment rects.

This allows large brush primitives to be split into a number
of segments that can be split among the opaque and alpha passes,
which means that large rectangles are often rendered with the
inner rectangle as part of the opaque pass. Additionally, edge
segments are rendered in the opaque pass when the primitive does
not have a rotation or perspective transform applied.

When a rectangle is split, the clip tasks are generated only
where required for individual segments. This can significantly
reduce the amount of allocated area for clip masks, and also
the amount of GPU time spent drawing clip masks.

Previously, rectangle splitting could only work on LocalClip
items specified as part of the item, which meant that rectangles
in Gecko would never be split. Now, we calculate splitting
during prepare_prim_for_render, consulting the current clip chain
for the primitive. This means that we get the benefits of rectangle
splitting in Gecko, which doesn't use LocalClip API.

Having rectangle splitting handled as segments also means that we
don't add extra top-level primitives that take extra time to do
visibility culling on, saving some CPU time.

Future work:

Support more cases of primitive splitting.
Port images and gradients to be brushes so they automatically get splitting.

This change is

glennw · 2017-12-01T02:05:59Z

For an example of what this changes, consider the rounded-rects reftest in wrench:

Previously, the clip mask render target looked like this, and each of the primitives was drawn completely in the alpha pass:

Now, there is a clip mask rendered for each clip region of interest, so we render a lot less clip mask pixels:

And, when we draw the main scene, the green segments are drawn in the opaque pass, while the red segments are drawn in the alpha pass:

glennw · 2017-12-01T02:07:13Z

r? @kvark and/or @nical and/or @mrobinson

(I still need to do a gecko try for this and fix any failures, but this is ready for review). Anecdotally, this makes clip mask generation time negligible on most real sites I tested on (exception where there are large inset box shadows, which still need work).

kvark · 2017-12-01T02:56:20Z

It's been a long way... the ideas we discussed in Hawaii took quite a bit to finally land. Amazing work!
I'm happy with the changes, anticipating to see how Gecko is running blazing fast :)
Left my feedback for your consideration.

Reviewed 18 of 18 files at r1, 1 of 1 files at r2, 3 of 7 files at r3.
Review status: 18 of 22 files reviewed at latest revision, 19 unresolved discussions.

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

    int clip_address;
    int z;
    int segment_kind;

the flags there were one per edge, given the need to apply AA on some of the edges and not the others. Is this not the case any more?

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

    vec2 p0 = brush_prim.local_rect.p0;
    vec2 p1 = brush_prim.local_rect.p0 + brush_prim.offsets.xy;

need some comments on what p0-3 are exactly

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

            break;

        case SEGMENT_TOP_LEFT:

I wonder if it would make sense to separate X and Y here, i.e. each can be computed independently and needs 2 bits (0 - whole primitive, 1 - left/top, 2 - right/bottom, 3 - center). They could even share the same helper routine with a switch statement.

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

    switch (brush_prim.aa_kind) {
        case AA_KIND_SEGMENT:
            break;

edge_aa_segment_mask seems to be uninitialized by this point?

webrender/src/border.rs, line 412 at r1 (raw file):

                    &inner_rect,
                    Some(&[
                        BrushSegmentKind::MidLeft,

why are we only including the middle here?

webrender/src/box_shadow.rs, line 304 at r1 (raw file):

                        },
                        None,
                        BrushAntiAliasMode::Default,

I think Primitive/Segment distinction would be cleaner for this enum than Default/Segment

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

    pub bottom_right_offset: LayerVector2D,
    pub segments: [BrushSegment; 9],
    pub enabled_segments: u16,

I see that you are concerned with this structure size for the fact of wrapping Option<Box<_>> to store it and pass it around. I think it's much bigger than it needs to be. Technically we only need a bit mask (enabled_segments) and 4 points to reconstruct all the needed segments wherever they are needed. This would be so small it wouldn't require a box.

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

        outer_rect: &LayerRect,
        inner_rect: &LayerRect,
        valid_segments: Option<&[BrushSegmentKind]>,

How about having a bitmask of the brush segment kinds, and accept the bitmask here directly instead of the slice?

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

                let mut enabled_segments = 0;

                for segment in valid_segments {

this is begging for fold() :)

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

                let brush = &mut self.cpu_brushes[metadata.cpu_prim_index.0];

                if brush.segments.is_none() && metadata.local_rect.size.area() > 256.0 * 256.0 {

let's have the constant lower and named somewhere

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

                            let local_clips = clip_store.get_opt(&clip.clip_sources).expect("bug");
                            for &(ref clip, _) in &local_clips.clips {
                                if let &ClipSource::RoundedRectangle(ref rect, ref radii, mode) = clip {

should put the ClipMode::Clip right in here

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

                                        );
                                        if brush.segments.is_some() {
                                            break;

what happens with others in local_clips.clips? do we just ignore them here? I imagine we should only be splitting with a single clip

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

                match brush.segments {
                    Some(ref mut segments) => {

let's rename it to segment_desc or something... too many segments :)

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

                    Some(ref mut segments) => {
                        for (i, segment) in segments.segments.iter_mut().enumerate() {
                            segment.clip_task_id = if i <= BrushSegmentKind::BottomLeft as usize {

why is the comparison made with BottomLeft?

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

            }
            _ => {
                let clip_task = RenderTask::new_mask(

looks to be the same code as the arm above. Can we merge?

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

    radii: &BorderRadius
) -> Option<Box<BrushSegmentDescriptor>> {
    let inner = match extract_inner_rect_safe(local_clip_rect, radii) {

another thing we could possibly do: if we know the primitive is axis-aligned, we should allow T-junctions and thus generate 3 opaque parts instead of 5

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

) -> Option<Box<BrushSegmentDescriptor>> {
    let inner = match extract_inner_rect_safe(local_clip_rect, radii) {
        Some(rect) => rect,

nit: just map this result instead of the early return

webrender/src/tiling.rs, line 429 at r1 (raw file):

                            let needs_blending = !prim_metadata.opacity.is_opaque ||
                                                 segment.clip_task_id.is_some() ||
                                                 (!is_inner && transform_kind == TransformedRectKind::Complex);

why do we need blending for non-centric opaque segments?

webrender/src/tiling.rs, line 2157 at r1 (raw file):

        }
    }
}

nit: newline

Comments from Reviewable

glennw · 2017-12-01T06:40:53Z

Review status: 14 of 22 files reviewed at latest revision, 19 unresolved discussions.

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