Closed Bug 1415987 Opened 7 years ago Closed 6 years ago

Hookup proper scale selection (ChooseScaleAndSetTransform)

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox66 --- fixed

People

(Reporter: jrmuizel, Assigned: tnikkel)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [wr-reserve[retest])

Attachments

(3 files, 2 obsolete files)

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
Having this in place would also help with bug 1415326
Blocks: 1415326
Whiteboard: [wr-mvp] [triage]
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Priority: P2 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
We need to determine how much of an issue this is still.
Flags: needinfo?(jmuizelaar)
Assigning to Jeff to reflect the NI.
Assignee: nobody → jmuizelaar
Whiteboard: [wr-reserve] → [wr-reserve[retest]
Flags: needinfo?(jmuizelaar)
Hey Jeff, just giving you a fresh needinfo so I get bugmail when you reply. Thanks!
Flags: needinfo?(jmuizelaar)
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
Flags: needinfo?(jmuizelaar)
Summary: Hookup proper scale selection → Hookup proper scale selection (ChooseScaleAndSetTransform)
We could add a scale parameter to StackingContextHelper that the nsDisplayTransform would set.
Assignee: jmuizelaar → nobody
Priority: P2 → P3
Assignee: nobody → jmuizelaar
This should be reusable by WebRender
Attachment #9026226 - Attachment mime type: application/octet-stream → text/plain
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
It can be adapted to yes.
Assignee: jmuizelaar → aosmond
Assignee: aosmond → tnikkel
Attached patch Use ChooseScale in wr (obsolete) (deleted) — Splinter Review
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).
Attached patch Use ChooseScale in wr (deleted) — Splinter Review
Attachment #9030165 - Attachment is obsolete: true
Attachment #9031944 - Flags: feedback?(jmuizelaar)
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.
Attachment #9031944 - Flags: feedback?(jmuizelaar) → feedback+
Attached patch ChooseScale in caller (deleted) — Splinter Review
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.
Attachment #9032103 - Flags: feedback?(jmuizelaar)
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.
Attachment #9032103 - Flags: feedback?(jmuizelaar) → feedback+
Attachment #9031944 - Flags: review?(jmuizelaar)
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.
Attachment #9030164 - Flags: review+
Attachment #9031944 - Flags: review?(jmuizelaar) → review+
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Blocks: 1495163
Blocks: 1448926
Blocks: 1493359

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

Regressions: 1569215
Regressions: 1614788
Blocks: 1626863

Adding a test for this in bug 1626863.

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

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

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

Regressions: 1689989

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

(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)

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

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: