1623821 - reftests/split/filter.yaml fails when picture caching optimization is disabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Glenn Watson [:gw]]

Attachment: 0001-wip.patch

Applying the attached patch disables the picture cache optimization code, which can collapse a picture that has a single primitive + opacity filter into a single primitive with transparency. We want to remove this picture cache optimization code, as it's fragile, complex and no longer needed with picture caching.

However, disabling that optimization makes the reftests/split/filter.yaml reftest fail. I took a quick look at it - it seems that the picture for the filter gets created, but it's incorrectly added to the 3d context root, or perhaps is missing an additional container picture before being added to the root.

Comment 1

[Glenn Watson [:gw]]

Dzmitry, I'm assuming you're probably too busy with wgpu stuff right now, but you might have some ideas on how to solve this? If not, I'll try take a look next week at what's going on.

Comment 2

[Dzmitry Malyshau [:kvark]]

Looks like we are not properly handling the override for a SC to drop the preserve-3d property if it has a filter? Doesn't sound too complex to investigate. Should be somewhere in the scene building where we check both.

Comment 3

[Glenn Watson [:gw]]

Bert, this could be another good one to work on, if you have time, while we work out the next steps for the gradient work?

Comment 4

[Bert Peers [:bpeers]]

Attachment: compare_no_caching.png

I expected the bug to be backfacing issues making the rect invisible; but it shows up, just with the wrong alpha blending. You can simplify the reftest a bit more:

root:
  items:
    - type: stacking-context
      bounds: [0, 0, 200, 200]
      transform-style: preserve-3d
      items:
        - type: stacking-context
          bounds: [0, 0, 200, 200]
          filters: [opacity(0.5)]
          items:
            - type: rect
              bounds: [0, 0, 200, 200]
              color: red

The key difference is here:

let trailing_children_instance = match self.sc_stack.last_mut() {
    // Preserve3D path (only relevant if there are no filters/mix-blend modes)
    Some(ref parent_sc) if parent_sc.is_3d() => {
        Some(cur_instance)
    }

Remove this and it works, ie. directly adding to sc.prim_list.add_prim is fine but going via add_primitive_instance_to_3d_root is buggy. I got as far as verifying that the picture we drain here still has the right filter on it, so, I guess it's an order of operations thing? Comparing good/bad in renderdoc (--no-batch --no-picture-caching), it looks like all blends still happen, just in the wrong order.

I'll keep looking tomorrow, meanwhile thoughts and tips welcome :)

Comment 5

[Bert Peers [:bpeers]]

Read/skimmed this, this and this, still not too sure what the code should do or even what it is doing, but it helped :)

I fired off a try run edit: new run that's not broken with this hack (and a bit of fuzzing):

diff --git a/gfx/wr/webrender/src/prim_store/mod.rs b/gfx/wr/webrender/src/prim_store/mod.rs
index 4a1671ace77d..8ce74484befa 100644
--- a/gfx/wr/webrender/src/prim_store/mod.rs
+++ b/gfx/wr/webrender/src/prim_store/mod.rs
@@ -2630,6 +2630,8 @@ impl PrimitiveStore {
         &mut self,
         pic_index: PictureIndex,
     ) {
+        return;   //--bpe hack patch from 1623821
+
         // Only handle opacity filters for now.
         let binding = match self.pictures[pic_index.0].requested_composite_mode {
             Some(PictureCompositeMode::Filter(Filter::Opacity(binding, _))) => {
diff --git a/gfx/wr/webrender/src/scene_building.rs b/gfx/wr/webrender/src/scene_building.rs
index af1a59d67aa9..6cbd3222fbd7 100644
--- a/gfx/wr/webrender/src/scene_building.rs
+++ b/gfx/wr/webrender/src/scene_building.rs
@@ -2154,6 +2154,8 @@ impl<'a> SceneBuilder<'a> {
             true,
         );
 
+        let has_filters = current_pic_index.0 != filtered_pic_index.0;
+
         current_pic_index = filtered_pic_index;
         cur_instance = filtered_instance;
 
@@ -2220,7 +2222,7 @@ impl<'a> SceneBuilder<'a> {
         // if it's a part of 3D hierarchy but not the root of it.
         let trailing_children_instance = match self.sc_stack.last_mut() {
             // Preserve3D path (only relevant if there are no filters/mix-blend modes)
-            Some(ref parent_sc) if parent_sc.is_3d() => {
+            Some(ref parent_sc) if !has_filters && parent_sc.is_3d() => {
                 Some(cur_instance)
             }
             // Regular parenting path

We'll see? :|

Comment 6

[Bert Peers [:bpeers]]

Attachment: Bug 1623821 - reftests/split/filter.yaml fails when picture caching optimization is disabled

Comment 7

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 8

[Bert Peers [:bpeers]]

Attachment: Bug 1623821 - reftests/split/filter.yaml fails when picture caching optimization is disabled

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f633d3245579
reftests/split/filter.yaml fails when picture caching optimization is disabled r=gw

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f633d3245579