Bug 182098 - [Web Animations] Account for provided easings when computing progress and resolving keyframe effect values
Summary: [Web Animations] Account for provided easings when computing progress and res...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-25 06:27 PST by Antoine Quint
Modified: 2018-01-25 12:21 PST (History)
4 users (show)

See Also:


Attachments
Patch (41.78 KB, patch)
2018-01-25 06:39 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (43.30 KB, patch)
2018-01-25 06:49 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.45 MB, application/zip)
2018-01-25 07:54 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.18 MB, application/zip)
2018-01-25 08:23 PST, EWS Watchlist
no flags Details
Patch (45.00 KB, patch)
2018-01-25 08:42 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (45.22 KB, patch)
2018-01-25 11:27 PST, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-01-25 06:27:58 PST
We've added support for parsing easing values in webkit.org/b/181978, we now need to use those values when computing the progress and resolving keyframe effect values.
Comment 1 Radar WebKit Bug Importer 2018-01-25 06:30:30 PST
<rdar://problem/36866149>
Comment 2 Antoine Quint 2018-01-25 06:39:35 PST
Created attachment 332260 [details]
Patch
Comment 3 Antoine Quint 2018-01-25 06:49:08 PST
Created attachment 332261 [details]
Patch
Comment 4 EWS Watchlist 2018-01-25 07:54:30 PST
Comment on attachment 332261 [details]
Patch

Attachment 332261 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6208343

New failing tests:
transitions/default-timing-function.html
Comment 5 EWS Watchlist 2018-01-25 07:54:31 PST
Created attachment 332266 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 EWS Watchlist 2018-01-25 08:23:40 PST
Comment on attachment 332261 [details]
Patch

Attachment 332261 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6208379

New failing tests:
transitions/default-timing-function.html
Comment 7 EWS Watchlist 2018-01-25 08:23:42 PST
Created attachment 332270 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Antoine Quint 2018-01-25 08:42:11 PST
Created attachment 332272 [details]
Patch
Comment 9 Dean Jackson 2018-01-25 10:00:14 PST
Comment on attachment 332272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=332272&action=review

All good, but I think you need to use optionals instead of the -1s everywhere. We've had animation bugs before when using special values, because someone eventually uses them as a boolean.

> Source/WebCore/ChangeLog:16
> +        and identify cubic-bezier(0, 0, 0, 0) and cubic-bezier(1, 1, 1, 1) as linear timing functions, as called out
> +        by the WPT tests.

Hmmm... the spec says this? I wonder what we do for our current animation code.

> Source/WebCore/animation/AnimationEffect.cpp:283
> +            bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards;

Fordwards

> Source/WebCore/animation/KeyframeEffect.cpp:691
> +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress)

There is a fair bit of work in this method. I wonder if we could cache some of the computed keyframe lists, rather than computing them every time iterationProgress changes.

> Source/WebCore/animation/KeyframeEffect.cpp:700
> +        // 1. If iteration progress is unresolved abort this procedure.

I assume this is impossible here?

> Source/WebCore/animation/KeyframeEffect.cpp:711
> +        Vector<int> propertySpecificKeyframes;
> +        for (size_t i = 0; i < m_keyframes.size(); ++i) {

Couldn't this be a Vector of keyframes, and use a for : loop?

> Source/WebCore/animation/KeyframeEffect.cpp:720
> +            propertySpecificKeyframes.append(i);

Is there any point appending multiple keyframes with the same offset? e.g. 0 or 1
And if that case is already handled elsewhere, those can be bools rather than unsigned.

> Source/WebCore/animation/KeyframeEffect.cpp:729
> +        //    offset of 0, a property value set to the neutral value for composition, and a composite operation of add, and prepend it to the beginning of
> +        //    property-specific keyframes.

It's a bit confusing that this last bit about setting up the default value happens much later.

> Source/WebCore/animation/KeyframeEffect.cpp:731
> +            propertySpecificKeyframes.insert(0, -1);

I don't like this hack of inserting a -1.

> Source/WebCore/animation/KeyframeEffect.cpp:750
> +            // If iteration progress < 0 and there is more than one keyframe in property-specific keyframes with a computed keyframe offset of 0,
> +            // Add the first keyframe in property-specific keyframes to interval endpoints.
> +            intervalEndpoints.append(propertySpecificKeyframes.first());

So we probably could have handled this above by ignoring the subsequent zero keyframes and keeping a bool.

> Source/WebCore/animation/KeyframeEffect.cpp:761
> +            size_t indexOfLastKeyframeWithZeroOffset;

Initialize this to 0.

> Source/WebCore/animation/KeyframeEffect.cpp:771
> +                double offset;
> +                if (keyframeIndex == -1)
> +                    offset = i ? 1 : 0;
> +                else {
> +                    auto& keyframe = m_keyframes[keyframeIndex];
> +                    offset = keyframe.key();
> +                }

Here's where you use the -1 hack.

I hate to suggest this but I wonder if the propertySpecificKeyframes list could be indexed from 1. Or hold optionals?

Also, there is a nice way to initialize offset immediately:

auto offset = [&] () -> double {
  if (keyframeIndex == -1)
    return i ? 1 : 0;
  return m_keyframes[keyframeIndex].key();
}();

> Source/WebCore/animation/KeyframeEffect.cpp:780
> +            if (indexOfFirstKeyframeToAddToIntervalEndpoints > -1) {

>= 0

Again, I don't like the -1. Can you use an optional<unsigned>?

> Source/WebCore/animation/KeyframeEffect.cpp:785
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]);
> +            } else {
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]);
> +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]);

