Bug 1589669 - Fix snapping and rounding errors causing picture caching invalidation when zoomed in. r=aosmond,botond
authorKris Taeleman <ktaeleman@mozilla.com>
Mon, 23 Dec 2019 19:23:43 +0000
changeset 508296 03fe3d76bd48bdd9be40568a136c131b1d129460
parent 508295 b94ca7493062738c50896212e9121c2cab4f1b3d
child 508297 75348f5742c6c01dd9db24c899edd42582d9f407
push id36945
push userapavel@mozilla.com
push dateTue, 24 Dec 2019 04:11:00 +0000
treeherdermozilla-central@03fe3d76bd48 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond, botond
bugs1589669
milestone73.0a1
first release with
nightly linux32
03fe3d76bd48 / 73.0a1 / 20191224041100 / files
nightly linux64
03fe3d76bd48 / 73.0a1 / 20191224041100 / files
nightly mac
03fe3d76bd48 / 73.0a1 / 20191224041100 / files
nightly win32
03fe3d76bd48 / 73.0a1 / 20191224041100 / files
nightly win64
03fe3d76bd48 / 73.0a1 / 20191224041100 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1589669 - Fix snapping and rounding errors causing picture caching invalidation when zoomed in. r=aosmond,botond * Fix crash due to shift left causing overflow (debug only) * Remove rounding of scrolling offsets and snap to view space instead of world space Differential Revision: https://phabricator.services.mozilla.com/D57017
gfx/layers/apz/src/APZCTreeManager.cpp
gfx/wr/webrender/src/picture.rs
gfx/wr/webrender/src/spatial_node.rs
--- a/gfx/layers/apz/src/APZCTreeManager.cpp
+++ b/gfx/layers/apz/src/APZCTreeManager.cpp
@@ -723,17 +723,17 @@ void APZCTreeManager::SampleForWebRender
 
     // The positive translation means the painted content is supposed to
     // move down (or to the right), and that corresponds to a reduction in
     // the scroll offset. Since we are effectively giving WR the async
     // scroll delta here, we want to negate the translation.
     LayoutDevicePoint asyncScrollDelta = -layerTranslation / zoom;
     aTxn.UpdateScrollPosition(wr::AsPipelineId(apzc->GetGuid().mLayersId),
                               apzc->GetGuid().mScrollId,
-                              wr::ToRoundedLayoutPoint(asyncScrollDelta));
+                              wr::ToLayoutPoint(asyncScrollDelta));
 
     apzc->ReportCheckerboard(aSampleTime);
   }
 
   // Now collect all the async transforms needed for the scrollthumbs.
   for (const ScrollThumbInfo& info : mScrollThumbInfo) {
     auto it = mApzcMap.find(info.mTargetGuid);
     if (it == mApzcMap.end()) {
--- a/gfx/wr/webrender/src/picture.rs
+++ b/gfx/wr/webrender/src/picture.rs
@@ -753,18 +753,18 @@ impl Tile {
         // validate dependencies. Instead, we will only compare the current tile
         // dependencies the next time it comes into view.
         if !self.is_visible {
             return;
         }
 
         // 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 - ctx.fract_offset.x).abs() > 0.001 ||
-                            (self.fract_offset.y - ctx.fract_offset.y).abs() > 0.001;
+        let fract_changed = (self.fract_offset.x - ctx.fract_offset.x).abs() > 0.01 ||
+                            (self.fract_offset.y - ctx.fract_offset.y).abs() > 0.01;
         if fract_changed {
             self.invalidate(None, InvalidationReason::FractionalOffset);
             self.fract_offset = ctx.fract_offset;
         }
 
         if ctx.background_color != self.background_color {
             self.invalidate(None, InvalidationReason::BackgroundColor);
             self.background_color = ctx.background_color;
@@ -3240,19 +3240,19 @@ impl PicturePrimitive {
         let spatial_node = &clip_scroll_tree.spatial_nodes[self.spatial_node_index.0 as usize];
         if spatial_node.is_ancestor_or_self_zooming {
             let scale_factors = clip_scroll_tree
                 .get_relative_transform(self.spatial_node_index, ROOT_SPATIAL_NODE_INDEX)
                 .scale_factors();
 
             // Round the scale up to the nearest power of 2, but don't exceed 8.
             let scale = scale_factors.0.max(scale_factors.1).min(8.0);
-            let rounded_up = 1 << scale.log2().ceil() as u32;
-
-            RasterSpace::Local(rounded_up as f32)
+            let rounded_up = 2.0f32.powf(scale.log2().ceil());
+
+            RasterSpace::Local(rounded_up)
         } else {
             self.requested_raster_space
         }
     }
 
     pub fn take_context(
         &mut self,
         pic_index: PictureIndex,
--- a/gfx/wr/webrender/src/spatial_node.rs
+++ b/gfx/wr/webrender/src/spatial_node.rs
@@ -2,19 +2,19 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use api::{ExternalScrollId, PipelineId, PropertyBinding, PropertyBindingId, ReferenceFrameKind, ScrollClamping, ScrollLocation};
 use api::{TransformStyle, ScrollSensitivity, StickyOffsetBounds};
 use api::units::*;
 use crate::clip_scroll_tree::{CoordinateSystem, CoordinateSystemId, SpatialNodeIndex, TransformUpdateState};
-use euclid::{Scale, SideOffsets2D};
+use euclid::{Point2D, Vector2D, SideOffsets2D};
 use crate::scene::SceneProperties;
-use crate::util::{LayoutFastTransform, MatrixHelpers, ScaleOffset, TransformedRectKind, VectorHelpers};
+use crate::util::{LayoutFastTransform, MatrixHelpers, ScaleOffset, TransformedRectKind, PointHelpers};
 
 #[derive(Clone, Debug)]
 pub enum SpatialNodeType {
     /// A special kind of node that adjusts its position based on the position
     /// of its parent node and a given set of sticky positioning offset bounds.
     /// Sticky positioned is described in the CSS Positioned Layout Module Level 3 here:
     /// https://www.w3.org/TR/css-position-3/#sticky-pos
     StickyFrame(StickyFrameInfo),
@@ -108,24 +108,28 @@ fn compute_offset_from(
 
 /// Snap an offset to be incorporated into a transform, where the local space
 /// may be considered the world space. We convert from world space to device
 /// space using the global device pixel scale, which may not always be correct
 /// if there are intermediate surfaces used, however those are either cases
 /// where snapping is not important (e.g. has perspective or is not axis
 /// aligned), or an edge case (e.g. SVG filters) which we can accept
 /// imperfection for now.
-fn snap_offset(
-    offset: LayoutVector2D,
+fn snap_offset<OffsetUnits, ScaleUnits>(
+    offset: Vector2D<f32, OffsetUnits>,
+    scale: Vector2D<f32, ScaleUnits>,
     global_device_pixel_scale: DevicePixelScale,
-) -> LayoutVector2D {
-    let world_offset = offset * Scale::new(1.0);
+) -> Vector2D<f32, OffsetUnits> {
+    let world_offset = Point2D::new(offset.x * scale.x, offset.y * scale.y);
     let snapped_device_offset = (world_offset * global_device_pixel_scale).snap();
     let snapped_world_offset = snapped_device_offset / global_device_pixel_scale;
-    snapped_world_offset * Scale::new(1.0)
+    Vector2D::new(
+        if scale.x != 0.0 { snapped_world_offset.x / scale.x } else { offset.x },
+        if scale.y != 0.0 { snapped_world_offset.y / scale.y } else { offset.y },
+    )
 }
 
 impl SpatialNode {
     pub fn new(
         pipeline_id: PipelineId,
         parent_index: Option<SpatialNodeIndex>,
         node_type: SpatialNodeType,
     ) -> Self {
@@ -229,18 +233,18 @@ impl SpatialNode {
                 let scrollable_height = scrollable_size.height;
 
                 if scrollable_height <= 0. && scrollable_width <= 0. {
                     return false;
                 }
 
                 let origin = LayoutPoint::new(origin.x.max(0.0), origin.y.max(0.0));
                 LayoutVector2D::new(
-                    (-origin.x).max(-scrollable_width).min(0.0).round(),
-                    (-origin.y).max(-scrollable_height).min(0.0).round(),
+                    (-origin.x).max(-scrollable_width).min(0.0),
+                    (-origin.y).max(-scrollable_height).min(0.0),
                 )
             }
             ScrollClamping::NoClamping => LayoutPoint::zero() - *origin,
         };
 
         let new_offset = normalized_offset - scrolling.external_scroll_offset;
 
         if new_offset == scrolling.offset {
@@ -344,17 +348,17 @@ impl SpatialNode {
                         LayoutFastTransform::with_vector(info.origin_in_parent_reference_frame)
                             .pre_transform(&source_transform);
 
                     // The transformation for this viewport in world coordinates is the transformation for
                     // our parent reference frame, plus any accumulated scrolling offsets from nodes
                     // between our reference frame and this node. Finally, we also include
                     // whatever local transformation this reference frame provides.
                     let relative_transform = resolved_transform
-                        .post_translate(snap_offset(state.parent_accumulated_scroll_offset, global_device_pixel_scale))
+                        .post_translate(snap_offset(state.parent_accumulated_scroll_offset, state.coordinate_system_relative_scale_offset.scale, global_device_pixel_scale))
                         .to_transform()
                         .with_destination::<LayoutPixel>();
 
                     let mut reset_cs_id = match info.transform_style {
                         TransformStyle::Preserve3D => !state.preserves_3d,
                         TransformStyle::Flat => state.preserves_3d,
                     };
 
@@ -419,23 +423,23 @@ impl SpatialNode {
                     &state.nearest_scrolling_ancestor_viewport,
                 );
 
                 // The transformation for the bounds of our viewport is the parent reference frame
                 // transform, plus any accumulated scroll offset from our parents, plus any offset
                 // provided by our own sticky positioning.
                 let accumulated_offset = state.parent_accumulated_scroll_offset + sticky_offset;
                 self.viewport_transform = state.coordinate_system_relative_scale_offset
-                    .offset(snap_offset(accumulated_offset, global_device_pixel_scale).to_untyped());
+                    .offset(snap_offset(accumulated_offset, state.coordinate_system_relative_scale_offset.scale, global_device_pixel_scale).to_untyped());
 
                 // The transformation for any content inside of us is the viewport transformation, plus
                 // whatever scrolling offset we supply as well.
                 let added_offset = accumulated_offset + self.scroll_offset();
                 self.content_transform = state.coordinate_system_relative_scale_offset
-                    .offset(snap_offset(added_offset, global_device_pixel_scale).to_untyped());
+                    .offset(snap_offset(added_offset, state.coordinate_system_relative_scale_offset.scale, global_device_pixel_scale).to_untyped());
 
                 if let SpatialNodeType::StickyFrame(ref mut info) = self.node_type {
                     info.current_offset = sticky_offset;
                 }
 
                 self.coordinate_system_id = state.current_coordinate_system_id;
             }
         }
@@ -626,25 +630,23 @@ impl SpatialNode {
 
         let scrollable_width = scrolling.scrollable_size.width;
         let scrollable_height = scrolling.scrollable_size.height;
         let original_layer_scroll_offset = scrolling.offset;
 
         if scrollable_width > 0. {
             scrolling.offset.x = (scrolling.offset.x + delta.x)
                 .min(0.0)
-                .max(-scrollable_width)
-                .round();
+                .max(-scrollable_width);
         }
 
         if scrollable_height > 0. {
             scrolling.offset.y = (scrolling.offset.y + delta.y)
                 .min(0.0)
-                .max(-scrollable_height)
-                .round();
+                .max(-scrollable_height);
         }
 
         scrolling.offset != original_layer_scroll_offset
     }
 
     pub fn scroll_offset(&self) -> LayoutVector2D {
         match self.node_type {
             SpatialNodeType::ScrollFrame(ref scrolling) => scrolling.offset,