[Webkit-unassigned] [Bug 5861] feConvolveMatrix filter is not implemented

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

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




--- Comment #30 from Nikolas Zimmermann <zimmermann at kde.org>  2010-06-30 05:12:52 PST ---
(From update of attachment 60097)
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?

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