| Summary: | Animation of "rotate" or "scale" property does not correctly account for static "translate" property | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||||||
| Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, darin, dino, simon.fraser, webkit-bug-importer, yurys | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 221209 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
Created attachment 416251 [details]
STP 117 (buggy)
Created attachment 416252 [details]
Firefox 86 (correct)
The rotation is also performed in the wrong direction. 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(). The orders of animations added to Core Animation does not match the order in which we call setAnimationOnLayer(). 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. 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. Actually, maybe we can leverage CAAnimationGroup for this, where each property would have its dedicated group. 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. 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. 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. 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. Created attachment 417698 [details]
patch for EWS alone
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 Created attachment 417702 [details]
Patch for EWS
Created attachment 417709 [details]
Patch for EWS
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. (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. Created attachment 418733 [details]
Patch for EWS
To make the final patch for this bug more targeted, I've split off the work to support CAAnimationGroup through PlatformCAAnimation in bug 221209. Created attachment 418931 [details]
Patch
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. (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 :) Committed r272201: <https://trac.webkit.org/changeset/272201> (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 (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. |
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.