Bug 5864

Created attachment 42742 [details] SVG feTurbulence imeplementation

Comment on attachment 42742 [details] SVG feTurbulence imeplementation This patch implements feTurbulence.

Created attachment 42743 [details] SVG feTurbulence imeplementation oops. Accidentally modified feMorpholohy. This is the fixed patch.

Comment on attachment 42743 [details] SVG feTurbulence imeplementation > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 50642) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,25 @@ > +2009-11-09 Dirk Schulze <krit@webkit.org> > + > + Reviewed by NOBODY (OOPS!). > + > + feTurbulence if not implemented > + [https://bugs.webkit.org/show_bug.cgi?id=5864] > + > + This is the implementation of feTurbulence. The > + calculation code is a modified version of what appears > + in the SVG 1.1 specification for feTurbulence. > + > + Test: svg/W3C-SVG-1.1/filters-turb-01-f.svg > + > + * svg/graphics/filters/SVGFETurbulence.cpp: > + (WebCore::FETurbulence::setup_seed): > + (WebCore::FETurbulence::random): > + (WebCore::FETurbulence::init): > + (WebCore::FETurbulence::noise2): > + (WebCore::FETurbulence::turbulence): > + (WebCore::FETurbulence::apply): > + * svg/graphics/filters/SVGFETurbulence.h: > + > 2009-11-09 Martin Robinson <martin.james.robinson@gmail.com> > > Reviewed by Jan Alonzo. > Index: WebCore/svg/graphics/filters/SVGFETurbulence.cpp > =================================================================== > --- WebCore/svg/graphics/filters/SVGFETurbulence.cpp (revision 50605) > +++ WebCore/svg/graphics/filters/SVGFETurbulence.cpp (working copy) > @@ -2,6 +2,7 @@ > Copyright (C) 2004, 2005, 2006, 2007 Nikolas Zimmermann <zimmermann@kde.org> > 2004, 2005 Rob Buis <buis@kde.org> > 2005 Eric Seidel <eric@webkit.org> > + 2009 Dirk Schulze <krit@webkit.org> > > This library is free software; you can redistribute it and/or > modify it under the terms of the GNU Library General Public > @@ -23,8 +24,13 @@ > > #if ENABLE(SVG) && ENABLE(FILTERS) > #include "SVGFETurbulence.h" > -#include "SVGRenderTreeAsText.h" > + > +#include "CanvasPixelArray.h" > #include "Filter.h" > +#include "ImageData.h" > +#include "SVGRenderTreeAsText.h" > + > +#include <math.h> > > namespace WebCore { > > @@ -106,8 +112,187 @@ void FETurbulence::setStitchTiles(bool s > m_stitchTiles = stitch; > } > > -void FETurbulence::apply(Filter*) > +// The turbulence calculation code is an adapted version of what > +// appears in the SVG 1.1 specification: > +// http://www.w3.org/TR/SVG11/filters.html#feTurbulence > +long FETurbulence::setup_seed(long lSeed) > +{ > + if (lSeed <= 0) lSeed = -(lSeed % (RAND_m - 1)) + 1; > + if (lSeed > RAND_m - 1) lSeed = RAND_m - 1; > + return lSeed; > +} > + > +long FETurbulence::random(long lSeed) > +{ > + long result; > + result = RAND_a * (lSeed % RAND_q) - RAND_r * (lSeed / RAND_q); should be: long result = RAND_a * (lSeed % RAND_q) - RAND_r * (lSeed / RAND_q); > +double FETurbulence::noise2(int nColorChannel, double vec[2], StitchInfo *pStitchInfo) > { > + int bx0, bx1, by0, by1, b00, b10, b01, b11; > + double rx0, rx1, ry0, ry1, *q, sx, sy, a, b, t, u, v; > + int i, j; > + t = vec[0] + PerlinN; > + bx0 = (int)t; should be static_cast<int> > + rx0 = t - (int)t; ditto > + by0 = (int)t; ditto > + ry0 = t - (int)t; ditto > +unsigned char FETurbulence::turbulence(int nColorChannel, double *point, const IntSize& filterSize) > +{ > + StitchInfo stitch; > + StitchInfo *pStitchInfo = NULL; // Not stitching when NULL. We use 0 not NULL > + double fLoFreq = double(floor(fTileWidth * m_baseFrequencyX)) / fTileWidth; > + double fHiFreq = double(ceil(fTileWidth * m_baseFrequencyX)) / fTileWidth; static_cast!!! ZOMBBQPWN13S, ignoring the remainder of these > Index: WebCore/svg/graphics/filters/SVGFETurbulence.h > =================================================================== > --- WebCore/svg/graphics/filters/SVGFETurbulence.h (revision 50605) > +++ WebCore/svg/graphics/filters/SVGFETurbulence.h (working copy) > +/* Produces results in the range [1, 2**31 - 2]. > +Algorithm is: r = (a * r) mod m > +where a = 16807 and m = 2**31 - 1 = 2147483647 > +See [Park & Miller], CACM vol. 31 no. 10 p. 1195, Oct. 1988 > +To test: the algorithm should produce the result 1043618065 > +as the 10,000th generated number if the original seed is 1. > +*/ > +#define RAND_m 2147483647 /* 2**31 - 1 */ > +#define RAND_a 16807 /* 7**5; primitive root of m */ > +#define RAND_q 127773 /* m / a */ > +#define RAND_r 2836 /* m % a */ > +#define BSize 0x100 > +#define BM 0xff > +#define PerlinN 0x1000 > +#define NP 12 /* 2^PerlinN */ > +#define NM 0xfff > +#define s_curve(t) ( t * t * (3. - 2. * t) ) > +#define lerp(t, a, b) ( a + t * (b - a) ) static const <int/long> and a pair of static inline functions. Finally, i'm _really_ concerned about the performance of this. It should be possible to do this purely in integer arithmetic (I think Ken Perlin describes this) and until we have effective recompute reduction this could be very expensive on repaint. r- pending the various style, etc issues. --Oliver

(In reply to comment #4) > Finally, i'm _really_ concerned about the performance of this. It should be > possible to do this purely in integer arithmetic (I think Ken Perlin describes > this) and until we have effective recompute reduction this could be very > expensive on repaint. > > r- pending the various style, etc issues. I made some more research. Perlin self published a paper to speed up his algorithm of about 10%. But still no integer algorithm. But the main problem is, that the results differ.

Created attachment 62162 [details] SVG feTurbulence imeplementation

Any comments are welcome for my patch.

Comment on attachment 62162 [details] SVG feTurbulence imeplementation Hi Renata, welcome to the filter hackers crowd :-) Some general notes: - You should set r? if you expect this to be reviewed. Otherwhise the Early Warnings System bots don't run and compilation is not tested on other platforms. Also the style bot doesn't run. - You should never change the layout of the ChangeLog template (you enclosed the url with [] brackets) - The "No new tests" statment is probably wrong, as it's covered by at least the existing W3C SVG tests. It probably also changes other test results, did you run the layout tests? It's necessary to run them on a Mac, to produce new pixel test results (ask Zoltan, who knows about the procedure) Now some specific review comments: WebCore/svg/graphics/filters/SVGFETurbulence.cpp:116 + // appears in the SVG 1.1 specification: No need for this line wrap, long comments are allowed. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:119 + inline long FETurbulence::PaintingData::random() If you want to force inlining, use ALWAYS_INLINE here, consistent with the other filter effects. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:121 + long result = randAmplitude * (seed % randQ) - randR * (seed / randQ); A comment explaining what happens here is very helpful. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:124 + return (seed = result); I'd prefer to use two lines here. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:127 + ALWAYS_INLINE float smoothCurve(float t) As smoothCurve is only used in this cpp file, I'd prefer to make it static inline. "static inline float smoothCurve". Also forcing inline is not needed in this case. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:132 + ALWAYS_INLINE float lerp(float t, float a, float b) Please don't use abbrevations, rather use "linearInterpolation" here. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:139 + float s; This variable needs a better name and should be moved inside the for loop, where's needed. I'd suggest a long descriptive name like "normalizationFactor". WebCore/svg/graphics/filters/SVGFETurbulence.cpp:140 + int i, j, k; Please move those variables directly in the for loops - no real need for them to be global, you're not reusing the old values anywhere. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:143 + paintingData.seed = -(paintingData.seed % (randMaximum - 1)) + 1; A comment is needed here, it's probably from the spec, so it should be quoted. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:145 + paintingData.seed = randMaximum - 1; Ditto. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:148 + for (k = 0; k < 4; ++k) { This loop needs a comment on top, what happens inside the loop. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:152 + gradient[0] = static_cast<float>((paintingData.random() % (blockSize + blockSize)) - blockSize) / blockSize; I'd prefer 2 * blockSize, instead of blockSize + blockSize here. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:153 + gradient[1] = static_cast<float>((paintingData.random() % (blockSize + blockSize)) - blockSize) / blockSize; Ditto. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:154 + s = sqrt(gradient[0] * gradient[0] + gradient[1] * gradient[1]); Use the sqrt() version for floats: sqrtf. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:161 + j = paintingData.random() % blockSize; I'd like to see an assertion here that 'j' is always in the correct range of latticeSelector array size: ASSERT(j >= 0); ASSERT(j < 2* blockSize + 2); WebCore/svg/graphics/filters/SVGFETurbulence.cpp:174 + float FETurbulence::noise2D(PaintingData& paintingData, float vec[2]) No need to use abbrevations, maybe rename 'vec' to 'noiseVector'? How about using a FloatPoint? WebCore/svg/graphics/filters/SVGFETurbulence.cpp:176 + int bx, by, b00, b10, b01, b11; All these variables need better, descriptive names. Also there shouldn't be a need to declare them on top. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:177 + float rx, ry, *q, sx, sy, a, b, t, u, v; Ditto. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:180 + bx = static_cast<int>(t); Why is this float -> int truncation desired? No need to round? WebCore/svg/graphics/filters/SVGFETurbulence.cpp:198 + bx &= blockMask; This needs a comment. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:200 + i = paintingData.latticeSelector[bx]; Same here. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:201 + j = paintingData.latticeSelector[(bx + 1) & blockMask]; Here. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:202 + b00 = paintingData.latticeSelector[i + by]; And for b00/b10/b01/b11 :-) WebCore/svg/graphics/filters/SVGFETurbulence.cpp:207 + sx = smoothCurve(rx); Ditto, should explain what is smoothed, why it's smoothed. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:210 + q = paintingData.gradient[paintingData.channel][b00]; The whole block here, needs to be commented, maybe with a single comment before the first 'q' statement. Maybe it's much clearer, when the variables names are renamed. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:223 + unsigned char FETurbulence::turbulence(PaintingData& paintingData, float* point) This function needs a better name "calculateTurbulenceValueForPoint"? WebCore/svg/graphics/filters/SVGFETurbulence.cpp:232 + float loFreq = floor(tileWidth * m_baseFrequencyX) / tileWidth; Please use the float specific verison floorf. Is tileWidth guaranteed to be non-zero? If so add an assertion, otherwhise check for it and abort? Also rename loFreq to lowFrequency. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:233 + float hiFreq = ceil(tileWidth * m_baseFrequencyX) / tileWidth; s/ceil/ceilf. s/hiFreq/highFrequency/. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:240 + float loFreq = floor(tileHeight * m_baseFrequencyY) / tileHeight; As above, s/loFreq/lowFrequency, s/floor/floorf/. Also either needs an assertion that tileHeight is non-zero, or an abortion in that case. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:241 + float hiFreq = ceil(tileHeight * m_baseFrequencyY) / tileHeight; s/hiFreq/highFrequency, s/ceil/ceilf/. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:242 + if (m_baseFrequencyY / loFreq < hiFreq / m_baseFrequencyY) Isn't it possible that lowFrequency gets rounded to 0, if it's very small? Maybe check here, to avoid a possible division by zero. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:248 + paintingData.width = static_cast<int>(tileWidth * m_baseFrequencyX + 0.5); Why the + 0.5 value? Isn't it possible to use roundf in the following 3 lines as well? WebCore/svg/graphics/filters/SVGFETurbulence.cpp:253 + float sum = 0; sum of what? :-) Please use a more descriptive name. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:254 + float vec[2]; I think two seperated variables would be better here, also with a better name. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:258 + for (int nOctave = 0; nOctave < m_numOctaves; nOctave++) { s/nOctave/octave/ s/nOctave++/++octave/ WebCore/svg/graphics/filters/SVGFETurbulence.cpp:277 + if (sum > 255) Maybe rewrite to use a combination of std::min / std::max? WebCore/svg/graphics/filters/SVGFETurbulence.cpp:294 + float point[2]; Maybe use two seperated variables here, as "++point[x]" looks a bit awkward below. Or even switch to use a "FloatPoint" datatype. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:301 + point[1] = filter->filterRegion().y(); Store the filterRegion() in a local variable, should be faster. WebCore/svg/graphics/filters/SVGFETurbulence.h:42 + static PassRefPtr<FETurbulence> create(TurbulanceType, float, float, int, float, bool); If you change the indention, reindent the whole file please. WebCore/svg/graphics/filters/SVGFETurbulence.h:75 + static const int blockSize = 256; All these variables need to have either a "s_" or a "g" prefix and should be moved to the .cpp file - no need to be in the header. WebCore/svg/graphics/filters/SVGFETurbulence.h:78 + static const long randMaximum = 2147483647; /* 2**31 - 1 */ You can use // c++ style comments here. WebCore/svg/graphics/filters/SVGFETurbulence.h:83 + struct PaintingData { Maybe add a constructor here, initilalizing all variables, better safe than sorry. Great first patch, looking forward to the next one! Keep up the good work!

> WebCore/svg/graphics/filters/SVGFETurbulence.cpp:119 > + inline long FETurbulence::PaintingData::random() > If you want to force inlining, use ALWAYS_INLINE here, consistent with the other filter effects. I disagree -- ALWAYS_INLINE should only be used if there's a measurable benefit in doing so -- the inline keyword doesn't necessarily guarantee inlining, but it does guarantee sensible linker behaviour. I don't like the overuse of ALWAYS_INLINE in the filter code. The purpose of ALWAYS_INLINE is to prevent the compiler from being able to make its own decisions _if_ the compiler is making a measurably incorrect choice. I have yet to see evidence of this occurring outside of JSC

Comment on attachment 62162 [details] SVG feTurbulence imeplementation Have you performance tested this? It looks like it will recompute the basis vectors for the noise function on every paint of every element.

(In reply to comment #9) > > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:119 > > + inline long FETurbulence::PaintingData::random() > > If you want to force inlining, use ALWAYS_INLINE here, consistent with the other filter effects. > > I disagree -- ALWAYS_INLINE should only be used if there's a measurable benefit in doing so -- the inline keyword doesn't necessarily guarantee inlining, but it does guarantee sensible linker behaviour. I don't like the overuse of ALWAYS_INLINE in the filter code. The purpose of ALWAYS_INLINE is to prevent the compiler from being able to make its own decisions _if_ the compiler is making a measurably incorrect choice. I have yet to see evidence of this occurring outside of JSC Okay, I'll keep this in mind for doing any further reviews. But using "inline" would still be fine? I think we should cleanup the use of ALWAYS_INLINE in the filter effects code, I have misinterpreted the macro, and thought one could always use it instead of inline.

(In reply to comment #10) > (From update of attachment 62162 [details]) > Have you performance tested this? It looks like it will recompute the basis vectors for the noise function on every paint of every element. The results of any filter operation is cached - just like all other resources (mask, clipper, etc.) per RenderObject. The filter will only be rebuild if it got invalidated (filter properties changed, client of the filter changed, etc..). Say you're using a filter on a rect with fixed coordinates (no percentages). Then the filter is never recalculated, unless, you zoom in or out. Panning, window size changes, etc.. don't influence the caching. These are all new concepts introduced while moving the resources/paintservers into the rendering/ tree. See RenderSVGResourceFilter for details on the caching, or (what might be easier to read) RenderSVGResourceMasker. Hope that solves your concerns?

(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 62162 [details] [details]) > > Have you performance tested this? It looks like it will recompute the basis vectors for the noise function on every paint of every element. > > The results of any filter operation is cached - just like all other resources (mask, clipper, etc.) per RenderObject. The filter will only be rebuild if it got invalidated (filter properties changed, client of the filter changed, etc..). > Say you're using a filter on a rect with fixed coordinates (no percentages). Then the filter is never recalculated, unless, you zoom in or out. Panning, window size changes, etc.. don't influence the caching. > > These are all new concepts introduced while moving the resources/paintservers into the rendering/ tree. See RenderSVGResourceFilter for details on the caching, or (what might be easier to read) RenderSVGResourceMasker. > > Hope that solves your concerns? The resource get cached, but olliej is still right. The basis vectors are recalculated for every element that uses the resource. The caching just avoids recalculating on redrawing the element. But I don't see a way to avoid this with the current design. All effects are owned by the filterBuilder, and every object has it's own filter and own filterBuilder.

> WebCore/svg/graphics/filters/SVGFETurbulence.cpp:121 > + long result = randAmplitude * (seed % randQ) - randR * (seed / randQ); > A comment explaining what happens here is very helpful. To tell the truth this implementation comes from the standard algorithm, with using those variable names. I renamed variables what were obvious. Any suggestions are welcome :) > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:143 > + paintingData.seed = -(paintingData.seed % (randMaximum - 1)) + 1; > A comment is needed here, it's probably from the spec, so it should be quoted. Ditto > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:148 > + for (k = 0; k < 4; ++k) { > This loop needs a comment on top, what happens inside the loop. Ditto. > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:176 > + int bx, by, b00, b10, b01, b11; > All these variables need better, descriptive names. Also there shouldn't be a need to declare them on top. > > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:177 > + float rx, ry, *q, sx, sy, a, b, t, u, v; > Ditto. Some of these variable are removed. The others are the same. > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:180 > + bx = static_cast<int>(t); > Why is this float -> int truncation desired? No need to round? No. According to the standard doesnt need to round. > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:198 > + bx &= blockMask; > This needs a comment. > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:207 > + sx = smoothCurve(rx); > Ditto, should explain what is smoothed, why it's smoothed. This is the standard :) > As above, s/loFreq/lowFrequency, s/floor/floorf/. Also either needs an assertion that tileHeight is non-zero, or an abortion in that case. Ok, tileSize is checked. > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:242 > + if (m_baseFrequencyY / loFreq < hiFreq / m_baseFrequencyY) > Isn't it possible that lowFrequency gets rounded to 0, if it's very small? Maybe check here, to avoid a possible division by zero. It's checked a few lines above: if (m_baseFrequencyX()) > WebCore/svg/graphics/filters/SVGFETurbulence.cpp:248 > + paintingData.width = static_cast<int>(tileWidth * m_baseFrequencyX + 0.5); > Why the + 0.5 value? Isn't it possible to use roundf in the following 3 lines as well? It converts the float value to the higher integer. Could be changed to roundf but I think it would be slower. > WebCore/svg/graphics/filters/SVGFETurbulence.h:75 > + static const int blockSize = 256; > All these variables need to have either a "s_" or a "g" prefix and should be moved to the .cpp file - no need to be in the header. The problem is that blockSize variable is used by struct definition in .h. Since it has to stay there but the others could go to the .cpp, if you think so. The other comments of your were very useful, thanks for them. =) > Great first patch, looking forward to the next one! Keep up the good work! Thanks =)

Created attachment 62688 [details] SVG feTurbulence imeplementation

Comment on attachment 62688 [details] SVG feTurbulence imeplementation Some issues from the last review are still present: 1) The "No new tests" statment is probably wrong, as it's covered by at least the existing W3C SVG tests. It probably also changes other test results, did you run the layout tests? It's necessary to run them on a Mac, to produce new pixel test results (ask Zoltan, who knows about the procedure) 2) WebCore/svg/graphics/filters/SVGFETurbulence.h:41 + static PassRefPtr<FETurbulence> create(TurbulanceType, float, float, int, float, bool); The indention is not correct, you either have to change the whole file, or leave at is - mixed indention is not correct :-) And some new ones: WebCore/svg/graphics/filters/SVGFETurbulence.h:5 + 2005 Dirk Schulze <krit@webkit.org> I guess you meant 2009 here? WebCore/svg/graphics/filters/SVGFETurbulence.h:66 + static const int blockSize = 256; static variables need a s_ prefix, so it should be s_blockSize. WebCore/svg/graphics/filters/SVGFETurbulence.h:67 + static const int blockMask = blockSize - 1; Ditto, should be named s_blockMask. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:46 + static const int perlinNoise = 4096; All these statics need the s_ prefix. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:53 + int numOctaves, float seed, bool stitchTiles) No need to wrap lines here, just make a long line. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:162 + // The seed value clamp to the range 1 to (randMaximum - 1). The seed value is clamped to the range [1, randMaximum - 1]. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:208 + int b; These are still not clear to me. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:209 + float r; Ditto. Really needs a better name :( WebCore/svg/graphics/filters/SVGFETurbulence.cpp:238 + int tmp = paintingData.latticeSelector[latticeIndex + noiseY.b]; Maybe add a comment here that this is 1:1 taken from the SVG spec, with another reference to http://www.w3.org/TR/SVG11/filters.html#feTurbulenceElement. Rename tmp to temp at least :-) WebCore/svg/graphics/filters/SVGFETurbulence.cpp:255 + unsigned char FETurbulence::calculateTurbulenceValueForPoint(PaintingData& paintingData, FloatPoint point) FloatPoint should be passed as const reference: const FloatPoint& point, to avoid copying. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:257 + float tileWidth = paintingData.filterSize.width(); I see that you're early returning in another place when the width() is zero. Still add an assertion here for tileWidth > 0, as it makes the code easier to read. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:258 + float tileHeight = paintingData.filterSize.height(); Ditto. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:267 + ASSERT(m_baseFrequencyX >= 0); Ok this is unncessary as you said on IRC, as you're checking if (m_baseFrequencyX) before. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:276 + ASSERT(m_baseFrequencyY >= 0); Ditto. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:282 + // Set up TurbulenceInitial stitch values. It's cast to the higher interer value. s/interere/integer/. I'd really appreciate if we'd use roundf though, as these hacks are harder to maintain, we're not using this + 0.5 tricks in other places, but round. As last resort, you could add a comment that the SVG spec pseudo-code does it this way, and you're just following it for the sake of compatibility. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:290 + float turbFunctionResult = 0; s/turbFunction/turbulenceFunction/. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:312 + turbFunctionResult = std::max(std::min(turbFunctionResult, static_cast<float>(255)), static_cast<float>(0)); maybe use 255.f here, and 0.f instead of casting to float. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:314 + return static_cast<unsigned char>(turbFunctionResult * 127.5f + 127.5f); // It comes form (turbFunctionResult * 255 + 255) / 2 s/form/from. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:322 + if (!resultImage()->size().width() || !resultImage()->size().height()) I'd optimize for the common case: Swap the order, first: IntRect imageRect(IntPoint(), resultImage()->size()); then: if (!imageRect.size().width() || !imageRect.size().height()) return; instead of calling resultImage()->size() multiple times. Looking forward to the next patch.

