Closed
Bug 1372603
Opened 7 years ago
Closed 7 years ago
Push the layer's local clip outside the stacking context
Categories
(Core :: Graphics: WebRender, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
sotaro
:
review+
jrmuizel
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
Right now when we generate the WR display list from the layer tree, we do this thing where we push the stacking context, and then inside the stacking context, we push the clip. The clip that we push is the layer's "local clip" (i.e. layer->GetClipRect() and layer->GetMaskLayer()) which conceptually are in the ParentLayerPixel space and apply "outside" of the layer's transform (as opposed to say the layer's visible region which is in LayerPixel space and is "inside" the layer's transform).
Because we are pushing the clip to WR "inside" the stacking context, we have to convert it to the correct coordinate space by running it through the inverse of the layer's transform. This is undesirable both for purposes of accuracy and performance. It makes much more sense to push the clip "outside" the stacking context, which is where it really belongs.
Assignee | ||
Comment 1•7 years ago
|
||
Also, I should mention: the main reason I want to do this is because we'll need it for APZ. Specifically, when it comes to dealing with fixed-position layers, the layer's local clip needs to be inserted into the stack of scrolling clips at the right point so that it's fixed to the right thing. It's practically impossible to do if the layer's local clip is inside the stacking context but is much more reasonable if it's outside the stacking context.
Try push with WIPs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d323ff576a3a41d7c0aa6460ffb5c3a739bef8ba
Slightly older try push with less patches that's a green checkpoint: https://treeherder.mozilla.org/#/jobs?repo=try&revision=627729515c38bd19062df2193777169d7669a19c
Assignee | ||
Comment 2•7 years ago
|
||
Latest try push is looking green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb768210df0295330bc605ec1dc75813712e62a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Got some minor tweaks after self-reviewing the patches. Updated patches coming.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
cc'ing mrobinson as well to take a look at the patches.
Comment 17•7 years ago
|
||
These changes look good from my perspective. It's good to see masks moving from per-item clips to full display list item clips. This dovetails nicely with what I am working on as well.
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8877592 [details]
Bug 1372603 - Push the layer's local clip rect outside the stacking context.
https://reviewboard.mozilla.org/r/149058/#review153574
::: gfx/layers/wr/ScrollingLayersHelper.cpp:128
(Diff revision 2)
> + }
> return;
> }
> -
> - Layer* layer = mLayer->GetLayer();
> + if (mPushedLayerLocalClip) {
> + mBuilder->PopClip();
> + }
Can you just lift:
if (mPushedLayerLocalClip) {
mBuilder->PopClip();
}
above the:
if (!mLayer->WrManager()->AsyncPanZoomEnabled()) {
instead of duplicating it?
Attachment #8877592 -
Flags: review?(jmuizelaar) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8877593 [details]
Bug 1372603 - Make sure the mask rect is relative to the enclosing stacking context.
https://reviewboard.mozilla.org/r/149060/#review153576
Attachment #8877593 -
Flags: review?(jmuizelaar) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8877594 [details]
Bug 1372603 - Remove the duplicated clip from WebRenderDisplayItemLayer.
https://reviewboard.mozilla.org/r/149062/#review153580
Attachment #8877594 -
Flags: review?(jmuizelaar) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8877596 [details]
Bug 1372603 - Remove unused functions and parameters.
https://reviewboard.mozilla.org/r/149066/#review153584
Attachment #8877596 -
Flags: review?(jmuizelaar) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8877597 [details]
Bug 1372603 - Remove unnecessary clip from container layers.
https://reviewboard.mozilla.org/r/149068/#review153586
Attachment #8877597 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 23•7 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> Can you just lift:
>
> if (mPushedLayerLocalClip) {
> mBuilder->PopClip();
> }
>
> above the:
>
> if (!mLayer->WrManager()->AsyncPanZoomEnabled()) {
>
> instead of duplicating it?
Sure.
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8877595 [details]
Bug 1372603 - Move the clip rect for async images outside the iframe item.
https://reviewboard.mozilla.org/r/149064/#review153592
Attachment #8877595 -
Flags: review?(jmuizelaar) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8877595 [details]
Bug 1372603 - Move the clip rect for async images outside the iframe item.
https://reviewboard.mozilla.org/r/149064/#review153768
Attachment #8877595 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 32•7 years ago
|
||
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5864f2de59f
Push the layer's local clip rect outside the stacking context. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/b6716c3de3f0
Make sure the mask rect is relative to the enclosing stacking context. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/7ece58ca5ca3
Remove the duplicated clip from WebRenderDisplayItemLayer. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/f4274f59cd27
Move the clip rect for async images outside the iframe item. r=jrmuizel,sotaro
https://hg.mozilla.org/integration/autoland/rev/7e729c07650c
Remove unused functions and parameters. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/20831c1fdba3
Remove unnecessary clip from container layers. r=jrmuizel
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b5864f2de59f
https://hg.mozilla.org/mozilla-central/rev/b6716c3de3f0
https://hg.mozilla.org/mozilla-central/rev/7ece58ca5ca3
https://hg.mozilla.org/mozilla-central/rev/f4274f59cd27
https://hg.mozilla.org/mozilla-central/rev/7e729c07650c
https://hg.mozilla.org/mozilla-central/rev/20831c1fdba3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•