Closed Bug 1362967 Opened 3 years ago Closed 3 years ago

Push clip region in WebRenderDisplayItemLayer

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 1359242
For background image layer, 'layout/reftests/backgrounds/background-clip-1.html' is an example to test the clipping result.
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.
Attachment #8865370 - Flags: feedback?(bugmail)
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!
Attachment #8865370 - Flags: feedback?(bugmail) → feedback+
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.
Attachment #8865370 - Attachment is obsolete: true
Attachment #8865754 - Flags: review?(bugmail)
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?
Attachment #8865754 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(bugmail)
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/a3916eccfa83
Push clip region in WebRenderDisplayItemLayer. r=kats
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.