Optimize box shadows - only apply blurs where they can affect the shadow. #1896

glennw · 2017-10-19T06:50:28Z

Add support for regions to blur render tasks. These allow render tasks
to specify one or more regions where the blur should be applied. If
no blur region is supplied, the entire render task is blurred.

Also start expanding the Picture struct, with methods to prepare
it for rendering etc.

Finally, add the basic infrastructure for render tasks to specify
what color they should be cleared to. This allows tasks to specify
a non-default color, if required. This is useful for inset shadows
where we will avoid blurring the inside of the shadow rect.

For outset box shadows with a large blur radius and/or border radius
this patch makes them ~3-4x faster than previously. A follow up will
be to add the same blur regions for inset box shadows.

This change is


        Optimize box shadows - only apply blurs where they can affect the sha…

…dow. Add support for regions to blur render tasks. These allow render tasks to specifiy one or more regions where the blur should be applied. If no blur region is supplied, the entire render task is blurred. Also start expanding the Picture struct, with methods to prepare it for rendering etc. Finally, add the basic infrastructure for render tasks to specify what color they should be cleared to. This allows tasks to specify a non-default color, if required. This is useful for inset shadows where we will avoid blurring the inside of the shadow rect. For outset box shadows with a large blur radius and/or border radius this patch makes them ~3-4x faster than previously. A follow up will be to add the same blur regions for inset box shadows.

glennw · 2017-10-19T06:52:07Z

r? @kvark

I still need to do a try run and WPT tests - but it should be OK to review this when you have time.

kvark · 2017-10-19T19:34:19Z

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

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

        // Borrowcheck dance
        let mut shadows = mem::replace(&mut self.shadow_prim_stack, Vec::new());

I'm quite surprised to see this. If we just iterated on self.shadow_prim_stack.drain(..) it wouldn't prevent us to use the prim_store. Could this dance be a leftover from the previous code?

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

                let metadata = &mut self.prim_store.cpu_metadata[prim_index.0];
                let prim = &self.prim_store.cpu_pictures[metadata.cpu_prim_index.0];
                let shadow = prim.as_text_shadow();

all shadows are considered to be text shadows now?

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

                    let pic_rect = shadow_rect.inflate(blur_offset, blur_offset);
                    let blur_range = 3.0 * blur_radius;

what is 3.0 here? would be nice to make it a constant with a comment on top

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

                    ) * 3.0;

                    blur_regions.push(LayerRect::from_floats(0.0, 0.0, tl.width, tl.height));

should we detect intersections here to avoid extra work on pixels covered by multiple regions?

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

        shadow: Shadow,
    },
    BoxShadow {

that's what I've been thinking of when reviewing the previous version of the PR, nice one!

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

            PrimitiveKind::Picture => {
                self.cpu_pictures[metadata.cpu_prim_index.0]
                    .prepare_for_render(

neat!

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

                let (rect, _) = task.get_target_rect();
                self.device
                    .clear_target_rect(Some(zero_color), None, rect);

so we clear on top of the previous clear here?

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

        } else {
            for region in &self.regions {
                instances.push(BlurInstance {

nit: could create a single BlurInstance before the branch, then doing BlurInstance { region, .. blur_instance } here

Comments from Reviewable

kvark · 2017-10-19T19:35:09Z

For outset box shadows with a large blur radius and/or border radius
this patch makes them ~3-4x faster than previously.

curious, what exactly made this case faster?

Review status: all files reviewed at latest revision, 8 unresolved discussions.

Comments from Reviewable


        Address review comments

glennw · 2017-10-20T00:33:15Z

Review status: 7 of 9 files reviewed at latest revision, 8 unresolved discussions.

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