feConvolveMatrix filter is not implemented

[Oliver Hunt]

[Dirk Schulze]

Created attachment 42717 [details]
SVG feConvolveMatrix implementation

SVG feConvolveMatrix implementation

[Dirk Schulze]

Created attachment 42718 [details]
SVG feConvolveMatrix tests

Some further tests for feConvolveMatrix.

[Oliver Hunt]

Comment on attachment 42717 [details]
SVG feConvolveMatrix implementation

I don't like the _foo names, i think it would be better if you did

foo = m_foo;

if (this->hasAttribute(SVGNames::divisorAttr) && _divisor == 0.f)

What happens if divisor is NaN ?

In the inner loop of FEConvolveMatrix::apply seems really horrifyingly expensive -- at the very lesat i think the edge case checks should be hoisted out of the main loop and done separately.

[Eric Seidel (no email)]

Comment on attachment 42718 [details]
SVG feConvolveMatrix tests

Test looks OK.  Why are the results platform specific?

[Zoltan Herczeg]

Created attachment 58342 [details]
draft patch (continue to work on Dirk's implementation)

[Zoltan Herczeg]

Created attachment 58634 [details]
base patch (contains the build and other things, many things come from Dirk's patch)

[WebKit Review Bot]

Attachment 58634 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGFEConvolveMatrixElement.cpp:112:  _kernelMatrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:120:  _orderX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:121:  _orderY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:129:  _targetX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:130:  _targetY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:140:  _divisor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:141:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:146:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:153:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:35:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 11 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58634 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3282105

[Zoltan Herczeg]

Created attachment 58758 [details]
next try

[WebKit Review Bot]

Attachment 58758 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/SVGFEConvolveMatrixElement.cpp:112:  _kernelMatrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:120:  _orderX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:121:  _orderY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:129:  _targetX is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:130:  _targetY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:140:  _divisor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:141:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:146:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/SVGFEConvolveMatrixElement.cpp:153:  Should have a space between // and comment  [whitespace/comments] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:35:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 11 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 58758 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3279154

[Zoltan Herczeg]

The cr-ews incorrectly reprots an error. Anyone up for a review?

[Nikolas Zimmermann]

(In reply to comment #12)
> The cr-ews incorrectly reprots an error. Anyone up for a review?
No, there is a problem with V8SVGFEConcolveMatrix, can you look into that please? Also there a several style fixes that can easily be fixed.

I'll give your patch a review soon as well.

[Nikolas Zimmermann]

Comment on attachment 58758 [details]
next try

WebCore/GNUmakefile.am:3411
 +      WebCore/svg/SVGFEConvolveMatrixElement.cpp \
Leading space.

WebCore/GNUmakefile.am:3412
 +      WebCore/svg/SVGFEConvolveMatrixElement.h \
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:59
 +          if (parseNumberOptionalNumber(value, x, y)) {
What happens if parsing failed, you might want to reset order x/y then? Think of scripting feConvolveMatrix. We have other code like this, that handles the error case.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:71
 +          kernelMatrixBaseValue()->parse(value);
What about the error case?

WebCore/svg/SVGFEConvolveMatrixElement.cpp:73
 +          setDivisorBaseValue(value.toFloat());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:75
 +          setBiasBaseValue(value.toFloat());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:77
 +          setTargetXBaseValue(value.toUIntStrict());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:79
 +          setTargetYBaseValue(value.toUIntStrict());
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:82
 +          if (parseNumberOptionalNumber(value, x, y)) {
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:112
 +      Vector<float> _kernelMatrix;
Style issue, as the bot reported.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:117
 +      for (int i = 0; i < nr; i++)
for (int i = 0; i < numberOfItems; ++i)

Please don't use abbrevations.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:120
 +      int _orderX = orderX();
Style issue, need to rename, to avoid leading "_".

WebCore/svg/SVGFEConvolveMatrixElement.cpp:122
 +      if (!this->hasAttribute(SVGNames::orderAttr)) {
No need to use "this->" prefix.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:126
 +      if (_orderX * _orderY != nr)
This needs a comment.



WebCore/svg/SVGFEConvolveMatrixElement.cpp:129
 +      int _targetX = targetX();
Style issue as well. Same for _targetY.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:131
 +      if (this->hasAttribute(SVGNames::targetXAttr) && (_targetX < 0 || _targetX >= _orderX))
"this->" not needed. Needs a comment explaining the range of this value.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:133
 +      if (!this->hasAttribute(SVGNames::targetXAttr))
"this->" not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:134
 +          _targetX = static_cast<int>(floor(_orderX / 2));
Needs a comment, why this is the default.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:135
 +      if (this->hasAttribute(SVGNames::targetYAttr) && (_targetY < 0 || _targetY >= _orderY))
"this->" not needed. Needs a comment explaining the range of this value.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:137
 +      if (!this->hasAttribute(SVGNames::targetYAttr))
"this->" not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:139
 +  
Needs a comment, why this is the default.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:141
 +      if (this->hasAttribute(SVGNames::divisorAttr) && _divisor == 0.f)
"this->" not needed. Shouldn't this check include negative values, then you could just check if (hasAttribute(SVGnames::divisorAttr) && !divisor)

WebCore/svg/SVGFEConvolveMatrixElement.cpp:143
 +      if (!this->hasAttribute(SVGNames::divisorAttr)) {
this-> not needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:146
 +          if (_divisor == 0.f)
Maybe just if (!divisor) divisor = 1;
No .f postfixes needed, per style rules.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:147
 +              _divisor = 1.f;
No .f needed.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:150
 +      return FEConvolveMatrix::create(input1, IntSize(_orderX, _orderY), _divisor, bias(), IntPoint(_targetX, _targetY), static_cast<EdgeModeType>(edgeMode()), FloatPoint(kernelUnitLengthX(), kernelUnitLengthX()), preserveAlpha(), _kernelMatrix);
You could wrap this in multiple lines with "commas" at the end, lining up at the create( opening brace.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:34
 +  using std::min;
Please use "using namespace std;".

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:42
 +      const float& divisor, const float& bias, const IntPoint& targetOffset, EdgeModeType edgeMode,
No const references for POD types needed., use just "float".

Please fix all style issues (you can easily do that, after running check-webkit-style on a patchset before submitting). Also the V8 problem is valid. No idea how to fix it, grep how the existing bindings work :-)
Other than style issues a great first start!

[Zoltan Herczeg]

Created attachment 59494 [details]
base patch

The ChangeLog will be something like this:

The patch was started by Dirk Schulze, and continued later by me. The patch was split into two parts: this first part contains a new ConvolveMatrixElement class, and the necessary build modifications. It does not, however, a paint implementation for convolve matrix filter.

[Zoltan Herczeg]

My notes:

Chromium ews incremental build feature needs fixed to detect new .idl files. I have opened a new bug report, and added a dependency for this patch (41011)

Most of the stlye bugs comes from the old patch, since the style was different at that time, and they are easy fixes.

> WebCore/svg/SVGFEConvolveMatrixElement.cpp:59
>  +          if (parseNumberOptionalNumber(value, x, y)) {
> What happens if parsing failed, you might want to reset order x/y then? Think of scripting feConvolveMatrix. We have other code like this, that handles the error case.

Other SVG elements do their parsing this way, so this is probably ok.

[WebKit Review Bot]

Attachment 59494 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3318681

[Dirk Schulze]

Comment on attachment 59494 [details]
base patch

Don't open new br's. The builds must clearly work with this patch. Search in the history of trac.webkit.org for vkern. The initial commit and some following commits will give you a hint what you need to change to get chromium building.

[Zoltan Herczeg]

Created attachment 59890 [details]
Updated patch - landed in r62092

[Zoltan Herczeg]

> Don't open new br's. The builds must clearly work with this patch. 

Hey Dirk, this was really a bug with the Chromium-ews, they fixed it and now the build works.

[Adam Barth]

> Hey Dirk, this was really a bug with the Chromium-ews, they fixed it and now the build works.

Yay.  Glad the fix worked.  :)

[Nikolas Zimmermann]

Comment on attachment 59890 [details]
Updated patch - landed in r62092

WebCore/svg/SVGFEConvolveMatrixElement.cpp:117
 +      for (int i = 0; i < numberOfItems; i++)
I'd prefer ++i here, but hey :-)

WebCore/svg/SVGFEConvolveMatrixElement.cpp:145
 +          for (int i = 0; i < numberOfItems; i++)
Ditto.

WebCore/svg/SVGFEConvolveMatrixElement.cpp:148
 +              divisorValue = 1.0f;
= 1; No .0f postfixes (new style rule).

Please fix before landing! r=me.

[Zoltan Herczeg]

Patch landed in http://trac.webkit.org/changeset/62092, but I plan to upload the second part of the work soon, so I don't close the bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62092 might have broken Qt Linux Release

[WebKit Review Bot]

http://trac.webkit.org/changeset/62093 might have broken Qt Windows 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62093
http://trac.webkit.org/changeset/62094
http://trac.webkit.org/changeset/62095

[Csaba Osztrogonác]

Update expected files landed in:
http://trac.webkit.org/changeset/62096
http://trac.webkit.org/changeset/62097

[Csaba Osztrogonác]

Comment on attachment 59890 [details]
Updated patch - landed in r62092

remove r+ to remove it from pending queue

[Zoltan Herczeg]

Created attachment 60096 [details]
part 2: the implementation

Unfortunatel I can update only the mac-leopard expected values. I would be happy if someone could help me run this patch on other macs. Otherwise I need to use the build-bot results to update the other macs.

[Zoltan Herczeg]

Created attachment 60097 [details]
part 2: the implementation

Oh, wrong patch before. Sorry.

[Nikolas Zimmermann]

Comment on attachment 60097 [details]
part 2: the implementation

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:158
 +     Where region C contains those pixels, which value
s/which value/whose values/

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:159
 +     can be calculated without crossing the edge of the rectange.
s/rectange/rectangle/

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:197
 +  static ALWAYS_INLINE unsigned char clampRGBAValue(float f)
I'd rename 'float f' to 'float rgba', to avoid abbreviations.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:199
 +      // Just a helper function
Doesn't help much to add this comment :-)

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:208
 +  template<bool preserveAlphaValues> ALWAYS_INLINE void FEConvolveMatrix::fastSetInteriorPixels(PaintingData& paintingData, int clipRight, int clipBottom)
I'd prefer a newline after the template<bool preserveAlphaValues>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:224
 +              totals[0] = 0.0f;
Juse use "= 0" here, it's the new style rule.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:231
 +                  totals[0] += m_kernelMatrix[kernelValue] * static_cast<float>(paintingData.srcPixelArray->get(kernelPixel++));
Would it help to store m_kernelMatrix[kernelValue] in a local variable? Not sure.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:244
 +              paintingData.dstPixelArray->set(pixel++, clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
Is m_divisor guarenteed to be non null? If so please add an assertion on top of this function.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:290
 +  template<bool preserveAlphaValues> void FEConvolveMatrix::fastSetOuterPixels(PaintingData& paintingData, int x1, int y1, int x2, int y2)
Please use a newline after the template<bool...>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:309
 +              totals[0] = 0.0f;
Use = 0, no 0.f postfix needed.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:333
 +              paintingData.dstPixelArray->set(pixel++, clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
Same comment as above regarding m_divisor.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:355
 +          fastSetInteriorPixels<true>(paintingData, clipRight, clipBottom);
Just wondering, is it possible to use m_preserveAlpha directly as template parameter? I guess not :-)

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:93
 +      template<bool preserveAlphaValues> ALWAYS_INLINE void fastSetInteriorPixels(PaintingData&, int clipRight, int clipBottom);
Use a newline after template<..>.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:97
 +      template<bool preserveAlphaValues> void fastSetOuterPixels(PaintingData&, int x1, int y1, int x2, int y2);
Ditto.

Great patch other than those small issues. Dirk might have additional comments?

[Zoltan Herczeg]

Created attachment 60210 [details]
updated patch

[Zoltan Herczeg]

I did your suggested changes. Thanks for the review.

> WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:231
>  +                  totals[0] += m_kernelMatrix[kernelValue] * static_cast<float>(paintingData.srcPixelArray->get(kernelPixel++));
> Would it help to store m_kernelMatrix[kernelValue] in a local variable? Not sure.

You mean copy the values to a local array? Not sure that will help.

> WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:244
>  +              paintingData.dstPixelArray->set(pixel++, clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
> Is m_divisor guarenteed to be non null? If so please add an assertion on top of this function.

Added the assert and a comment. m_divisor cannot be 0, otherwise the filter will be not created.

> WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:355
>  +          fastSetInteriorPixels<true>(paintingData, clipRight, clipBottom);
> Just wondering, is it possible to use m_preserveAlpha directly as template parameter? I guess not :-)

Template arguments must be compile time constants.

I still not have expected pixel test for the generic mac, only for mac-leopard :(

[Nikolas Zimmermann]

(In reply to comment #32)
> I did your suggested changes. Thanks for the review.
> 
> > WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:231
> >  +                  totals[0] += m_kernelMatrix[kernelValue] * static_cast<float>(paintingData.srcPixelArray->get(kernelPixel++));
> > Would it help to store m_kernelMatrix[kernelValue] in a local variable? Not sure.
> 
> You mean copy the values to a local array? Not sure that will help.
Oh, I meant just m_kernelMatrix[kernelValue], as you're calling it multiple times, but it's not worth it - just fine as it is.

> 
> > WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:244
> >  +              paintingData.dstPixelArray->set(pixel++, clampRGBAValue(totals[0] / m_divisor + paintingData.bias));
> > Is m_divisor guarenteed to be non null? If so please add an assertion on top of this function.
> 
> Added the assert and a comment. m_divisor cannot be 0, otherwise the filter will be not created.
Thanks.

> 
> > WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:355
> >  +          fastSetInteriorPixels<true>(paintingData, clipRight, clipBottom);
> > Just wondering, is it possible to use m_preserveAlpha directly as template parameter? I guess not :-)
> 
> Template arguments must be compile time constants.
Ah right, forgot.

> 
> I still not have expected pixel test for the generic mac, only for mac-leopard :(
Same here, but I don't care about that, as long as we don't have pixel test bots, where I could grab the right results from. Let someone else update them if he/she notices diffs.
I'm just generating mac-leopard results, and put them into mac. If there are already mac-lepard specific results, I'm just regenerating them, and copy the same file into mac/ - until someone else can fix them for me...

Waiting for EWS before giving the final review.

[Nikolas Zimmermann]

Comment on attachment 60210 [details]
updated patch

Great patch, thanks so much! r=me.WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:92
 +      // Only once usage -> can be ALWAYS_INLINE
Just remove this comment, I don't think it helps much.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:96
 +      // Only once usage -> can be ALWAYS_INLINE
Ditto.

[Nikolas Zimmermann]

Comment on attachment 60210 [details]
updated patch

Great patch, thanks so much! r=me.

Please fix some last small issues before landing:

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:92
 +      // Only once usage -> can be ALWAYS_INLINE
Just remove this comment, I don't think it helps much.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:96
 +      // Only once usage -> can be ALWAYS_INLINE
Ditto.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.h:98
 +      template<bool preserveAlphaValues>
Can you please add a newline between this template<bool> and the line before. Looks less cluttered.

[Eric Seidel (no email)]

ALWAYS_INLINE is a hammer we use when the compiler does the wrong thing.  It isn't just to be sprinkled around. :)  We add it for large functions which the compiler refuses to inline on its own.  Just placing something in a class definition (in a header) implicitly marks it "inline".  No need to add ALWAYS_INLINE unless we have demonstrated the compiler fails to comply.  It's better to let the compiler figure things out than to force its hand with ALWAYS_INLINE

[Nikolas Zimmermann]

(In reply to comment #36)
> ALWAYS_INLINE is a hammer we use when the compiler does the wrong thing.  It isn't just to be sprinkled around. :)  We add it for large functions which the compiler refuses to inline on its own.  Just placing something in a class definition (in a header) implicitly marks it "inline".  No need to add ALWAYS_INLINE unless we have demonstrated the compiler fails to comply.  It's better to let the compiler figure things out than to force its hand with ALWAYS_INLINE

Well, clearly the function should be inlined. I'd say better safe than sorry - and thus have no problem with the ALWAYS_INLINE usage. It's done like this in the other filter effects (lighting etc.) as well.

[Eric Seidel (no email)]

Comment on attachment 60210 [details]
updated patch

 92     // Only once usage -> can be ALWAYS_INLINE

is not english.

[Nikolas Zimmermann]

(In reply to comment #38)
> (From update of attachment 60210 [details])
>  92     // Only once usage -> can be ALWAYS_INLINE
> 
> is not english.

It has been removed already :-)

[Zoltan Herczeg]

> It has been removed already :-)

Yeah, and the patch landed in: http://trac.webkit.org/changeset/62244

If the bots like the patch, I will close the bug.

[Zoltan Herczeg]

Closing bug.