Bug 94980 – [CSS Shaders] Implement transform parameter animations for CSS Custom Filters

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

Custom filters allow parameters with transform types. This bug will implement the animations support for that type.

[Joshua Netterfield]

Created attachment 161300 [details]
Patch

[Joshua Netterfield]

N.B. the above patch depends on https://bugs.webkit.org/show_bug.cgi?id=71401. I'm posting this here for review so I can make changes before my last day Friday, if necessary.

[Joshua Netterfield]

Created attachment 161310 [details]
Patch

[Joshua Netterfield]

Rebased.

[Joshua Netterfield]

Created attachment 161315 [details]
Patch

[Alexandru Chiculita]

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

Thanks for looking into this. I have a few comments on how we do the blending. I'm not a reviewer, so feel free to ignore my comments.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:-54
> -static inline int blendFunc(const AnimationBase*, int from, int to, double progress)

Why do you need all these changes?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:34
> +#include "CSSPropertyAnimation.h"

Don't add this to the header file. You just need to forward define the AnimationBase.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:34
> +#include "CSSPropertyAnimation.h"

Ditto.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:50
> +    virtual PassRefPtr<CustomFilterParameter> blend(const AnimationBase* anim, const CustomFilterParameter* fromParameter, double progress)

This function will pull in the AnimationBase class, so I think we would better move it to CustomFilterTransformParameter.cpp.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:58
> +        TransformOperations result = CSSPropertyAnimation::blendFunc(anim, from, to, progress);

This function uses isTransformFunctionListValid internally, which is only set for the -webkit-transform property. For CSS Custom Filters, I think we would need to check for matching transforms everytime, as there's no place where we could store that info right now.

I think it would be better if we moved the blending of the transforms to the TransformOperations class: 
transformOperations.blendByMatchingOperations(const TransformOperations& from, double progress) - this one can assert if the operations are not matching and it's the caller responsibility to make sure that the operations are matching.
transformOperations.blendByMatrix(const TransformOperations& from, double progress, const LayoutSize&)
and finally
transformOperations.blend(const TransformOperations& from, double progress, const LayoutSize&) - will do the decision inside

> LayoutTests/css3/filters/custom/custom-filter-animation.html:105
> +        @-webkit-keyframes custom-mix-transforms-anim {

You should have tests checking for number of rotations. Ie. rotateZ(0deg) to rotateZ(360deg) should make a full spin. In the current patch it wouldn't animate at all I suppose.

> LayoutTests/css3/filters/custom/custom-filter-animation.html:107
> +            to   { -webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform rotateZ(10deg)); }

Add a test with multiple transforms in the same custom function.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:67
> +function transform(token)

You should be looking for matrix and matrix3d instead, you need to add tests for multiple transform attributes.

[Joshua Netterfield]

The makes a lot more sense. Will do.

[Peter Beverloo (cr-android ews)]

Comment on attachment 161315 [details]
Patch

Attachment 161315 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13693444

[Build Bot]

Comment on attachment 161315 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 161315 [details]
Patch

Attachment 161315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13682478

[Joshua Netterfield]

Created attachment 161577 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 161577 [details]
Patch

Attachment 161577 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13687882

[WebKit Review Bot]

Comment on attachment 161577 [details]
Patch

Attachment 161577 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13688953

[Joshua Netterfield]

Created attachment 161590 [details]
Patch

[Joshua Netterfield]

Created attachment 161594 [details]
Patch

[Build Bot]

Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13693950

[WebKit Review Bot]

Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13688981

[Alexandru Chiculita]

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

Thanks! Looks better. Still some comments on the tests.

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:64
> +        if (!from.size() || !to.size()) {

I think there's no way we could have empty transform operations here. The parser takes care of that, so maybe an assert would be good enough.

> LayoutTests/css3/filters/custom/custom-filter-animation.html:151
> +            from { -webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform rotateX(0deg) scale3d(1, 1, 1)); }

I wish we had a test with two different transform attributes like:
-webkit-filter: custom(url(../resources/vertex-transform-parameter.vs), transform1 rotateX(0deg), transform2 rotateY(90deg));

> LayoutTests/css3/filters/resources/custom-filter-parser.js:157
> +            if (m.ahead().isA("(") && token.value != "matrix" && token.value != "matrix3d")

