Fix some redundant cases of clip mask generation. #2115

glennw · 2017-11-28T06:00:30Z

There are two fixes here:

Image pictures were generating clip masks, but they are never
actually used. In the future, we might want to support this,
but at the moment it just results in clip masks that are then
never used.
If a primitive is in the root coordinate system, then we know
that it is axis-aligned, and that there are no coordinate
system changes in between this primitive and the root. In that
case, we can infer that the local_clip_rect of the clip scroll
node will correctly handle the screen space clip rect.

In Gecko with the current WR code, I was seeing a lot of very large
clip masks being generated (4x extra A8 render targets) on HN. The
test case included here is a very reduced test case for these issues.

The test case makes use of the new reftest annotations to assert
that no alpha targets get allocated while rendering the yaml file.

This change is

glennw · 2017-11-28T06:01:46Z

r? @mrobinson

Do these fixes seem correct to you?

Gecko try run is pending:
https://hg.mozilla.org/try/rev/f17ce9d263711325d1fe2c3e485903d16307e98c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f17ce9d263711325d1fe2c3e485903d16307e98c

glennw · 2017-11-28T06:04:23Z

PS: In manual testing, this does appear to greatly reduce the amount of redundant clips masks (though there are still quite a few) on many web pages.

glennw · 2017-11-28T06:53:31Z

Even with this change, I'm still seeing heaps of clip masks being drawn when I wouldn't expect there to be. I don't have a good repro yet - I'll try to track this down more tomorrow.

glennw · 2017-11-28T07:17:03Z

Gecko try run above looks green.

mrobinson

Nice! Just a couple nits, but this looks fine to me.

mrobinson · 2017-11-28T08:10:40Z

webrender/src/prim_store.rs

                    };
-                    (Some((pic.pipeline_id, mem::replace(&mut pic.runs, Vec::new()), rfid)), pic.cull_children)
+                    (Some((pic.pipeline_id, mem::replace(&mut pic.runs, Vec::new()),rfid)),


Nit: Missing space in "Vec::new()),rfid"

mrobinson · 2017-11-28T08:10:40Z

wrench/reftests/performance/no-clip-mask.yaml

+      items:
+        -
+          bounds: [0, 111, 1887, 1365]
+          "clip-rect": [0, 111, 1887, 1365]


It might be worth leaving a comment here explaining why this test doesn't generate any clip masks.

mrobinson · 2017-11-28T08:11:58Z

This does suggest that there is another bug. If the locally clipping of the layer is taking effect, we probably shouldn't see the outer rectangle of the clip shrinking at all. I'll investigate this today.


        Fix some redundant cases of clip mask generation.

There are two fixes here: * Image pictures were generating clip masks, but they are never actually used. In the future, we *might* want to support this, but at the moment it just results in clip masks that are then never used. * If a primitive is in the root coordinate system, then we know that it is axis-aligned, and that there are no coordinate system changes in between this primitive and the root. In that case, we can infer that the local_clip_rect of the clip scroll node will correctly handle the screen space clip rect. In Gecko with the current WR code, I was seeing a lot of very large clip masks being generated (4x extra A8 render targets) on HN. The test case included here is a very reduced test case for these issues. The test case makes use of the new reftest annotations to assert that no alpha targets get allocated while rendering the yaml file.

glennw · 2017-11-28T08:35:28Z

Thanks, fixed up those nits. I do think there's another bug, like you said - since I see a lot of redundant clip masks still after this patch. If you want to hold off on merging this until you investigate and see if the bug fix also covers these issues, that's fine with me. Otherwise, we can merge this and revert if we find a better solution.

mrobinson · 2017-11-28T08:41:06Z

@glennw I think it's better to merge this ASAP, since it's unclear how much work it will be to fix the deeper issue.

mrobinson · 2017-11-28T08:41:14Z

@bors-servo r+

bors-servo · 2017-11-28T08:41:15Z

📌 Commit 5f0aee1 has been approved by mrobinson

bors-servo · 2017-11-28T08:41:18Z

⌛ Testing commit 5f0aee1 with merge e30886d...


        Auto merge of #2115 - glennw:fix-clips-more, r=mrobinson

Fix some redundant cases of clip mask generation. There are two fixes here: * Image pictures were generating clip masks, but they are never actually used. In the future, we *might* want to support this, but at the moment it just results in clip masks that are then never used. * If a primitive is in the root coordinate system, then we know that it is axis-aligned, and that there are no coordinate system changes in between this primitive and the root. In that case, we can infer that the local_clip_rect of the clip scroll node will correctly handle the screen space clip rect. In Gecko with the current WR code, I was seeing a lot of very large clip masks being generated (4x extra A8 render targets) on HN. The test case included here is a very reduced test case for these issues. The test case makes use of the new reftest annotations to assert that no alpha targets get allocated while rendering the yaml file.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2115)

