If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Push clip region in WebRenderDisplayItemLayer

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: WebRender
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 months ago
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.
(Assignee)

Updated

5 months ago
Blocks: 1359242
(Assignee)

Comment 1

5 months ago
For background image layer, 'layout/reftests/backgrounds/background-clip-1.html' is an example to test the clipping result.
(Assignee)

Comment 2

5 months ago
Created attachment 8865370 [details] [diff] [review]
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.
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.
(Assignee)

Comment 5

5 months ago
Created attachment 8865754 [details] [diff] [review]
Push clip region in WebRenderDisplayItemLayer
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+
(Assignee)

Comment 7

5 months ago
(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)
(Assignee)

Comment 9

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

Comment 10

5 months ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/a3916eccfa83
Push clip region in WebRenderDisplayItemLayer. r=kats
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED

Comment 11

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3916eccfa83
status-firefox55: --- → fixed
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.