Bug 1525740 - Visual artifacts in add-on menu with webrender enabled on mac intel. r=jrmuizel
authorGlenn Watson <github@intuitionlibrary.com>
Thu, 07 Feb 2019 15:42:06 +0000
changeset 457629 f526887aa3ae0087bbb9553627453eb9ecdec871
parent 457628 0258dc6318a821647ec61519590de275b6663ad6
child 457630 08fd81ac849bd369901e1d00989499aa0d27dc1c
push id35516
push userrmaries@mozilla.com
push dateFri, 08 Feb 2019 04:23:26 +0000
treeherdermozilla-central@d599d1a73a3a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1525740
milestone67.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 1525740 - Visual artifacts in add-on menu with webrender enabled on mac intel. r=jrmuizel The existing picture caching code in WR assumes that the tiles are being drawn into the main framebuffer. This is true to the main content frame, however it's not the case for all popup windows. In the case of popup windows on mac, they have a rounded rect clip, which results in a surface being used. This breaks some assumptions in the picture caching code. The long term fix involves supporting picture caching on surfaces. However, we don't want picture caching on for non-content windows anyway (due to wasting texture memory), so for now we will simply disable picture cache composite modes if they are being drawn on a non-root surface. Differential Revision: https://phabricator.services.mozilla.com/D18917
gfx/wr/webrender/src/batch.rs
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/prim_store/mod.rs
--- a/gfx/wr/webrender/src/batch.rs
+++ b/gfx/wr/webrender/src/batch.rs
@@ -1070,16 +1070,38 @@ impl AlphaBatchBuilder {
                         let brush_flags = if raster_config.establishes_raster_root {
                             BrushFlags::PERSPECTIVE_INTERPOLATION
                         } else {
                             BrushFlags::empty()
                         };
 
                         match raster_config.composite_mode {
                             PictureCompositeMode::TileCache { .. } => {
+                                let tile_cache = picture.tile_cache.as_ref().unwrap();
+
+                                // If the tile cache is disabled, just recurse into the
+                                // picture like a normal pass-through picture, adding
+                                // any child primitives into the parent surface batches.
+                                if !tile_cache.is_enabled {
+                                    self.add_pic_to_batch(
+                                        picture,
+                                        task_id,
+                                        ctx,
+                                        gpu_cache,
+                                        render_tasks,
+                                        deferred_resolves,
+                                        prim_headers,
+                                        transforms,
+                                        root_spatial_node_index,
+                                        z_generator,
+                                    );
+
+                                    return;
+                                }
+
                                 // Construct a local clip rect that ensures we only draw pixels where
                                 // the local bounds of the picture extend to within the edge tiles.
                                 let local_clip_rect = prim_info
                                     .combined_local_clip_rect
                                     .intersection(&picture.local_rect)
                                     .and_then(|rect| {
                                         rect.intersection(&picture.local_clip_rect)
                                     });
@@ -1087,18 +1109,16 @@ impl AlphaBatchBuilder {
                                 if let Some(local_clip_rect) = local_clip_rect {
                                     // Step through each tile in the cache, and draw it with an image
                                     // brush primitive if visible.
 
                                     let kind = BatchKind::Brush(
                                         BrushBatchKind::Image(ImageBufferKind::Texture2DArray)
                                     );
 
-                                    let tile_cache = picture.tile_cache.as_ref().unwrap();
-
                                     for tile_index in &tile_cache.tiles_to_draw {
                                         let tile = &tile_cache.tiles[tile_index.0];
 
                                         // Get the local rect of the tile.
                                         let tile_rect = tile.local_rect;
 
                                         let prim_header = PrimitiveHeader {
                                             local_rect: tile_rect,
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -593,16 +593,19 @@ pub struct TileCache {
     /// World space clip rect of the root clipping node. Every primitive
     /// has this as the root of the clip chain attached to the primitive.
     root_clip_rect: WorldRect,
     /// List of reference primitive information used for
     /// correlating the position between display lists.
     reference_prims: ReferencePrimitiveList,
     /// The root clip chain for this tile cache.
     root_clip_chain_id: ClipChainId,
+    /// If true, this tile cache is enabled. For now, it doesn't
+    /// support tile caching if the surface is not the main framebuffer.
+    pub is_enabled: bool,
 }
 
 /// Stores information about a primitive in the cache that we will
 /// try to use to correlate positions between display lists.
 #[derive(Clone)]
 struct ReferencePrimitive {
     uid: ItemUid,
     local_pos: LayoutPoint,
@@ -716,16 +719,17 @@ impl TileCache {
             world_tile_size: WorldSize::zero(),
             tile_count: TileSize::zero(),
             scroll_offset: None,
             pending_blits: Vec::new(),
             world_bounding_rect: WorldRect::zero(),
             root_clip_rect: WorldRect::max_rect(),
             reference_prims,
             root_clip_chain_id,
+            is_enabled: true,
         }
     }
 
     /// Get the tile coordinates for a given rectangle.
     fn get_tile_coords_for_rect(
         &self,
         rect: &WorldRect,
     ) -> (TileOffset, TileOffset) {
@@ -753,17 +757,34 @@ impl TileCache {
     }
 
     /// Update transforms, opacity bindings and tile rects.
     pub fn pre_update(
         &mut self,
         pic_rect: LayoutRect,
         frame_context: &FrameVisibilityContext,
         frame_state: &mut FrameVisibilityState,
+        surface_index: SurfaceIndex,
     ) {
+        // If the tile cache is the first surface on the root
+        // surface, then we can enable it. If the client has
+        // requested caching on an offscreen surface, we will
+        // need to disable it (for now).
+        self.is_enabled = surface_index == SurfaceIndex(1);
+        if !self.is_enabled {
+            // TODO(gw): It's technically possible that this tile cache
+            //           might have been enabled in a valid state, and
+            //           then got an offscreen surface. In this case,
+            //           there may be some pre-cached tiles still existing.
+            //           They will expire from the texture cache as normal,
+            //           but we should check this path a bit more carefully
+            //           to see if any other memory should be freed.
+            return;
+        }
+
         let DeviceIntSize { width: tile_width, height: tile_height, _unit: _ } =
             self.tile_dimensions(frame_context.config.testing);
 
         // Work out the scroll offset to apply to the world reference point.
         let scroll_transform = frame_context.clip_scroll_tree.get_relative_transform(
             ROOT_SPATIAL_NODE_INDEX,
             self.spatial_node_index,
         ).expect("bug: unable to get scroll transform");
@@ -1028,16 +1049,23 @@ impl TileCache {
         clip_scroll_tree: &ClipScrollTree,
         data_stores: &DataStores,
         clip_chain_nodes: &[ClipChainNode],
         pictures: &[PicturePrimitive],
         resource_cache: &ResourceCache,
         opacity_binding_store: &OpacityBindingStorage,
         image_instances: &ImageInstanceStorage,
     ) -> bool {
+        // If the tile cache is disabled, there's no need to update
+        // the primitive dependencies.
+        if !self.is_enabled {
+            // Return true to signal that we didn't early cull this primitive.
+            return true;
+        }
+
         self.map_local_to_world.set_target_spatial_node(
             prim_instance.spatial_node_index,
             clip_scroll_tree,
         );
 
         // Map the primitive local rect into world space.
         let world_rect = match self.map_local_to_world.map(&prim_rect) {
             Some(rect) => rect,
@@ -1348,16 +1376,21 @@ impl TileCache {
         resource_cache: &mut ResourceCache,
         gpu_cache: &mut GpuCache,
         frame_context: &FrameVisibilityContext,
         scratch: &mut PrimitiveScratchBuffer,
     ) -> LayoutRect {
         self.dirty_region.clear();
         self.pending_blits.clear();
 
+        // If the tile cache is disabled, just return a no-op local clip rect.
+        if !self.is_enabled {
+            return LayoutRect::max_rect();
+        }
+
         let dim = self.tile_dimensions(frame_context.config.testing);
         let descriptor = ImageDescriptor::new(
             dim.width,
             dim.height,
             ImageFormat::BGRA8,
             true,
             false,
         );
@@ -2364,18 +2397,22 @@ impl PicturePrimitive {
         // Still disable subpixel AA if parent forbids it
         let allow_subpixel_aa = parent_allows_subpixel_aa && allow_subpixel_aa;
 
         let mut dirty_region_count = 0;
 
         // If this is a picture cache, push the dirty region to ensure any
         // child primitives are culled and clipped to the dirty rect(s).
         if let Some(ref tile_cache) = self.tile_cache {
-            frame_state.push_dirty_region(tile_cache.dirty_region.clone());
-            dirty_region_count += 1;
+            // If the tile cache is disabled, it doesn't have a valid
+            // dirty region to exclude primitives from.
+            if tile_cache.is_enabled {
+                frame_state.push_dirty_region(tile_cache.dirty_region.clone());
+                dirty_region_count += 1;
+            }
         }
 
         if inflation_factor > 0.0 {
             let inflated_region = frame_state.current_dirty_region().inflate(inflation_factor);
             frame_state.push_dirty_region(inflated_region);
             dirty_region_count += 1;
         }
 
--- a/gfx/wr/webrender/src/prim_store/mod.rs
+++ b/gfx/wr/webrender/src/prim_store/mod.rs
@@ -1759,16 +1759,17 @@ impl PrimitiveStore {
 
                 // If we have a tile cache for this picture, see if any of the
                 // relative transforms have changed, which means we need to
                 // re-map the dependencies of any child primitives.
                 tile_cache.pre_update(
                     pic.local_rect,
                     frame_context,
                     frame_state,
+                    surface_index,
                 );
 
                 frame_state.tile_cache = Some(tile_cache);
             }
 
             (prim_list, surface_index, pic.apply_local_clip_rect)
         };