Bug 94980 - [CSS Shaders] Implement transform parameter animations for CSS Custom Filters
Summary: [CSS Shaders] Implement transform parameter animations for CSS Custom Filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandru Chiculita
URL:
Keywords:
Depends on:
Blocks: 96437
  Show dependency treegraph
 
Reported: 2012-08-24 16:36 PDT by Alexandru Chiculita
Modified: 2012-09-12 16:45 PDT (History)
9 users (show)

See Also:


Attachments
Patch (42.39 KB, patch)
2012-08-29 13:37 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (44.66 KB, patch)
2012-08-29 14:08 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (40.77 KB, patch)
2012-08-29 14:16 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (35.89 KB, patch)
2012-08-30 16:16 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (36.08 KB, patch)
2012-08-30 16:52 PDT, Joshua Netterfield
no flags Details | Formatted Diff | Diff
Patch (36.24 KB, patch)
2012-08-30 17:23 PDT, Joshua Netterfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch V1 (29.01 KB, patch)
2012-09-11 16:30 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V2 (29.03 KB, patch)
2012-09-11 16:36 PDT, Alexandru Chiculita
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2012-08-24 16:36:59 PDT
Custom filters allow parameters with transform types. This bug will implement the animations support for that type.
Comment 1 Joshua Netterfield 2012-08-29 13:37:10 PDT
Created attachment 161300 [details]
Patch
Comment 2 Joshua Netterfield 2012-08-29 13:38:36 PDT
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.
Comment 3 Joshua Netterfield 2012-08-29 14:08:55 PDT
Created attachment 161310 [details]
Patch
Comment 4 Joshua Netterfield 2012-08-29 14:09:06 PDT
Rebased.
Comment 5 Joshua Netterfield 2012-08-29 14:16:12 PDT
Created attachment 161315 [details]
Patch
Comment 6 Alexandru Chiculita 2012-08-29 14:32:59 PDT
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.
Comment 7 Joshua Netterfield 2012-08-29 14:48:16 PDT
The makes a lot more sense. Will do.
Comment 8 Peter Beverloo (cr-android ews) 2012-08-29 16:05:17 PDT
Comment on attachment 161315 [details]
Patch

Attachment 161315 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13693444
Comment 9 Build Bot 2012-08-29 16:34:27 PDT
Comment on attachment 161315 [details]
Patch

Attachment 161315 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13693475
Comment 10 WebKit Review Bot 2012-08-29 19:39:17 PDT
Comment on attachment 161315 [details]
Patch

Attachment 161315 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13682478
Comment 11 Joshua Netterfield 2012-08-30 16:16:32 PDT
Created attachment 161577 [details]
Patch
Comment 12 kov's GTK+ EWS bot 2012-08-30 16:46:02 PDT
Comment on attachment 161577 [details]
Patch

Attachment 161577 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13687882
Comment 13 WebKit Review Bot 2012-08-30 16:52:30 PDT
Comment on attachment 161577 [details]
Patch

Attachment 161577 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13688953
Comment 14 Joshua Netterfield 2012-08-30 16:52:39 PDT
Created attachment 161590 [details]
Patch
Comment 15 Joshua Netterfield 2012-08-30 17:23:32 PDT
Created attachment 161594 [details]
Patch
Comment 16 Build Bot 2012-08-30 17:54:29 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13693950
Comment 17 WebKit Review Bot 2012-08-30 18:12:17 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13688981
Comment 18 Alexandru Chiculita 2012-08-30 18:14:29 PDT
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.
Comment 19 Early Warning System Bot 2012-08-30 18:38:07 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13682963
Comment 20 Early Warning System Bot 2012-08-30 19:02:09 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13693969
Comment 21 Peter Beverloo (cr-android ews) 2012-08-30 19:34:19 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13681933
Comment 22 Build Bot 2012-08-31 14:25:04 PDT
Comment on attachment 161594 [details]
Patch

Attachment 161594 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13720323
Comment 23 Alexandru Chiculita 2012-09-11 16:30:50 PDT
Created attachment 163474 [details]
Patch V1
Comment 24 WebKit Review Bot 2012-09-11 16:33:52 PDT
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.
Comment 25 Alexandru Chiculita 2012-09-11 16:36:23 PDT
Created attachment 163478 [details]
Patch V2

Fixed style.
Comment 26 Dean Jackson 2012-09-11 17:18:42 PDT
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.
Comment 27 Alexandru Chiculita 2012-09-11 17:33:20 PDT
Thanks Dean! I will take a look tomorrow.
Comment 28 Alexandru Chiculita 2012-09-12 16:45:50 PDT
Landed in http://trac.webkit.org/changeset/128380 .