Fix some issues related to clip optimization across coordinate systems

webrender/src/prim_store.rs

@@ -1316,6 +1316,7 @@ impl PrimitiveStore {
         clip_store: &mut ClipStore,
         node_data: &[ClipScrollNodeData],
         clips: &Vec<ClipWorkItem>,
+        has_clips_from_other_coordinate_systems: bool,
     ) {
         match brush.segment_desc {
             Some(ref segment_desc) => {
@@ -1343,21 +1344,20 @@ impl PrimitiveStore {
             metadata.local_clip_rect
         );
 
-        // If true, we need a clip mask for the entire primitive. This
-        // is either because we don't handle segmenting this clip source,
-        // or we have a clip source from a different coordinate system.
-        let mut clip_mask_kind = BrushClipMaskKind::Individual;
+        // If this primitive is clipped by clips from a different coordinate system, then we
+        // need to apply a clip mask for the entire primitive.
+        let mut clip_mask_kind = match has_clips_from_other_coordinate_systems {
+            true => BrushClipMaskKind::Global,
+            false => BrushClipMaskKind::Individual,
+        };
 
-        // Segment the primitive on all the local-space clip sources
-        // that we can.
+        // Segment the primitive on all the local-space clip sources that we can.
         for clip_item in clips {
             if clip_item.coordinate_system_id != prim_context.scroll_node.coordinate_system_id {
-                clip_mask_kind = BrushClipMaskKind::Global;
                 continue;
             }
 
             let local_clips = clip_store.get_opt(&clip_item.clip_sources).expect("bug");
-
             for &(ref clip, _) in &local_clips.clips {
                 let (local_clip_rect, radius, mode) = match *clip {
                     ClipSource::RoundedRectangle(rect, radii, clip_mode) => {
@@ -1394,11 +1394,7 @@ impl PrimitiveStore {
                     relative_transform.transform_rect(&local_clip_rect)
                 };
 
-                segment_builder.push_rect(
-                    local_clip_rect,
-                    radius,
-                    mode
-                );
+                segment_builder.push_rect(local_clip_rect, radius, mode);
             }
         }
 
@@ -1442,6 +1438,7 @@ impl PrimitiveStore {
         node_data: &[ClipScrollNodeData],
         clips: &Vec<ClipWorkItem>,
         combined_outer_rect: &DeviceIntRect,
+        has_clips_from_other_coordinate_systems: bool,
     ) -> bool {
         let metadata = &self.cpu_metadata[prim_index.0];
         let brush = match metadata.prim_kind {
@@ -1462,7 +1459,8 @@ impl PrimitiveStore {
             prim_context,
             clip_store,
             node_data,
-            clips
+            clips,
+            has_clips_from_other_coordinate_systems,
         );
 
         let segment_desc = match brush.segment_desc {
@@ -1472,28 +1470,30 @@ impl PrimitiveStore {
         let clip_mask_kind = segment_desc.clip_mask_kind;
 
         for segment in &mut segment_desc.segments {
-            segment.clip_task_id = if segment.may_need_clip_mask || clip_mask_kind == BrushClipMaskKind::Global {
-                let segment_screen_rect = calculate_screen_bounding_rect(
-                    &prim_context.scroll_node.world_content_transform,
-                    &segment.local_rect,
-                    prim_context.device_pixel_scale,
-                );
+            if !segment.may_need_clip_mask && clip_mask_kind != BrushClipMaskKind::Global {
+                segment.clip_task_id = None;
+                continue;
+            }
 
-                combined_outer_rect.intersection(&segment_screen_rect).map(|bounds| {
-                    let clip_task = RenderTask::new_mask(
-                        bounds,
-                        clips.clone(),
-                        prim_context.scroll_node.coordinate_system_id,
-                    );
+            let segment_screen_rect = calculate_screen_bounding_rect(
+                &prim_context.scroll_node.world_content_transform,
+                &segment.local_rect,
+                prim_context.device_pixel_scale,
+            );
 
-                    let clip_task_id = render_tasks.add(clip_task);
-                    tasks.push(clip_task_id);
+            let intersected_rect = combined_outer_rect.intersection(&segment_screen_rect);
+            segment.clip_task_id = intersected_rect.map(|bounds| {
+                let clip_task = RenderTask::new_mask(
+                    bounds,
+                    clips.clone(),
+                    prim_context.scroll_node.coordinate_system_id,
+                );
 
-                    clip_task_id
-                })
-            } else {
-                None
-            };
+                let clip_task_id = render_tasks.add(clip_task);
+                tasks.push(clip_task_id);
+
+                clip_task_id
+            })
         }
 
         true
@@ -1572,20 +1572,24 @@ impl PrimitiveStore {
             }
         };
 
+        let mut has_clips_from_other_coordinate_systems = false;
         let mut combined_inner_rect = *screen_rect;
         let clips = convert_clip_chain_to_clip_vector(
             clip_chain,
             extra_clip,
             &combined_outer_rect,
-            &mut combined_inner_rect
+            &mut combined_inner_rect,
+            prim_context.scroll_node.coordinate_system_id,
+            &mut has_clips_from_other_coordinate_systems
         );
 
+        // This can happen if we had no clips or if all the clips were optimized away. In
+        // some cases we still need to create a clip mask in order to create a rectangular
+        // clip in screen space coordinates.
         if clips.is_empty() {
-            // If this item is in the root coordinate system, then
-            // we know that the local_clip_rect in the clip node
-            // will take care of applying this clip, so no need
-            // for a mask.
-            if prim_coordinate_system_id == CoordinateSystemId::root() {
+            // If we don't have any clips from other coordinate systems, the local clip
+            // calculated from the clip chain should be sufficient to ensure proper clipping.
+            if !has_clips_from_other_coordinate_systems {
                 return true;
             }
 
@@ -1613,6 +1617,7 @@ impl PrimitiveStore {
             node_data,
             &clips,
             &combined_outer_rect,
+            has_clips_from_other_coordinate_systems,
         ) {
             return true;
         }
@@ -1973,11 +1978,16 @@ fn convert_clip_chain_to_clip_vector(
     extra_clip: ClipChainNodeRef,
     combined_outer_rect: &DeviceIntRect,
     combined_inner_rect: &mut DeviceIntRect,
+    prim_coordinate_system: CoordinateSystemId,
+    has_clips_from_other_coordinate_systems: &mut bool,
 ) -> Vec<ClipWorkItem> {
     // Filter out all the clip instances that don't contribute to the result.
     ClipChainNodeIter { current: extra_clip }
         .chain(ClipChainNodeIter { current: clip_chain_nodes })
         .filter_map(|node| {
+            *has_clips_from_other_coordinate_systems |=
+                prim_coordinate_system != node.work_item.coordinate_system_id;
+
             *combined_inner_rect = if !node.screen_inner_rect.is_empty() {
                 // If this clip's inner area contains the area of the primitive clipped
                 // by previous clips, then it's not going to affect rendering in any way.

wrench/reftests/clip/reftest.list

@@ -4,3 +4,4 @@
 == clip-3d-transform.yaml clip-3d-transform-ref.yaml
 == clip-corner-overlap.yaml clip-corner-overlap-ref.yaml
 == custom-clip-chains.yaml custom-clip-chains-ref.yaml
+== segmentation-with-other-coordinate-system-clip.yaml segmentation-with-other-coordinate-system-clip-ref.yaml

wrench/reftests/clip/segmentation-with-other-coordinate-system-clip-ref.yaml

@@ -0,0 +1,8 @@
+---
+root:
+  items:
+  -
+    type: rect
+    color: green
+    bounds: [0, 0, 100, 100]
+    "clip-rect": [0, 0, 100, 100]

wrench/reftests/clip/segmentation-with-other-coordinate-system-clip.yaml

@@ -0,0 +1,47 @@
+# This is testing whether a clip properly clips a primitive in another
+# coordinate system that is segmented.
+# See https://github.com/servo/webrender/issues/2294
+---
+root:
+  items:
+  -
+    type: "stacking-context"
+    items:
+    -
+      bounds: [0, 0, 100, 100]
+      "clip-rect": [0, 0, 100, 100]
+      type: clip
+      items:
+      -
+        type: "stacking-context"
+        transform: [0.98883086, 0.14904226, 0, 0, -0.14904226, 0.98883086, 0, 0, 0, 0, 1, 0, 1533.3134, 109.21605, 0, 1]
+        items:
+        -
+          type: "stacking-context"
+          transform: [0.7818315, 0.6234898, 0, 0, -0.6234898, 0.7818315, 0, 0, 0, 0, 1, 0, 132.98201, -64.04077, 0, 1]
+          items:
+          -
+            type: "stacking-context"
+            transform: [0.93087375, 0.36534107, 0, 0, -0.36534107, 0.93087375, 0, 0, 0, 0, 1, 0, 68.64584, -46.80194, 0, 1]
+            items:
+            -
+              type: "stacking-context"
+              transform: [0.8262389, 0.56332004, 0, 0, -0.56332004, 0.8262389, 0, 0, 0, 0, 1, 0, 116.458824, -61.550323, 0, 1]
+              items:
+              -
+                type: "stacking-context"
+                transform: [0.90096885, 0.43388373, 0, 0, -0.43388373, 0.90096885, 0, 0, 0, 0, 1, 0, 84.200554, -52.906708, 0, 1]
+                items:
+                -
+                  type: "stacking-context"
+                  transform: [0.98883086, 0.14904226, 0, 0, -0.14904226, 0.98883086, 0, 0, 0, 0, 1, 0, 25.3134, -21.78395, 0, 1]
+                  items:
+                  -
+                    type: "stacking-context"
+                    transform: [0.73305184, 0.68017274, 0, 0, -0.68017274, 0.73305184, 0, 0, 0, 0, 1, 0, 149.64511, -65.28949, 0, 1]
+                    items:
+                    -
+                      bounds: [1000, 0, 1000, 1000]
+                      "clip-rect": [1000, 0, 1000, 1000]
+                      type: rect
+                      color: green