Bug 1566712 - Fix quality issues with picture caching when the transform has a fractional offset. r=kvark

This patch reverts the previous attempted fix for snapping issues
with picture caching, and implements a better solution.

This fixes the main visual issue by ensuring that any fractional
offset in the root transform is accounted for by:

 * Offsetting the tile rects by this amount, so that the content
   origin is a whole device pixel.
 * Invalidating all tiles if the fractional part of the root
   transform changes. This is required since it can affect the
   snapping logic that WR applies. Fortunately, this occurs
   very rarely - Gecko typically has a constant fractional part
   for each page.

Differential Revision: https://phabricator.services.mozilla.com/D38267

--HG--
extra : moz-landing-system : lando

gfx/wr/webrender/res/brush_image.glsl

@@ -122,7 +122,15 @@ void brush_vs(
 
     vec2 f = (vi.local_pos - local_rect.p0) / local_rect.size;
 
+#ifdef WR_FEATURE_ALPHA_PASS
+    int color_mode = prim_user_data.x & 0xffff;
+    int blend_mode = prim_user_data.x >> 16;
     int raster_space = prim_user_data.y;
+
+    if (color_mode == COLOR_MODE_FROM_PASS) {
+        color_mode = uMode;
+    }
+
     // Derive the texture coordinates for this image, based on
     // whether the source image is a local-space or screen-space
     // image.
@@ -137,14 +145,6 @@ void brush_vs(
         default:
             break;
     }
-
-#ifdef WR_FEATURE_ALPHA_PASS
-    int color_mode = prim_user_data.x & 0xffff;
-    int blend_mode = prim_user_data.x >> 16;
-
-    if (color_mode == COLOR_MODE_FROM_PASS) {
-        color_mode = uMode;
-    }
 #endif
 
     // Offset and scale vUv here to avoid doing it in the fragment shader.

gfx/wr/webrender/src/batch.rs

@@ -1226,7 +1226,7 @@ impl BatchBuilder {
                                         textures,
                                         [
                                             ShaderColorMode::Image as i32 | ((AlphaType::PremultipliedAlpha as i32) << 16),
-                                            RasterizationSpace::Screen as i32,
+                                            RasterizationSpace::Local as i32,
                                             get_shader_opacity(1.0),
                                             0,
                                         ],

gfx/wr/webrender/src/frame_builder.rs

@@ -240,7 +240,7 @@ impl FrameBuilder {
         retained_tiles: RetainedTiles,
         globals: FrameGlobalResources,
     ) {
-        debug_assert!(self.pending_retained_tiles.tiles.is_empty());
+        assert!(self.pending_retained_tiles.caches.is_empty());
         self.pending_retained_tiles = retained_tiles;
         self.globals = globals;
     }

gfx/wr/webrender/src/picture.rs

@@ -111,26 +111,35 @@ struct PictureInfo {
     _spatial_node_index: SpatialNodeIndex,
 }
 
+pub struct PictureCacheState {
+    /// The tiles retained by this picture cache.
+    pub tiles: FastHashMap<TileOffset, Tile>,
+    /// The current fractional offset of the cache transform root.
+    fract_offset: PictureVector2D,
+}
+
 /// Stores a list of cached picture tiles that are retained
 /// between new scenes.
 #[cfg_attr(feature = "capture", derive(Serialize))]
 pub struct RetainedTiles {
     /// The tiles retained between display lists.
     #[cfg_attr(feature = "capture", serde(skip))] //TODO
-    pub tiles: FastHashMap<TileKey, Tile>,
+    pub caches: FastHashMap<usize, PictureCacheState>,
 }
 
 impl RetainedTiles {
     pub fn new() -> Self {
         RetainedTiles {
-            tiles: FastHashMap::default(),
+            caches: FastHashMap::default(),
         }
     }
 
     /// Merge items from one retained tiles into another.
     pub fn merge(&mut self, other: RetainedTiles) {
-        assert!(self.tiles.is_empty() || other.tiles.is_empty());
-        self.tiles.extend(other.tiles);
+        assert!(self.caches.is_empty() || other.caches.is_empty());
+        if self.caches.is_empty() {
+            self.caches = other.caches;
+        }
     }
 }
 
@@ -198,14 +207,6 @@ impl From<PropertyBinding<f32>> for OpacityBinding {
 #[derive(Debug, Copy, Clone, PartialEq)]
 pub struct TileId(usize);
 
-#[derive(Debug, Copy, Clone, PartialEq, Hash, Eq)]
-pub struct TileKey {
-    /// The picture cache slice that this belongs to.
-    slice: usize,
-    /// Offset (in tile coords) of the tile within this slice.
-    offset: TileOffset,
-}
-
 /// Information about a cached tile.
 #[derive(Debug)]
 pub struct Tile {
@@ -504,7 +505,7 @@ pub struct TileCacheInstance {
     /// The positioning node for this tile cache.
     pub spatial_node_index: SpatialNodeIndex,
     /// Hash of tiles present in this picture.
-    pub tiles: FastHashMap<TileKey, Tile>,
+    pub tiles: FastHashMap<TileOffset, Tile>,
     /// A helper struct to map local rects into surface coords.
     map_local_to_surface: SpaceMapper<LayoutPixel, PicturePixel>,
     /// List of opacity bindings, with some extra information
@@ -525,7 +526,7 @@ pub struct TileCacheInstance {
     /// Local clip rect for this tile cache.
     pub local_clip_rect: PictureRect,
     /// A list of tiles that are valid and visible, which should be drawn to the main scene.
-    pub tiles_to_draw: Vec<TileKey>,
+    pub tiles_to_draw: Vec<TileOffset>,
     /// The world space viewport that this tile cache draws into.
     /// Any clips outside this viewport can be ignored (and must be removed so that
     /// we can draw outside the bounds of the viewport).
@@ -542,6 +543,10 @@ pub struct TileCacheInstance {
     /// The allowed subpixel mode for this surface, which depends on the detected
     /// opacity of the background.
     pub subpixel_mode: SubpixelMode,
+    /// The current fractional offset of the cache transform root. If this changes,
+    /// all tiles need to be invalidated and redrawn, since snapping differences are
+    /// likely to occur.
+    fract_offset: PictureVector2D,
 }
 
 impl TileCacheInstance {
@@ -572,6 +577,7 @@ impl TileCacheInstance {
             background_color,
             opaque_rect: PictureRect::zero(),
             subpixel_mode: SubpixelMode::Allow,
+            fract_offset: PictureVector2D::zero(),
         }
     }
 
@@ -639,6 +645,35 @@ impl TileCacheInstance {
             frame_context.clip_scroll_tree,
         );
 
+        // If there are pending retained state, retrieve it.
+        if let Some(prev_state) = frame_state.retained_tiles.caches.remove(&self.slice) {
+            self.tiles.extend(prev_state.tiles);
+            self.fract_offset = prev_state.fract_offset;
+        }
+
+        // Map an arbitrary point in picture space to world space, to work out
+        // what the fractional translation is that's applied by this scroll root.
+        // TODO(gw): I'm not 100% sure this is right. At least, in future, we should
+        //           make a specific API for this, and/or enforce that the picture
+        //           cache transform only includes scale and/or translation (we
+        //           already ensure it doesn't have perspective).
+        let world_origin = pic_to_world_mapper
+            .map(&PictureRect::new(PicturePoint::zero(), PictureSize::new(1.0, 1.0)))
+            .expect("bug: unable to map origin to world space")
+            .origin;
+        let fract_offset = PictureVector2D::new(
+            world_origin.x.fract(),
+            world_origin.y.fract(),
+        );
+
+        // Determine if the fractional offset of the transform is different this frame
+        // from the currently cached tile set.
+        let fract_changed = (self.fract_offset.x - fract_offset.x).abs() > 0.001 ||
+                            (self.fract_offset.y - fract_offset.y).abs() > 0.001;
+        if fract_changed {
+            self.fract_offset = fract_offset;
+        }
+
         let spatial_node = &frame_context
             .clip_scroll_tree
             .spatial_nodes[self.spatial_node_index.0 as usize];
@@ -751,31 +786,16 @@ impl TileCacheInstance {
         self.tile_bounds_p0 = TileOffset::new(x0, y0);
         self.tile_bounds_p1 = TileOffset::new(x1, y1);
 
-        // TODO(gw): Tidy this up as we add better support for retaining
-        //           slices and sub-grid dirty areas.
-        let mut keys = Vec::new();
-        for key in frame_state.retained_tiles.tiles.keys() {
-            if key.slice == self.slice {
-                keys.push(*key);
-            }
-        }
-        for key in keys {
-            self.tiles.insert(key, frame_state.retained_tiles.tiles.remove(&key).unwrap());
-        }
+        let mut world_culling_rect = WorldRect::zero();
 
         let mut old_tiles = mem::replace(
             &mut self.tiles,
             FastHashMap::default(),
         );
 
-        let mut world_culling_rect = WorldRect::zero();
-
         for y in y0 .. y1 {
             for x in x0 .. x1 {
-                let key = TileKey {
-                    offset: TileOffset::new(x, y),
-                    slice: self.slice,
-                };
+                let key = TileOffset::new(x, y);
 
                 let mut tile = old_tiles
                     .remove(&key)
@@ -784,10 +804,13 @@ impl TileCacheInstance {
                         Tile::new(next_id)
                     });
 
+                // Ensure each tile is offset by the appropriate amount from the
+                // origin, such that the content origin will be a whole number and
+                // the snapping will be consistent.
                 tile.rect = PictureRect::new(
                     PicturePoint::new(
-                        x as f32 * self.tile_size.width,
-                        y as f32 * self.tile_size.height,
+                        x as f32 * self.tile_size.width - fract_offset.x,
+                        y as f32 * self.tile_size.height - fract_offset.y,
                     ),
                     self.tile_size,
                 );
@@ -808,8 +831,9 @@ impl TileCacheInstance {
 
         // Do tile invalidation for any dependencies that we know now.
         for (_, tile) in &mut self.tiles {
-            // Start frame assuming that the tile has the same content.
-            tile.is_same_content = true;
+            // Start frame assuming that the tile has the same content,
+            // unless the fractional offset of the transform root changed.
+            tile.is_same_content = !fract_changed;
 
             // Content has changed if any opacity bindings changed.
             for binding in tile.descriptor.opacity_bindings.items() {
@@ -1043,10 +1067,7 @@ impl TileCacheInstance {
         for y in p0.y .. p1.y {
             for x in p0.x .. p1.x {
                 // TODO(gw): Convert to 2d array temporarily to avoid hash lookups per-tile?
-                let key = TileKey {
-                    slice: self.slice,
-                    offset: TileOffset::new(x, y),
-                };
+                let key = TileOffset::new(x, y);
                 let tile = self.tiles.get_mut(&key).expect("bug: no tile");
 
                 // Mark if the tile is cacheable at all.
@@ -1239,29 +1260,9 @@ impl TileCacheInstance {
                         TILE_SIZE_WIDTH,
                         TILE_SIZE_HEIGHT,
                     );
-
-                    let content_origin_f = tile.world_rect.origin * frame_context.global_device_pixel_scale;
-                    let content_origin_i = content_origin_f.floor();
-
-                    // Calculate the UV coords for this tile. These are generally 0-1, but if the
-                    // local rect of the cache has fractional coordinates, then the content origin
-                    // of the tile is floor'ed, and so we need to adjust the UV rect in order to
-                    // ensure a correct 1:1 texel:pixel mapping and correct snapping.
-                    let s0 = (content_origin_f.x - content_origin_i.x) / tile.world_rect.size.width;
-                    let t0 = (content_origin_f.y - content_origin_i.y) / tile.world_rect.size.height;
-                    let s1 = 1.0;
-                    let t1 = 1.0;
-
-                    let uv_rect_kind = UvRectKind::Quad {
-                        top_left: DeviceHomogeneousVector::new(s0, t0, 0.0, 1.0),
-                        top_right: DeviceHomogeneousVector::new(s1, t0, 0.0, 1.0),
-                        bottom_left: DeviceHomogeneousVector::new(s0, t1, 0.0, 1.0),
-                        bottom_right: DeviceHomogeneousVector::new(s1, t1, 0.0, 1.0),
-                    };
                     resource_cache.texture_cache.update_picture_cache(
                         tile_size,
                         &mut tile.handle,
-                        uv_rect_kind,
                         gpu_cache,
                     );
                 }
@@ -2048,7 +2049,15 @@ impl PicturePrimitive {
         retained_tiles: &mut RetainedTiles,
     ) {
         if let Some(tile_cache) = self.tile_cache.take() {
-            retained_tiles.tiles.extend(tile_cache.tiles);
+            if !tile_cache.tiles.is_empty() {
+                retained_tiles.caches.insert(
+                    tile_cache.slice,
+                    PictureCacheState {
+                        tiles: tile_cache.tiles,
+                        fract_offset: tile_cache.fract_offset,
+                    },
+                );
+            }
         }
     }
 
@@ -2434,10 +2443,10 @@ impl PicturePrimitive {
                                 continue;
                             }
 
-                            // The content origin for surfaces is always an integer value (this preserves
-                            // the same snapping on a surface as would occur if drawn directly to parent).
                             let content_origin_f = tile.world_rect.origin * device_pixel_scale;
-                            let content_origin = content_origin_f.floor().to_i32();
+                            let content_origin = content_origin_f.round();
+                            debug_assert!((content_origin_f.x - content_origin.x).abs() < 0.01);
+                            debug_assert!((content_origin_f.y - content_origin.y).abs() < 0.01);
 
                             let cache_item = frame_state.resource_cache.texture_cache.get(&tile.handle);
 
@@ -2449,7 +2458,7 @@ impl PicturePrimitive {
                                 },
                                 tile_size,
                                 pic_index,
-                                content_origin,
+                                content_origin.to_i32(),
                                 UvRectKind::Rect,
                                 surface_spatial_node_index,
                                 device_pixel_scale,

gfx/wr/webrender/src/texture_cache.rs

@@ -1316,7 +1316,6 @@ impl TextureCache {
         &mut self,
         tile_size: DeviceIntSize,
         handle: &mut TextureCacheHandle,
-        uv_rect_kind: UvRectKind,
         gpu_cache: &mut GpuCache,
     ) {
         debug_assert!(self.now.is_valid());
@@ -1348,11 +1347,10 @@ impl TextureCache {
         }
 
         // Upload the resource rect and texture array layer.
-        let entry = self.entries
+        self.entries
             .get_opt_mut(handle)
-            .expect("BUG: handle must be valid now");
-        entry.uv_rect_kind = uv_rect_kind;
-        entry.update_gpu_cache(gpu_cache);
+            .expect("BUG: handle must be valid now")
+            .update_gpu_cache(gpu_cache);
     }
 }