WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
219894
Animation of "rotate" or "scale" property does not correctly account for static "translate" property
https://bugs.webkit.org/show_bug.cgi?id=219894
Summary
Animation of "rotate" or "scale" property does not correctly account for stat...
Antoine Quint
Reported
2020-12-15 08:22:12 PST
Created
attachment 416249
[details]
Test The test case combines the translate and rotate property and shows that the element does not rotate around its center as expected while being animated. This issue specific to accelerated animations as enforcing the software path by using a steps() timing function shows the correct behavior.
Attachments
Test
(440 bytes, text/html)
2020-12-15 08:22 PST
,
Antoine Quint
no flags
Details
STP 117 (buggy)
(11.57 MB, video/quicktime)
2020-12-15 08:36 PST
,
Antoine Quint
no flags
Details
Firefox 86 (correct)
(14.83 MB, video/quicktime)
2020-12-15 08:36 PST
,
Antoine Quint
no flags
Details
patch for EWS alone
(31.07 KB, patch)
2021-01-15 07:25 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch for EWS
(34.54 KB, patch)
2021-01-15 08:50 PST
,
Antoine Quint
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch for EWS
(34.54 KB, patch)
2021-01-15 09:29 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch for EWS
(40.14 KB, patch)
2021-01-29 09:01 PST
,
Antoine Quint
no flags
Details
Formatted Diff
Diff
Patch
(22.28 KB, patch)
2021-02-01 14:59 PST
,
Antoine Quint
dino
: review+
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-12-15 08:22:33 PST
<
rdar://problem/72342798
>
Antoine Quint
Comment 2
2020-12-15 08:36:27 PST
Created
attachment 416251
[details]
STP 117 (buggy)
Antoine Quint
Comment 3
2020-12-15 08:36:53 PST
Created
attachment 416252
[details]
Firefox 86 (correct)
Antoine Quint
Comment 4
2020-12-15 08:37:33 PST
The rotation is also performed in the wrong direction.
Antoine Quint
Comment 5
2021-01-07 06:44:33 PST
Replacing the translate property with a translate animation using the same value for from and to produces the expected result. This must be something wrong with addBaseValueTransformAnimation().
Antoine Quint
Comment 6
2021-01-07 09:28:19 PST
The orders of animations added to Core Animation does not match the order in which we call setAnimationOnLayer().
Antoine Quint
Comment 7
2021-01-07 09:56:35 PST
As it turns out, Core Animation will sort animations by begin time, which is the issue here because the base transform animations are always set to use 1 as their begin time and will sort earlier.
Antoine Quint
Comment 8
2021-01-07 10:10:52 PST
The alternative to the system we put in place already is to use an intermediary transform layer for each animated property, potentially creating 3 extra layers to accommodate all of translate, rotate, scale and transform properties, using the existing layer to animate one of those properties.
Antoine Quint
Comment 9
2021-01-07 11:31:28 PST
Actually, maybe we can leverage CAAnimationGroup for this, where each property would have its dedicated group.
Antoine Quint
Comment 10
2021-01-08 05:16:08 PST
From testing in a test app, sorting of animations is done accepting for nesting, so we can indeed have a CAAnimationGroup for each individual property all timed to start at the same time, and then have the expected begin time set in the animation nested inside the group.
Antoine Quint
Comment 11
2021-01-12 00:47:52 PST
Integration of CAAnimationGroup in WebCore seems to be working well, although I have some issues with begin times that I have yet to figure out. I also need to test with remote layers on iOS.
Antoine Quint
Comment 12
2021-01-13 09:01:27 PST
So I have the test for this bug working fine with the adoption of animation groups. That said, there are couple of issues that came up during integration. The first issue is that some testing code no longer works. Specifically, GraphicsLayerCA::acceleratedAnimationsForTesting() crashes in a couple of tests because animationForKey() no longer returns an animation. I expect, named animations under groups aren't returned by this method. I assume we can find a workaround by using the identifier of the group instead, and then using the `animations` array to find the animation itself. The second issue is that we now have to set explicit begin times on all animations hosted in groups since the groups themselves require an explicit begin time to guarantee they all have the same begin time and sort in the order in which they were created. This means we can no longer let Core Animation tell us what the actual begin time of the animation would be by not setting the beginTime property in the first place.
Antoine Quint
Comment 13
2021-01-14 09:56:53 PST
After talking with Simon Fraser about this, I think the second issue is not a real problem. Times should be dictated by being times on Web Animations, and not by Core Animation. So using explicit begin times will be alright.
Antoine Quint
Comment 14
2021-01-15 07:25:07 PST
Created
attachment 417698
[details]
patch for EWS alone
Antoine Quint
Comment 15
2021-01-15 08:24:25 PST
What's left to do: 1. make this work on iOS by adding support for CAAnimationGroup with remote layer hosting 2. make this work on Windows with setAnimations() overrides in CAAnimationPlatformWin
Antoine Quint
Comment 16
2021-01-15 08:50:35 PST
Created
attachment 417702
[details]
Patch for EWS
Antoine Quint
Comment 17
2021-01-15 09:29:16 PST
Created
attachment 417709
[details]
Patch for EWS
Darin Adler
Comment 18
2021-01-15 14:21:00 PST
Comment on
attachment 417709
[details]
Patch for EWS View in context:
https://bugs.webkit.org/attachment.cgi?id=417709&action=review
> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:245 > + return "";
emptyString() is more efficient than "" to create an empty string.
> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:569 > + return nullptr;
Should be nil instead of nullptr.
> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:562 > + RetainPtr<CFMutableArrayRef> array = adoptCF(CFArrayCreateMutable(0, value.size(), &kCFTypeArrayCallBacks));
auto makes code like this read better, I think.
> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:564 > + for (size_t i = 0; i < value.size(); ++i) { > + RefPtr<PlatformCAAnimation> animation = value[i];
This should have been a modern for loop. for (auto& animation : value) { The rest of the code below is unaffected.
> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:566 > + CFArrayAppendValue(array.get(), downcast<PlatformCAAnimationWin>(animation.get())->m_animation.get());
I prefer "(*animation)." instead of "(animation.get())->" and either will work.
Antoine Quint
Comment 19
2021-01-29 09:00:39 PST
(In reply to Darin Adler from
comment #18
)
> Comment on
attachment 417709
[details]
> Patch for EWS > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=417709&action=review
> > > Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:245 > > + return ""; > > emptyString() is more efficient than "" to create an empty string. > > > Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:569 > > + return nullptr; > > Should be nil instead of nullptr. > > > Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:562 > > + RetainPtr<CFMutableArrayRef> array = adoptCF(CFArrayCreateMutable(0, value.size(), &kCFTypeArrayCallBacks)); > > auto makes code like this read better, I think. > > > Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:564 > > + for (size_t i = 0; i < value.size(); ++i) { > > + RefPtr<PlatformCAAnimation> animation = value[i]; > > This should have been a modern for loop. > > for (auto& animation : value) { > > The rest of the code below is unaffected. > > > Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:566 > > + CFArrayAppendValue(array.get(), downcast<PlatformCAAnimationWin>(animation.get())->m_animation.get()); > > I prefer "(*animation)." instead of "(animation.get())->" and either will > work.
Thanks for all this useful feedback, I'll integrate it in a future patch for review. In the meantime, I'll be uploading another patch for EWS that is not ready for review yet. I just need to check the iOS failures are fixed now that I've implemented the stubs for PlatformCAAnimationRemote.
Antoine Quint
Comment 20
2021-01-29 09:01:43 PST
Created
attachment 418733
[details]
Patch for EWS
Antoine Quint
Comment 21
2021-02-01 08:18:32 PST
To make the final patch for this bug more targeted, I've split off the work to support CAAnimationGroup through PlatformCAAnimation in
bug 221209
.
Antoine Quint
Comment 22
2021-02-01 14:59:06 PST
Created
attachment 418931
[details]
Patch
Dean Jackson
Comment 23
2021-02-02 02:26:32 PST
Comment on
attachment 418931
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=418931&action=review
> Source/WebCore/ChangeLog:24 > + Note that animations 2 and 3 are additive and thus added in the inverse order that we expect animations to be > + applied, which is a peculiarity of Core Animations introduced in macOS 10.15, hence the build-time flag > + CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED.
I think you could word this better. Due to a peculiarity of Core Animation, additive animations are applied in an inverse order.
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2893 > + // we need to have them wrapped individually in an animation group because Core Animation sorts not only in the order in which > + // animations are added to a layer, but also based on their begin time. Since a rotate animation can have an earlier begin time
... applies animations first by their begin time, and then by the order in which they were added (for those with the same begin time).
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2901 > + auto animationGroupBeginTime = 1_s;
Why 1_s rather than 0?
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2902 > + auto eternalDuration = std::numeric_limits<double>::max();
I think infiniteDuration is a better name.
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2910 > + auto animationGroup = LayerPropertyAnimation(WTFMove(caAnimationGroup), "group-" + createCanonicalUUIDString(), property, 0, 0, 0_s);
Do you need to use a UUID? Why not an incrementing static uint64_t? I assume the group only has to be unique on a layer.
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2917 > enum class Additive { Yes, No };
I'm definitely not a C++ expert, but I see lots of these in WebCore. I wonder why we don't define this somewhere common and use "using Additive = BoolClass;"?
> LayoutTests/webanimations/relative-ordering-of-translate-and-rotate-properties-accelerated.html:27 > + 0% { rotate: 180deg; } > + 100% { rotate: 180deg; }
You should probably pick something like 30deg. It doesn't matter in this case, but an upside down square is going to look the same as if it was the right way up (or turned -90/90). In other words, this test would not be able to tell if the animation is actually applied.
Antoine Quint
Comment 24
2021-02-02 02:50:26 PST
(In reply to Dean Jackson from
comment #23
)
> Comment on
attachment 418931
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=418931&action=review
> > > Source/WebCore/ChangeLog:24 > > + Note that animations 2 and 3 are additive and thus added in the inverse order that we expect animations to be > > + applied, which is a peculiarity of Core Animations introduced in macOS 10.15, hence the build-time flag > > + CA_WHERE_ADDITIVE_TRANSFORMS_ARE_REVERSED. > > I think you could word this better. Due to a peculiarity of Core Animation, > additive animations are applied in an inverse order.
Sure thing.
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2893 > > + // we need to have them wrapped individually in an animation group because Core Animation sorts not only in the order in which > > + // animations are added to a layer, but also based on their begin time. Since a rotate animation can have an earlier begin time > > ... applies animations first by their begin time, and then by the order in > which they were added (for those with the same begin time). > > > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2901 > > + auto animationGroupBeginTime = 1_s; > > Why 1_s rather than 0?
0 has special meaning in Core Animation, meaning whatever the CACurrentMediaTime() is during the next commit.
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2902 > > + auto eternalDuration = std::numeric_limits<double>::max(); > > I think infiniteDuration is a better name.
Will use that.
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2910 > > + auto animationGroup = LayerPropertyAnimation(WTFMove(caAnimationGroup), "group-" + createCanonicalUUIDString(), property, 0, 0, 0_s); > > Do you need to use a UUID? Why not an incrementing static uint64_t? I assume > the group only has to be unique on a layer.
I don't think I have to use it but it's handy enough and when logging through multiple layers it helps.
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2917 > > enum class Additive { Yes, No }; > > I'm definitely not a C++ expert, but I see lots of these in WebCore. I > wonder why we don't define this somewhere common and use "using Additive = > BoolClass;"? > > > LayoutTests/webanimations/relative-ordering-of-translate-and-rotate-properties-accelerated.html:27 > > + 0% { rotate: 180deg; } > > + 100% { rotate: 180deg; } > > You should probably pick something like 30deg. It doesn't matter in this > case, but an upside down square is going to look the same as if it was the > right way up (or turned -90/90). In other words, this test would not be able > to tell if the animation is actually applied.
The test uses 180deg because using 30deg would produce flaky results with a ref test due to aliasing. I've added an ::after pseudo-element in the test to locate a square in the top-left corner of the element so you can tell how it was rotated, which I assume wasn't super obvious just reading the test source :)
Antoine Quint
Comment 25
2021-02-02 02:59:09 PST
Committed
r272201
: <
https://trac.webkit.org/changeset/272201
>
Yury Semikhatsky
Comment 26
2021-02-02 16:12:54 PST
(In reply to Antoine Quint from
comment #25
)
> Committed
r272201
: <
https://trac.webkit.org/changeset/272201
>
This broke Mojave build (
https://build.webkit.org/builders/Apple-Mojave-Release-Build/builds/22958
): ./platform/graphics/ca/GraphicsLayerCA.cpp:3080:21: error: use of undeclared identifier 'caAnimations'; did you mean 'animations'? caAnimations.append(baseValueTransformAnimation->m_animation); ^~~~~~~~~~~~ animations
Antoine Quint
Comment 27
2021-02-03 01:45:38 PST
(In reply to Yury Semikhatsky from
comment #26
)
> (In reply to Antoine Quint from
comment #25
) > > Committed
r272201
: <
https://trac.webkit.org/changeset/272201
> > > This broke Mojave build > (
https://build.webkit.org/builders/Apple-Mojave-Release-Build/builds/22958
): > > ./platform/graphics/ca/GraphicsLayerCA.cpp:3080:21: error: use of undeclared > identifier 'caAnimations'; did you mean 'animations'? > > caAnimations.append(baseValueTransformAnimation->m_animation); > ^~~~~~~~~~~~ > animations
Thanks for catching this Yury, I fixed this in
r272303
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug