Closed
Bug 1332688
Opened 7 years ago
Closed 7 years ago
Apply layer transform to WRLayer's clip rect
Categories
(Core :: Graphics: WebRender, defect)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: ethlin, Assigned: ethlin)
References
Details
Attachments
(1 file, 4 obsolete files)
28.01 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
Currently we only apply the transform to clip rect in WRCanvasLayer and WRTextLayer. I think we should consider the transform in other WRXXXLayer.
Comment 1•7 years ago
|
||
See also bug 1331713
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
Remove some fails-if after the clip rect fix.
Attachment #8828913 -
Flags: review?(bugmail)
Comment 4•7 years ago
|
||
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•7 years ago
|
Attachment #8828912 -
Flags: review?(bugmail)
Assignee | ||
Updated•7 years ago
|
Attachment #8828913 -
Flags: review?(bugmail)
Assignee | ||
Comment 5•7 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•7 years ago
|
||
I set layer's transform to stack context directly, so we don't need to do some calculations for transform.
Assignee | ||
Updated•7 years ago
|
Attachment #8828912 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8830621 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
Attachment #8828913 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8830621 -
Flags: review?(jmuizelaar) → review+
Comment 8•7 years ago
|
||
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•7 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•7 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•7 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.
Comment 12•7 years ago
|
||
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•7 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•7 years ago
|
||
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)
Assignee | ||
Comment 15•7 years ago
|
||
Here is the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=952ee0d5daf5e56832a22cf6247d2f87a432cedb
Comment 16•7 years ago
|
||
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•7 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.
Comment 18•7 years ago
|
||
(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 19•7 years ago
|
||
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•7 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
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Target Milestone: --- → mozilla54
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/473edb39ee1c
You need to log in
before you can comment on or make changes to this bug.
Description
•