Clip chain refactor for local picture raster support. #2951

gw3583 · 2018-08-06T10:31:55Z

There's a large number of related changes in this
patch. The overall goal is to make clip chain handling
not depend on screen rectangle generation. This
will make it much easier to implement local space
picture rasterization for perspective elements. In
addition to that, we unify how user / hierarchical and
primitive clip chain nodes are handled.

Clip chain instances are built on demand now, as
required by primitives. Right now, none of the
clip chain information is cached between primitives.
In general, now that the calculations are done in
local space, they are much faster - however we can
easily start to cache the common clip-chain state
(per raster coordinate system) if it ever shows up
in a profile. Additionally, since we only build
clip chains on demand, and clip chains for non-
visible primitives are not built, which is a
decent saving on many pages.

Remove ClipNode altogether. Instead, during flattening
we map directly from a ClipId to a ClipChainId
as appropriate. Clip chains are now created during
flattening and the links remain constant. It's only
during construction of a clip chain instance that we
include and exclude parts of the specified clip
chain. This unifies how we handle user defined clip
chains and legacy hierarchical clip chains. At the
same time, unify how the optional primitive specific
complex clip is handled with clip chains. Now, if
a primitive has a complex clip, it's added as a
normal clip chain node, and linked to the parent
clip chain that this primitive would otherwise
be attached to.

Primitive runs now only depend on the spatial node.
This results in far fewer primitive run breaks.

ClipWorkItems are now stored as a single contiguous
array in the ClipStore. To achieve this they are
stored as an index buffer, and a clip node range
is simply an (index, count). These are a lot
cheaper to pass to clip masks, and also require
fewer heap allocations.

Port hit testing to use the same clip chain data
structures that the main clip code uses, now that
the hierarchical and user defined clip chains are
handled in a uniform way.

Remove the need for Arc in clip chain code, since
clip chains are now stored in a contiguous index
buffer style structure.

This change is

gw3583 · 2018-08-06T10:34:08Z

This still needs some more work, specifically:

Several test failures on a try run.
More code comments / documentation.
Code tidy up.

Anecdotally it seems to be quite a bit faster on the sites I've tested against than previously, although I haven't done detailed performance numbers yet.

It's probably not quite ready for review yet, but if anyone wants to start taking a look at it, go for it 👍

gw3583 · 2018-08-06T10:36:15Z

If anyone has any general comments on the approach taken, and anything obvious missing, please let me know :)

gw3583 · 2018-08-07T07:18:32Z

I added some poorly drawn ASCII diagrams to try and describe how the data structures fit together (a65f564#diff-97cd013eed8f4591bd7659faf4352fd2R23).

I have fixed the test failues in wrench, and most of the test failures in gecko try.

Tomorrow I will try to resolve the remaining failures in gecko try, which are mostly 3d transform related.


        There's a large number of related changes in this

patch. The overall goal is to make clip chain handling not depend on screen rectangle generation. This will make it much easier to implement local space picture rasterization for perspective elements. In addition to that, we unify how user / hierarchical and primitive clip chain nodes are handled. Clip chain instances are built on demand now, as required by primitives. Right now, none of the clip chain information is cached between primitives. In general, now that the calculations are done in local space, they are much faster - however we can easily start to cache the common clip-chain state (per raster coordinate system) if it ever shows up in a profile. Additionally, since we only build clip chains on demand, and clip chains for non- visible primitives are not built, which is a decent saving on many pages. Remove ClipNode altogether. Instead, during flattening we map directly from a ClipId to a ClipChainId as appropriate. Clip chains are now created during flattening and the links remain constant. It's only during construction of a clip chain instance that we include and exclude parts of the specified clip chain. This unifies how we handle user defined clip chains and legacy hierarchical clip chains. At the same time, unify how the optional primitive specific complex clip is handled with clip chains. Now, if a primitive has a complex clip, it's added as a normal clip chain node, and linked to the parent clip chain that this primitive would otherwise be attached to. Primitive runs now only depend on the spatial node. This results in far fewer primitive run breaks. ClipWorkItems are now stored as a single contiguous array in the ClipStore. To achieve this they are stored as an index buffer, and a clip node range is simply an (index, count). These are a lot cheaper to pass to clip masks, and also require fewer heap allocations. Port hit testing to use the same clip chain data structures that the main clip code uses, now that the hierarchical and user defined clip chains are handled in a uniform way. Remove the need for Arc in clip chain code, since clip chains are now stored in a contiguous index buffer style structure.

gw3583 · 2018-08-08T04:42:23Z

Try run is looking very green now - just a few orange items with unrelated intermittents.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e084e4749dbb158bc76efcb504db8e10173b2e1d

r? @kvark
cc @mrobinson

kvark

This is an amazing change. I like how the function signatures got lighter, and the forest of clip types is now easier to grasp.
Left my notes below. Major concern is naming.

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

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

 ClipStore - Main interface used by other modules.

 ClipSource - A single clip item (e.g. a rounded rect, or a box shadow).

maybe rename to ClipItem?
When I read source I imagine it being able to produce multiple things, hence the singular semantic of this is not obvious, and ClipSources causes confusion

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

       +----------------+

 ClipSourcesId - A clip sources id identifies a range of clip nodes. It is stored

maybe ClipItemRange, or even Range<ClipNodeIndex> or something like this?

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

// Result of comparing a clip node instance against a local rect.
#[derive(Debug)]
enum ClipKind {

sounds more like ClipResult

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

// node, a bounds error will occur.
impl ClipChainId {
    pub const NONE: Self = ClipChainId(u32::MAX);

we should eventually get rid of NONE and use Option<ClipChainId>, given that NonZeroU32 is stable now.

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

#[cfg_attr(feature = "capture", derive(Serialize))]
#[cfg_attr(feature = "replay", derive(Deserialize))]
pub struct ClipNodeRange {

why not just Range<ClipNodeInstanceIndex>? (not entirely clear what types are being indexed - and this should be made more clear by typedeffing this index type to a meaningful name

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

#[derive(Debug)]
enum ClipSpaceConversion {
    Local,

does this variant give us anything over Offset(0)?

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

    }

    fn transform_to_clip_space(&self, rect: &LayoutRect) -> Option<LayoutRect> {

this is confusing, given that "clip space" has a much different semantics in computer graphics
maybe transform_from_prim_space instead?

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

                }
            }
            _ => {}

nit: let's list those explicitly

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

    pub clips_range: ClipNodeRange,
    pub local_clip_rect: LayoutRect,
    pub has_clips_from_other_coordinate_systems: bool,

would it make sense to use ClipNodeFlags here?

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

                }
            })
            .collect();

this collection is not needed.

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

                    Some(ClipSpaceConversion::Offset(offset))
                } else {
                    // TODO(gw): We still have issues with clip nodes and primitives wher

typo "wher"

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

                } else {
                    // TODO(gw): We still have issues with clip nodes and primitives wher
                    //           there is perspective occurring. We intend to fix these

perspective transform?

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

                            if spatial_node_index == clip_node.spatial_node_index ||
                               ref_spatial_node.coordinate_system_id == clip_spatial_node.coordinate_system_id {

the second condition could shadow the first one (i.e the first one seems unnecessary)

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

                                    Some(new_local_clip_rect) => new_local_clip_rect,
                                    None => {
                                        return None

we'll need to route the chasing logic here (as a followup by me, presumably)

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

                        // building logic.
                        let flags = match node_info.conversion {
                            ClipSpaceConversion::Local => {

interesting, so Local is useful here

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

    // used to eliminate redundant clips, and reduce the size of
    // any clip mask that eventually gets drawn.
    fn get_local_clip_rect(&self) -> Option<LayoutRect> {

that function is so nice, can't get my eyes off it :)

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

                }

                match clip_rect.intersection(prim_rect) {

nit: could also contain the intersection with prim_rect itself instead of checking for contains_rect earlier?

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

                }

                match clip_rect.intersection(prim_rect) {

nit: might be tidier to do match (clip_mode, clip_rect.intersection(prim_rect))
just as an idea

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

                // TODO(gw): extract_inner_rect_safe is overly
                //           conservative for this code!
                let inner_clip_rect = extract_inner_rect_safe(clip_rect, radius);

iirc, it's fairly fast

webrender/src/display_list_flattener.rs, line 759 at r1 (raw file):

        clip_sources: Vec<ClipSource>,
        spatial_node_index: SpatialNodeIndex,
        default_clip_chain_id: ClipChainId,

shouldn't it be called parent_clip_chain_id?

webrender/src/display_list_flattener.rs, line 1284 at r1 (raw file):

        I: IntoIterator<Item = ComplexClipRegion>
    {
        // Add a new ClipNode - this is a ClipId that identifies a list of clip sources,

let me get this straight: so, add_clip_node returns a ClpChainId that is actually a ClipId that identifies xxxx
Either the comment or the signature need to be updated so that it's more clear

webrender/src/display_list_flattener.rs, line 1291 at r1 (raw file):

            .id_to_index_mapper
            .get_clip_chain_id(&parent_id);
        // Map the ClipId for the positioning node to a spatial node index.

uh, we need to completely split ClipId and positional information, eventually

webrender/src/display_list_flattener.rs, line 1295 at r1 (raw file):

        // Build the clip sources from the supplied region.
        // TODO(gw): We should fix this up to take advantage of the recent

I'll clean this up

webrender/src/hit_test.rs, line 78 at r1 (raw file):

enum HitTestRegion {
    Invalid,

does it make sense versus passing Result<HitTestRegion, SomeError> where appropriate?

webrender/src/hit_test.rs, line 152 at r1 (raw file):

        for clip_chain in &clip_store.clip_chain_nodes {
            self.clip_chains.push(clip_chain.clone());

nit: self.clip_chains.extend_from_slice(&clip_store.clip_chain_nodes)


        Address review comments.

gw3583

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

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