Created attachment 62872 [details] SVG feTurbulence imeplementation

Okay, I think I have made all of your suggestions :)

Attachment 62872 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] WebCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Total errors found: 8 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 62876 [details] SVG feTurbulence imeplementation

Comment on attachment 62876 [details] SVG feTurbulence imeplementation WebCore/svg/graphics/filters/SVGFETurbulence.cpp:34 + #include <math.h> Can you use <wtf/MathExtras.h> here? math.h shouldn't be used. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:212 + float tempPosition = component + s_perlinNoise; Maybe rename to just 'position', looks slightly better. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:327 + FloatPoint point; Can you move this declaration after the "FloatRect filterRegion = .." line. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:326 + int indexOfPixelChannel = 0; This declaration could be moved right before the for loop. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:329 + Newline can be removed as well. WebCore/svg/graphics/filters/SVGFETurbulence.h:41 + static PassRefPtr<FETurbulence> create(TurbulanceType, float, float, int, float, bool); Header indention is still wrong :-) Almost there, r- for the issues above.

Renata, can you update http://webkit.org/projects/svg/status.xml once this patch got landed, please? This can be done in an followup. It looks like all effects in the SVG Filter section can turn to green now :-) Thanks. A question to the implementation. Did you check the implementation with the second version of the SVG test suite? They added a new test for feTurbulence: http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/filters-turb-02-f.html

