Fix issue with optimized away clip segments

When clip segments were optimized away due to having no intersection
with the outer screen bounds, they were treated as opaque regions. This
is due to the fact that an Option is not expressive enough to represent
the three different kinds of distinct destinies of clip segments.

This change replaces the Option with an enum that is either a
RenderTaskId, Opaque, or Empty to represent a clip segment that should
not be rendered.

Fixes #2700.

webrender/src/batch.rs

@@ -19,8 +19,9 @@ use gpu_types::ZBufferIdGenerator;
 use internal_types::{FastHashMap, SavedTargetIndex, SourceTexture};
 use picture::{PictureCompositeMode, PicturePrimitive, PictureSurface};
 use plane_split::{BspSplitter, Polygon, Splitter};
-use prim_store::{CachedGradient, ImageSource, PrimitiveIndex, PrimitiveKind, PrimitiveMetadata, PrimitiveStore};
-use prim_store::{BrushPrimitive, BrushKind, DeferredResolve, EdgeAaSegmentMask, PictureIndex, PrimitiveRun};
+use prim_store::{BrushKind, BrushPrimitive, BrushSegmentTaskId, CachedGradient, DeferredResolve};
+use prim_store::{EdgeAaSegmentMask, ImageSource, PictureIndex, PrimitiveIndex, PrimitiveKind};
+use prim_store::{PrimitiveMetadata, PrimitiveRun, PrimitiveStore};
 use render_task::{RenderTaskAddress, RenderTaskId, RenderTaskKind, RenderTaskTree};
 use renderer::{BlendMode, ImageBufferKind};
 use renderer::{BLOCKS_PER_UV_RECT, ShaderColorMode};
@@ -1270,12 +1271,15 @@ impl AlphaBatchBuilder {
                 for (i, segment) in segment_desc.segments.iter().enumerate() {
                     let is_inner = segment.edge_flags.is_empty();
                     let needs_blending = !prim_metadata.opacity.is_opaque ||
-                                         segment.clip_task_id.is_some() ||
+                                         segment.clip_task_id.needs_blending() ||
                                          (!is_inner && transform_kind == TransformedRectKind::Complex);
 
-                    let clip_task_address = segment
-                        .clip_task_id
-                        .map_or(OPAQUE_TASK_ADDRESS, |id| render_tasks.get_task_address(id));
+                    let clip_task_address = match segment.clip_task_id {
+                        BrushSegmentTaskId::RenderTaskId(id) =>
+                            render_tasks.get_task_address(id),
+                        BrushSegmentTaskId::Opaque => OPAQUE_TASK_ADDRESS,
+                        BrushSegmentTaskId::Empty => continue,
+                    };
 
                     let instance = PrimitiveInstance::from(BrushInstance {
                         segment_index: i as i32,

webrender/src/prim_store.rs

@@ -331,10 +331,26 @@ bitflags! {
     }
 }
 
+#[derive(Debug)]
+pub enum BrushSegmentTaskId {
+    RenderTaskId(RenderTaskId),
+    Opaque,
+    Empty,
+}
+
+impl BrushSegmentTaskId {
+    pub fn needs_blending(&self) -> bool {
+        match *self {
+            BrushSegmentTaskId::RenderTaskId(..) => true,
+            BrushSegmentTaskId::Opaque | BrushSegmentTaskId::Empty => false,
+        }
+    }
+}
+
 #[derive(Debug)]
 pub struct BrushSegment {
     pub local_rect: LayoutRect,
-    pub clip_task_id: Option<RenderTaskId>,
+    pub clip_task_id: BrushSegmentTaskId,
     pub may_need_clip_mask: bool,
     pub edge_flags: EdgeAaSegmentMask,
 }
@@ -348,7 +364,7 @@ impl BrushSegment {
     ) -> BrushSegment {
         BrushSegment {
             local_rect: LayoutRect::new(origin, size),
-            clip_task_id: None,
+            clip_task_id: BrushSegmentTaskId::Opaque,
             may_need_clip_mask,
             edge_flags,
         }
@@ -1923,7 +1939,7 @@ impl PrimitiveStore {
 
         for segment in &mut segment_desc.segments {
             if !segment.may_need_clip_mask && clip_mask_kind != BrushClipMaskKind::Global {
-                segment.clip_task_id = None;
+                segment.clip_task_id = BrushSegmentTaskId::Opaque;
                 continue;
             }
 
@@ -1933,23 +1949,27 @@ impl PrimitiveStore {
                 frame_context.device_pixel_scale,
             );
 
-            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_run_context.scroll_node.coordinate_system_id,
-                    frame_state.clip_store,
-                    frame_state.gpu_cache,
-                    frame_state.resource_cache,
-                    frame_state.render_tasks,
-                );
+            let bounds = match combined_outer_rect.intersection(&segment_screen_rect) {
+                Some(bounds) => bounds,
+                None => {
+                    segment.clip_task_id = BrushSegmentTaskId::Empty;
+                    continue;
+                }
+            };
 
-                let clip_task_id = frame_state.render_tasks.add(clip_task);
-                pic_state.tasks.push(clip_task_id);
+            let clip_task = RenderTask::new_mask(
+                bounds,
+                clips.clone(),
+                prim_run_context.scroll_node.coordinate_system_id,
+                frame_state.clip_store,
+                frame_state.gpu_cache,
+                frame_state.resource_cache,
+                frame_state.render_tasks,
+            );
 
-                clip_task_id
-            })
+            let clip_task_id = frame_state.render_tasks.add(clip_task);
+            pic_state.tasks.push(clip_task_id);
+            segment.clip_task_id = BrushSegmentTaskId::RenderTaskId(clip_task_id);
         }
 
         true
@@ -1961,7 +1981,7 @@ impl PrimitiveStore {
         if metadata.prim_kind == PrimitiveKind::Brush {
             if let Some(ref mut desc) = self.cpu_brushes[metadata.cpu_prim_index.0].segment_desc {
                 for segment in &mut desc.segments {
-                    segment.clip_task_id = None;
+                    segment.clip_task_id = BrushSegmentTaskId::Opaque;
                 }
             }
         }

wrench/reftests/clip/reftest.list

@@ -8,5 +8,6 @@ platform(linux,mac) == clip-45-degree-rotation.yaml clip-45-degree-rotation-ref.
 == custom-clip-chain-node-ancestors.yaml custom-clip-chain-node-ancestors-ref.yaml
 == fixed-position-clipping.yaml fixed-position-clipping-ref.yaml
 == segmentation-with-other-coordinate-system-clip.yaml segmentation-with-other-coordinate-system-clip-ref.yaml
+== segmentation-across-rotation.yaml segmentation-across-rotation-ref.yaml
 == stacking-context-clip.yaml stacking-context-clip-ref.yaml
 == snapping.yaml snapping-ref.yaml

wrench/reftests/clip/segmentation-across-rotation-ref.yaml

@@ -0,0 +1,6 @@
+---
+root:
+  items:
+    - type: rect
+      color: [0, 255, 0, 1]
+      bounds: [100, 100, 100, 100]

wrench/reftests/clip/segmentation-across-rotation.yaml

@@ -0,0 +1,30 @@
+# This test ensures that a clip that is segmented, with segments
+# that have no intersection with the world-space outer bounds of
+# the clip rectangle render correctly. In this case the first clip
+# defines the outer bounds of the clip rectangle and the rotation
+# ensures that the inner clip isn't optimized away completely. The
+# segments of the rounded corner clip don't have any intersection at
+# all with the clip in area from the outer clip, so they shouldn't
+# affect the rendering of the green square.
+---
+root:
+  items:
+    - type: clip
+      bounds: [100, 100, 100, 100]
+      id: 2
+    -
+      type: stacking-context
+      clip-and-scroll: 2
+      transform: rotate(0.25)
+      items:
+       - type: clip
+         bounds: [0, 0, 2400, 900]
+         id: 3
+         complex:
+           - rect: [ 0, 0, 2400, 900 ]
+             radius: 50
+       - type: rect
+         color: [0, 255, 0, 1]
+         bounds: [0, 0, 2400, 900]
+         clip-and-scroll: 3
+