Bug 1601865 - Fix scrollbar cache slices having extra primitives. r=nical
authorGlenn Watson <gw@intuitionlibrary.com>
Tue, 04 Feb 2020 09:21:16 +0000
changeset 512443 e1a4153cfb39a6e1706e88f036d6874bd0ddc39c
parent 512442 391df9744258815e2a2cd2e9c7bf41439118de52
child 512444 b5044c454dfc6eb8183c272bd1a6444c72a71738
push id37089
push usernerli@mozilla.com
push dateTue, 04 Feb 2020 16:20:28 +0000
treeherdermozilla-central@a117660624ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnical
bugs1601865
milestone74.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 1601865 - Fix scrollbar cache slices having extra primitives. r=nical There's two potential cases handled by this patch: (1) A scrollbar container followed by another scrollbar container. In this case, we need to ensure these are placed into separate clusters, even though the cluster flags otherwise match, to ensure that slice creation will see the two clusters. (2) If a fixed position scroll root trails a scrollbar container. In this case, ensure that a scrollbar contains marks the cluster flags to create a slice straight after the scrollbar, to avoid other primitives with the same scroll root sneaking into the scrollbar container. Differential Revision: https://phabricator.services.mozilla.com/D61519
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/scene_building.rs
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -3261,16 +3261,23 @@ impl PrimitiveCluster {
     }
 
     /// Return true if this cluster is compatible with the given params
     pub fn is_compatible(
         &self,
         spatial_node_index: SpatialNodeIndex,
         flags: ClusterFlags,
     ) -> bool {
+        // If this cluster is a scrollbar, ensure that a matching scrollbar
+        // container that follows is split up, so we don't combine the
+        // scrollbars into a single slice.
+        if self.flags.contains(ClusterFlags::SCROLLBAR_CONTAINER) {
+            return false;
+        }
+
         self.flags == flags && self.spatial_node_index == spatial_node_index
     }
 
     /// Add a primitive instance to this cluster, at the start or end
     fn push(
         &mut self,
         prim_instance: PrimitiveInstance,
         prim_size: LayoutSize,
--- a/gfx/wr/webrender/src/scene_building.rs
+++ b/gfx/wr/webrender/src/scene_building.rs
@@ -3601,33 +3601,45 @@ impl FlattenedStackingContext {
         spatial_tree: &SpatialTree,
         clip_store: &ClipStore,
         interners: &Interners,
         quality_settings: &QualitySettings,
     ) -> usize {
         struct SliceInfo {
             cluster_index: usize,
             scroll_root: SpatialNodeIndex,
+            cluster_flags: ClusterFlags,
         }
 
         let mut content_slice_count = 0;
         let mut slices: Vec<SliceInfo> = Vec::new();
 
         // Step through each cluster, and work out where the slice boundaries should be.
         for (cluster_index, cluster) in self.prim_list.clusters.iter().enumerate() {
             let scroll_root = spatial_tree.find_scroll_root(
                 cluster.spatial_node_index,
             );
 
             // We want to create a slice in the following conditions:
             // (1) This cluster is a scrollbar
             // (2) Certain conditions when the scroll root changes (see below)
             // (3) No slice exists yet
-            let create_new_slice =
-                cluster.flags.contains(ClusterFlags::SCROLLBAR_CONTAINER) ||
+            let mut cluster_flags = ClusterFlags::empty();
+
+            if cluster.flags.contains(ClusterFlags::SCROLLBAR_CONTAINER) {
+                // Scrollbar containers need to ensure that a new slice is
+                // created both before and after the scrollbar, so that no
+                // other prims with the same scroll root sneak into this slice.
+                cluster_flags.insert(
+                    ClusterFlags::CREATE_PICTURE_CACHE_PRE |
+                    ClusterFlags::CREATE_PICTURE_CACHE_POST
+                );
+            }
+
+            let create_new_slice_for_scroll_root =
                 slices.last().map(|slice| {
                     match (slice.scroll_root, scroll_root) {
                         (ROOT_SPATIAL_NODE_INDEX, ROOT_SPATIAL_NODE_INDEX) => {
                             // Both current slice and this cluster are fixed position, no need to cut
                             false
                         }
                         (ROOT_SPATIAL_NODE_INDEX, _) => {
                             // A real scroll root is being established, so create a cache slice
@@ -3666,21 +3678,26 @@ impl FlattenedStackingContext {
                         }
                         (curr_scroll_root, scroll_root) => {
                             // Two scrolling roots - only need a new slice if they differ
                             curr_scroll_root != scroll_root
                         }
                     }
                 }).unwrap_or(true);
 
+            if create_new_slice_for_scroll_root {
+                cluster_flags.insert(ClusterFlags::CREATE_PICTURE_CACHE_PRE);
+            }
+
             // Create a new slice if required
-            if create_new_slice {
+            if !cluster_flags.is_empty() {
                 slices.push(SliceInfo {
                     cluster_index,
-                    scroll_root
+                    scroll_root,
+                    cluster_flags,
                 });
             }
         }
 
         // If the page would create too many slices (an arbitrary definition where
         // it's assumed the GPU memory + compositing overhead would be too high)
         // then just create a single picture cache for the entire content. This at
         // least means that we can cache small content changes efficiently when
@@ -3697,17 +3714,17 @@ impl FlattenedStackingContext {
             }
         } else {
             // Walk the list of slices, setting appropriate flags on the clusters which are
             // later used during setup_picture_caching.
             for slice in slices.drain(..) {
                 content_slice_count += 1;
                 let cluster = &mut self.prim_list.clusters[slice.cluster_index];
                 // Mark that this cluster creates a picture cache slice
-                cluster.flags.insert(ClusterFlags::CREATE_PICTURE_CACHE_PRE);
+                cluster.flags.insert(slice.cluster_flags);
                 cluster.cache_scroll_root = Some(slice.scroll_root);
             }
         }
 
         // Always end the cache at the end of the stacking context, so that we don't
         // cache anything from primitives outside this pipeline in the same slice.
         if let Some(cluster) = self.prim_list.clusters.last_mut() {
             cluster.flags.insert(ClusterFlags::CREATE_PICTURE_CACHE_POST);