Apply layer transform to WRLayer's clip rect

RESOLVED FIXED in mozilla54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
Currently we only apply the transform to clip rect in WRCanvasLayer and WRTextLayer. I think we should consider the transform in other WRXXXLayer.
See also bug 1331713
(Assignee)

Comment 2

2 years ago
Created attachment 8828912 [details] [diff] [review]
Part1. Apply layer transform to WebRenderLayer's clip rect

Apply the layer transform to clip for some WRLayers. It's a little bit ugly right now. I think I can have another bug to extract the clip/bound/IPC codes from WRXXXLayer::RenderLayer(), since every layer does similar things.
Attachment #8828912 - Flags: review?(bugmail)
(Assignee)

Comment 3

2 years ago
Created attachment 8828913 [details] [diff] [review]
Part2. remove some fails-if

Remove some fails-if after the clip rect fix.
Attachment #8828913 - Flags: review?(bugmail)
Comment on attachment 8828912 [details] [diff] [review]
Part1. Apply layer transform to WebRenderLayer's clip rect

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

I don't think this is necessarily the way that we want to be doing things. We should be including the transform in the stacking context.
(Assignee)

Updated

2 years ago
Attachment #8828912 - Flags: review?(bugmail)
(Assignee)

Updated

2 years ago
Attachment #8828913 - Flags: review?(bugmail)
(Assignee)

Comment 5

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8828912 [details] [diff] [review]
> Part1. Apply layer transform to WebRenderLayer's clip rect
> 
> Review of attachment 8828912 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think this is necessarily the way that we want to be doing things.
> We should be including the transform in the stacking context.

I see. I'll try to pass the transform to the stacking context but not calculating the boundary manually.
(Assignee)

Comment 6

2 years ago
Created attachment 8830621 [details] [diff] [review]
Apply the transform to stacking context

I set layer's transform to stack context directly, so we don't need to do some calculations for transform.
(Assignee)

Updated

2 years ago
Attachment #8828912 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8830621 - Flags: review?(jmuizelaar)
(Assignee)

Comment 7

2 years ago
Created attachment 8830624 [details] [diff] [review]
change flags of reftests

Some reftests pass now because we forgot to apply the transform to clip rect. Some 3D transform reftests fail now because they passed incidentally.
Attachment #8830624 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8828913 - Attachment is obsolete: true
Attachment #8830621 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8830624 [details] [diff] [review]
change flags of reftests

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

Please land these as a single commit
Attachment #8830624 - Flags: review?(jmuizelaar) → review+

Comment 9

2 years ago
So I was accidentally simultaneously working on this. I have a few notes from what I found.

> -  gfx::Matrix4x4 transform;// = GetTransform();
> -  gfx::Rect relBounds = TransformedVisibleBoundsRelativeToParent();
> +  gfx::Matrix4x4 transform = GetTransform();
> +  gfx::Rect relBounds = VisibleBoundsRelativeToParent();
>    gfx::Rect overflow(0, 0, relBounds.width, relBounds.height);

