Bug 95223 - Use arrays in shaders
Summary: Use arrays in shaders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Michelangelo De Simone
URL:
Keywords:
Depends on: 94226
Blocks: 71392 71395 96437
  Show dependency treegraph
 
Reported: 2012-08-28 11:32 PDT by Michelangelo De Simone
Modified: 2012-09-14 10:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (31.86 KB, patch)
2012-09-11 14:41 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (30.38 KB, patch)
2012-09-11 15:59 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (30.54 KB, patch)
2012-09-11 18:37 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (30.61 KB, patch)
2012-09-12 10:55 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff
Patch (51.43 KB, patch)
2012-09-13 13:14 PDT, Michelangelo De Simone
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michelangelo De Simone 2012-08-28 11:32:31 PDT
As per summary the array passed via array() shall be used within shaders.
Comment 1 Michelangelo De Simone 2012-09-11 14:41:57 PDT
Created attachment 163445 [details]
Patch
Comment 2 Michelangelo De Simone 2012-09-11 14:44:15 PDT
check-webkit-style reports the following false positive:

Source/WebCore/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]

Bug #95266 already addresses this. The patch will be landed manually.
Comment 3 WebKit Review Bot 2012-09-11 14:47:53 PDT
Attachment 163445 [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/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Alexandru Chiculita 2012-09-11 15:23:59 PDT
Comment on attachment 163445 [details]
Patch

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

Looks good. I have some minor comments.

> Source/WebCore/ChangeLog:11
> +        Values within array() are comma-separated even though the actual revision

Is there an issue added on the spec? If not we should have one and link it here.

> Source/WebCore/css/StyleResolver.cpp:5308
> +    if (parameterValue->isWebKitCSSArrayFunctionValue())

The fact that parameterValue is a value list which is also happens to be a CSSArrayFunctionValue is very subtle. 

I would at the very least add a comment before the check for the value list, saying that all the parameter values have the value list as the base class. Also move the check for the array function before the transforms.

> Source/WebCore/platform/graphics/filters/CustomFilterArrayParameter.h:64
> +    {
> +        if (!from || !isSameType(*from))
> +            return this;
> +
> +        const CustomFilterArrayParameter* fromArray = static_cast<const CustomFilterArrayParameter*>(from);
> +        if (size() != fromArray->size())
> +            return this;
> +
> +        RefPtr<CustomFilterArrayParameter> result = CustomFilterArrayParameter::create(name());
> +        for (size_t i = 0; i < size(); ++i)
> +            result->addValue(WebCore::blend(fromArray->valueAt(i), valueAt(i), progress));
> +
> +        return result.release();

You didn't add any test for this method. I have another patch that fixes the filter animation test framework to handle functions: 
https://bugs.webkit.org/show_bug.cgi?id=94980

I think you will need to wait for that one first. So extract it from this patch and fill a different bug for it (add a fixme here and "return this").

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-45
> -    

Can you revert these changes? The whole file is untouched.

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-50
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-63
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-71
> -    

ditto

> Source/WebCore/platform/graphics/filters/CustomFilterNumberParameter.h:-77
> -    

ditto

> Source/WebCore/platform/graphics/filters/FECustomFilter.cpp:267
> +    unsigned parameterSize = arrayParameter->size();

nit: you could put this on the same line with the for.

for (unsigned i = 0, size = arrayParameter->size(); i < size; ++i)

> LayoutTests/css3/filters/custom/custom-filter-array-expected.html:15
> +        <script>
> +            if (window.testRunner)
> +                window.testRunner.waitUntilDone();
> +
> +            function runTest()
> +            {
> +                // We need to run the tests after the downloading succeeded.
> +                if (window.testRunner)
> +                    window.testRunner.notifyDone();
> +            }
> +        </script>

You don't need this in the ref html.

> LayoutTests/css3/filters/custom/custom-filter-array.html:28
> +    <body onload="runTest()">

Add a comment explaining what happens and what I should expect.
Comment 5 Michelangelo De Simone 2012-09-11 15:59:32 PDT
Created attachment 163465 [details]
Patch
Comment 6 WebKit Review Bot 2012-09-11 16:04:56 PDT
Attachment 163465 [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/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Alexandru Chiculita 2012-09-11 16:49:00 PDT
Comment on attachment 163465 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:5298
> +    // All functions (eg.: array(), mix(), ...) are ValueLists.

Not all the possible values of the paramterValue are functions. That's why I wanted a comment here. The idea was that even simple number parameters are encapsulated in CSSValueLists. 

What about using the following comment instead?

// Number parameters are wrapped inside a CSSValueList and all the other functions values inherit from CSSValueList.

> Source/WebCore/css/StyleResolver.cpp:5310
> +    // At this point we assume that we're dealing with a transform and,
> +    // therefore, we check whether its first value is a valid transform.

Hm, I think this would read better as: "If the first value of the list is a transform function, then we could safely assume that all the remaining items are transforms. parseCustomFilterTransformParameter will return 0 if that assumption is incorrect."
Comment 8 Michelangelo De Simone 2012-09-11 18:37:15 PDT
Created attachment 163491 [details]
Patch
Comment 9 WebKit Review Bot 2012-09-11 18:43:13 PDT
Attachment 163491 [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/css/StyleResolver.cpp:5253:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Build Bot 2012-09-11 19:47:21 PDT
Comment on attachment 163491 [details]
Patch

Attachment 163491 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13821578
Comment 11 Michelangelo De Simone 2012-09-12 10:07:06 PDT
I guess the(In reply to comment #10)

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

I guess there was something not working on the Mac bot.:-/
Comment 12 Michelangelo De Simone 2012-09-12 10:55:33 PDT
Created attachment 163655 [details]
Patch
Comment 13 WebKit Review Bot 2012-09-12 10:58:23 PDT
Attachment 163655 [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/css/StyleResolver.cpp:5255:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Dean Jackson 2012-09-12 16:03:31 PDT
Comment on attachment 163655 [details]
Patch

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

Looks good - only a few small changes.

But I think we need more tests:

- testing other valid and invalid arrays e.g. array(), array(1,), array(NaN), array(1, 2, foo), array(1, 2, translate(10)), array(with many many values) etc
- testing the transform fallback code

> Source/WebCore/ChangeLog:12
> +        Values within array() are comma-separated; specs will be updated
> +        accordingly: https://www.w3.org/Bugs/Public/show_bug.cgi?id=18839

"specs" -> "The specification"

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:779
> +    for (unsigned i = 0; i < arrayParameter->size(); ++i)

Might as well do length = arrayParameter->size(); i < length

> Source/WebCore/css/StyleResolver.cpp:5258
> +    for (unsigned i = 0; i < values->length(); ++i) {

Ditto here.

> LayoutTests/css3/filters/resources/fragment-color.fs:6
> +uniform float coolArray[4];
> +
> +void main()
> +{
> +    gl_FragColor = vec4(coolArray[0], coolArray[1], coolArray[2], coolArray[3]);

Should probably call this colorArray[4]. My first instinct was that this shader was "cooling" the saturation.
Comment 15 Alexandru Chiculita 2012-09-12 16:17:54 PDT
(In reply to comment #14)
> (From update of attachment 163655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163655&action=review
> 
> Looks good - only a few small changes.
> 
> But I think we need more tests:
> 
> - testing other valid and invalid arrays e.g. array(), array(1,), array(NaN), array(1, 2, foo), array(1, 2, translate(10)), array(with many many values) etc
> - testing the transform fallback code

I think we already have a couple of those covered in the invalid parsing test file.

http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js

Some of the things not covered there are:
- array(NaN)
- arrays with many values

Also I didn't see any test to check for floating point numbers.
Comment 16 Michelangelo De Simone 2012-09-13 13:14:18 PDT
Created attachment 163947 [details]
Patch
Comment 17 WebKit Review Bot 2012-09-13 13:19:24 PDT
Attachment 163947 [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/css/StyleResolver.cpp:5256:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Michelangelo De Simone 2012-09-13 16:15:05 PDT
(In reply to comment #14)

> Looks good - only a few small changes.

All the review comments have been address; let me know if there is anything else you wish me to change.

Thanks.
Comment 19 Dean Jackson 2012-09-14 09:53:13 PDT
(In reply to comment #15)

> I think we already have a couple of those covered in the invalid parsing test file.
> 
> http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js
> 
> Some of the things not covered there are:
> - array(NaN)
> - arrays with many values

It was this one that worried me most, because parsing might succeed but binding to the uniforms could go wrong.
Comment 20 Alexandru Chiculita 2012-09-14 10:05:03 PDT
(In reply to comment #19)
> (In reply to comment #15)
> 
> > I think we already have a couple of those covered in the invalid parsing test file.
> > 
> > http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/script-tests/custom-filter-property-parsing-invalid.js
> > 
> > Some of the things not covered there are:
> > - array(NaN)
> > - arrays with many values
> 
> It was this one that worried me most, because parsing might succeed but binding to the uniforms could go wrong.

Do you think we should enforce a maximum size for the array?
Comment 21 WebKit Review Bot 2012-09-14 10:17:37 PDT
Comment on attachment 163947 [details]
Patch

Clearing flags on attachment: 163947

Committed r128626: <http://trac.webkit.org/changeset/128626>
Comment 22 WebKit Review Bot 2012-09-14 10:17:42 PDT
All reviewed patches have been landed.  Closing bug.