Why isn't parseFunction good enough to parse the matrix?

> LayoutTests/css3/filters/resources/custom-filter-parser.js:178
> +        } else if (token.isA(transform)) {

Why do you need this? You should not be needed to parse the parameter names.

[Early Warning System Bot]

Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13682963

[Early Warning System Bot]

Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13693969

[Peter Beverloo (cr-android ews)]

Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13681933

[Build Bot]

Comment on attachment 161594 [details]
Patch

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

[Alexandru Chiculita]

Created attachment 163474 [details]
Patch V1

[WebKit Review Bot]

Attachment 163474 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/anim..." exit_code: 1
Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:29:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexandru Chiculita]

Created attachment 163478 [details]
Patch V2

Fixed style.

[Dean Jackson]

Comment on attachment 163478 [details]
Patch V2

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

> LayoutTests/css3/filters/custom/custom-filter-transforms-animation.html:45
> +        @-webkit-keyframes custom-mix-multi-transform-anim {
> +            from { -webkit-filter: custom(url(no-shader-needed.vs), transform1 scale(0), transform2 scale(100) rotate(0deg)); }
> +            to   { -webkit-filter: custom(url(no-shader-needed.vs), transform1 scale(30), transform2 rotate(0deg) scale(100)); }
> +        }

Could you add two by two more tests (a) where from and to have different numbers of operations (and reverse) and (b) where one of from and to are empty/not-present?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:151
> -            RefPtr<FilterOperation> blendedOp = toOp ? toOp->blend(fromOp.get(), progress) : (fromOp ? fromOp->blend(0, progress, true) : PassRefPtr<FilterOperation>(0));
> +            RefPtr<FilterOperation> blendedOp = toOp ? blendFunc(anim, fromOp.get(), toOp.get(), progress) : (fromOp ? blendFunc(anim, 0, fromOp.get(), progress, true) : PassRefPtr<FilterOperation>(0));

It seems we can replace the PassRefPtr<FilterOperation>(0) with just 0 here now (i'm not sure what the history here is, although it was also in the code removed above - PassRefPtr<TransformOperation>(0)).

> Source/WebCore/platform/graphics/filters/CustomFilterTransformParameter.h:61
> +        RefPtr<CustomFilterTransformParameter> ret = CustomFilterTransformParameter::create(name());

Nit: need a better name than "ret"

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:78
> +        RefPtr<TransformOperation> fromOp = (i < fromSize) ? from.operations()[i].get() : 0;
> +        RefPtr<TransformOperation> toOp = (i < toSize) ? operations()[i].get() : 0;
> +        RefPtr<TransformOperation> blendedOp = toOp ? toOp->blend(fromOp.get(), progress) : (fromOp ? fromOp->blend(0, progress, true) : PassRefPtr<TransformOperation>(0));

Nit: I prefer long variable names fromOp -> fromOperation (there are a few places in other files in the patch that have this too)

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:83
> +            if (progress > 0.5)

At some point I'd like to work out if this is the right thing to do (it's copied from the CSS animation blends of filters and transforms), and maybe add a comment explaining it :) I can't remember where it comes from.

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:93
> +TransformOperations TransformOperations::blendUsingMatricesInterpolation(const TransformOperations& from, double progress, const LayoutSize& size) const

This should be blendByUsingMatrixInterpolation

> Source/WebCore/platform/graphics/transforms/TransformOperations.cpp:99
> +    TransformationMatrix fromT;
> +    TransformationMatrix toT;

Nit: I prefer long variable names like fromT -> fromTransform

> Source/WebCore/platform/graphics/transforms/TransformOperations.h:77
> +    TransformOperations blendByMatchingOperations(const TransformOperations&, const double& progress) const;
> +    TransformOperations blendUsingMatricesInterpolation(const TransformOperations& from, double progress, const LayoutSize&) const;
> +    TransformOperations blend(const TransformOperations& from, double progress, const LayoutSize&) const;

Nit: you're defining "from" on 2 out of 3 of these.

[Alexandru Chiculita]

Thanks Dean! I will take a look tomorrow.

[Alexandru Chiculita]

Landed in http://trac.webkit.org/changeset/128380 .