When WR creates the final transform for a StackingContext it first translates by the bounds and then applies the transform (the size of bounds aren't used anywhere) [1]. Whereas we were transforming the bounds before giving it to WR (TransformedVisibleBoundsRelativeToParent). This means that if there is a scaling component to the transform, the bounds offset won't have it, and when WR builds the transform it will be offset wrong.

When I had this set up for CanvasLayer there were a few reftests that failed because of this difference. [2]

This can be fixed by changing the order that WebRender builds the transform, but I'm not sure if that's relied on elsewhere. Need to talk to Glenn about that.

> clip = RelativeToTransformedVisible(IntRectToRect(GetClipRect().ref().ToUnknownRect()));

Another issue is that ClipRect() is given in the layer space of its parent [3] (which in this case means our parent stacking context). That is, it shouldn't be transformed by GetTransform(). One solution is to apply the inverse of GetTransform() to clipRect, so that it is now in the correct layer space to be in the new stacking context.

I have a try run here [4] with what I had working. There are still some tests that use canvas layer that fail because of something with clipping.

[1] https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L369
[2] layout/reftests/w3c-css/submitted/images3/object-fit-contain-png-001c.html (and friends)
[3] http://searchfox.org/mozilla-central/source/gfx/layers/Layers.h#1004
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd868ed49ff8b72ce1a646ba54136e473df7ca0
(Assignee)

Comment 10

2 years ago
(In reply to Ryan Hunt [:rhunt] from comment #9)
> So I was accidentally simultaneously working on this. I have a few notes
> from what I found.
> 
> > -  gfx::Matrix4x4 transform;// = GetTransform();
> > -  gfx::Rect relBounds = TransformedVisibleBoundsRelativeToParent();
> > +  gfx::Matrix4x4 transform = GetTransform();
> > +  gfx::Rect relBounds = VisibleBoundsRelativeToParent();
> >    gfx::Rect overflow(0, 0, relBounds.width, relBounds.height);
> 
> When WR creates the final transform for a StackingContext it first
> translates by the bounds and then applies the transform (the size of bounds
> aren't used anywhere) [1]. Whereas we were transforming the bounds before
> giving it to WR (TransformedVisibleBoundsRelativeToParent). This means that
> if there is a scaling component to the transform, the bounds offset won't
> have it, and when WR builds the transform it will be offset wrong.
> 
> When I had this set up for CanvasLayer there were a few reftests that failed
> because of this difference. [2]
> 
> This can be fixed by changing the order that WebRender builds the transform,
> but I'm not sure if that's relied on elsewhere. Need to talk to Glenn about
> that.
> 
> > clip = RelativeToTransformedVisible(IntRectToRect(GetClipRect().ref().ToUnknownRect()));
> 
> Another issue is that ClipRect() is given in the layer space of its parent
> [3] (which in this case means our parent stacking context). That is, it
> shouldn't be transformed by GetTransform(). One solution is to apply the
> inverse of GetTransform() to clipRect, so that it is now in the correct
> layer space to be in the new stacking context.
> 
> I have a try run here [4] with what I had working. There are still some
> tests that use canvas layer that fail because of something with clipping.
> 
> [1]
> https://github.com/servo/webrender/blob/master/webrender/src/frame.rs#L369
> [2]
> layout/reftests/w3c-css/submitted/images3/object-fit-contain-png-001c.html
> (and friends)
> [3] http://searchfox.org/mozilla-central/source/gfx/layers/Layers.h#1004
> [4]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4dd868ed49ff8b72ce1a646ba54136e473df7ca0

Thanks! I got very similar try result. I'm trying to fix the canvas problem.
(Assignee)

Comment 11

2 years ago
There is another test 'layout/reftests/bugs/641770-1.html' which is not about canvas, but there is also a clipping problem after applying the transform to stacking context.
It's worth noting that a bunch of the clipping stuff has changed in WebRender. Here's the update that I hope to land tomorrow: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47fb12c58a5a887077fe04a5b2c48df9e628e88a
(Assignee)

Comment 13

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> It's worth noting that a bunch of the clipping stuff has changed in
> WebRender. Here's the update that I hope to land tomorrow:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=47fb12c58a5a887077fe04a5b2c48df9e628e88a

The WR update fixed the clipping problems! But I got a new crash problem in 'layout/reftests/transform-3d/mask-layer-2.html' [1]. It looks like a WR bug. I would like to skip the test and fix it later.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd00dc457cee8ff7d1375364ba707e6f826f18f7&selectedJob=77439346
(Assignee)

Comment 14

2 years ago
Created attachment 8837491 [details] [diff] [review]
Apply the transform to stacking context

I combined the two patches into one patch. Per comment 9, the clip rect is transformed and the relBound's top-left isn't scaled/rotated by WR. So in this patch, I invert the transform for clip rect and do scale/ratate for relBound's top-left. [1] is a good test to check the correctness of the transformation. Please help to review again.

[1] layout/reftests/w3c-css/submitted/images3/object-fit-fill-png-001c.html
Attachment #8830621 - Attachment is obsolete: true
Attachment #8830624 - Attachment is obsolete: true
Attachment #8837491 - Flags: review?(jmuizelaar)
Comment on attachment 8837491 [details] [diff] [review]
Apply the transform to stacking context

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

::: gfx/layers/wr/WebRenderBorderLayer.cpp
@@ +35,5 @@
> +  if (!transform.IsIdentity()) {
> +    gfx::Matrix4x4 boundTransform;
> +    boundTransform._11 = transform._11; boundTransform._12 = transform._12; boundTransform._13 = transform._13;
> +    boundTransform._21 = transform._21; boundTransform._22 = transform._22; boundTransform._23 = transform._23;
> +    boundTransform._31 = transform._31; boundTransform._32 = transform._32; boundTransform._33 = transform._33;

Why can't this just be boundTransform = transform?
(Assignee)

Comment 17

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> Comment on attachment 8837491 [details] [diff] [review]
> Apply the transform to stacking context
> 
> Review of attachment 8837491 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderBorderLayer.cpp
> @@ +35,5 @@
> > +  if (!transform.IsIdentity()) {
> > +    gfx::Matrix4x4 boundTransform;
> > +    boundTransform._11 = transform._11; boundTransform._12 = transform._12; boundTransform._13 = transform._13;
> > +    boundTransform._21 = transform._21; boundTransform._22 = transform._22; boundTransform._23 = transform._23;
> > +    boundTransform._31 = transform._31; boundTransform._32 = transform._32; boundTransform._33 = transform._33;
> 
> Why can't this just be boundTransform = transform?

It's about the top-left position when applying transform. When applying the transform, WR takes the top-left as origin of coordinate. Which means the first 3x3 array isn't really applied to the relBounds's top-left, but the 'translate' is applied. For gecko's layer system, we want whole transform matrix be applied to the relBounds. So I have to do this workaround.
(In reply to Ethan Lin[:ethlin] from comment #13)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> > It's worth noting that a bunch of the clipping stuff has changed in
> > WebRender. Here's the update that I hope to land tomorrow:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=47fb12c58a5a887077fe04a5b2c48df9e628e88a
> 
> The WR update fixed the clipping problems! But I got a new crash problem in
> 'layout/reftests/transform-3d/mask-layer-2.html' [1]. It looks like a WR
> bug. I would like to skip the test and fix it later.

I filed https://github.com/servo/webrender/issues/894 for this.
Comment on attachment 8837491 [details] [diff] [review]
Apply the transform to stacking context

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

Lets set the translation to 0 instead of copying over the non-zero members. Also please add a comment about why. Other than that this should be good enough.
Attachment #8837491 - Flags: review?(jmuizelaar) → review+

Comment 20

2 years ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/473edb39ee1c
Pass transform to webrender stacking context. r=jrmuizel
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.