1415987 - Hookup proper scale selection (ChooseScaleAndSetTransform)

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

Once we have https://github.com/servo/webrender/issues/1542 we'll need to the transforms properly. We'll probably want to reuse: ChooseScaleAndSetTransform from FrameLayerBuilder.cpp

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Having this in place would also help with bug 1415326

Comment 2

[Jeff Muizelaar [:jrmuizel]]

We need to determine how much of an issue this is still.

Comment 3

[Bobby Holley (:bholley)]

Assigning to Jeff to reflect the NI.

Comment 4

[Maire Reavy [:mreavy]]

Hey Jeff, just giving you a fresh needinfo so I get bugmail when you reply.  Thanks!

Comment 5

[Jeff Muizelaar [:jrmuizel]]

This is definitely still an issue.

We want to be doing the more elaborate thing that ChooseScaleAndSetTransform does here: https://searchfox.org/mozilla-central/rev/28fe656de6a022d1fc01bbd3811023fca99cf223/gfx/layers/wr/StackingContextHelper.cpp#55

Comment 6

[Jeff Muizelaar [:jrmuizel]]

We could add a scale parameter to StackingContextHelper that the nsDisplayTransform would set.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Factor out the ChooseScale part of ChooseScacleAndSetTransform

This should be reusable by WebRender

Comment 8

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

Hmm, does this bug also cover using ChooseScaleAndSetTransform in the blob code? There's a comment about it here: https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/gfx/layers/wr/WebRenderCommandBuilder.cpp#1906-1907

Comment 9

[Jeff Muizelaar [:jrmuizel]]

It can be adapted to yes.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Factor out the ChooseScale part of ChooseScacleAndSetTransform (rebased)

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: Use ChooseScale in wr

This at least fixes bug 1492859 and passes try server on linux, but still need to understand the webrender bits a bit more to make sure they are correct (an obvious mistake in an earlier patch only failed one reftest).

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: Use ChooseScale in wr

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9031944 [details] [diff] [review]
Use ChooseScale in wr

Review of attachment 9031944 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if it makes more sense to move the call to ChooseScale out into the caller so that we're only calling it for transform items and not for everything that pushes a stacking context. Other than that this seems reasonable.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: ChooseScale in caller

Not sure if this is better, I guess we could compute mInheritedTransform and mSnappingSurfaceTransform in the caller, but that's just more things to pass.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 9032103 [details] [diff] [review]
ChooseScale in caller

Review of attachment 9032103 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, why don't we go with the first one. I think the better solution here is to add a second constructor for StackingContextHelper but we can do that in a follow up.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9030164 [details] [diff] [review]
Factor out the ChooseScale part of ChooseScacleAndSetTransform (rebased)

This is Jeff's patch so I can review it.

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/640e15aa79b9
https://hg.mozilla.org/mozilla-central/rev/996bf60e0fce

Comment 19

[Mayank Bansal]

This bug massively improved bug 1448926 and bug 1493359 with WR enabled.

Comment 20

[Timothy Nikkel (:tnikkel)]

Adding a test for this in bug 1626863.

Comment 21

[Timothy Nikkel (:tnikkel)]

Since this bug (and the ones that came before it and after it) seem to keep causing regressions or we require further fixes to this code I made myself a summary of all the changes here to understand better and verify which bugs are in automated testing or not.

2017-09-29 bug 1395501 (Ethan Lin) Where we started storing the inherited scale in the in stacking context helper.

2018-03-27 bug 1449640 (jrmuizel) Made it so that we check for preserve3d and prespective to match what FrameLayerBuilder::ChooseScale does (which we aren't using yet)
-landed with test layout/reftests/transform-3d/preserve3d-scale.html
-in bug 1626865 (2020-04-02) I improve the test to check it against a close enough 2d ref version of the 3d test instead of just checking that it's not blank

2018-03-27 bug 1449958 (jrmuizel) use the parent scale if we didn't compute one so we don't default to 1.0 and the scale gets very large

2018-12-21 bug 1415987 (tnikkel) use FrameLayerBuilder::ChooseScale in webrender
-in bug 1626863 (2020-04-02) I add a lame test that should work for this
-bug 1495163 has a usable testcase

2019-08-01 bug 1569215 (tnikkel) pass parent scale into choose scale (regression caused by by bug 1415987)
-has test landed layout/reftests/transform/1569215-1.html
-this bug and bug 1564698 also have testcases that were fixed by this

2019-08-29 bug 1553571 (tnikkel) pass the right size from display transform into stackingcontext helper (it was zero) (caused by bug 1519444 which allowed small layers with webrender)
-has test landed layout/reftests/bugs/1553571-1.html

2020-02-20 bug 1614788 key frame effect always says its animated when its not, fixed in bug 1615156 (hiro) (bug 1415987 original regressor, got worse for unknown reason, bug 1569215 improved it back to original buggy version)
-has test landed layout/reftests/bugs/1614788-1.svg

2020-03-31 bug 1626259 (tnikkel) fixed the preserve3d check to look at self and not parent to match ChooseScale (caused by bug 1569215, but the incorrect code came from bug 1449640)
-has tests landed layout/reftests/bugs/1626259-{1,2}.html

Comment 22

[Timothy Nikkel (:tnikkel)]

2020-05-20 bug 1637067 (tnikkel) use Combines3DTransformWithAncestors instead of preserve3d to match ChooseScale and 3d transforms get a 1.0 scale instead of inheriting the parent scale to match ChooseScale (bug 1626259 exposed this bug but didn't cause it per-say)
-has test landed layout/reftests/transform-3d/1637067-1.html

Comment 23

[Timothy Nikkel (:tnikkel)]

2020-11-19 bug 1662062 (tnikkel) include the presshell resolution that is used by pinch zooming in the scale. this isn't a regression, just a case that was missed.
-has test landed gfx/tests/reftest/1662062-1.html

Comment 24

[Timothy Nikkel (:tnikkel)]

2021-08-17 bug 1724901 (tnikkel) include resolution that comes from ancestor documents (whether via transforms or pinch zoom) in other processes in the scale. this isn't a regression, just a case that was missed.
-has test landed gfx/tests/reftest/1724901-1.html

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #24)

2021-08-17 bug 1724901 (tnikkel) include resolution that comes from ancestor documents (whether via transforms or pinch zoom) in other processes in the scale. this isn't a regression, just a case that was missed.
-has test landed gfx/tests/reftest/1724901-1.html

Added another, better test gfx/tests/reftest/1724901-2.html (used bug 1739015 to land that)

Comment 26

[Timothy Nikkel (:tnikkel)]

2023-04-17 bug 1804872 (tnikkel) use the stacking context scale instead of the stacking context transform when computing the size to decode images as when the scale is being animated because the transform does not take into account the animated scale
-3 tests landed layout/reftests/bugs/1804872-*.html

Comment 27

[Timothy Nikkel (:tnikkel)]

2023-05-24 bug 1714763 (tnikkel) when computing the local scale use the parent scale combined with the local transform instead of the parent transform combined with the local transform. when the scale of the parent transform is increasing the parent scale will be much larger than the scale of the parent transform.
-3 tests landed layout/reftests/bugs/1714763-*.html

Comment 28

[Timothy Nikkel (:tnikkel)]

2024-02-28 bug 1878294 (tnikkel) svg transforms changing translate incorrectly determined to have changing scale which makes them subject to max scale limiting in ChooseScale.
-test landed layout/reftests/bugs/1878294-1.html