Bug 1696089 - Clip chains can incorrectly propagate to inner primitives in some cases. r=gfx-reviewers,kvark,bradwerth

Fix a bug that can occur when:
 - Parent stacking context is considered redundant
 - Parent stacking context has a transform
 - Parent stacking context establishes a raster root
 - Parent stacking context has a clip
 - Child stacking context has a filter (or other feature requiring a surface)

In these cases, the clips would be incorrectly propagated to the
primitives inside the child stacking context, instead of applied
to the child stacking context surface itself. This can cause correctness
issues when raster roots are established, and potential performance
issues if raster roots are not established.

Differential Revision: https://phabricator.services.mozilla.com/D107024

gfx/wr/webrender/src/clip.rs

@@ -220,7 +220,9 @@ impl ClipChainBuilder {
                 )
             }
             None => {
-                ClipChainId::NONE
+                // Even if the clip id is None, it's possible that there were parent clips in the builder
+                // that need to be applied and set as the root of this clip-chain builder.
+                parent_clip_chain_id
             }
         };
 

gfx/wr/webrender/src/scene_building.rs

@@ -660,6 +660,7 @@ impl<'a> SceneBuilder<'a> {
                             info.stacking_context.clip_id,
                             info.stacking_context.raster_space,
                             info.stacking_context.flags,
+                            bc.pipeline_id,
                         );
 
                         self.rf_mapper.push_offset(info.origin.to_vector());
@@ -1737,6 +1738,7 @@ impl<'a> SceneBuilder<'a> {
         clip_id: Option<ClipId>,
         requested_raster_space: RasterSpace,
         flags: StackingContextFlags,
+        pipeline_id: PipelineId,
     ) -> StackingContextInfo {
         profile_scope!("push_stacking_context");
 
@@ -1844,7 +1846,7 @@ impl<'a> SceneBuilder<'a> {
         }
 
         let mut sc_info = StackingContextInfo {
-            pop_clip_root: false,
+            pop_hit_testing_clip: false,
             pop_stacking_context: false,
             pop_containing_block: false,
             set_tile_cache_barrier,
@@ -1856,31 +1858,36 @@ impl<'a> SceneBuilder<'a> {
             self.containing_block_stack.push(spatial_node_index);
         }
 
-        // This muct be built before the push_clip_root logic below
-        let clip_chain_id = match clip_id {
-            Some(clip_id) => self.clip_store.get_or_build_clip_chain_id(clip_id),
-            None => ClipChainId::NONE,
+        // If this stacking context is redundant, we don't care about getting a clip-chain for it.
+        // However, if we _do_ have a clip, we must build it here before the `push_clip_root`
+        // calls below, to ensure we get the clips for drawing this stacking context itself.
+        let clip_chain_id = if is_redundant {
+            ClipChainId::NONE
+        }  else {
+            // Get a clip-chain for this stacking context - even if the stacking context
+            // itself has no clips, it's possible that there are clips to collect from
+            // the previous clip-chain builder.
+            let clip_id = clip_id.unwrap_or(ClipId::root(pipeline_id));
+            self.clip_store.get_or_build_clip_chain_id(clip_id)
         };
 
-        // If this has a valid clip, it will create a new clip root
+        // If this has a valid clip, register with the hit-testing scene
         if let Some(clip_id) = clip_id {
-            sc_info.pop_clip_root = true;
-
-            // If this stacking context is redundant (prims will be pushed into
-            // the parent during pop) but it has a valid clip, then we need to
-            // add that clip to the current clip chain builder, so it's correctly
-            // applied to any primitives within this redundant stacking context.
-            // For the normal case, we start a new clip root, knowing that the
-            // clip on this stacking context will be pushed onto the stack during
-            // frame building.
-            if is_redundant {
-                self.clip_store.push_clip_root(Some(clip_id), true);
-            } else {
-                self.clip_store.push_clip_root(None, false);
-            }
-
-            // Push this clip id into the hit-testing scene for child primitives
             self.hit_testing_scene.push_clip(clip_id);
+            sc_info.pop_hit_testing_clip = true;
+        }
+
+        // If this stacking context is redundant (prims will be pushed into
+        // the parent during pop) but it has a valid clip, then we need to
+        // add that clip to the current clip chain builder, so it's correctly
+        // applied to any primitives within this redundant stacking context.
+        // For the normal case, we start a new clip root, knowing that the
+        // clip on this stacking context will be pushed onto the stack during
+        // frame building.
+        if is_redundant {
+            self.clip_store.push_clip_root(clip_id, true);
+        } else {
+            self.clip_store.push_clip_root(None, false);
         }
 
         // If not redundant, create a stacking context to hold primitive clusters
@@ -1916,6 +1923,9 @@ impl<'a> SceneBuilder<'a> {
         // Pop off current raster space (pushed unconditionally in push_stacking_context)
         self.raster_space_stack.pop().unwrap();
 
+        // Pop off clip builder root (pushed unconditionally in push_stacking_context)
+        self.clip_store.pop_clip_root();
+
         // If the stacking context formed a containing block, pop off the stack
         if info.pop_containing_block {
             self.containing_block_stack.pop().unwrap();
@@ -1926,8 +1936,7 @@ impl<'a> SceneBuilder<'a> {
         }
 
         // If the stacking context established a clip root, pop off the stack
-        if info.pop_clip_root {
-            self.clip_store.pop_clip_root();
+        if info.pop_hit_testing_clip {
             self.hit_testing_scene.pop_clip();
         }
 
@@ -3590,8 +3599,8 @@ struct ExtendedPrimitiveInstance {
 /// Internal tracking information about the currently pushed stacking context.
 /// Used to track what operations need to happen when a stacking context is popped.
 struct StackingContextInfo {
-    /// If true, pop an entry from the clip root stack.
-    pop_clip_root: bool,
+    /// If true, pop an entry from the hit-testing scene.
+    pop_hit_testing_clip: bool,
     /// If true, pop and entry from the containing block stack.
     pop_containing_block: bool,
     /// If true, pop an entry from the flattened stacking context stack.

gfx/wr/wrench/reftests/filters/blend-clipped-raster-root.yaml

@@ -0,0 +1,22 @@
+---
+root:
+  items:
+    -
+      bounds: [0, 100, 1000, 1000]
+      type: clip
+      id: 3
+    -
+      bounds: [200, 200, 0, 0]
+      clip-node: 3
+      type: "stacking-context"
+      transform: rotate(10)
+      items:
+        -
+          type: "stacking-context"
+          filters:
+            - opacity(0.5)
+          items:
+            -
+              bounds: [0, 0, 1000, 500]
+              type: rect
+              color: green

gfx/wr/wrench/reftests/filters/reftest.list

@@ -66,3 +66,4 @@ platform(linux,mac) == backdrop-filter-perspective.yaml backdrop-filter-perspect
 platform(linux,max) == svg-filter-offset.yaml svg-filter-offset-ref.yaml
 skip_on(android,device) == fuzzy(1,100) svg-filter-composite.yaml svg-filter-composite-ref.yaml
 skip_on(android,device) == filter-mix-blend-scaling.yaml filter-mix-blend-scaling-ref.yaml
+platform(linux) == blend-clipped-raster-root.yaml blend-clipped-raster-root.png