1361356 - Fix untransform applied in BuildWrMaskLayer

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

The BuildWrMaskLayer function is to convert a mask layer into a thing that we can hand to WR as part of a clip region. We do this from a bunch of layers (WebRenderColorLayer, WebRenderDisplayItemLayer, etc.). In some of these (e.g. WebRenderColorLayer) we push a stacking context for the layer before we pass the clip region to WR. So the clip region needs to be relative to the stacking context in those cases. In other places (e.g. WebRenderDisplayItemLayer) we do not push a stacking context for the layer, so the clip region needs to be relative to the parent stacking context.

In order to deal with this discrepancy the BuildWrMaskLayer function takes a flag and tries to convert into the proper coordinate space [1]. I believe this code is slightly incorrect and needs updating. In particular, the code builds a transform which is the inverse of a presumed stacking context offset, followed by the mask layer's transform, followed by the layer's transform. However, logically, the order of operations should first be "convert from the child stacking context to the parent stacking context" and then "apply the mask layer's transform". So putting the mask layer's transform in the middle of the other two operations makes no sense at all.

[1] http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/gfx/layers/wr/WebRenderLayer.cpp#90-94

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1361356 - Properly unapply the stacking context transform and origin translation when building mask layers.

Review commit: https://reviewboard.mozilla.org/r/135548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135548/

Comment 2

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=30d62fac822dbf6ae6819ac52642cd9309ddaf83 includes this patch

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Ethan - if you find this is causing some extra tests to fail with advanced layer prefs turned on, let me know and I'm happy to investigate. Same goes for bug 1361357.

Comment 4

[Ethan Lin[:ethlin]]

Could you check the reftest 'layout/reftests/backgrounds/attachment-local-clipping-color-4.html' on MacOS? The patch seems to make it failed on my Mac. I haven't done further investigate.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I just tried that reftest on OS X, both with and without layers.advanced.canvas-background-color=true, and it passed in all cases. I ran it using:
  ./mach reftest layout/reftests/backgrounds/attachment-local-clipping-color-4.html --setpref reftest.ignoreWindowSize=true
and:
  ./mach reftest layout/reftests/backgrounds/attachment-local-clipping-color-4.html --setpref reftest.ignoreWindowSize=true --setpref layers.advanced.canvas-background-color=true

I also tried without the change, and it passed for me. Note that this patch applies on top of the stuff from bug 1360246, so that's what I tried. Do you have any other patches applied locally that might be impacting this?

Comment 6

[Ethan Lin[:ethlin]]

Strange...I used git-cinnbar to fetch the revision directly and I didn't have any pref on or local change. I still can reproduce the problem with this command "./mach reftest layout/reftests/backgrounds/attachment-local-clipping-color-4.html --setpref reftest.ignoreWindowSize=true". If without the change, the test passed for me. I will do more investigate and change another OS X device to double check.

Comment 7

[Ethan Lin[:ethlin]]

Retina display has this problem. If I use non-retina external screen, the test will pass. I think that's because the order of transform multiplication.

Comment 8

[Ethan Lin[:ethlin]]

Comment on attachment 8863797 [details]
Bug 1361356 - Properly unapply the stacking context transform and origin translation when building mask layers.

https://reviewboard.mozilla.org/r/135548/#review139120

::: gfx/layers/wr/WebRenderLayer.cpp:54
(Diff revision 1)
>      // The size of mask layer is transformed, and we may set the layer transform
>      // to wr stacking context. So we should apply inverse transform for mask layer
>      // and reverse the offset of the stacking context.
>      gfx::Matrix4x4 transform = maskLayer->GetLayer()->GetTransform();
> -    if (aUnapplyLayerTransform) {
> -      gfx::Rect bounds = IntRectToRect(GetLayer()->GetVisibleRegion().GetBounds().ToUnknownRect());
> +    if (aUnapplySc) {
> +      transform = aUnapplySc->TransformToParentSC() * transform;

We should use "transform = transform * aUnapplySc->TransformToParentSC();" or the mask layer's transform will be effected by the UnapplySc's transform. With this fix, the test passed on my retina device.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Thanks for investigating! I'll think about this some more - if changing the order there fixes the problem then my mental model is incorrect somehow and I want to figure out why.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Ok, I gave it some more thought and it made sense. The transform is eventually used to transform the bounds of the mask layer. What we want to do there conceptually is take the transformed bounds of the mask layer, and move that to be relative to the correct stacking context. So conceptually the operation should be bounds * maskLayer.GetTransform() * transformToCorrectStackingContext(). So the mask layer's transform needs to go first, which is exactly the change you proposed. I'll land it with that change.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/ec5a1251f38d
Properly unapply the stacking context transform and origin translation when building mask layers. r=ethlin

Comment 12

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/ec5a1251f38d