Bug 71406 - [CSS Shaders] Implement CSS Animations and Transitions for CSS Shaders
Summary: [CSS Shaders] Implement CSS Animations and Transitions for CSS Shaders
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: InRadar
Depends on:
Blocks: 71392
  Show dependency treegraph
 
Reported: 2011-11-02 14:25 PDT by Dean Jackson
Modified: 2012-07-19 12:18 PDT (History)
6 users (show)

See Also:


Attachments
Code (10.66 KB, patch)
2012-04-25 18:13 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V2 (28.39 KB, patch)
2012-04-26 15:56 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V3 (30.66 KB, patch)
2012-04-26 16:25 PDT, Alexandru Chiculita
dino: review-
Details | Formatted Diff | Diff
Patch V4 (33.30 KB, patch)
2012-04-27 12:44 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 Dean Jackson 2011-11-02 14:25:39 PDT
We need a new PropertyWrapper to transition shader parameters
Comment 1 Radar WebKit Bug Importer 2011-11-02 14:26:08 PDT
<rdar://problem/10386040>
Comment 2 Alexandru Chiculita 2012-04-25 18:13:37 PDT
Created attachment 138915 [details]
Code

Saving current work on this bug. No tests yet and no code cleanup.
Comment 3 Alexandru Chiculita 2012-04-26 15:56:38 PDT
Created attachment 139088 [details]
Patch V2
Comment 4 Alexandru Chiculita 2012-04-26 16:25:05 PDT
Created attachment 139094 [details]
Patch V3

Also added a test that checks that the mesh and other custom filter attribute cannot animate.
Comment 5 Dean Jackson 2012-04-27 11:21:49 PDT
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.
Comment 6 Alexandru Chiculita 2012-04-27 11:36:22 PDT
(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.
Comment 7 Alexandru Chiculita 2012-04-27 12:44:00 PDT
Created attachment 139250 [details]
Patch V4
Comment 8 Dean Jackson 2012-04-27 13:07:20 PDT
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?
Comment 9 Dean Jackson 2012-04-27 13:08:22 PDT
(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 :)
Comment 10 Alexandru Chiculita 2012-04-27 13:09:19 PDT
(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.
Comment 11 Alexandru Chiculita 2012-04-27 13:09:38 PDT
(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.
Comment 12 Alexandru Chiculita 2012-04-27 13:35:48 PDT
Landed in http://trac.webkit.org/changeset/115487 .
Comment 13 Jer Noble 2012-07-19 11:36:40 PDT
(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.
Comment 14 Jer Noble 2012-07-19 12:18:18 PDT
(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.