Closed Bug 1360246 Opened 3 years ago Closed 3 years ago

Add strongly typed coordinate systems to gfx/layers/wr, part 2

Categories

(Core :: Graphics: WebRender, enhancement, P3)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(9 files)

59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
+++ This bug was initially created as a clone of Bug #1359842 +++

The patches in bug 1359842 converted a bunch of gfx::Rect and gfx::Point to LayerRect/LayerPoint in gfx/layers/wr, along with propagating some of those changes out into webrender code in layout/generic and layout/painting. However there were a few things that I left as-is because I wasn't quite sure about, such as in WebRenderTextLayer. There's more that can be fixed here.
Updated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ded49f03ec757fcf1ad7b3a784addc6b771a0e0c

This patchset now gets rid of all the RelativeToParent stuff. In general, it gets rid of the assumptions that the parent layer always pushes a stacking context. This allows us to fix bug 1345577. It also uncovered a couple of potential bugs, which I'll file/fix separately (see bug 1359242 comment 4 and bug 1359242 comment 5).
The above try push contained an R8 orange, which is actually an unexpected-PASS (of layout/reftests/transform-3d/sorting-1a.html). I investigated and it looks like our existing code is already (even without bug 1345577) broken in the case of preserve-3d container layers. These 3D container layers sort their descendant layers in z-order and render them that way, so they don't get rendered in tree order necessarily. If you have a tree like this:

ContainerLayer [preserve3D]
  ContainerLayer A
    PaintedLayer B

right now B assumes that when it is being rendered, it is inside a stacking context pushed by A. However that is not the case, because the preserve3D ancestor can call B to get rendered directly without having called A.

So my patches fix this scenario and make the reftest pass.

New try push with some more minor cleanups and updated commit messages:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8faf841004703bf360b6b4992817e45222ea411c
Comment on attachment 8863445 [details]
Bug 1360246 - Propagate the StackingContextHelper through CreateWebRenderCommands.

https://reviewboard.mozilla.org/r/135224/#review138322
Attachment #8863445 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863444 [details]
Bug 1360246 - Propagate a StackingContextHelper all the way through the RenderLayer traversal.

https://reviewboard.mozilla.org/r/135222/#review138324
Attachment #8863444 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863446 [details]
Bug 1360246 - Clean up WebRenderTextLayer::RenderLayer.

https://reviewboard.mozilla.org/r/135226/#review138326

I don't know enough about our coordinate system gymnastics to confirm that comment about glyph tranforms but it looks like it makes sense. If you want a real confirmation you can ask for another pair of eyes on this, otherwise, LGTM.
Attachment #8863446 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863447 [details]
Bug 1360246 - Clean up WebRenderRefLayer::RenderLayer.

https://reviewboard.mozilla.org/r/135228/#review138328
Attachment #8863447 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863448 [details]
Bug 1360246 - Propagate the StackingContextHelper to the rest of the displaylist-building code.

https://reviewboard.mozilla.org/r/135230/#review138330
Attachment #8863448 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863449 [details]
Bug 1360246 - Stop calling RelativeToParent on rects for which we only use the size, since that doesn't change with RelativeToParent.

https://reviewboard.mozilla.org/r/135232/#review138338

::: layout/painting/nsCSSRenderingGradients.cpp:1053
(Diff revision 1)
>  
>    nscoord appUnitsPerDevPixel = mPresContext->AppUnitsPerDevPixel();
>  
>    // Translate the parameters into device coordinates
>    LayoutDeviceRect clipBounds = LayoutDevicePixel::FromAppUnits(aFillArea, appUnitsPerDevPixel);
>    LayoutDeviceRect firstTileBounds = LayoutDevicePixel::FromAppUnits(aDest, appUnitsPerDevPixel);

Since we are going to only use the size, could you make this ```LayerSize tileSize```. that way we don't need to worry about the origin of the rect beeing wrong or unused.
Comment on attachment 8863450 [details]
Bug 1360246 - Update code to use StackingContextHelper::ToRelativeWr* instead of RelativeToParent.

https://reviewboard.mozilla.org/r/135234/#review138362

::: gfx/layers/wr/StackingContextHelper.h:57
(Diff revision 1)
>    // that is relative to the stacking context. This is useful because most
>    // things that are pushed inside the stacking context need to be relative
>    // to the stacking context.
> +  // We allow passing in a LayoutDeviceRect for convenience because in a lot of
> +  // cases with WebRender display item generate the layout device space is the
> +  // same as the layer space.

You are better qualified to say whether this shortcut is fine, but I would feel better about making the LayoutDeviceFoo>LayoutFoo conversion in the places where it is explicitly/structurally safe to do so, rather than in a place where we consume them.
I don't know in which places the assumption that they are the same holds or doesn't, so your choice.
Attachment #8863450 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863451 [details]
Bug 1360246 - Remove the RelativeToParent functions by propagating StackingContextHelper chains.

https://reviewboard.mozilla.org/r/135236/#review138380

Nice!
Attachment #8863451 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8863452 [details]
Bug 1360246 - Remove the ParentBounds and TransformedVisibleBoundsRelativeToParent functions.

https://reviewboard.mozilla.org/r/135238/#review138384

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp:44
(Diff revision 1)
>  
>    Maybe<WrImageMask> mask = BuildWrMaskLayer(false);
>    WrImageMask* imageMask = mask.ptrOr(nullptr);
>    if (imageMask) {
> -    gfx::Rect rect = TransformedVisibleBoundsRelativeToParent();
> +    gfx::Rect rect = GetTransform().TransformBounds(Bounds().ToUnknownRect());
> +    // XXX: this is probably not correct. fix it.

Unless you are planning to fix this in the short term, please add a bit more detail about why it's probably not correct (if I understand correctly, because this code assumes that the stacking context tree maps 1-1 with the layer tree which is not necessarily true?)
Attachment #8863452 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #15)
> I don't know enough about our coordinate system gymnastics to confirm that
> comment about glyph tranforms but it looks like it makes sense. If you want
> a real confirmation you can ask for another pair of eyes on this, otherwise,
> LGTM.

I asked around and I didn't find anybody who could say with confidence whether it is right or wrong. So I figured I'd comment my understanding of it, and then later if we discover that it's wrong at least we'll know what the code is assuming and we can fix it more easily.

(In reply to Nicolas Silva [:nical] from comment #18)
> >    LayoutDeviceRect firstTileBounds = LayoutDevicePixel::FromAppUnits(aDest, appUnitsPerDevPixel);
> 
> Since we are going to only use the size, could you make this ```LayerSize
> tileSize```. that way we don't need to worry about the origin of the rect
> beeing wrong or unused.

I made this change, replacing `LayerRect layerFirstTileBounds` with `LayerSize layerFirstTileSize`. I had to add a ViewAs<> which is a little ugly but fine. I'm not sure this change provides that much value though - let me know if I misunderstood what you were asking for, or if you'd like to see something else. Note that we do actually use `firstTileBounds.TopLeft()` in a few places, we just never used `layerFirstTileBounds.TopLeft()`.

(In reply to Nicolas Silva [:nical] from comment #19)
> You are better qualified to say whether this shortcut is fine, but I would
> feel better about making the LayoutDeviceFoo>LayoutFoo conversion in the
> places where it is explicitly/structurally safe to do so, rather than in a
> place where we consume them.
> I don't know in which places the assumption that they are the same holds or
> doesn't, so your choice.

I admit I'm not 100% happy with the way I did this because it does introduce a footgun where people might incorrectly convert from LayoutDevice to Layer space by accidental. However I also don't have a fully clear idea of exactly when the two spaces will be different, if ever. So I think I'd like to keep this shortcut for now and then once we have a better idea of when using it is wrong, we can revisit and try to bulletproof the API a bit.

(In reply to Nicolas Silva [:nical] from comment #21)
> > +    // XXX: this is probably not correct. fix it.
> 
> Unless you are planning to fix this in the short term, please add a bit more
> detail about why it's probably not correct (if I understand correctly,
> because this code assumes that the stacking context tree maps 1-1 with the
> layer tree which is not necessarily true?)

I was planning to fix it, yes. I filed bug 1361357 to track the issue, and updated the comment here to reference that bug.

Patches rebased onto graphics tip (no notable changes, just had to propagate changes to the new WebRenderPaintedLayerBlob class as well) and with the review comments addressed coming up.
Comment on attachment 8863449 [details]
Bug 1360246 - Stop calling RelativeToParent on rects for which we only use the size, since that doesn't change with RelativeToParent.

https://reviewboard.mozilla.org/r/135232/#review138834
Attachment #8863449 - Flags: review?(nical.bugzilla) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/8e529a1e760e
Propagate a StackingContextHelper all the way through the RenderLayer traversal. r=nical
https://hg.mozilla.org/projects/graphics/rev/3b25a123e53f
Propagate the StackingContextHelper through CreateWebRenderCommands. r=nical
https://hg.mozilla.org/projects/graphics/rev/ca74bdc958e1
Clean up WebRenderTextLayer::RenderLayer. r=nical
https://hg.mozilla.org/projects/graphics/rev/4776be9cd57a
Clean up WebRenderRefLayer::RenderLayer. r=nical
https://hg.mozilla.org/projects/graphics/rev/e291b3dc7825
Propagate the StackingContextHelper to the rest of the displaylist-building code. r=nical
https://hg.mozilla.org/projects/graphics/rev/a4b49a2aa1bd
Stop calling RelativeToParent on rects for which we only use the size, since that doesn't change with RelativeToParent. r=nical
https://hg.mozilla.org/projects/graphics/rev/2e29ab7dc180
Update code to use StackingContextHelper::ToRelativeWr* instead of RelativeToParent. r=nical
https://hg.mozilla.org/projects/graphics/rev/0f0a5f78c75c
Remove the RelativeToParent functions by propagating StackingContextHelper chains. r=nical
https://hg.mozilla.org/projects/graphics/rev/7cb3c4963a3f
Remove the ParentBounds and TransformedVisibleBoundsRelativeToParent functions. r=nical
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.