1400034 - Consider different approach for dealing with scaling when sending transforms to webrender

Status: RESOLVED FIXED

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

Description

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

Filing a new bug just to explore this in more detail without cluttering up existing bugs. The summary: bug 1391499 added some code that made the StackingContextHelper stash scale factors and pass them down to children and so on. According to comment 0 on that bug it was done to fix the layout/reftests/transform/compound-1-ref.html test in layers-free mode.

However, that fix has now resulted in other scaling changes such as bug 1394308, bug 1399050, and most recently, bug 1399043. This seems like we're going into a rabbithole where we will end up doing a lot more work than we need to. I'd like to rewind back to before these bugs landed and see if we can figure out a different approach.

Comment 1

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

So far what I've done is unapplied the patches from bug 1391499, bug 1394308 and bug 1399050, and reproduced an issue with the layers-free rendering of the layout/reftests/transform/compound-1-ref.html file. The issue is that the box is rendered at around x=500 instead of x=300. By playing with the transforms it seems clear that the transform-origin of the scale(2) transform is not being respected; instead of using the top-left of the translated div, it's using some other point as the transform origin and rotating the div around that point.

I added some logging to see if we were missing the transform origin in nsDisplayTransform. We might be, but the computed transform origin there is (0,0,0) so that's not the problem anyway. I'm still fairly sure this is a problem with the transform origin though, so I'll keep digging along this line tomorrow.

Current patch set is at https://github.com/staktrace/gecko-dev/commits/scale if anybody else wants to try.

Comment 2

[Peter Chang[:pchang]]

Kats, you can find a good example about nested transform from [1] by searching for 'inherit properties from parents'. You can see the red and blue boxes. Mapped to our test case 'compound-1-ref.html', the scale of blue boxes are correct, but the translation of blue boxes are different. After I discussed with Morris and Ethan, we probably could deal with the inherited scale inside WebRender. But we still need to consider the inherited scale for the fallback path(ni Ethan to add more comment for this).

[1]https://staff.washington.edu/fmf/2011/07/15/css3-transform-attribute-order/

Comment 3

[Peter Chang[:pchang]]

(In reply to Peter Chang[:pchang] from comment #2)
> Kats, you can find a good example about nested transform from [1] by
> searching for 'inherit properties from parents'. You can see the red and
> blue boxes. Mapped to our test case 'compound-1-ref.html', the scale of blue
> boxes are correct, but the translation of blue boxes are different. After I
> discussed with Morris and Ethan, we probably could deal with the inherited
> scale inside WebRender. 

I made above example in https://jsfiddle.net/2oo8aa8q/1/

>But we still need to consider the inherited scale
> for the fallback path(ni Ethan to add more comment for this).
> 
> [1]https://staff.washington.edu/fmf/2011/07/15/css3-transform-attribute-
> order/

Comment 4

[Ethan Lin[:ethlin]]

The FLB handles the inherited scale at [1]. One point is that the translation of child's transform shouldn't be affected by the scale of parent's transform. That's why we need to extract the scale value from the transform. If we set the scale to stacking context directly, then the whole children's transform will be multiplied by parent's scale.
The thing we need to handle for fallback path is just about resolution and I have some patches in bug 1395501.

For original gecko, we apply the inherited scale to the draw target.
For webrender, in my thinking there are several ways:
1. As my patch in bug 1399043, we apply the inherited scale to all parameters of the webrender commands. Basically the result will be correct.
2. Pass the scale as a parameter to each webrender display item and scale the item in webrender.
3. Add a stacking context with inherited scale for each leaf display item. But we may create too many stacking contexts. So we need to apply a inherited scale stacking context to multiple leaf display items and skip the container type display items. 
4. Extract the inherited scale in webrender and do not change the transform on the gecko side. Note that we still need the inherited scale value for fallback.

[1] https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/layout/painting/FrameLayerBuilder.cpp#5411

Comment 5

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

(In reply to Peter Chang[:pchang] from comment #2)
> Kats, you can find a good example about nested transform from [1] by
> searching for 'inherit properties from parents'.

Thanks. I read through the document and it seems to make sense to me. I also loaded both that document and the jsfiddle in my build (which has the inherited-scale patches backed out) and most of the nested box examples rendered correctly. All the scaling/translation ones were fine, it only started going wrong once I got to the rotation/skew section. So I'm not convinced yet that this is a scaling problem, I still think it has to do with the transform origin being used for rotation/skew transforms. I'm going to continue investigating until I fully understand this.

Comment 6

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

So after some more fiddling I got a patch which deletes a bunch of code and everything I tested works great!
- All the examples on https://staff.washington.edu/fmf/2011/07/15/css3-transform-attribute-order/ render properly
- The fiddle at https://jsfiddle.net/2oo8aa8q/1/ works properly
- All the tests in layout/reftests/transform work (0 failure differences between layersfull and layersfree)

I started a try push to get a sense of whether this breaks anything really badly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=319442ec50fb2209864887155dddbf090efee206

Patches can be taken from the try push or in my branch at https://github.com/staktrace/gecko-dev/commits/scale, they are rebased to current m-c.

Comment 7

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

I also did a try push with current m-c and layers-free enabled, here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44eb40bcac3fa6e0066873fcf938afc1ad302e56

We can use this as a "baseline" against which to compare my try push in comment 6. I collected all the UNEXPECTED-FAIL and UNEXPECTED-PASS results from the two try pushes (opt reftests) and my patches make things strictly better.

Baseline UNEXPECTED-FAIL: 290 tests
UNEXPECTED-FAIL with my patches: 275 tests
Baseline UNEXPECTED-PASS: 47 tests
UNEXPECTED-PASS with my patches: 53 tests

This is an improvement of 15 + 6 = 23 tests.

I also compared the results from the individual hunks and there were no "regressions" (i.e. no tests that are passing in the baseline that are failing with my patches).

Comment 8

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

Note also that one of the tests that is failing in the baseline push but passing with my patches is "layout/reftests/bugs/586683-1.html" which is the reftest cited in bug 1399043 comment 0. This makes me feel even more that this is a better approach to be taking.

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Baseline UNEXPECTED-FAIL: 290 tests
> UNEXPECTED-FAIL with my patches: 275 tests
> Baseline UNEXPECTED-PASS: 47 tests
> UNEXPECTED-PASS with my patches: 53 tests
> 
> This is an improvement of 15 + 6 = 23 tests.

Sorry, I missed that R7 had > 100 failures and the TreeHerder display cuts off at 100. Taking the full raw log gives me these numbers:

Baseline UNEXPECTED-FAIL: 322 tests
UNEXPECTED-FAIL with my patches: 299 tests
Baseline UNEXPECTED-PASS: 48 tests
UNEXPECTED-PASS with my patches: 54 tests

Net improvement: 23 + 6 = 29 tests.

Comment 10

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

Attachment: Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.

This backs out bug 1399050, bug 1394308 (2 patches), and bug 1391499. I
believe these patches sent us down a path that would make the code
increasingly more complex, when in fact we can do a more "direct"
translation from the gecko display list to the WR display list and make
things a lot simpler and more correct.

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

Comment 11

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

Attachment: Bug 1400034 - Do a more direct translation of transforms from Gecko to WR in layers-free mode.

In layers-free mode the gecko display list and coordinate system is very
similar to what WR is expecting. Instead of having each
StackingContextHelper shift the origin of the coordinate system, we can
leave it in one spot and just pass everything relative to that. The
semantics of the Gecko display list already matches this; the exception
is that nsDisplayTransform items are also considered reference frames,
and anything inside them is relative to the nsDisplayTransform. On the
WR side this is also the case, because stacking contexts with a
transform are implicitly turned into reference frames.

Additionally, the size of the bounds passed to the WR stacking context
is never actually used, except on the root stacking context (which is
not created by StackingContextHelper). Since we want a zero origin (as
explained above) and the size is never used, we can just pass a zero
rect to the WR stacking context from StackingContextHelper.

In terms of the actual transform matrix, this patch now passes the full
unmodified transform from nsDisplayTransform into WR. This transform
gets applied onto the contents of the nsDisplayTransform. The contents'
coordinate system is relative to the frame that generated the
nsDisplayTransform. Again this maps directly to WR, where the transform
on the stacking context gets applied to the contents of the stacking
context; the contents' coordinates are relative to the stacking context.

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

Comment 12

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

Attachment: Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

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

Comment 13

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

Added a third patch to turn on layers-free for the transform/ and transform-3d/ reftests so that we can catch regressions for those in layers-free mode and stop worrying about layers-full mode.

Try push with these three patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=583a3dec5b1d4099aec9d3916229ff3bb1b4e0ba

Comment 14

[Ethan Lin[:ethlin]]

Great! So the problem is we did the workaround for transform in layers mode and we keep using it in layers-free mode. That's why we needed to follow the FLB implementations. I will double check the result later.

Comment 15

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

Comment on attachment 8908784 [details]
Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180398/diff/1-2/

Comment 16

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

Comment on attachment 8908785 [details]
Bug 1400034 - Do a more direct translation of transforms from Gecko to WR in layers-free mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180400/diff/1-2/

Comment 17

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

Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180410/diff/1-2/

Comment 18

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

(In reply to Ethan Lin[:ethlin] from comment #14)
> Great! So the problem is we did the workaround for transform in layers mode
> and we keep using it in layers-free mode. That's why we needed to follow the
> FLB implementations.

Yup, exactly.

Updated the second patch to mark three async-scrolling reftests as passing as well, which I missed earlier. That folder is already run in layers-free mode.

Comment 19

[Peter Chang[:pchang]]

Comment on attachment 8908784 [details]
Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.

https://reviewboard.mozilla.org/r/180398/#review185806

Comment 20

[Peter Chang[:pchang]]

Comment on attachment 8908785 [details]
Bug 1400034 - Do a more direct translation of transforms from Gecko to WR in layers-free mode.

https://reviewboard.mozilla.org/r/180400/#review185810

Comment 21

[Peter Chang[:pchang]]

Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

https://reviewboard.mozilla.org/r/180410/#review185812

Comment 22

[Ethan Lin[:ethlin]]

Comment on attachment 8908784 [details]
Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.

https://reviewboard.mozilla.org/r/180398/#review185826

::: gfx/layers/wr/StackingContextHelper.h
(Diff revision 2)
>  private:
>    wr::DisplayListBuilder* mBuilder;
>    LayerPoint mOrigin;
>    gfx::Matrix4x4 mTransform;
> -  bool mHasPerspectiveTransform;
> -  float mXScale;

We may still need the inherited scale for fallback. I think it's fine to add it back in the fallback resolution bug.

Comment 23

[Ethan Lin[:ethlin]]

Comment on attachment 8908785 [details]
Bug 1400034 - Do a more direct translation of transforms from Gecko to WR in layers-free mode.

https://reviewboard.mozilla.org/r/180400/#review185832

Comment 24

[Ethan Lin[:ethlin]]

Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

https://reviewboard.mozilla.org/r/180410/#review185834

Comment 25

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

https://hg.mozilla.org/integration/autoland/file/cd9baf0faf83
https://hg.mozilla.org/integration/autoland/file/5524d88599f6
https://hg.mozilla.org/integration/autoland/file/1e2233c98ff1

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out the third patch for leaks and reftest failures.
https://hg.mozilla.org/integration/autoland/rev/776c180fb82ac7e04ffc9b72a9691930a3eae019

Comment 27

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

Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

Third patch was backed out (see comment above). I'm going to move it to bug 1400888 instead since I'm not sure how long it will take to clean up the failures.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd9baf0faf83
https://hg.mozilla.org/mozilla-central/rev/5524d88599f6