ComputeSuitableScaleForAnimation should check other transform-like properties
Categories
(Core :: Layout, enhancement, P3)
Tracking
()
People
(Reporter: boris, Assigned: boris)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
In order to support compositor animations for individual transforms, I'd like to replace the hardcode of eCSSProperty_transform with an array or property set. Most of the replacements are simple, but the scale calculation is a little bit tricky [1], because it seems we don't really do interpolation in this function, instead, we roughly calculation the scale factor. It's ok if we only have transform
property. However, we want to support rotate
and scale
properties for layer animations, so we need a more sophisticate way to calculate it.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
For the other replacements, I will file other bugs.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
So we can let KeyframeEffect::ContainsAnimatedScale check individual
transforms in the future.
We update ComputeSuitableScaleForAnimation in the next patch.
Assignee | ||
Comment 3•6 years ago
|
||
For now, we only have eCSSProperty_transform. However, after we support
compositor animations on individual transform and motion-path, we will
extend this list.
Note: For individual transform, the scale factors are affected by rotate and
scale properties. However, we still can use this list because the scale
factors of translate should always be (1.0, 1.0). If there is any
performance impact, we can add a new list without translate property.
Depends on D19525
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
hiro, I sent you review requests of this bug because I think it should be ok to land this separately from Bug 1505225. The current DisplayItemType::TYPE_TRANSFORM is still mapping {eCSSProperty_transform}, so the result of these patches should be the same as that before, until we extend the list for individual transforms in Bug 1425837.
Comment 5•6 years ago
|
||
I think we can land this, but it's not testable, right? The only way I can think of to check the result of ComputeSuitableScaleForAnimation is to run individual transform properties on the compositor and see the results of its rendering.
Assignee | ||
Comment 6•6 years ago
|
||
Yes. For now, all we can do is something like that these patches don't break the current tree, and check if the minor refactoring looks reasonable.
Comment 7•6 years ago
|
||
So for me, this can be landed lastly with some valid test cases (and I think we should).
Assignee | ||
Comment 8•6 years ago
|
||
I see. I will hold this for now. :)
Comment 9•6 years ago
|
||
Comment on attachment 9043321 [details]
Bug 1526847 - Part 1: Let GetScaleValue support individual transforms.
Revision D19525 was moved to bug 1529422. Setting attachment 9043321 [details] to obsolete.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 10•6 years ago
|
||
Still trying to find a suitable test case for this. It seems we add this function for B2G in the beginning, and fix it due to some other bugs.
Comment 11•6 years ago
|
||
I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.
Just tried a scaling animation box containing some text, but unfortunately, this may not reflect the change I did because the reftest was not failed without the fix. Maybe need a trickier way. ;(
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #12)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
I think the test case would be a reftest which has a scaling animation box containing some text. We need to add reftest-no-flush. The reftest might not be failed without the fix though.
Just tried a scaling animation box containing some text, but unfortunately, this may not reflect the change I did because the reftest was not failed without the fix. Maybe need a trickier way. ;(
And I found a gfx issue. If we use reftest-no-flush
, the edges of the animation box look blurred (AA issue, I guess) when taking the screenshot. This happens on both transform: scale(..)
and scale: ..
. And this blur makes the result of this test intermittent.
Updated•6 years ago
|
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•