Bug 1594934 - Handle clip rects in determining picture cache primitive dependencies. r=nical
authorGlenn Watson <git@intuitionlibrary.com>
Sun, 10 Nov 2019 19:05:23 +0000
changeset 501449 afa3fdcab69155f37dc35daa0cdb22dbb849b907
parent 501448 aafdd02eb30a2dae68eefb1505083254634d8034
child 501450 5b7f42d356518dc7a92efd3a1bfc8b4340d4ff10
push id114170
push usermalexandru@mozilla.com
push dateTue, 12 Nov 2019 21:58:32 +0000
treeherdermozilla-inbound@9e3f44e87a1a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnical
bugs1594934
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1594934 - Handle clip rects in determining picture cache primitive dependencies. r=nical This reduces the amount of invalidation on some pages very significantly. It also fixes a number of cases where picture caching "fails", resulting in everything invalidating every frame. Differential Revision: https://phabricator.services.mozilla.com/D52275
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/prim_store/mod.rs
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -375,18 +375,18 @@ struct PrimitiveDependencyInfo {
     is_cacheable: bool,
 
     /// If true, we should clip the prim rect to the tile boundaries.
     clip_by_tile: bool,
 
     /// Unique content identifier of the primitive.
     prim_uid: ItemUid,
 
-    /// The (conservative) area in picture space this primitive occupies.
-    prim_rect: PictureRect,
+    /// The picture space origin of this primitive.
+    prim_origin: PicturePoint,
 
     /// The (conservative) clipped area in picture space this primitive occupies.
     prim_clip_rect: PictureRect,
 
     /// Image keys this primitive depends on.
     image_keys: SmallVec<[ImageKey; 8]>,
 
     /// Opacity bindings this primitive depends on.
@@ -398,27 +398,28 @@ struct PrimitiveDependencyInfo {
     /// Spatial nodes references by the clip dependencies of this primitive.
     spatial_nodes: SmallVec<[SpatialNodeIndex; 4]>,
 }
 
 impl PrimitiveDependencyInfo {
     /// Construct dependency info for a new primitive.
     fn new(
         prim_uid: ItemUid,
-        prim_rect: PictureRect,
+        prim_origin: PicturePoint,
+        prim_clip_rect: PictureRect,
         is_cacheable: bool,
     ) -> Self {
         PrimitiveDependencyInfo {
             prim_uid,
-            prim_rect,
+            prim_origin,
             is_cacheable,
             image_keys: SmallVec::new(),
             opacity_bindings: SmallVec::new(),
             clip_by_tile: false,
-            prim_clip_rect: PictureRect::zero(),
+            prim_clip_rect,
             clips: SmallVec::new(),
             spatial_nodes: SmallVec::new(),
         }
     }
 }
 
 /// A stable ID for a given tile, to help debugging. These are also used
 /// as unique identfiers for tile surfaces when using a native compositor.
@@ -784,17 +785,17 @@ impl Tile {
         // If this tile isn't currently visible, we don't want to update the dependencies
         // for this tile, as an optimization, since it won't be drawn anyway.
         if !self.is_visible {
             return;
         }
 
         // Mark if the tile is cacheable at all.
         if !info.is_cacheable {
-            self.invalidate(Some(info.prim_rect), InvalidationReason::NonCacheable);
+            self.invalidate(Some(info.prim_clip_rect), InvalidationReason::NonCacheable);
         }
 
         // Include any image keys this tile depends on.
         self.current_descriptor.image_keys.extend_from_slice(&info.image_keys);
 
         // Include any opacity bindings this primitive depends on.
         self.current_descriptor.opacity_bindings.extend_from_slice(&info.opacity_bindings);
 
@@ -818,29 +819,29 @@ impl Tile {
 
             let clip_p1 = PicturePoint::new(
                 clampf(info.prim_clip_rect.origin.x + info.prim_clip_rect.size.width, tile_p0.x, tile_p1.x),
                 clampf(info.prim_clip_rect.origin.y + info.prim_clip_rect.size.height, tile_p0.y, tile_p1.y),
             );
 
             (
                 PicturePoint::new(
-                    clampf(info.prim_rect.origin.x, tile_p0.x, tile_p1.x),
-                    clampf(info.prim_rect.origin.y, tile_p0.y, tile_p1.y),
+                    clampf(info.prim_origin.x, tile_p0.x, tile_p1.x),
+                    clampf(info.prim_origin.y, tile_p0.y, tile_p1.y),
                 ),
                 PictureRect::new(
                     clip_p0,
                     PictureSize::new(
                         clip_p1.x - clip_p0.x,
                         clip_p1.y - clip_p0.y,
                     ),
                 ),
             )
         } else {
-            (info.prim_rect.origin, info.prim_clip_rect)
+            (info.prim_origin, info.prim_clip_rect)
         };
 
         // Update the tile descriptor, used for tile comparison during scene swaps.
         let prim_index = PrimitiveDependencyIndex(self.current_descriptor.prims.len() as u32);
 
         // We know that the casts below will never overflow because the array lengths are
         // truncated to MAX_PRIM_SUB_DEPS during update_prim_dependencies.
         debug_assert!(info.spatial_nodes.len() <= MAX_PRIM_SUB_DEPS);
@@ -854,17 +855,17 @@ impl Tile {
             prim_clip_rect: prim_clip_rect.into(),
             transform_dep_count: info.spatial_nodes.len()  as u8,
             clip_dep_count: info.clips.len() as u8,
             image_dep_count: info.image_keys.len() as u8,
             opacity_binding_dep_count: info.opacity_bindings.len() as u8,
         });
 
         // Add this primitive to the dirty rect quadtree.
-        self.root.add_prim(prim_index, &info.prim_rect);
+        self.root.add_prim(prim_index, &info.prim_clip_rect);
     }
 
     /// Called during tile cache instance post_update. Allows invalidation and dirty
     /// rect calculation after primitive dependencies have been updated.
     fn post_update(
         &mut self,
         ctx: &TilePostUpdateContext,
         state: &mut TilePostUpdateState,
@@ -1440,16 +1441,18 @@ pub struct TileCacheInstance {
     /// The currently selected tile size to use for this cache
     pub current_tile_size: DeviceIntSize,
     /// The positioning node for this tile cache.
     pub spatial_node_index: SpatialNodeIndex,
     /// Hash of tiles present in this picture.
     pub tiles: FastHashMap<TileOffset, Tile>,
     /// A helper struct to map local rects into surface coords.
     map_local_to_surface: SpaceMapper<LayoutPixel, PicturePixel>,
+    /// A helper struct to map child picture rects into picture cache surface coords.
+    map_child_pic_to_surface: SpaceMapper<PicturePixel, PicturePixel>,
     /// List of opacity bindings, with some extra information
     /// about whether they changed since last frame.
     opacity_bindings: FastHashMap<PropertyBindingId, OpacityBindingInfo>,
     /// List of spatial nodes, with some extra information
     /// about whether they changed since last frame.
     spatial_nodes: FastHashMap<SpatialNodeIndex, SpatialNodeDependency>,
     /// A set of spatial nodes that primitives / clips depend on found
     /// during dependency creation. This is used to avoid trying to
@@ -1512,16 +1515,20 @@ impl TileCacheInstance {
         TileCacheInstance {
             slice,
             spatial_node_index,
             tiles: FastHashMap::default(),
             map_local_to_surface: SpaceMapper::new(
                 ROOT_SPATIAL_NODE_INDEX,
                 PictureRect::zero(),
             ),
+            map_child_pic_to_surface: SpaceMapper::new(
+                ROOT_SPATIAL_NODE_INDEX,
+                PictureRect::zero(),
+            ),
             opacity_bindings: FastHashMap::default(),
             spatial_nodes: FastHashMap::default(),
             used_spatial_nodes: FastHashSet::default(),
             dirty_region: DirtyRegion::new(),
             tile_size: PictureSize::zero(),
             tile_rect: TileRect::zero(),
             tile_bounds_p0: TileOffset::zero(),
             tile_bounds_p1: TileOffset::zero(),
@@ -1592,16 +1599,20 @@ impl TileCacheInstance {
         // during the prim dependency checks.
         self.backdrop = BackdropInfo::empty();
         self.subpixel_mode = SubpixelMode::Allow;
 
         self.map_local_to_surface = SpaceMapper::new(
             self.spatial_node_index,
             PictureRect::from_untyped(&pic_rect.to_untyped()),
         );
+        self.map_child_pic_to_surface = SpaceMapper::new(
+            self.spatial_node_index,
+            PictureRect::from_untyped(&pic_rect.to_untyped()),
+        );
 
         let pic_to_world_mapper = SpaceMapper::new_with_target(
             ROOT_SPATIAL_NODE_INDEX,
             self.spatial_node_index,
             frame_context.global_screen_world_rect,
             frame_context.clip_scroll_tree,
         );
 
@@ -1872,76 +1883,97 @@ impl TileCacheInstance {
         clip_scroll_tree: &ClipScrollTree,
         data_stores: &DataStores,
         clip_store: &ClipStore,
         pictures: &[PicturePrimitive],
         resource_cache: &ResourceCache,
         opacity_binding_store: &OpacityBindingStorage,
         image_instances: &ImageInstanceStorage,
         surface_index: SurfaceIndex,
+        surface_spatial_node_index: SpatialNodeIndex,
     ) -> bool {
+        // If the primitive is completely clipped out by the clip chain, there
+        // is no need to add it to any primitive dependencies.
+        let prim_clip_chain = match prim_clip_chain {
+            Some(prim_clip_chain) => prim_clip_chain,
+            None => return false,
+        };
+
         self.map_local_to_surface.set_target_spatial_node(
             prim_spatial_node_index,
             clip_scroll_tree,
         );
 
         // Map the primitive local rect into picture space.
         let prim_rect = match self.map_local_to_surface.map(&local_prim_rect) {
             Some(rect) => rect,
             None => return false,
         };
 
         // If the rect is invalid, no need to create dependencies.
         if prim_rect.size.is_empty_or_negative() {
             return false;
         }
 
+        // If the primitive is directly drawn onto this picture cache surface, then
+        // the pic_clip_rect is in the same space. If not, we need to map it from
+        // the surface space into the picture cache space.
+        let on_picture_surface = surface_index == self.surface_index;
+        let pic_clip_rect = if on_picture_surface {
+            prim_clip_chain.pic_clip_rect
+        } else {
+            self.map_child_pic_to_surface.set_target_spatial_node(
+                surface_spatial_node_index,
+                clip_scroll_tree,
+            );
+            self.map_child_pic_to_surface
+                .map(&prim_clip_chain.pic_clip_rect)
+                .expect("bug: unable to map clip rect to picture cache space")
+        };
+
         // Get the tile coordinates in the picture space.
-        let (p0, p1) = self.get_tile_coords_for_rect(&prim_rect);
+        let (p0, p1) = self.get_tile_coords_for_rect(&pic_clip_rect);
 
         // If the primitive is outside the tiling rects, it's known to not
         // be visible.
         if p0.x == p1.x || p0.y == p1.y {
             return false;
         }
 
         // Some primitives can not be cached (e.g. external video images)
         let is_cacheable = prim_instance.is_cacheable(
             &data_stores,
             resource_cache,
         );
 
         // Build the list of resources that this primitive has dependencies on.
         let mut prim_info = PrimitiveDependencyInfo::new(
             prim_instance.uid(),
-            prim_rect,
+            prim_rect.origin,
+            pic_clip_rect,
             is_cacheable,
         );
 
         // Include the prim spatial node, if differs relative to cache root.
         if prim_spatial_node_index != self.spatial_node_index {
             prim_info.spatial_nodes.push(prim_spatial_node_index);
         }
 
         // If there was a clip chain, add any clip dependencies to the list for this tile.
-        if let Some(prim_clip_chain) = prim_clip_chain {
-            prim_info.prim_clip_rect = prim_clip_chain.pic_clip_rect;
-
-            let clip_instances = &clip_store
-                .clip_node_instances[prim_clip_chain.clips_range.to_range()];
-            for clip_instance in clip_instances {
-                prim_info.clips.push(clip_instance.handle.uid());
-
-                // If the clip has the same spatial node, the relative transform
-                // will always be the same, so there's no need to depend on it.
-                let clip_node = &data_stores.clip[clip_instance.handle];
-                if clip_node.item.spatial_node_index != self.spatial_node_index {
-                    if !prim_info.spatial_nodes.contains(&clip_node.item.spatial_node_index) {
-                        prim_info.spatial_nodes.push(clip_node.item.spatial_node_index);
-                    }
+        let clip_instances = &clip_store
+            .clip_node_instances[prim_clip_chain.clips_range.to_range()];
+        for clip_instance in clip_instances {
+            prim_info.clips.push(clip_instance.handle.uid());
+
+            // If the clip has the same spatial node, the relative transform
+            // will always be the same, so there's no need to depend on it.
+            let clip_node = &data_stores.clip[clip_instance.handle];
+            if clip_node.item.spatial_node_index != self.spatial_node_index {
+                if !prim_info.spatial_nodes.contains(&clip_node.item.spatial_node_index) {
+                    prim_info.spatial_nodes.push(clip_node.item.spatial_node_index);
                 }
             }
         }
 
         // Certain primitives may select themselves to be a backdrop candidate, which is
         // then applied below.
         let mut backdrop_candidate = None;
 
@@ -2020,30 +2052,28 @@ impl TileCacheInstance {
                 return false;
             }
             PrimitiveInstanceKind::TextRun { data_handle, .. } => {
                 // Only do these checks if we haven't already disabled subpx
                 // text rendering for this slice.
                 if self.subpixel_mode == SubpixelMode::Allow && !self.is_opaque() {
                     let run_data = &data_stores.text_run[data_handle];
 
-                    // If a text run is on a child surface, the subpx mode will be
-                    // correctly determined as we recurse through pictures in take_context.
-                    let on_picture_surface = surface_index == self.surface_index;
-
                     // Only care about text runs that have requested subpixel rendering.
                     // This is conservative - it may still end up that a subpx requested
                     // text run doesn't get subpx for other reasons (e.g. glyph size).
                     let subpx_requested = match run_data.font.render_mode {
                         FontRenderMode::Subpixel => true,
                         FontRenderMode::Alpha | FontRenderMode::Mono => false,
                     };
 
+                    // If a text run is on a child surface, the subpx mode will be
+                    // correctly determined as we recurse through pictures in take_context.
                     if on_picture_surface && subpx_requested {
-                        if !self.backdrop.rect.contains_rect(&prim_info.prim_clip_rect) {
+                        if !self.backdrop.rect.contains_rect(&pic_clip_rect) {
                             self.subpixel_mode = SubpixelMode::Deny;
                         }
                     }
                 }
             }
             PrimitiveInstanceKind::Clear { .. } => {
                 backdrop_candidate = Some(BackdropKind::Clear);
             }
@@ -2071,38 +2101,34 @@ impl TileCacheInstance {
                     // Check a number of conditions to see if we can consider this
                     // primitive as an opaque backdrop rect. Several of these are conservative
                     // checks and could be relaxed in future. However, these checks
                     // are quick and capture the common cases of background rects and images.
                     // Specifically, we currently require:
                     //  - The primitive is on the main picture cache surface.
                     //  - Same coord system as picture cache (ensures rects are axis-aligned).
                     //  - No clip masks exist.
-                    let on_picture_surface = surface_index == self.surface_index;
-
                     let same_coord_system = {
                         let prim_spatial_node = &clip_scroll_tree
                             .spatial_nodes[prim_spatial_node_index.0 as usize];
                         let surface_spatial_node = &clip_scroll_tree
                             .spatial_nodes[self.spatial_node_index.0 as usize];
 
                         prim_spatial_node.coordinate_system_id == surface_spatial_node.coordinate_system_id
                     };
 
                     same_coord_system && on_picture_surface
                 }
             };
 
             if is_suitable_backdrop {
-                if let Some(ref clip_chain) = prim_clip_chain {
-                    if !clip_chain.needs_mask && clip_chain.pic_clip_rect.contains_rect(&self.backdrop.rect) {
-                        self.backdrop = BackdropInfo {
-                            rect: clip_chain.pic_clip_rect,
-                            kind: backdrop_candidate,
-                        }
+                if !prim_clip_chain.needs_mask && pic_clip_rect.contains_rect(&self.backdrop.rect) {
+                    self.backdrop = BackdropInfo {
+                        rect: pic_clip_rect,
+                        kind: backdrop_candidate,
                     }
                 }
             }
         }
 
         // Record any new spatial nodes in the used list.
         self.used_spatial_nodes.extend(&prim_info.spatial_nodes);
 
--- a/gfx/wr/webrender/src/prim_store/mod.rs
+++ b/gfx/wr/webrender/src/prim_store/mod.rs
@@ -2140,16 +2140,17 @@ impl PrimitiveStore {
                             frame_context.clip_scroll_tree,
                             frame_state.data_stores,
                             frame_state.clip_store,
                             &self.pictures,
                             frame_state.resource_cache,
                             &self.opacity_bindings,
                             &self.images,
                             surface_index,
+                            surface.surface_spatial_node_index,
                         ) {
                             prim_instance.visibility_info = PrimitiveVisibilityIndex::INVALID;
                             // Ensure the primitive clip is popped - perhaps we can use
                             // some kind of scope to do this automatically in future.
                             frame_state.clip_chain_stack.pop_clip();
                             continue;
                         }
                     }