(In reply to comment #22) > Renata, can you update http://webkit.org/projects/svg/status.xml once this patch got landed, please? This can be done in an followup. It looks like all effects in the SVG Filter section can turn to green now :-) Thanks. Sure. I'll do it :) > A question to the implementation. Did you check the implementation with the second version of the SVG test suite? They added a new test for feTurbulence: http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/filters-turb-02-f.html Yeah, I checked it as well. The result of this was fine. Should I create a new test from it into WebKit too?

(In reply to comment #23) > (In reply to comment #22) > > Renata, can you update http://webkit.org/projects/svg/status.xml once this patch got landed, please? This can be done in an followup. It looks like all effects in the SVG Filter section can turn to green now :-) Thanks. > Sure. I'll do it :) > > > A question to the implementation. Did you check the implementation with the second version of the SVG test suite? They added a new test for feTurbulence: http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/filters-turb-02-f.html > Yeah, I checked it as well. The result of this was fine. Should I create a new test from it into WebKit too? I'd like to move the complete new test suite in, once it gets released. But if you think it provides more information than the other test, you can of course add them. Add as many tests as you want:-)

Created attachment 62942 [details] SVG feTurbulence imeplementation

Comment on attachment 62942 [details] SVG feTurbulence imeplementation Sorry Renata, I missed some obvious issues before: WebCore/svg/graphics/filters/SVGFETurbulence.cpp:131 + FETurbulence::PaintingData::PaintingData(long m_seed, IntSize filterSize) You shouldn't name arguments passed to a struct with a m_. Use "long paintingSeed" and "const IntSize& paintingSize". WebCore/svg/graphics/filters/SVGFETurbulence.cpp:134 + filterSize = filterSize; Ouch! This can't work :-) I've no idea why this ever worked. I'd suggest to initialize _all_ variables to be sure they are never read uninitialized: FETurbulence::PaintingData::PaintingData(long paintingSeed, const IntSize& paintingSize) : seed(paintingSeed) , filterSize(paintingSize) , width(0) , height(0) , wrapX(0) , wrapY(0) , channel(0) { } WebCore/svg/graphics/filters/SVGFETurbulence.h:34 + FETURBULENCE_TYPE_UNKNOWN = 0, We've stopped lining up these values, just remove the spaces inbetween: FETURBULENCE_TYPE_UNKNOWN = 0, .. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:204 + float FETurbulence::noise2D(PaintingData& paintingData, FloatPoint noiseVector) FloatPoint -> const FloatPoint&. Okay, I hope these are the last problems, sorry for forcing you for yet another iteration.

Comment on attachment 62942 [details] SVG feTurbulence imeplementation Another small comment: WebCore/svg/graphics/filters/SVGFETurbulence.cpp:237 + // This is 1:1 taken implementation of SVG spec: http://www.w3.org/TR/SVG11/filters.html#feTurbulenceElement. // This is taken 1:1 from the SVG spec... sounds slightly better.

Created attachment 62949 [details] SVG feTurbulence imeplementation

Comment on attachment 62949 [details] SVG feTurbulence imeplementation Hey Renata, looks good to me, though I can't r+, as your patch contains unrelated changes to ResourceHandleMac.mm. Two issues remaining: WebCore/svg/graphics/filters/SVGFETurbulence.cpp:140 + seed = paintingSeed; This is superflous, you're already initializing seed a few lines above. WebCore/svg/graphics/filters/SVGFETurbulence.cpp:141 + filterSize = paintingSize; Ditto. Please upload a new version, and I'll r+ it. Can you commit on your own? If not, set cq? next time you upload.

Created attachment 62993 [details] SVG feTurbulence imeplementation

Comment on attachment 62993 [details] SVG feTurbulence imeplementation Looks great, r=me!

Comment on attachment 62993 [details] SVG feTurbulence imeplementation Clearing flags on attachment: 62993 Committed r64340: <http://trac.webkit.org/changeset/64340>

All reviewed patches have been landed. Closing bug.