Bug 1761299 - Fix raster spatial node indices being out of sync r=gfx-reviewers,nical

Glenn Watson · Glenn Watson · commit afcf7bc8dd70 · 2022-03-25T16:10:40.000Z
When the max surface size is exceeded, the raster spatial node for a given surface is adjusted in `get_surface_rects`. However a locally cached variable of the old raster spatial node index was being used as a parameter when creating the render task. Instead, only retrieve the raster spatial node after the call to `get_surface_rects`, ensuring that it gets the updated value. Also add support to wrench reftests to set the maximum surface size, so that we can exercise this code path in reftests. Differential Revision: https://phabricator.services.mozilla.com/D142015 [ghsync] From https://hg.mozilla.org/mozilla-central/rev/3b9aede36b948000eee4351484b8bcb88f1e83e0
diff --git a/webrender/src/frame_builder.rs b/webrender/src/frame_builder.rs
@@ -72,6 +72,7 @@ pub struct FrameBuilderConfig {
     pub background_color: Option<ColorF>,
     pub compositor_kind: CompositorKind,
     pub tile_size_override: Option<DeviceIntSize>,
+    pub max_surface_override: Option<usize>,
     pub max_depth_ids: i32,
     pub max_target_size: i32,
     pub force_invalidation: bool,
@@ -525,8 +526,6 @@ impl FrameBuilder {
                 .pictures[pic_index.0]
                 .take_context(
                     *pic_index,
-                    root_spatial_node_index,
-                    root_spatial_node_index,
                     None,
                     SubpixelMode::Allow,
                     &mut frame_state,
diff --git a/webrender/src/picture.rs b/webrender/src/picture.rs
@@ -280,7 +280,7 @@ pub const TILE_SIZE_SCROLLBAR_VERTICAL: DeviceIntSize = DeviceIntSize {
 
 /// The maximum size per axis of a surface,
 ///  in WorldPixel coordinates.
-const MAX_SURFACE_SIZE: f32 = 4096.0;
+const MAX_SURFACE_SIZE: usize = 4096;
 /// Maximum size of a compositor surface.
 const MAX_COMPOSITOR_SURFACES_SIZE: f32 = 8192.0;
 
@@ -4433,8 +4433,6 @@ impl PicturePrimitive {
     pub fn take_context(
         &mut self,
         pic_index: PictureIndex,
-        surface_spatial_node_index: SpatialNodeIndex,
-        raster_spatial_node_index: SpatialNodeIndex,
         parent_surface_index: Option<SurfaceIndex>,
         parent_subpixel_mode: SubpixelMode,
         frame_state: &mut FrameBuildingState,
@@ -4451,27 +4449,11 @@ impl PicturePrimitive {
 
         profile_scope!("take_context");
 
-        // Extract the raster and surface spatial nodes from the raster
-        // config, if this picture establishes a surface. Otherwise just
-        // pass in the spatial node indices from the parent context.
-        let (raster_spatial_node_index, surface_spatial_node_index, surface_index) = match self.raster_config {
-            Some(ref raster_config) => {
-                let surface = &frame_state.surfaces[raster_config.surface_index.0];
-
-                (
-                    surface.raster_spatial_node_index,
-                    self.spatial_node_index,
-                    raster_config.surface_index,
-                )
-            }
-            None => {
-                (
-                    raster_spatial_node_index,
-                    surface_spatial_node_index,
-                    parent_surface_index.expect("bug: no parent"),
-                )
-            }
+        let surface_index = match self.raster_config {
+            Some(ref raster_config) => raster_config.surface_index,
+            None => parent_surface_index.expect("bug: no parent"),
         };
+        let surface_spatial_node_index = frame_state.surfaces[surface_index.0].surface_spatial_node_index;
 
         let map_pic_to_world = SpaceMapper::new_with_target(
             frame_context.root_spatial_node_index,
@@ -4809,7 +4791,8 @@ impl PicturePrimitive {
                                             tile_cache.current_tile_size.to_f32(),
                                             content_origin,
                                             surface_spatial_node_index,
-                                            raster_spatial_node_index,
+                                            // raster == surface implicitly for picture cache tiles
+                                            surface_spatial_node_index,
                                             device_pixel_scale,
                                             Some(tile_key),
                                             Some(scissor_rect),
@@ -4950,18 +4933,27 @@ impl PicturePrimitive {
                     self.prev_local_rect = local_rect;
                 }
 
+                let max_surface_size = frame_context
+                    .fb_config
+                    .max_surface_override
+                    .unwrap_or(MAX_SURFACE_SIZE) as f32;
+
                 let surface_rects = match get_surface_rects(
                     raster_config.surface_index,
                     &raster_config.composite_mode,
                     parent_surface_index,
                     &mut frame_state.surfaces,
                     frame_context.spatial_tree,
+                    max_surface_size,
                 ) {
                     Some(rects) => rects,
                     None => return None,
                 };
 
-                let device_pixel_scale = frame_state.surfaces[raster_config.surface_index.0].device_pixel_scale;
+                let (raster_spatial_node_index, device_pixel_scale) = {
+                    let surface = &frame_state.surfaces[surface_index.0];
+                    (surface.raster_spatial_node_index, surface.device_pixel_scale)
+                };
                 let cmd_buffer_builder = CommandBufferBuilder::new_simple(
                     surface_rects.clipped_local,
                 );
@@ -5412,7 +5404,7 @@ impl PicturePrimitive {
         let context = PictureContext {
             pic_index,
             apply_local_clip_rect: self.apply_local_clip_rect,
-            raster_spatial_node_index,
+            raster_spatial_node_index: frame_state.surfaces[surface_index.0].raster_spatial_node_index,
             surface_spatial_node_index,
             surface_index,
             dirty_region_count,
@@ -6762,6 +6754,7 @@ fn get_surface_rects(
     parent_surface_index: SurfaceIndex,
     surfaces: &mut [SurfaceInfo],
     spatial_tree: &SpatialTree,
+    max_surface_size: f32,
 ) -> Option<SurfaceAllocInfo> {
     let parent_surface = &surfaces[parent_surface_index.0];
 
@@ -6871,19 +6864,19 @@ fn get_surface_rects(
 
     let task_size_f = clipped.size();
 
-    if task_size_f.width > MAX_SURFACE_SIZE || task_size_f.height > MAX_SURFACE_SIZE {
+    if task_size_f.width > max_surface_size || task_size_f.height > max_surface_size {
         let max_dimension = clipped_local.width().max(clipped_local.height()).ceil();
 
         surface.raster_spatial_node_index = surface.surface_spatial_node_index;
-        surface.device_pixel_scale = Scale::new(MAX_SURFACE_SIZE / max_dimension);
+        surface.device_pixel_scale = Scale::new(max_surface_size / max_dimension);
 
         clipped = (clipped_local.cast_unit() * surface.device_pixel_scale).round();
         unclipped = unclipped_local.cast_unit() * surface.device_pixel_scale;
     }
 
     let task_size = clipped.size().to_i32();
-    debug_assert!(task_size.width <= MAX_SURFACE_SIZE as i32);
-    debug_assert!(task_size.height <= MAX_SURFACE_SIZE as i32);
+    debug_assert!(task_size.width <= max_surface_size as i32);
+    debug_assert!(task_size.height <= max_surface_size as i32);
 
     let uv_rect_kind = calculate_uv_rect_kind(
         clipped,
@@ -6990,5 +6983,6 @@ fn test_large_surface_scale_1() {
         SurfaceIndex(0),
         &mut surfaces,
         &spatial_tree,
+        MAX_SURFACE_SIZE as f32,
     );
 }
diff --git a/webrender/src/prepare.rs b/webrender/src/prepare.rs
@@ -148,8 +148,6 @@ fn prepare_prim_for_render(
 
         match pic.take_context(
             pic_index,
-            pic_context.surface_spatial_node_index,
-            pic_context.raster_spatial_node_index,
             Some(pic_context.surface_index),
             pic_context.subpixel_mode,
             frame_state,
diff --git a/webrender/src/render_api.rs b/webrender/src/render_api.rs
@@ -904,6 +904,8 @@ pub enum DebugCommand {
     SimulateLongSceneBuild(u32),
     /// Set an override tile size to use for picture caches
     SetPictureTileSize(Option<DeviceIntSize>),
+    /// Set an override for max off-screen surface size
+    SetMaximumSurfaceSize(Option<usize>),
 }
 
 /// Message sent by the `RenderApi` to the render backend thread.
diff --git a/webrender/src/render_backend.rs b/webrender/src/render_backend.rs
@@ -968,6 +968,12 @@ impl RenderBackend {
 
                         return RenderBackendStatus::Continue;
                     }
+                    DebugCommand::SetMaximumSurfaceSize(surface_size) => {
+                        self.frame_config.max_surface_override = surface_size;
+                        self.update_frame_builder_config();
+
+                        return RenderBackendStatus::Continue;
+                    }
                     #[cfg(feature = "capture")]
                     DebugCommand::SaveCapture(root, bits) => {
                         let output = self.save_capture(root, bits);
diff --git a/webrender/src/renderer/mod.rs b/webrender/src/renderer/mod.rs
@@ -1180,6 +1180,7 @@ impl Renderer {
             background_color: Some(options.clear_color),
             compositor_kind,
             tile_size_override: None,
+            max_surface_override: None,
             max_depth_ids: device.max_depth_ids(),
             max_target_size: max_internal_texture_size,
             force_invalidation: false,
@@ -1659,7 +1660,8 @@ impl Renderer {
     fn handle_debug_command(&mut self, command: DebugCommand) {
         match command {
             DebugCommand::EnableDualSourceBlending(_) |
-            DebugCommand::SetPictureTileSize(_) => {
+            DebugCommand::SetPictureTileSize(_) |
+            DebugCommand::SetMaximumSurfaceSize(_) => {
                 panic!("Should be handled by render backend");
             }
             DebugCommand::SaveCapture(..) |
diff --git a/webrender/src/scene.rs b/webrender/src/scene.rs
@@ -325,6 +325,7 @@ impl BuiltScene {
                 background_color: None,
                 compositor_kind: CompositorKind::default(),
                 tile_size_override: None,
+                max_surface_override: None,
                 max_depth_ids: 0,
                 max_target_size: 0,
                 force_invalidation: false,
diff --git a/wrench/reftests/snap/1761299-ref.yaml b/wrench/reftests/snap/1761299-ref.yaml
@@ -0,0 +1,6 @@
+---
+root:
+  items:
+    - type: rect
+      bounds: [0, 0, 512, 128]
+      color: red
diff --git a/wrench/reftests/snap/1761299.yaml b/wrench/reftests/snap/1761299.yaml
@@ -0,0 +1,12 @@
+# verify that the raster spatial node selected when a surface is too large
+# is correct propagated to the picture render task
+---
+root:
+  items:
+    - type: stacking-context
+      filters: [identity]
+      transform: scale(0.5,1,1)
+      items:
+        - type: rect
+          bounds: [0, 0, 1024, 128]
+          color: red
diff --git a/wrench/reftests/snap/reftest.list b/wrench/reftests/snap/reftest.list
@@ -3,3 +3,4 @@ platform(linux,mac) == snap.yaml snap.png
 platform(linux,mac) == preserve-3d.yaml preserve-3d.png
 fuzzy(128,200) == subpixel-raster-root.yaml subpixel-raster-root-ref.yaml
 platform(linux,mac) == fractional-filter.yaml fractional-filter-ref.yaml
+max_surface_size(256) == 1761299.yaml 1761299.yaml
diff --git a/wrench/src/reftest.rs b/wrench/src/reftest.rs
@@ -105,6 +105,7 @@ pub struct Reftest {
     disable_dual_source_blending: bool,
     allow_mipmaps: bool,
     force_subpixel_aa_where_possible: Option<bool>,
+    max_surface_override: Option<usize>,
 }
 
 impl Reftest {
@@ -362,6 +363,7 @@ impl ReftestManifest {
             let mut disable_dual_source_blending = false;
             let mut allow_mipmaps = false;
             let mut force_subpixel_aa_where_possible = None;
+            let mut max_surface_override = None;
 
             let mut parse_command = |token: &str| -> bool {
                 match token {
@@ -419,6 +421,10 @@ impl ReftestManifest {
                         let (_, args, _) = parse_function(function);
                         extra_checks.push(ExtraCheck::ColorTargets(args[0].parse().unwrap()));
                     }
+                    function if function.starts_with("max_surface_size(") => {
+                        let (_, args, _) = parse_function(function);
+                        max_surface_override = Some(args[0].parse().unwrap());
+                    }
                     options if options.starts_with("options(") => {
                         let (_, args, _) = parse_function(options);
                         if args.iter().any(|arg| arg == &OPTION_DISABLE_SUBPX) {
@@ -541,6 +547,7 @@ impl ReftestManifest {
                 disable_dual_source_blending,
                 allow_mipmaps,
                 force_subpixel_aa_where_possible,
+                max_surface_override,
             });
         }
 
@@ -746,6 +753,13 @@ impl<'a> ReftestHarness<'a> {
                     DebugCommand::EnableDualSourceBlending(false)
                 );
         }
+        if let Some(max_surface_override) = t.max_surface_override {
+            self.wrench
+                .api
+                .send_debug_cmd(
+                    DebugCommand::SetMaximumSurfaceSize(Some(max_surface_override))
+                );
+        }
 
         let window_size = self.window.get_inner_size();
         let reference_image = match t.reference.extension().unwrap().to_str().unwrap() {
@@ -837,6 +851,13 @@ impl<'a> ReftestHarness<'a> {
                     DebugCommand::EnableDualSourceBlending(true)
                 );
         }
+        if let Some(_) = t.max_surface_override {
+            self.wrench
+                .api
+                .send_debug_cmd(
+                    DebugCommand::SetMaximumSurfaceSize(None)
+                );
+        }
 
         for extra_check in t.extra_checks.iter() {
             if !extra_check.run(&results) {

Original file line number	Diff line number	Diff line change
`@@ -904,6 +904,8 @@ pub enum DebugCommand {`
`904`	`904`	`SimulateLongSceneBuild(u32),`
`905`	`905`	`/// Set an override tile size to use for picture caches`
`906`	`906`	`SetPictureTileSize(Option<DeviceIntSize>),`
	`907`	`+ /// Set an override for max off-screen surface size`
	`908`	`+ SetMaximumSurfaceSize(Option<usize>),`
`907`	`909`	`}`
`908`	`910`
`909`	`911`	/// Message sent by the `RenderApi` to the render backend thread.