Bug 1589669 - Fix snapping and rounding errors causing picture caching invalidation when zoomed in. r=aosmond,botond
☠☠ backed out by 691a752d9319 ☠ ☠
authorKris Taeleman <ktaeleman@mozilla.com>
Fri, 20 Dec 2019 17:03:24 +0000
changeset 508276 6493da33ecacf937e915726cb71f9f1ec3f68da1
parent 508275 c9c4e554a53bfb7b8abbc8480edfeea4b6cfa5f0
child 508277 f19e270a7343c2e8bd5360e9f598900ad6152cea
push id36944
push userapavel@mozilla.com
push dateMon, 23 Dec 2019 21:41:35 +0000
treeherdermozilla-central@99d782b09b7d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond, botond
bugs1589669
milestone73.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 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> {
+
+    debug_assert!(scale.x != 0.0 && scale.y != 0.0);
+
+    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(snapped_world_offset.x / scale.x, snapped_world_offset.y / scale.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,