Bug 1566901 - Make picture caching more robust to float issues. a=RyanVM
☠☠ backed out by c4fa4d3613fb ☠ ☠
authorGlenn Watson <github@intuitionlibrary.com>
Mon, 22 Jul 2019 23:35:37 +0300
changeset 544726 c134c5e050d891441abdacaf5785aee1c91a7fe0
parent 544725 d5aa3653975b6cf937210eadef553a6683861bde
child 544727 86408649349fa4338f4a40636efbaa4dc6aa2a92
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersRyanVM
bugs1566901
milestone69.0
Bug 1566901 - Make picture caching more robust to float issues. a=RyanVM This patch fixes a couple of picture caching issues that could cause more invalidations than required. Specifically: * Ensure the viewport rect is included in child surfaces, so that redundant clips are filtered out correctly. * Use epsilon comparisons where appropriate for tile descriptor comparisons, to avoid invalidations due to float inaccuracies. Differential Revision: https://phabricator.services.mozilla.com//D38455
gfx/wr/webrender/src/clip.rs
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/prim_store/mod.rs
testing/web-platform/meta/css/motion/offset-rotate-005.html.ini
--- a/gfx/wr/webrender/src/clip.rs
+++ b/gfx/wr/webrender/src/clip.rs
@@ -677,16 +677,31 @@ impl ClipChainStack {
     }
 
     /// When a surface is created, it takes all clips and establishes a new
     /// stack of clips to be propagated.
     pub fn push_surface(
         &mut self,
         viewport: WorldRect,
     ) {
+        // Ensure that sub-surfaces (e.g. filters) of a tile
+        // cache intersect with any parent viewport. This ensures
+        // that we correctly filter out redundant clips on these
+        // child surfaces.
+        let viewport = match self.stack.last() {
+            Some(parent_level) => {
+                parent_level.viewport
+                    .intersection(&viewport)
+                    .unwrap_or(WorldRect::zero())
+            }
+            None => {
+                viewport
+            }
+        };
+
         let level = ClipChainLevel::new(viewport);
         self.stack.push(level);
     }
 
     /// Pop a surface from the clip chain stack
     pub fn pop_surface(&mut self) {
         let level = self.stack.pop().unwrap();
         assert!(level.clip_counts.is_empty() && level.clips.is_empty());
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -55,53 +55,74 @@ use crate::filterdata::{FilterDataHandle
 #[derive(Debug, Copy, Clone, PartialEq)]
 pub enum SubpixelMode {
     /// This surface allows subpixel AA text
     Allow,
     /// Subpixel AA text cannot be drawn on this surface
     Deny,
 }
 
+/// A comparable transform matrix, that compares with epsilon checks.
+#[derive(Debug, Clone)]
+struct MatrixKey {
+    m: [f32; 16],
+}
+
+impl PartialEq for MatrixKey {
+    fn eq(&self, other: &Self) -> bool {
+        const EPSILON: f32 = 0.001;
+
+        // TODO(gw): It's possible that we may need to adjust the epsilon
+        //           to be tighter on most of the matrix, except the
+        //           translation parts?
+        for (i, j) in self.m.iter().zip(other.m.iter()) {
+            if !i.approx_eq_eps(j, &EPSILON) {
+                return false;
+            }
+        }
+
+        true
+    }
+}
+
 /// A comparable / hashable version of a coordinate space mapping. Used to determine
 /// if a transform dependency for a tile has changed.
 #[derive(Debug, PartialEq, Clone)]
 enum TransformKey {
     Local,
     ScaleOffset {
         scale_x: f32,
         scale_y: f32,
         offset_x: f32,
         offset_y: f32,
     },
     Transform {
-        m: [f32; 16],
+        m: MatrixKey,
     }
 }
 
 impl<Src, Dst> From<CoordinateSpaceMapping<Src, Dst>> for TransformKey {
     fn from(transform: CoordinateSpaceMapping<Src, Dst>) -> TransformKey {
         match transform {
             CoordinateSpaceMapping::Local => {
                 TransformKey::Local
             }
             CoordinateSpaceMapping::ScaleOffset(ref scale_offset) => {
-                // TODO(gw): We should consider quantizing / rounding these values
-                //           to avoid invalidations due to floating point inaccuracies.
                 TransformKey::ScaleOffset {
                     scale_x: scale_offset.scale.x,
                     scale_y: scale_offset.scale.y,
                     offset_x: scale_offset.offset.x,
                     offset_y: scale_offset.offset.y,
                 }
             }
             CoordinateSpaceMapping::Transform(ref m) => {
-                // TODO(gw): We should consider quantizing / rounding these values
-                //           to avoid invalidations due to floating point inaccuracies.
                 TransformKey::Transform {
-                    m: m.to_row_major_array(),
+                    m: MatrixKey {
+                        m: m.to_row_major_array(),
+                    },
                 }
             }
         }
     }
 }
 
 /// Information about a picture that is pushed / popped on the
 /// PictureUpdateState during picture traversal pass.
@@ -274,49 +295,110 @@ impl Tile {
         // Check if the contents of the primitives, clips, and
         // other dependencies are the same.
         self.is_same_content &= self.descriptor.is_same_content(self.id);
         self.is_valid &= self.is_same_content;
     }
 }
 
 /// Defines a key that uniquely identifies a primitive instance.
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone)]
 pub struct PrimitiveDescriptor {
     /// Uniquely identifies the content of the primitive template.
     prim_uid: ItemUid,
     /// The origin in world space of this primitive.
     origin: PointKey,
     /// The clip rect for this primitive. Included here in
     /// dependencies since there is no entry in the clip chain
     /// dependencies for the local clip rect.
     prim_clip_rect: RectangleKey,
     /// The first clip in the clip_uids array of clips that affect this tile.
     first_clip: u16,
     /// The number of clips that affect this primitive instance.
     clip_count: u16,
 }
 
