Bug 95223

Summary:

Use arrays in shaders

Product:

WebKit

Reporter:

Michelangelo De Simone <michelangelo>

Component:

CSS

Assignee:

Michelangelo De Simone <michelangelo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achicu, cmarcelo, dino, krit, macpherson, menard, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

94226

Bug Blocks:

71392, 71395, 96437

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Michelangelo De Simone

Reported 2012-08-28 11:32:31 PDT

As per summary the array passed via array() shall be used within shaders.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Michelangelo De Simone

Comment 1 2012-09-11 14:41:57 PDT

Created attachment 163445 [details] Patch

Michelangelo De Simone

Comment 2 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.

WebKit Review Bot

Comment 3 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.

Alexandru Chiculita

Comment 4 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.

Michelangelo De Simone

Comment 5 2012-09-11 15:59:32 PDT

Created attachment 163465 [details] Patch

WebKit Review Bot

Comment 6 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.

Alexandru Chiculita

Comment 7 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."

Michelangelo De Simone

Comment 8 2012-09-11 18:37:15 PDT

Created attachment 163491 [details] Patch

WebKit Review Bot

Comment 9 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.

Build Bot

Comment 10 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

Michelangelo De Simone

Comment 11 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.:-/

Michelangelo De Simone

Comment 12 2012-09-12 10:55:33 PDT

Created attachment 163655 [details] Patch

WebKit Review Bot

Comment 13 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.

Dean Jackson

Comment 14 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.

Alexandru Chiculita

Comment 15 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.

Michelangelo De Simone

Comment 16 2012-09-13 13:14:18 PDT

Created attachment 163947 [details] Patch

WebKit Review Bot

Comment 17 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.

Michelangelo De Simone

Comment 18 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.

Dean Jackson

Comment 19 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.

Alexandru Chiculita

Comment 20 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?

WebKit Review Bot

Comment 21 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>

WebKit Review Bot

Comment 22 2012-09-14 10:17:42 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.