1355012 - Add mask support for WebrenderDisplayItemLayer

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ethan Lin[:ethlin]]

Currently WebrenderDisplayItemLayer doesn't handle mask layer. It causes some reftest failures. I'll add mask layer support for WebrenderDisplayItemLayer.

Comment 1

[Ethan Lin[:ethlin]]

The reftest 'layout/reftests/border-radius/iframe-1.html' is an example. When I turning on the pref 'layers.advanced.canvas-background-color', we will generate a mask layer for the DisplayItemLayer(canvas-background-color).

Comment 2

[Ethan Lin[:ethlin]]

Attachment: apply mask in WebRenderDisplayItemLayer

WebRenderLayer already has the function 'BuildWrMaskLayer', so we can just reuse the function and create a ScrollLayer for the mask. The tricky thing is WRDisplayItemLayer doesn't push transform to stacking context, so we shouldn't apply the reverse transform to mask layer. And the WR built display list doesn't support ScrollLayer[1], so I push the ScrollLayer to the base DisplayListBuilder.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_traits/src/display_list.rs#516

Comment 3

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8856431 [details] [diff] [review]
apply mask in WebRenderDisplayItemLayer

Review of attachment 8856431 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +26,5 @@
> +    Maybe<WrImageMask> mask = BuildWrMaskLayer(gfx::Matrix4x4());
> +    if (mask.ptrOr(nullptr)) {
> +      Rect rect = TransformedVisibleBoundsRelativeToParent();
> +      Rect overflow(0.0, 0.0, rect.width, rect.height);
> +      aBuilder.PushScrollLayer(wr::ToWrRect(rect),

Why use a scroll layer instead of a stacking context? I guess we could just create a stacking context here instead since that's what all the other layers use?

Comment 4

[Ethan Lin[:ethlin]]

(In reply to Mason Chang [:mchang] from comment #3)
> Comment on attachment 8856431 [details] [diff] [review]
> apply mask in WebRenderDisplayItemLayer
> 
> Review of attachment 8856431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
> @@ +26,5 @@
> > +    Maybe<WrImageMask> mask = BuildWrMaskLayer(gfx::Matrix4x4());
> > +    if (mask.ptrOr(nullptr)) {
> > +      Rect rect = TransformedVisibleBoundsRelativeToParent();
> > +      Rect overflow(0.0, 0.0, rect.width, rect.height);
> > +      aBuilder.PushScrollLayer(wr::ToWrRect(rect),
> 
> Why use a scroll layer instead of a stacking context? I guess we could just
> create a stacking context here instead since that's what all the other
> layers use?

Originally I thought scroll layer is simpler since the stacking context will change the coordinate. But it looks like scroll layer may clip other display items. I'll upload a patch with using stacking context for mask layer.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: apply mask in WebRenderDisplayItemLayer

In this patch I use stacking context to do the masking. I still have to push it in base display list builder since wr_dp_push_stacking_context also push scroll layer[1].


[1] https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#1101

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ethan Lin[:ethlin] from comment #5)
> Created attachment 8856847 [details] [diff] [review]
> apply mask in WebRenderDisplayItemLayer
> 
> In this patch I use stacking context to do the masking. I still have to push
> it in base display list builder since wr_dp_push_stacking_context also push
> scroll layer[1].

Note that pushing a scroll layer with every stacking context is probably not the right thing to be doing. I'm looking into this and trying to line up the way we push stacking contexts and scroll layers to be more correct with respect to what webrender is expecting.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: apply mask in WebRenderDisplayItemLayer

Rebase the patch.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Note that pushing a scroll layer with every stacking context is probably not
> the right thing to be doing. I'm looking into this and trying to line up the
> way we push stacking contexts and scroll layers to be more correct with
> respect to what webrender is expecting.

Bug 1355791 does a good chunk of this.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8857384 [details] [diff] [review]
apply mask in WebRenderDisplayItemLayer

Review of attachment 8857384 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +26,5 @@
>    }
>  
> +  Maybe<WrImageMask> mask = BuildWrMaskLayer(Matrix4x4());
> +  WrImageMask* imageMask = mask.ptr();
> +  if (imageMask) {

This doesn't look right, you can only call ptr() if you know there is something inside the Maybe. You should use ptrOr(nullptr), or do |if (mask)|

@@ +58,5 @@
>    aBuilder.PushBuiltDisplayList(Move(mBuiltDisplayList));
>    WrBridge()->AddWebRenderParentCommands(mParentCommands);
> +
> +  if (imageMask) {
> +    aBuilder.PopStackingContext();

You're missing a PopScrollLayer here

Comment 10

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Comment on attachment 8857384 [details] [diff] [review]
> apply mask in WebRenderDisplayItemLayer
> 
> Review of attachment 8857384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
> @@ +26,5 @@
> >    }
> >  
> > +  Maybe<WrImageMask> mask = BuildWrMaskLayer(Matrix4x4());
> > +  WrImageMask* imageMask = mask.ptr();
> > +  if (imageMask) {
> 
> This doesn't look right, you can only call ptr() if you know there is
> something inside the Maybe. You should use ptrOr(nullptr), or do |if (mask)|
> 

Oh..right, I'll fix it.

> @@ +58,5 @@
> >    aBuilder.PushBuiltDisplayList(Move(mBuiltDisplayList));
> >    WrBridge()->AddWebRenderParentCommands(mParentCommands);
> > +
> > +  if (imageMask) {
> > +    aBuilder.PopStackingContext();
> 
> You're missing a PopScrollLayer here

Sorry, I will add pop function. I think I should remove stacking context since we already separated stacking context and scroll layer. And I just want do masking here. kats, Is that right?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ethan Lin[:ethlin] from comment #10)
> Sorry, I will add pop function. I think I should remove stacking context
> since we already separated stacking context and scroll layer. And I just
> want do masking here. kats, Is that right?

I think yes, you don't need the stacking context. I'm not sure you need the scroll layer either, actually. Can you not put the mask image in the existing clip region for the items? So for the example you mentioned in comment 1, where we generate a canvas background color, I think you really want the mask to be added to the clip region at [1]. But I don't know if the display item has a reference to the mask and if so, what coordinate space it's in. If you can access it there that would be the best fix I think.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/layout/generic/nsCanvasFrame.cpp#322

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Oh, what you could do is pass the |Maybe<WrImageMask> mask| as a parameter to CreateWebRenderCommands, and then any code inside there that creates a clip region can incorporate the mask into it.

Comment 13

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Oh, what you could do is pass the |Maybe<WrImageMask> mask| as a parameter
> to CreateWebRenderCommands, and then any code inside there that creates a
> clip region can incorporate the mask into it.

I'm afraid it will be more complicate. I tried this solution before. But we may add multiple wr display items in CreateWebRenderCommands. Each wr item's position may have an offset to the layer, so we'll need to calculate a new mask rect for each wr item. And we also need to do above things for each gecko display items. It will be hard to test or debug if we set the mask parameters slightly incorrectly. So in the end I add a global mask in DisplayItemLayer.

Comment 14

[Ethan Lin[:ethlin]]

Attachment: apply mask in WebRenderDisplayItemLayer

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8857571 [details] [diff] [review]
apply mask in WebRenderDisplayItemLayer

Review of attachment 8857571 [details] [diff] [review]:
-----------------------------------------------------------------

I think eventually we'll want to get rid of the WebRender*Layer classes and at that point we'll probably need to push the mask into CreateWebRenderCommands anyway. I guess this is ok for now though.

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +13,5 @@
>  #include "mozilla/gfx/Matrix.h"
>  
>  namespace mozilla {
> +
> +using namespace gfx;

Please no "using namespace". It results in unified build errors much more frequently.

::: gfx/layers/wr/WebRenderLayerManager.cpp
@@ +102,5 @@
>    return RelativeToParent(transformed);
>  }
>  
>  Maybe<WrImageMask>
> +WebRenderLayer::BuildWrMaskLayer(const gfx::Matrix4x4& aLayerTransform)

I think I would prefer if you passed in a |bool aUnapplyLayerTransform| here instead of the matrix. Inside the function you can then either do what the function was doing before (if aUnapplyLayerTransform is true) or just use an empty matrix (if aUnapplyLayerTransform is false). Not a big deal either way but I think reducing it's better to keep the number of places we're calling GetWrBoundTransform() small. More importantly it reduces the chances of an error where somebody passes in a garbage matrix.

Comment 16

[Ethan Lin[:ethlin]]

I addressed the comment 15 and try result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f8f3c927fe41bf8867b0904a0bb144e0a528d26

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/3fc9e5cc78fe
Add mask layer support for WebRenderDisplayItemLayer. r=kats

Comment 18

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/3fc9e5cc78fe