Bug 71406 – [CSS Shaders] Implement CSS Animations and Transitions for CSS Shaders

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

We need a new PropertyWrapper to transition shader parameters

[Radar WebKit Bug Importer]

<rdar://problem/10386040>

[Alexandru Chiculita]

Created attachment 138915 [details]
Code

Saving current work on this bug. No tests yet and no code cleanup.

[Alexandru Chiculita]

Created attachment 139088 [details]
Patch V2

[Alexandru Chiculita]

Created attachment 139094 [details]
Patch V3

Also added a test that checks that the mesh and other custom filter attribute cannot animate.

[Dean Jackson]

Comment on attachment 139094 [details]
Patch V3

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

Minor comments.

> LayoutTests/css3/filters/custom/custom-filter-animation.html:114
> +            // CSS Shaders do not get good FPS in debug mode, so the tolerance needs to be higher.

I think we should have a bug open to reduce the tolerance in the future.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:2
> +(function() {
> +

We don't normally follow this style in WebKit, as far as I know. I understand it's common on the Web. This isn't a big deal to me.

In general I'd like to see some comments in this file to explain what the various parts do. I expect other tests might want to copy this for their own use.

> LayoutTests/css3/filters/resources/custom-filter-parser.js:127
> +            else if (m.ahead().isA(",", endToken))
> +                result.push({
> +                    type: "keyword", 
> +                    value: m.skip().value
> +                });

suggest {} here

> Source/WebCore/ChangeLog:11
> +         between the "from" and "to" states.

indentation.

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:43
> +bool equalCustomFilterParameters(const CustomFilterParameterList& listA, const CustomFilterParameterList& listB)

I suggest customFilterParametersEqual

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:49
> +        if (listA.at(i).get() != listB.at(i).get() 
> +            && !(*listA.at(i).get() == *listB.at(i).get()))

Any reason to not use != here?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:62
> +    for (unsigned i = 1; i < parameters.size(); ++i) {
> +        // Break for equal or not-sorted parameters.
> +        if (!codePointCompareLessThan(parameters.at(i - i)->name(), parameters.at(i)->name()))
> +            return false;
> +    }

I'm confused. at(i - i) ?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:71
> +    ASSERT(checkCustomFilterParametersOrder(fromList));
> +    ASSERT(checkCustomFilterParametersOrder(toList));

Doesn't this need guarding? It's #ifndef above.

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:107
> +    ASSERT(checkCustomFilterParametersOrder(m_parameters));

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:117
> +    // FIXME: There's no way to decide what is the "passthrough filter" for shaders using the current CSS Syntax.
> +    // https://bugs.webkit.org/show_bug.cgi?id=84903

Link to spec bug on w3.org too?

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:122
> +    if (!(*m_program.get() == *fromOp->m_program.get())

Ditto on the != question

> Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:46
> +bool equalCustomFilterParameters(const CustomFilterParameterList& listA, const CustomFilterParameterList& listB);

I guess we don't really need the parameter names here, following WK style.

> Source/WebCore/rendering/style/StyleCustomFilterProgram.h:125
> +        return cachedVertexShader() == other->cachedVertexShader()
> +            && cachedFragmentShader() == other->cachedFragmentShader();

Suggest just one line here.

[Alexandru Chiculita]

(In reply to comment #5)
> (From update of attachment 139094 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139094&action=review
> 
Thanks for reviewing this. I will post a new patch soon.

> Minor comments.
> 
> > LayoutTests/css3/filters/custom/custom-filter-animation.html:114
> > +            // CSS Shaders do not get good FPS in debug mode, so the tolerance needs to be higher.
> 
> I think we should have a bug open to reduce the tolerance in the future.
Ok. Will add one.

> 
> > LayoutTests/css3/filters/resources/custom-filter-parser.js:2
> > +(function() {
> > +
> 
> We don't normally follow this style in WebKit, as far as I know. I understand it's common on the Web. This isn't a big deal to me.
Ok. I will remove it. I didn't want to pollute the "window" global object.

> 
> In general I'd like to see some comments in this file to explain what the various parts do. I expect other tests might want to copy this for their own use.

I will add more comments.

> 
> > LayoutTests/css3/filters/resources/custom-filter-parser.js:127
> > +            else if (m.ahead().isA(",", endToken))
> > +                result.push({
> > +                    type: "keyword", 
> > +                    value: m.skip().value
> > +                });
> 
> suggest {} here
> 
> > Source/WebCore/ChangeLog:11
> > +         between the "from" and "to" states.
> 
> indentation.
Ok.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:43
> > +bool equalCustomFilterParameters(const CustomFilterParameterList& listA, const CustomFilterParameterList& listB)
> 
> I suggest customFilterParametersEqual
Ok.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:49
> > +        if (listA.at(i).get() != listB.at(i).get() 
> > +            && !(*listA.at(i).get() == *listB.at(i).get()))
> 
> Any reason to not use != here?
It complained about not having one. I guess I can implement the != based on the == operator.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:62
> > +    for (unsigned i = 1; i < parameters.size(); ++i) {
> > +        // Break for equal or not-sorted parameters.
> > +        if (!codePointCompareLessThan(parameters.at(i - i)->name(), parameters.at(i)->name()))
> > +            return false;
> > +    }
> 
> I'm confused. at(i - i) ?
This is a typo. Sorry, it should be i - 1.
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:71
> > +    ASSERT(checkCustomFilterParametersOrder(fromList));
> > +    ASSERT(checkCustomFilterParametersOrder(toList));
> 
> Doesn't this need guarding? It's #ifndef above.
I thought that ASSERT will guard it by default. That's why I ifndef it, because otherwise there will be no reference to it.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:107
> > +    ASSERT(checkCustomFilterParametersOrder(m_parameters));
> 
> ditto
> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:117
> > +    // FIXME: There's no way to decide what is the "passthrough filter" for shaders using the current CSS Syntax.
> > +    // https://bugs.webkit.org/show_bug.cgi?id=84903
> 
> Link to spec bug on w3.org too?
Ok.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:122
> > +    if (!(*m_program.get() == *fromOp->m_program.get())
> 
> Ditto on the != question
Ok.

> 
> > Source/WebCore/platform/graphics/filters/CustomFilterOperation.h:46
> > +bool equalCustomFilterParameters(const CustomFilterParameterList& listA, const CustomFilterParameterList& listB);
> 
> I guess we don't really need the parameter names here, following WK style.
Ok.
> 
> > Source/WebCore/rendering/style/StyleCustomFilterProgram.h:125
> > +        return cachedVertexShader() == other->cachedVertexShader()
> > +            && cachedFragmentShader() == other->cachedFragmentShader();
> 
> Suggest just one line here.

Ok.

[Alexandru Chiculita]

Created attachment 139250 [details]
Patch V4

[Dean Jackson]

Comment on attachment 139250 [details]
Patch V4

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

> LayoutTests/css3/filters/custom/custom-filter-animation.html:54
> +    
> +        #custom-from-no-params-box {
> +            -webkit-animation: custom-from-no-params-anim 2s linear
> +        }
> +    
> +        #custom-to-no-params-box {
> +            -webkit-animation: custom-to-no-params-anim 2s linear
> +        }
> +        
> +        #custom-mix-attributes-box {
> +            -webkit-animation: custom-mix-attributes-anim 2s linear
> +        }
> +    
> +        #custom-from-diff-params-box {
> +            -webkit-animation: custom-from-diff-params-anim 2s linear
> +        }
> +    
> +        #custom-to-diff-params-box {
> +            -webkit-animation: custom-to-diff-params-anim 2s linear
> +        }
> +    
> +        #custom-mix-params-box {
> +            -webkit-animation: custom-mix-params-anim 2s linear
> +        }
> +        
> +        #custom-mix-numbers-box {
> +            -webkit-animation: custom-mix-numbers-anim 2s linear
> +        }

Could you add semicolons to the end of the rules?

[Dean Jackson]

(In reply to comment #6)

> > > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:71
> > > +    ASSERT(checkCustomFilterParametersOrder(fromList));
> > > +    ASSERT(checkCustomFilterParametersOrder(toList));
> > 
> > Doesn't this need guarding? It's #ifndef above.
> I thought that ASSERT will guard it by default. That's why I ifndef it, because otherwise there will be no reference to it.

Cool. I hadn't thought about that :)

[Alexandru Chiculita]

(In reply to comment #9)
> (In reply to comment #6)
> 
> > > > Source/WebCore/platform/graphics/filters/CustomFilterOperation.cpp:71
> > > > +    ASSERT(checkCustomFilterParametersOrder(fromList));
> > > > +    ASSERT(checkCustomFilterParametersOrder(toList));
> > > 
> > > Doesn't this need guarding? It's #ifndef above.
> > I thought that ASSERT will guard it by default. That's why I ifndef it, because otherwise there will be no reference to it.
> 
> Cool. I hadn't thought about that :)

Yes, I found that others are using #if !ASSERT_DISABLED so I used that instead.

[Alexandru Chiculita]

(In reply to comment #8)
> (From update of attachment 139250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139250&action=review
> 
> > LayoutTests/css3/filters/custom/custom-filter-animation.html:54
> > +    
> > +        #custom-from-no-params-box {
> > +            -webkit-animation: custom-from-no-params-anim 2s linear
> > +        }
> > +    
> > +        #custom-to-no-params-box {
> > +            -webkit-animation: custom-to-no-params-anim 2s linear
> > +        }
> > +        
> > +        #custom-mix-attributes-box {
> > +            -webkit-animation: custom-mix-attributes-anim 2s linear
> > +        }
> > +    
> > +        #custom-from-diff-params-box {
> > +            -webkit-animation: custom-from-diff-params-anim 2s linear
> > +        }
> > +    
> > +        #custom-to-diff-params-box {
> > +            -webkit-animation: custom-to-diff-params-anim 2s linear
> > +        }
> > +    
> > +        #custom-mix-params-box {
> > +            -webkit-animation: custom-mix-params-anim 2s linear
> > +        }
> > +        
> > +        #custom-mix-numbers-box {
> > +            -webkit-animation: custom-mix-numbers-anim 2s linear
> > +        }
> 
> Could you add semicolons to the end of the rules?

Sure, will add it before committing.

[Alexandru Chiculita]

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

[Jer Noble]

(In reply to comment #12)
> Landed in http://trac.webkit.org/changeset/115487 .

The test LayoutTests/css3/filters/custom/custom-filter-animation.html has been broken since it was added.  Bug #84813 was filed to track this error.

[Jer Noble]

(In reply to comment #13)
> (In reply to comment #12)
> > Landed in http://trac.webkit.org/changeset/115487 .
> 
> The test LayoutTests/css3/filters/custom/custom-filter-animation.html has been broken since it was added.  Bug #84813 was filed to track this error.

Apologies, the correct bug is bug #91769.