Add ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size())

> Source/WebCore/animation/KeyframeEffect.cpp:795
> +            auto keyframeStyle = keyframeIndex == -1 ? &targetStyle : m_keyframes[keyframeIndex].style();

Another -1.

> Source/WebCore/platform/animation/TimingFunction.cpp:91
> +        // decrement current step by one.

Indent.

> Source/WebCore/platform/animation/TimingFunction.h:167
> +    bool isLinear() const
> +    {
> +        return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0);
> +    }

It's also linear if 0, 0, 1, 1
Comment 10 Antoine Quint 2018-01-25 11:27:01 PST
Created attachment 332292 [details]
Patch
Comment 11 Antoine Quint 2018-01-25 11:31:38 PST
(In reply to Dean Jackson from comment #9)
> Comment on attachment 332272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332272&action=review
> 
> All good, but I think you need to use optionals instead of the -1s
> everywhere. We've had animation bugs before when using special values,
> because someone eventually uses them as a boolean.

I've changed the two Vector<int> to Vector<std::optional<size_t>>.

> > Source/WebCore/animation/AnimationEffect.cpp:283
> > +            bool goingFordwards = currentDirection() == AnimationEffect::ComputedDirection::Forwards;
> 
> Fordwards

Fixed in new patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:691
> > +void KeyframeEffect::setAnimatedPropertiesInStyle(RenderStyle& targetStyle, double iterationProgress)
> 
> There is a fair bit of work in this method. I wonder if we could cache some
> of the computed keyframe lists, rather than computing them every time
> iterationProgress changes.
> 
> > Source/WebCore/animation/KeyframeEffect.cpp:700
> > +        // 1. If iteration progress is unresolved abort this procedure.
> 
> I assume this is impossible here?

Yes, this would be caught at call sites.

> > Source/WebCore/animation/KeyframeEffect.cpp:711
> > +        Vector<int> propertySpecificKeyframes;
> > +        for (size_t i = 0; i < m_keyframes.size(); ++i) {
> 
> Couldn't this be a Vector of keyframes, and use a for : loop?

This would mean dropping KeyframeList which is a large undertaking that I'm considering but is ought of the scope of this patch.

> > Source/WebCore/animation/KeyframeEffect.cpp:731
> > +            propertySpecificKeyframes.insert(0, -1);
> 
> I don't like this hack of inserting a -1.

Using std::nullopt now.

> > Source/WebCore/animation/KeyframeEffect.cpp:761
> > +            size_t indexOfLastKeyframeWithZeroOffset;
> 
> Initialize this to 0.

Done.

> > Source/WebCore/animation/KeyframeEffect.cpp:771
> > +                double offset;
> > +                if (keyframeIndex == -1)
> > +                    offset = i ? 1 : 0;
> > +                else {
> > +                    auto& keyframe = m_keyframes[keyframeIndex];
> > +                    offset = keyframe.key();
> > +                }
> 
> Here's where you use the -1 hack.
> 
> I hate to suggest this but I wonder if the propertySpecificKeyframes list
> could be indexed from 1. Or hold optionals?
> 
> Also, there is a nice way to initialize offset immediately:
> 
> auto offset = [&] () -> double {
>   if (keyframeIndex == -1)
>     return i ? 1 : 0;
>   return m_keyframes[keyframeIndex].key();
> }();

Done, nice.

> > Source/WebCore/animation/KeyframeEffect.cpp:785
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfFirstKeyframeToAddToIntervalEndpoints + 1]);
> > +            } else {
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset]);
> > +                intervalEndpoints.append(propertySpecificKeyframes[indexOfLastKeyframeWithZeroOffset + 1]);
> 
> Add ASSERT(indexOfLastKeyframeWithZeroOffset <
> propertySpecificKeyframes.size())

I added ASSERT(indexOfLastKeyframeWithZeroOffset < propertySpecificKeyframes.size() - 1);

> > Source/WebCore/platform/animation/TimingFunction.cpp:91
> > +        // decrement current step by one.
> 
> Indent.

Fixed.

> > Source/WebCore/platform/animation/TimingFunction.h:167
> > +    bool isLinear() const
> > +    {
> > +        return (!m_x1 && !m_y1 && !m_x2 && !m_y2) || (m_x1 == 1.0 && m_y1 == 1.0 && m_x2 == 1.0 && m_y2 == 1.0);
> > +    }
> 
> It's also linear if 0, 0, 1, 1

Added that case too, nice.
Comment 12 Antoine Quint 2018-01-25 12:21:54 PST
Committed r227622: <https://trac.webkit.org/changeset/227622>