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

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62688|review?                     |review-
               Flag|                            |




--- Comment #16 from Nikolas Zimmermann <zimmermann at kde.org>  2010-07-27 08:08:50 PST ---
(From update of attachment 62688)
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 at 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.

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