From 5ba48769933b8c7fafeacbef414722d0db483d6d Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Tue, 14 Dec 2021 01:06:24 +0000 Subject: [PATCH] Bug 1745775 - Fix invalid surface info indexing r=gfx-reviewers,bradwerth Some parts of the visibility pass have been ported to use the picture graph infrastructure (update pass assignment and bounding rect propagation) but the main visibility pass still relies on the old-style recursive traversal, for now. If an off-screen surface with a filter has a child primitive that has backface-visibility: false, it was possible for it to be excluded from surface assignment during the picture graph setup, but still visited by the old-style recursive visibility pass. In future, the main visibility pass will be ported to be based on the picture graph infrastructure. In the meantime, this introduces a band-aid fix by including the backface visibility check for a picture in the general `is_visible` method, which is already checked by the visibility and prepare passes, ensuring that the traversals match. Differential Revision: https://phabricator.services.mozilla.com/D133704 --- gfx/tests/crashtests/1745775.html | 24 +++++++++++ gfx/tests/crashtests/crashtests.list | 1 + gfx/wr/webrender/src/picture.rs | 61 +++++++++++++-------------- gfx/wr/webrender/src/picture_graph.rs | 5 ++- gfx/wr/webrender/src/visibility.rs | 2 +- 5 files changed, 59 insertions(+), 34 deletions(-) create mode 100644 gfx/tests/crashtests/1745775.html diff --git a/gfx/tests/crashtests/1745775.html b/gfx/tests/crashtests/1745775.html new file mode 100644 index 000000000000..d21e78e69512 --- /dev/null +++ b/gfx/tests/crashtests/1745775.html @@ -0,0 +1,24 @@ + + + + + + + \ No newline at end of file diff --git a/gfx/tests/crashtests/crashtests.list b/gfx/tests/crashtests/crashtests.list index b8dcba22275c..51de82e4aee5 100644 --- a/gfx/tests/crashtests/crashtests.list +++ b/gfx/tests/crashtests/crashtests.list @@ -208,3 +208,4 @@ load 1700232.html load 1704321-1.html load 1702638.html load 1730695.html +load 1745775.html diff --git a/gfx/wr/webrender/src/picture.rs b/gfx/wr/webrender/src/picture.rs index e71aa29f4ab1..ac1190421beb 100644 --- a/gfx/wr/webrender/src/picture.rs +++ b/gfx/wr/webrender/src/picture.rs @@ -4248,7 +4248,7 @@ impl PicturePrimitive { } } - fn resolve_scene_properties(&mut self, properties: &SceneProperties) -> bool { + fn resolve_scene_properties(&mut self, properties: &SceneProperties) { match self.composite_mode { Some(PictureCompositeMode::Filter(ref mut filter)) => { match *filter { @@ -4257,20 +4257,35 @@ impl PicturePrimitive { } _ => {} } - - filter.is_visible() } - _ => true, + _ => {} } } - pub fn is_visible(&self) -> bool { - match self.composite_mode { - Some(PictureCompositeMode::Filter(ref filter)) => { - filter.is_visible() + pub fn is_visible( + &self, + spatial_tree: &SpatialTree, + ) -> bool { + if let Some(PictureCompositeMode::Filter(ref filter)) = self.composite_mode { + if !filter.is_visible() { + return false; } - _ => true, } + + // For out-of-preserve-3d pictures, the backface visibility is determined by + // the local transform only. + // Note: we aren't taking the transform relativce to the parent picture, + // since picture tree can be more dense than the corresponding spatial tree. + if !self.is_backface_visible { + if let Picture3DContext::Out = self.context_3d { + match spatial_tree.get_local_visible_face(self.spatial_node_index) { + VisibleFace::Front => {} + VisibleFace::Back => return false, + } + } + } + + true } // TODO(gw): We have the PictureOptions struct available. We @@ -4321,7 +4336,7 @@ impl PicturePrimitive { self.primary_render_task_id = None; self.secondary_render_task_id = None; - if !self.is_visible() { + if !self.is_visible(frame_context.spatial_tree) { return None; } @@ -5661,30 +5676,12 @@ impl PicturePrimitive { /// Do initial checks to determine whether this picture should be drawn as part of the /// frame build. - pub fn pre_update_visibility_check( + pub fn pre_update( &mut self, frame_context: &FrameBuildingContext, - ) -> bool { - // Resolve animation properties, and early out if the filter - // properties make this picture invisible. - if !self.resolve_scene_properties(frame_context.scene_properties) { - return false; - } - - // For out-of-preserve-3d pictures, the backface visibility is determined by - // the local transform only. - // Note: we aren't taking the transform relativce to the parent picture, - // since picture tree can be more dense than the corresponding spatial tree. - if !self.is_backface_visible { - if let Picture3DContext::Out = self.context_3d { - match frame_context.spatial_tree.get_local_visible_face(self.spatial_node_index) { - VisibleFace::Front => {} - VisibleFace::Back => return false, - } - } - } - - true + ) { + // Resolve animation properties + self.resolve_scene_properties(frame_context.scene_properties); } /// Called during initial picture traversal, before we know the diff --git a/gfx/wr/webrender/src/picture_graph.rs b/gfx/wr/webrender/src/picture_graph.rs index 1c578b5e9965..daff7017024f 100644 --- a/gfx/wr/webrender/src/picture_graph.rs +++ b/gfx/wr/webrender/src/picture_graph.rs @@ -162,6 +162,9 @@ fn assign_update_pass( info.parent = parent_pic_index; + // Run pre-update to resolve animation properties etc + pic.pre_update(frame_context); + let can_be_drawn = match info.update_pass { Some(update_pass) => { // No point in recursing into paths in the graph if this picture already @@ -174,7 +177,7 @@ fn assign_update_pass( } None => { // Check if this picture can be dropped from the graph we're building this frame - pic.pre_update_visibility_check(frame_context) + pic.is_visible(frame_context.spatial_tree) } }; diff --git a/gfx/wr/webrender/src/visibility.rs b/gfx/wr/webrender/src/visibility.rs index bd41d251e920..0d157a94e5c8 100644 --- a/gfx/wr/webrender/src/visibility.rs +++ b/gfx/wr/webrender/src/visibility.rs @@ -270,7 +270,7 @@ pub fn update_primitive_visibility( PrimitiveInstanceKind::Picture { pic_index, .. } => { let (is_visible, is_passthrough) = { let pic = &store.pictures[pic_index.0]; - (pic.is_visible(), pic.raster_config.is_none()) + (pic.is_visible(frame_context.spatial_tree), pic.raster_config.is_none()) }; if !is_visible {