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)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(6 files)

Bug 1372603 - Push the layer's local clip rect outside the stacking context.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1372603 - Make sure the mask rect is relative to the enclosing stacking context.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1372603 - Remove the duplicated clip from WebRenderDisplayItemLayer.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1372603 - Move the clip rect for async images outside the iframe item.
59 bytes, text/x-review-board-request
sotaro
: review+
jrmuizel
: review+
Details
Bug 1372603 - Remove unused functions and parameters.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1372603 - Remove unnecessary clip from container layers.
59 bytes, 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.
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
Got some minor tweaks after self-reviewing the patches. Updated patches coming.
cc'ing mrobinson as well to take a look at the patches.
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 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 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 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 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 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+
(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 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 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+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: