Created attachment 42717 [details] SVG feConvolveMatrix implementation SVG feConvolveMatrix implementation

Created attachment 42718 [details] SVG feConvolveMatrix tests Some further tests for feConvolveMatrix.

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 on attachment 42718 [details] SVG feConvolveMatrix tests Test looks OK. Why are the results platform specific?

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

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

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.

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

Created attachment 58758 [details] next try

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.

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

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

(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 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!

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.

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.

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

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.

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

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

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

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.

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

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

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

Comment on attachment 59890 [details] Updated patch - landed in r62092 remove r+ to remove it from pending queue

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.

Created attachment 60097 [details] part 2: the implementation Oh, wrong patch before. Sorry.

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?

Created attachment 60210 [details] updated patch

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 :(

(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 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 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.

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

(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 on attachment 60210 [details] updated patch 92 // Only once usage -> can be ALWAYS_INLINE is not english.

(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 :-)

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

Closing bug.