Bug 76253 - CSS Shaders: Parse float parameters for the custom() filter syntax
Summary: CSS Shaders: Parse float parameters for the custom() filter syntax
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: https://dvcs.w3.org/hg/FXTF/raw-file/...
Keywords:
Depends on:
Blocks: 71395
  Show dependency treegraph
 
Reported: 2012-01-13 01:20 PST by Alexandru Chiculita
Modified: 2012-01-18 07:42 PST (History)
5 users (show)

See Also:


Attachments
Saved work (32.31 KB, patch)
2012-01-13 06:33 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V1 (98.80 KB, patch)
2012-01-16 05:17 PST, Alexandru Chiculita
cmarrin: review-
Details | Formatted Diff | Diff
Patch V2 (92.29 KB, patch)
2012-01-17 05:03 PST, Alexandru Chiculita
zimmermann: review-
zimmermann: commit-queue-
Details | Formatted Diff | Diff
Patch V3 (108.05 KB, patch)
2012-01-18 05:49 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch V4 (108.02 KB, patch)
2012-01-18 05:56 PST, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch for landing (109.88 KB, patch)
2012-01-18 06:21 PST, Alexandru Chiculita
no flags 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-01-13 01:20:15 PST
Parse float paramaters for CSS Filters custom() syntax.

.shaded {
    filter: custom(
                   url(distort.vs) url(tint.fs), 
                   distortAmount 0.5, lightVector 1.0 1.0 0.0
                );
}
Comment 1 Alexandru Chiculita 2012-01-13 06:33:13 PST
Created attachment 122423 [details]
Saved work

Saving my work on this bug. The code should be complete, but not yet ready for review because there are no tests yet.

I need to create tests for:
1. computed-style float, vec2, vec3, vec4
2. painting with float, vec2, vec3, vec4
3. [done] css parsing for float, vec2, vec3, vec4 was already done
Comment 2 Alexandru Chiculita 2012-01-16 05:17:07 PST
Created attachment 122618 [details]
Patch V1
Comment 3 Chris Marrin 2012-01-16 15:43:44 PST
Comment on attachment 122618 [details]
Patch V1

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

r- because you should really just have a single Vector parameter class with a variable number of elements rather than a bunch of vector classes of different sizes.

> Source/WebCore/css/CSSStyleSelector.cpp:5281
> +            // TODO: Implement other parameters types parsing.

