[Webkit-unassigned] [Bug 5864] feTurbulence is not implemented

Wed Jul 21 04:16:10 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=5864

Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62162|                            |review-
               Flag|                            |

--- Comment #8 from Nikolas Zimmermann <zimmermann at kde.org>  2010-07-21 04:16:09 PST ---
(From update of attachment 62162)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.