Closed Bug 1623821 Opened 6 months ago Closed 4 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: gw, Assigned: bpeers)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch 0001-wip.patchSplinter Review

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.

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.

Flags: needinfo?(dmalyshau)

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.

Flags: needinfo?(dmalyshau)
Priority: -- → P3

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?

Flags: needinfo?(bpeers)
Flags: needinfo?(bpeers)
Attached image 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 :)

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? :|

Assignee: nobody → bpeers
Status: NEW → ASSIGNED

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.)

Assignee: bpeers → nobody
Severity: normal → S3
Status: ASSIGNED → NEW
Assignee: nobody → bpeers
Status: NEW → ASSIGNED
Attachment #9149201 - Attachment is obsolete: true
Pushed by bpeers@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f633d3245579
reftests/split/filter.yaml fails when picture caching optimization is disabled r=gw
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
You need to log in before you can comment on or make changes to this bug.