Bug 5861 - feConvolveMatrix filter is not implemented
: feConvolveMatrix filter is not implemented
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: SVG
: 523.x (Safari 3)
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To: Nobody
:
Depends on: 41011
Blocks: 68469 5857 26389
  Show dependency treegraph
 
Reported: 2005-11-28 17:19 PST by Oliver Hunt
Modified: 2014-05-12 05:54 PDT (History)
9 users (show)

See Also:


Attachments
SVG feConvolveMatrix implementation (41.64 KB, patch)
2009-11-08 11:25 PST, Dirk Schulze
oliver: review-
Details | Formatted Diff | Diff
SVG feConvolveMatrix tests (65.51 KB, patch)
2009-11-08 11:28 PST, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
draft patch (continue to work on Dirk's implementation) (47.53 KB, patch)
2010-06-10 01:14 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
base patch (contains the build and other things, many things come from Dirk's patch) (47.48 KB, patch)
2010-06-14 04:42 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
next try (49.75 KB, patch)
2010-06-14 23:54 PDT, Zoltan Herczeg
zimmermann: review-
Details | Formatted Diff | Diff
base patch (52.29 KB, patch)
2010-06-23 02:13 PDT, Zoltan Herczeg
krit: review-
Details | Formatted Diff | Diff
Updated patch - landed in r62092 (56.25 KB, patch)
2010-06-28 03:39 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
part 2: the implementation (47.53 KB, patch)
2010-06-30 01:31 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
part 2: the implementation (114.43 KB, patch)
2010-06-30 01:33 PDT, Zoltan Herczeg
zimmermann: review-
Details | Formatted Diff | Diff
updated patch (114.59 KB, patch)
2010-07-01 01:01 PDT, Zoltan Herczeg
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2005-11-28 17:19:34 PST
 
Comment 1 Dirk Schulze 2009-11-08 11:25:08 PST
Created attachment 42717 [details]
SVG feConvolveMatrix implementation

SVG feConvolveMatrix implementation
Comment 2 Dirk Schulze 2009-11-08 11:28:01 PST
Created attachment 42718 [details]
SVG feConvolveMatrix tests

Some further tests for feConvolveMatrix.
Comment 3 Oliver Hunt 2009-11-09 13:37:58 PST
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.
Comment 4 Eric Seidel 2009-11-25 22:08:11 PST
Comment on attachment 42718 [details]
SVG feConvolveMatrix tests

Test looks OK.  Why are the results platform specific?
Comment 5 Zoltan Herczeg 2010-06-10 01:14:43 PDT
Created attachment 58342 [details]
draft patch (continue to work on Dirk's implementation)
Comment 6 Zoltan Herczeg 2010-06-14 04:42:22 PDT
Created attachment 58634 [details]
base patch (contains the build and other things, many things come from Dirk's patch)
Comment 7 WebKit Review Bot 2010-06-14 04:45:57 PDT
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.
Comment 8 WebKit Review Bot 2010-06-14 05:30:59 PDT
Attachment 58634 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3282105
Comment 9 Zoltan Herczeg 2010-06-14 23:54:33 PDT
Created attachment 58758 [details]
next try
Comment 10 WebKit Review Bot 2010-06-14 23:56:09 PDT
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.
Comment 11 WebKit Review Bot 2010-06-15 00:52:12 PDT
Attachment 58758 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3279154
Comment 12 Zoltan Herczeg 2010-06-22 13:45:21 PDT
The cr-ews incorrectly reprots an error. Anyone up for a review?
Comment 13 Nikolas Zimmermann 2010-06-23 00:17:15 PDT
(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.
Comment 14 Nikolas Zimmermann 2010-06-23 00:46:13 PDT
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!
Comment 15 Zoltan Herczeg 2010-06-23 02:13:22 PDT
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.
Comment 16 Zoltan Herczeg 2010-06-23 02:17:26 PDT
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.
Comment 17 WebKit Review Bot 2010-06-23 02:35:04 PDT
Attachment 59494 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3318681
Comment 18 Dirk Schulze 2010-06-23 04:27:06 PDT
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.
Comment 19 Zoltan Herczeg 2010-06-28 03:39:27 PDT
Created attachment 59890 [details]
Updated patch - landed in r62092
Comment 20 Zoltan Herczeg 2010-06-28 04:32:48 PDT
> 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.
Comment 21 Adam Barth 2010-06-28 09:16:10 PDT
> Hey Dirk, this was really a bug with the Chromium-ews, they fixed it and now the build works.

Yay.  Glad the fix worked.  :)
Comment 22 Nikolas Zimmermann 2010-06-28 23:17:16 PDT
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.
Comment 23 Zoltan Herczeg 2010-06-29 00:33:06 PDT
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.
Comment 24 WebKit Review Bot 2010-06-29 00:35:12 PDT
http://trac.webkit.org/changeset/62092 might have broken Qt Linux Release
Comment 25 WebKit Review Bot 2010-06-29 01:00:15 PDT
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
Comment 26 Csaba Osztrogonác 2010-06-29 01:59:23 PDT
Update expected files landed in:
http://trac.webkit.org/changeset/62096
http://trac.webkit.org/changeset/62097
Comment 27 Csaba Osztrogonác 2010-06-29 02:00:09 PDT
Comment on attachment 59890 [details]
Updated patch - landed in r62092

remove r+ to remove it from pending queue
Comment 28 Zoltan Herczeg 2010-06-30 01:31:31 PDT
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.
Comment 29 Zoltan Herczeg 2010-06-30 01:33:49 PDT
Created attachment 60097 [details]
part 2: the implementation

Oh, wrong patch before. Sorry.
Comment 30 Nikolas Zimmermann 2010-06-30 05:12:52 PDT
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?
Comment 31 Zoltan Herczeg 2010-07-01 01:01:49 PDT
Created attachment 60210 [details]
updated patch
Comment 32 Zoltan Herczeg 2010-07-01 01:06:06 PDT
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 :(
Comment 33 Nikolas Zimmermann 2010-07-01 01:19:56 PDT
(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.
Comment 34 Nikolas Zimmermann 2010-07-01 02:09:21 PDT
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.
Comment 35 Nikolas Zimmermann 2010-07-01 02:09:45 PDT
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.
Comment 36 Eric Seidel 2010-07-01 02:13:14 PDT
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
Comment 37 Nikolas Zimmermann 2010-07-01 02:29:21 PDT
(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.
Comment 38 Eric Seidel 2010-07-01 02:46:11 PDT
Comment on attachment 60210 [details]
updated patch

 92     // Only once usage -> can be ALWAYS_INLINE

is not english.
Comment 39 Nikolas Zimmermann 2010-07-01 02:49:08 PDT
(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 :-)
Comment 40 Zoltan Herczeg 2010-07-01 02:52:30 PDT
> 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.
Comment 41 Zoltan Herczeg 2010-07-01 03:14:32 PDT
Closing bug.