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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Renata Hodovan <hodovan at inf.u-szeged.hu>  2010-07-27 07:21:52 PST ---
> 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 =)

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