bors-servo · 2017-11-28T09:53:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: mrobinson
Pushing e30886d to master...

kvark

~~@glennw did you happen to run a gecko try push with this change?~~
nvm, I see it now :)

kvark · 2017-11-28T14:41:50Z

webrender/src/prim_store.rs

+            // we know that the local_clip_rect in the clip node
+            // will take care of applying this clip, so no need
+            // for a mask.
+            if prim_coordinate_system_id == CoordinateSystemId::root() {


I don't fully understand why this is needed. Wouldn't the same apply to any axis-aligned coordinate system, which should already be handled?

It doesn't seem to handle it - @mrobinson thinks there might be a deeper issue that he is looking into. If / when that's resolved we could hopefully remove this.

kvark · 2017-11-28T14:41:50Z

webrender/src/prim_store.rs

@@ -1476,7 +1490,7 @@ impl PrimitiveStore {
            (local_rect, xf_rect.bounding_rect)
        };

-        if !self.update_clip_task(
+        if perform_culling && may_need_clip_mask && !self.update_clip_task(


It doesn't appear obviously correct to me that we move the perform_culling check before the update_clip_task. This will result in stale metadata.screen_rect and metadata.clip_task being present in the rendered primitives.

perform_culling is effectively a constant - it never changes from initial value. Therefore, the clip task will never be set, and the screen rect should only depend on the previous culling values, not part of the clip stack.

glennw mentioned this pull request Nov 28, 2017

Better ClipChain optimization #2104

Merged

mrobinson approved these changes Nov 28, 2017

View changes

glennw force-pushed the glennw:fix-clips-more branch from d23c1c6 to 5f0aee1 Nov 28, 2017

kvark reviewed Nov 28, 2017

View changes

glennw deleted the glennw:fix-clips-more branch Nov 28, 2017

servo / webrender

Fix some redundant cases of clip mask generation. #2115

Fix some redundant cases of clip mask generation. #2115

glennw commented Nov 28, 2017 •

edited by larsbergstrom

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

mrobinson commented Nov 28, 2017

glennw commented Nov 28, 2017

mrobinson commented Nov 28, 2017

mrobinson commented Nov 28, 2017

bors-servo commented Nov 28, 2017

bors-servo commented Nov 28, 2017

bors-servo commented Nov 28, 2017

kvark left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / webrender

Join GitHub today

Fix some redundant cases of clip mask generation. #2115

Fix some redundant cases of clip mask generation. #2115

Conversation

glennw commented Nov 28, 2017 • edited by larsbergstrom

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

glennw commented Nov 28, 2017

mrobinson left a comment

This comment has been minimized.

mrobinson Nov 28, 2017

This comment has been minimized.

mrobinson Nov 28, 2017

mrobinson commented Nov 28, 2017

glennw commented Nov 28, 2017

mrobinson commented Nov 28, 2017

mrobinson commented Nov 28, 2017

bors-servo commented Nov 28, 2017

bors-servo commented Nov 28, 2017

bors-servo commented Nov 28, 2017

kvark left a comment • edited

This comment has been minimized.

kvark Nov 28, 2017

This comment has been minimized.

glennw Nov 28, 2017

This comment has been minimized.

kvark Nov 28, 2017

This comment has been minimized.

glennw Nov 28, 2017

glennw commented Nov 28, 2017 •

edited by larsbergstrom

kvark left a comment •

edited