[webkit-reviews] review granted: [Bug 92123] Fix arithmetic composite filter for auto-vectorization : [Attachment 154293] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 92123: Fix arithmetic composite filter for auto-vectorization
https://bugs.webkit.org/show_bug.cgi?id=92123

Attachment 154293: Patch
https://bugs.webkit.org/attachment.cgi?id=154293&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154293&action=review


Looks good, r=me with minor suggestions:

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:167
> +	   scaledK1 = k1 / 255.f;

Nitpick, we use 255.0f in WebCore/SVG code.

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:189
> +    float upperLimit = fmaxf(0.f, k1) + fmaxf(0.f, k2) + fmaxf(0.f, k3) +
k4;

Any specific reason for not using std::max here? Both parameters are floats.

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:193
> +    if (k4 >= 0.f && k4 <= 1.f
> +	   && upperLimit >= 0.f && upperLimit <= 1.f
> +	   && lowerLimit >= 0.f && lowerLimit <= 1.f) {

No need for additional lines here, we don't care for line lengths in SVG code.

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:198
> +	   if (!k4) {
> +	       if (!k1)
> +		   computeArithmeticPixelsUnclamped<0, 0>(source, destination,
pixelArrayLength, k1, k2, k3, k4);
> +	       else
> +		   computeArithmeticPixelsUnclamped<1, 0>(source, destination,
pixelArrayLength, k1, k2, k3, k4);

Can you swap all of these? if (k4) { if (k1) { ...} to avoid the negations.

> Source/WebCore/platform/graphics/filters/FEComposite.cpp:208
> +    if (!k4) {

Ditto.