Bug 219894 - Animation of "rotate" or "scale" property does not correctly account for static "translate" property
Summary: Animation of "rotate" or "scale" property does not correctly account for stat...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on: 221209
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-15 08:22 PST by Antoine Quint
Modified: 2021-02-03 01:45 PST (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 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.
Comment 1 Radar WebKit Bug Importer 2020-12-15 08:22:33 PST
<rdar://problem/72342798>
Comment 2 Antoine Quint 2020-12-15 08:36:27 PST
Created attachment 416251 [details]
STP 117 (buggy)
Comment 3 Antoine Quint 2020-12-15 08:36:53 PST
Created attachment 416252 [details]
Firefox 86 (correct)
Comment 4 Antoine Quint 2020-12-15 08:37:33 PST
The rotation is also performed in the wrong direction.
Comment 5 Antoine Quint 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().
Comment 6 Antoine Quint 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().
Comment 7 Antoine Quint 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.
Comment 8 Antoine Quint 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.
Comment 9 Antoine Quint 2021-01-07 11:31:28 PST
Actually, maybe we can leverage CAAnimationGroup for this, where each property would have its dedicated group.
Comment 10 Antoine Quint 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.
Comment 11 Antoine Quint 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.
Comment 12 Antoine Quint 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.
Comment 13 Antoine Quint 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.
Comment 14 Antoine Quint 2021-01-15 07:25:07 PST
Created attachment 417698 [details]
patch for EWS alone
Comment 15 Antoine Quint 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
Comment 16 Antoine Quint 2021-01-15 08:50:35 PST
Created attachment 417702 [details]
Patch for EWS
Comment 17 Antoine Quint 2021-01-15 09:29:16 PST
Created attachment 417709 [details]
Patch for EWS
Comment 18 Darin Adler 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.
Comment 19 Antoine Quint 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.
Comment 20 Antoine Quint 2021-01-29 09:01:43 PST
Created attachment 418733 [details]
Patch for EWS
Comment 21 Antoine Quint 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.
Comment 22 Antoine Quint 2021-02-01 14:59:06 PST
Created attachment 418931 [details]
Patch
Comment 23 Dean Jackson 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.
Comment 24 Antoine Quint 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 :)
Comment 25 Antoine Quint 2021-02-02 02:59:09 PST
Committed r272201: <https://trac.webkit.org/changeset/272201>
Comment 26 Yury Semikhatsky 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
Comment 27 Antoine Quint 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.