Closed Bug 1400034 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- unaffected

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(3 files)

Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.
59 bytes, text/x-review-board-request
ethlin
: review+
pchang
: review+
Details
Bug 1400034 - Do a more direct translation of transforms from Gecko to WR in layers-free mode.
59 bytes, text/x-review-board-request
ethlin
: review+
pchang
: review+
Details
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.
59 bytes, text/x-review-board-request
ethlin
: review+
pchang
: review+
kats
: checkin-
Details 
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.
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.
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/
Flags: needinfo?(ethlin)
(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/
Flags: needinfo?(glenn)
Flags: needinfo?(glenn)
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
Flags: needinfo?(ethlin)
(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.
Priority: P3 → P2
Whiteboard: [gfx-noted][wr-mvp][triage] → [wr-mvp] [gfx-noted]
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.
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).
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla57
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.
(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 on attachment 8908784 [details]
Bug 1400034 - Back out changes that introduce scaling complexity to StackingContextHelper.

https://reviewboard.mozilla.org/r/180398/#review185806
Attachment #8908784 - Flags: review?(howareyou322) → review+
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
Attachment #8908785 - Flags: review?(howareyou322) → review+
Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

https://reviewboard.mozilla.org/r/180410/#review185812
Attachment #8908792 - Flags: review?(howareyou322) → review+
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.
Attachment #8908784 - Flags: review?(ethlin) → review+
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
Attachment #8908785 - Flags: review?(ethlin) → review+
Comment on attachment 8908792 [details]
Bug 1400034 - Turn on layers-free for transform/ and transform-3d/ reftests.

https://reviewboard.mozilla.org/r/180410/#review185834
Attachment #8908792 - Flags: review?(ethlin) → review+
Depends on: 1400868
Depends on: 1400881
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.
Attachment #8908792 - Flags: checkin-
You need to log in before you can comment on or make changes to this bug.