+impl PartialEq for PrimitiveDescriptor {
+    fn eq(&self, other: &Self) -> bool {
+        const EPSILON: f32 = 0.001;
+
+        if self.prim_uid != other.prim_uid {
+            return false;
+        }
+        if self.first_clip != other.first_clip {
+            return false;
+        }
+        if self.clip_count != other.clip_count {
+            return false;
+        }
+
+        if !self.origin.x.approx_eq_eps(&other.origin.x, &EPSILON) {
+            return false;
+        }
+        if !self.origin.y.approx_eq_eps(&other.origin.y, &EPSILON) {
+            return false;
+        }
+
+        if !self.prim_clip_rect.x.approx_eq_eps(&other.prim_clip_rect.x, &EPSILON) {
+            return false;
+        }
+        if !self.prim_clip_rect.y.approx_eq_eps(&other.prim_clip_rect.y, &EPSILON) {
+            return false;
+        }
+        if !self.prim_clip_rect.w.approx_eq_eps(&other.prim_clip_rect.w, &EPSILON) {
+            return false;
+        }
+        if !self.prim_clip_rect.h.approx_eq_eps(&other.prim_clip_rect.h, &EPSILON) {
+            return false;
+        }
+
+        true
+    }
+}
+
+/// Defines a key that uniquely identifies a clip instance.
+#[derive(Debug, Clone)]
+pub struct ClipDescriptor {
+    /// The uid is guaranteed to uniquely describe the content of the clip node.
+    uid: ItemUid,
+    /// The origin defines the relative position of this clip template.
+    origin: PointKey,
+}
+
+impl PartialEq for ClipDescriptor {
+    fn eq(&self, other: &Self) -> bool {
+        const EPSILON: f32 = 0.001;
+
+        if self.uid != other.uid {
+            return false;
+        }
+
+        if !self.origin.x.approx_eq_eps(&other.origin.x, &EPSILON) {
+            return false;
+        }
+
+        if !self.origin.y.approx_eq_eps(&other.origin.y, &EPSILON) {
+            return false;
+        }
+
+        true
+    }
+}
+
 /// Uniquely describes the content of this tile, in a way that can be
 /// (reasonably) efficiently hashed and compared.
 #[derive(Debug)]
 pub struct TileDescriptor {
     /// List of primitive instance unique identifiers. The uid is guaranteed
     /// to uniquely describe the content of the primitive template, while
     /// the other parameters describe the clip chain and instance params.
     pub prims: ComparableVec<PrimitiveDescriptor>,
 
-    /// List of clip node unique identifiers. The uid is guaranteed
-    /// to uniquely describe the content of the clip node.
-    clip_uids: ComparableVec<ItemUid>,
-
-    /// List of local offsets of the clip node origins. This
-    /// ensures that if a clip node is supplied but has a different
-    /// transform between frames that the tile is invalidated.
-    clip_vertices: ComparableVec<PointKey>,
+    /// List of clip node descriptors.
+    clips: ComparableVec<ClipDescriptor>,
 
     /// List of image keys that this tile depends on.
     image_keys: ComparableVec<ImageKey>,
 
     /// The set of opacity bindings that this tile depends on.
     // TODO(gw): Ugh, get rid of all opacity binding support!
     opacity_bindings: ComparableVec<OpacityBinding>,
 
@@ -324,49 +406,44 @@ pub struct TileDescriptor {
     /// tracking for this tile.
     transforms: ComparableVec<TransformKey>,
 }
 
 impl TileDescriptor {
     fn new() -> Self {
         TileDescriptor {
             prims: ComparableVec::new(),
-            clip_uids: ComparableVec::new(),
-            clip_vertices: ComparableVec::new(),
+            clips: ComparableVec::new(),
             opacity_bindings: ComparableVec::new(),
             image_keys: ComparableVec::new(),
             transforms: ComparableVec::new(),
         }
     }
 
     /// Clear the dependency information for a tile, when the dependencies
     /// are being rebuilt.
     fn clear(&mut self) {
         self.prims.reset();
-        self.clip_uids.reset();
-        self.clip_vertices.reset();
+        self.clips.reset();
         self.opacity_bindings.reset();
         self.image_keys.reset();
         self.transforms.reset();
     }
 
     /// Return true if the content of the tile is the same
     /// as last frame. This doesn't check validity of the
     /// tile based on the currently valid regions.
     fn is_same_content(&self, _id: TileId) -> bool {
         if !self.image_keys.is_valid() {
             return false;
         }
         if !self.opacity_bindings.is_valid() {
             return false;
         }
-        if !self.clip_uids.is_valid() {
-            return false;
-        }
-        if !self.clip_vertices.is_valid() {
+        if !self.clips.is_valid() {
             return false;
         }
         if !self.prims.is_valid() {
             return false;
         }
         if !self.transforms.is_valid() {
             return false;
         }
@@ -911,18 +988,17 @@ impl TileCacheInstance {
         // 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;
         }
 
         // Build the list of resources that this primitive has dependencies on.
         let mut opacity_bindings: SmallVec<[OpacityBinding; 4]> = SmallVec::new();
-        let mut clip_chain_uids: SmallVec<[ItemUid; 8]> = SmallVec::new();
-        let mut clip_vertices: SmallVec<[LayoutPoint; 8]> = SmallVec::new();
+        let mut clips: SmallVec<[ClipDescriptor; 8]> = SmallVec::new();
         let mut image_keys: SmallVec<[ImageKey; 8]> = SmallVec::new();
         let mut clip_spatial_nodes = FastHashSet::default();
         let mut prim_clip_rect = PictureRect::zero();
 
         // Some primitives can not be cached (e.g. external video images)
         let is_cacheable = prim_instance.is_cacheable(
             &data_stores,
             resource_cache,
@@ -930,25 +1006,26 @@ impl TileCacheInstance {
 
         // 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_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 {
-                clip_chain_uids.push(clip_instance.handle.uid());
+                clips.push(ClipDescriptor {
+                    uid: clip_instance.handle.uid(),
+                    origin: clip_instance.local_pos.into(),
+                });
 
                 // 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.
                 if clip_instance.spatial_node_index != self.spatial_node_index {
                     clip_spatial_nodes.insert(clip_instance.spatial_node_index);
                 }
-
-                clip_vertices.push(clip_instance.local_pos);
             }
         }
 
         // For pictures, we don't (yet) know the valid clip rect, so we can't correctly
         // use it to calculate the local bounding rect for the tiles. If we include them
         // then we may calculate a bounding rect that is too large, since it won't include
         // the clip bounds of the picture. Excluding them from the bounding rect here
         // fixes any correctness issues (the clips themselves are considered when we
@@ -1125,25 +1202,22 @@ impl TileCacheInstance {
                 } else {
                     (prim_rect.origin, prim_clip_rect)
                 };
 
                 // Update the tile descriptor, used for tile comparison during scene swaps.
                 tile.descriptor.prims.push(PrimitiveDescriptor {
                     prim_uid: prim_instance.uid(),
                     origin: prim_origin.into(),
-                    first_clip: tile.descriptor.clip_uids.len() as u16,
-                    clip_count: clip_chain_uids.len() as u16,
+                    first_clip: tile.descriptor.clips.len() as u16,
+                    clip_count: clips.len() as u16,
                     prim_clip_rect: prim_clip_rect.into(),
                 });
 
-                tile.descriptor.clip_uids.extend_from_slice(&clip_chain_uids);
-                for clip_vertex in &clip_vertices {
-                    tile.descriptor.clip_vertices.push((*clip_vertex).into());
-                }
+                tile.descriptor.clips.extend_from_slice(&clips);
 
                 // If the primitive has the same spatial node, the relative transform
                 // will always be the same, so there's no need to depend on it.
                 if prim_instance.spatial_node_index != self.spatial_node_index {
                     tile.transforms.insert(prim_instance.spatial_node_index);
                 }
                 for spatial_node_index in &clip_spatial_nodes {
                     tile.transforms.insert(*spatial_node_index);
--- a/gfx/wr/webrender/src/prim_store/mod.rs
+++ b/gfx/wr/webrender/src/prim_store/mod.rs
@@ -302,20 +302,20 @@ pub struct PrimitiveSceneData {
     pub prim_size: LayoutSize,
     pub is_backface_visible: bool,
 }
 
 #[cfg_attr(feature = "capture", derive(Serialize))]
 #[cfg_attr(feature = "replay", derive(Deserialize))]
 #[derive(Copy, Debug, Clone, MallocSizeOf, PartialEq)]
 pub struct RectangleKey {
-    x: f32,
-    y: f32,
-    w: f32,
-    h: f32,
+    pub x: f32,
+    pub y: f32,
+    pub w: f32,
+    pub h: f32,
 }
 
 impl Eq for RectangleKey {}
 
 impl hash::Hash for RectangleKey {
     fn hash<H: hash::Hasher>(&self, state: &mut H) {
         self.x.to_bits().hash(state);
         self.y.to_bits().hash(state);
deleted file mode 100644
--- a/testing/web-platform/meta/css/motion/offset-rotate-005.html.ini
+++ /dev/null
@@ -1,3 +0,0 @@
-[offset-rotate-005.html]
-  expected:
-    if webrender and (os == "linux"): FAIL