1362967 - Push clip region in WebRenderDisplayItemLayer

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ethan Lin[:ethlin]]

Currently we don't handle the clip region in WebRenderDisplayItemLayer. Some display items use their own clip rect to clip, and some items haven't handled it. I think we should use GetClipRect in WebRenderDisplayItemLayer, same as WebRenderXXXLayer.

Comment 1

[Ethan Lin[:ethlin]]

For background image layer, 'layout/reftests/backgrounds/background-clip-1.html' is an example to test the clipping result.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Push clip region in WebRenderDisplayItemLayer

The patch can fix the clipping problems for background image layer. If this way is correct, I will remove other individual clipping for some display items.

Comment 3

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

Comment on attachment 8865370 [details] [diff] [review]
Push clip region in WebRenderDisplayItemLayer

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

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +41,5 @@
>    WrImageMask* imageMask = mask.ptrOr(nullptr);
> +
> +  ParentLayerRect clip = GetLocalTransformTyped().TransformBounds(Bounds());
> +  if (GetClipRect()) {
> +    clip = ParentLayerRect(GetClipRect().ref());

I'd prefer using valueOr instead of the if condition. e.g.:
  ParentLayerRect rect = GetLocalTransformTyped().TransformBounds(Bounds());
  ParentLayerRect clip = GetClipRect().valueOr(rect);

But this change looks correct, thanks!

Comment 4

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

You can probably also use refOr instead of valueOr. I'm not sure if it'll save a copy or not... I assume the compiler will optimize them to the same thing anyway.

Comment 5

[Ethan Lin[:ethlin]]

Attachment: Push clip region in WebRenderDisplayItemLayer

Comment 6

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

Comment on attachment 8865754 [details] [diff] [review]
Push clip region in WebRenderDisplayItemLayer

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

r=me if you drop the RoundedToInt

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +40,5 @@
>    Maybe<WrImageMask> mask = BuildWrMaskLayer(nullptr);
>    WrImageMask* imageMask = mask.ptrOr(nullptr);
> +
> +  ParentLayerRect clip(GetClipRect().refOr(
> +                    RoundedToInt(GetLocalTransformTyped().TransformBounds(Bounds()))));

Why RoundedToInt?

Comment 7

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Comment on attachment 8865754 [details] [diff] [review]
> Push clip region in WebRenderDisplayItemLayer
> 
> Review of attachment 8865754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you drop the RoundedToInt
> 
> ::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
> @@ +40,5 @@
> >    Maybe<WrImageMask> mask = BuildWrMaskLayer(nullptr);
> >    WrImageMask* imageMask = mask.ptrOr(nullptr);
> > +
> > +  ParentLayerRect clip(GetClipRect().refOr(
> > +                    RoundedToInt(GetLocalTransformTyped().TransformBounds(Bounds()))));
> 
> Why RoundedToInt?

Right, the conversion is a little bit weird since in the end we want a ParentLayerRect type for clip. The GetClipRect() returns ParentLayerIntRect, so I have to put a IntRect for 'refOr'. Is there any better way to write this? Maybe I should still use "if (GetClipRect())" to get the clip?

Comment 8

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

You could use map or apply on the Maybe to convert it to a ParentLayerRect. But that might end up being pretty messy too. Might be better to stick with the if condition. But rounding definitely seems wrong.

Comment 9

[Ethan Lin[:ethlin]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39359eb7cbd22ff91fb1e83d1440f2e51525ae19&selectedJob=97561783

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/a3916eccfa83
Push clip region in WebRenderDisplayItemLayer. r=kats

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a3916eccfa83