This should be FIXME

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:85
> +class CustomFilterNumberVectorOf2Parameter : public CustomFilterParameter {

It would be much better to just have a single Vector class that can hold any number of values. Then you could use it for the current vector type's you've defined as well as all the matrix types. It would be way less code with not much runtime cost (and extra pointer and length field). Given that we don't expect thousands of custom filters on the page or thousands of parameters per custom filter, this cost seems reasonable.

I think it's reasonable to keep CustomFilterNumberParameter (as opposed to just using a vector of 1 number) if you want. But even getting rid of that wouldn't be bad.
Comment 4 Alexandru Chiculita 2012-01-17 05:03:53 PST
Created attachment 122751 [details]
Patch V2

Thanks for the review! 

I've updated the patch. Now I only have one CustomFilterNumberParameter class for all the float values with a Vector<double, 4> inside.
I've also removed the HashMap and used a simple Vector. I've enforced a sorted vector in CustomFilterOperation, so that blending of two CustomFilterOperations can be done fast.
Comment 5 Nikolas Zimmermann 2012-01-18 01:46:13 PST
Comment on attachment 122751 [details]
Patch V2

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

This is completly fine, but it needs refactoring! The code easily grows to be unreadable, this way. Please use smaller helper functions, to gain readability. r- for now:

> LayoutTests/css3/filters/effect-custom-combined-missing.html:18
> +                // We need to run the tests after the downloading succeeded.
> +                // Using a timer is not ideal, but there seems to be no better options.
> +                setTimeout(function() {

As discussed on IRC, this is not a good idea. But there are already existing tests exhibing the same problem.
http://trac.webkit.org/changeset/101858 fixed the same issue for SVG Fonts, we may reuse the idea behind this for shaders as well, and ilsten to the documents load event here.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:13
> +

One newline too much.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:15
> +    float perspective = - 1.0 / p;

I'd avoid the local.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:25
> +    return mat4(
> +     1.0, 0.0, 0.0, 0.0, 

Indentation.

> LayoutTests/css3/filters/resources/vertex-offset-parameters.vs:63
> +    gl_Position = u_projectionMatrix * perspectiveMatrix(perspective) * translateMatrix(vertex_offset) * rotateMatrix(rotate * 3.14 / 180.0) * a_position;

Hm, no M_PI available.... Is 2 digit precision enough? I guess it doesn't matter.

> Source/WebCore/ChangeLog:16
> +        * WebCore.xcodeproj/project.pbxproj:

What about all the other build systems? You still should add the headers, where needed, even if there's no cpp file to add.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:835
> +            if (parameters.size()) {

Early exit, if parameters.size is 0.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:838
> +                for (CustomFilterParameterList::const_iterator iter = parameters.begin(), end = parameters.end(); iter != end; ++iter) {

It's faster to traverse a vector by index, instead of using the iterators.
size_t parametersSize = parameter.size();
for (size_t i = 0; i < parametersSize; ++i) {
     CustomFilterParameter* parameter = parameters[i].get();
...

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:846
> +                    RefPtr<CSSValue> parameterCSSValue;
> +                    
> +                    switch (parameter->parameterType()) {
> +                    case CustomFilterParameter::NUMBER: {
> +                        CustomFilterNumberParameter* numberParameter = static_cast<CustomFilterNumberParameter*>(parameter);
> +                        RefPtr<CSSValueList> numberParameterValue = CSSValueList::createSpaceSeparated();

That cries for a helper function, that returns PassRefPtr<CSSValue> to construct the "parameterCSSValue" out of a CustomFilterParameter.
If you add more cases to the enum, it will get unreadable otherwise.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:852
> +                    default:

I'd rather avoid the default: here, so any missing value from the enum can be detected at compile-time.

> Source/WebCore/css/CSSStyleSelector.cpp:5228
> +            // having just two big values: list of shaders and list of parameters.

two big values? big?

> Source/WebCore/css/CSSStyleSelector.cpp:5240
> +    if (parametersValue) {

Early exit, if parametersValue is 0.

> Source/WebCore/css/CSSStyleSelector.cpp:5246
> +        CSSValueListIterator parameterIterator(parametersValue);
> +        for (; parameterIterator.hasMore(); parameterIterator.advance()) {
> +            if (!parameterIterator.value()->isValueList())
> +                return 0;
> +            CSSValueListIterator iterator(parameterIterator.value());
> +            if (!iterator.isPrimitiveValue())

This whole procedure could be moved somewhere else, in a static inline helper function.

> Source/WebCore/css/CSSStyleSelector.cpp:5283
> +                if (firstPrimitiveValue->primitiveType() == CSSPrimitiveValue::CSS_NUMBER) {
> +                    RefPtr<CustomFilterNumberParameter> numberParameter = CustomFilterNumberParameter::create(name);

Same here, the creation of CustomFilterXXXParameters should be in a seperated method, to make this code more readable.

> Source/WebCore/css/CSSValueList.h:98
> +    bool hasAdvanced() { return m_position; }

Why not just use index() ?

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:44
> +        NUMBER

Maybe add a FIXME here about the still missing types

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:60
> +    

Newline too much.

> Source/WebCore/platform/graphics/filters/CustomFilterParameter.h:65
> +class CustomFilterNumberParameter : public CustomFilterParameter {

We normally only use one class per file.

> Source/WebCore/platform/graphics/filters/CustomFilterShader.h:67
> +    int uniformLocationByName(const String& name);

s/ name//

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:207
> +    for (CustomFilterParameterList::iterator iter = m_parameters.begin(), end = m_parameters.end(); iter != end; ++iter) {

Traverse Vector by index.

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:213
> +        switch (parameter->parameterType()) {
> +        case CustomFilterParameter::NUMBER: {

This could again, go into another function, that takes both a CustomFilterNumberParameter* and operates on m_context.
Comment 6 Alexandru Chiculita 2012-01-18 05:49:06 PST
Created attachment 122906 [details]
Patch V3

Thanks again for the feedback. I think I've covered all of it, except for the setTimeout in the test case. For that I've added https://bugs.webkit.org/show_bug.cgi?id=76535 to track that one.
Comment 7 WebKit Review Bot 2012-01-18 05:52:36 PST
Attachment 122906 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1

Source/WebCore/ChangeLog:27:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:35:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:72:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Alexandru Chiculita 2012-01-18 05:56:25 PST
Created attachment 122909 [details]
Patch V4

Removing tabs from changelog file.
Comment 9 Nikolas Zimmermann 2012-01-18 06:11:50 PST
Comment on attachment 122909 [details]
Patch V4

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

Looks great to me. Much better readable, eh? :-)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:730
> +    // FIXME: Add here computed style for the other types: boolean, transform, matrix, texture.

I'd ASSERT(parameter) here as well.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:861
> +            const CustomFilterParameterList& parameters = customOperation->parameters();
> +            if (!parameters.size())
> +                break;
> +                
> +            RefPtr<CSSValueList> parametersCSSValue = CSSValueList::createCommaSeparated();
> +            
> +            size_t parametersSize = parameters.size();

I'd write this as:
size_t parametersSize = parameters.size();
if (!parametersSize)
    break;
RefPtr<CSSValueList> ....

you can also omit the extra newline.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:871
> +            

One newline too much.
Comment 10 Alexandru Chiculita 2012-01-18 06:21:37 PST
Created attachment 122915 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-01-18 07:42:52 PST
Comment on attachment 122915 [details]
Patch for landing

Clearing flags on attachment: 122915

Committed r105276: <http://trac.webkit.org/changeset/105276>
Comment 12 WebKit Review Bot 2012-01-18 07:42:58 PST
All reviewed patches have been landed.  Closing bug.