[webkit-reviews] review denied: [Bug 48212] SVG FELighting performance issues : [Attachment 71991] draft patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 48212: SVG FELighting performance issues
https://bugs.webkit.org/show_bug.cgi?id=48212

Attachment 71991: draft patch
https://bugs.webkit.org/attachment.cgi?id=71991&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71991&action=review

Have you measured the performance with ByteArray*? I couldn't see a difference
between usigned char* and ByteArray* on feGaussianBlur with shark. Of course
you save some address calculations on using unsigned char* (pixels[0] = ...
pixels[1]), but would still interest me you see a difference.
The changes on FloatPoint3D are ok.

> WebCore/platform/graphics/filters/FELighting.cpp:95
> +inline void FELighting::LightingData::topRight(int offset, IntPair& intPair)

>  {
> -    return static_cast<int>(pixels->get(offset - widthMultipliedByPixelSize
+ cPixelSize + cAlphaChannelOffset));
> +    unsigned char* pixel = pixels + offset;
> +    int left = static_cast<int>(pixel[-cPixelSize + cAlphaChannelOffset]);
> +    int center = static_cast<int>(pixel[cAlphaChannelOffset]);
> +    pixel += widthMultipliedByPixelSize;
> +    int bottomLeft = static_cast<int>(pixel[-cPixelSize +
cAlphaChannelOffset]);
> +    int bottom = static_cast<int>(pixel[cAlphaChannelOffset]);
> +    intPair.normalX = -(left << 1) + (center << 1) - bottomLeft + bottom;
> +    intPair.normalY = -left - (center << 1) + bottomLeft + (bottom << 1);
>  }

Is there no way to share more of the code with the other edge functions?

> WebCore/platform/graphics/filters/FELighting.cpp:196
> +	       if (m_specularExponent == 1.0f)

== 1

> WebCore/platform/graphics/filters/FELighting.cpp:205
> +	   normalVector.setZ(1.0f);

just ...setZ(1);

> WebCore/platform/graphics/filters/FELighting.cpp:214
> +	       if (m_specularExponent == 1.0f)

== 1

> WebCore/platform/graphics/filters/FELighting.cpp:-128
> -    if (data.lightStrength > 1.0f)
> -	   data.lightStrength = 1.0f;
> -    if (data.lightStrength < 0.0f)
> -	   data.lightStrength = 0.0f;

1 and 0

> WebCore/platform/graphics/filters/FELighting.cpp:237
> +    IntPair intPair;
> +    int offset;

Please initialize these values. And declare them on the first use.

> WebCore/platform/graphics/filters/FELighting.cpp:258
> +    setPixel(offset, data, paintingData, 0, 0, -2.0f / 3.0f, -2.0f / 3.0f,
intPair);

Normally the compiler should optimize it, but can you add a static const for -2
/ 3.f ? This is reused multiple times. The same for some other constant
calculations, a name is also easier to understand.

> WebCore/platform/graphics/filters/FELighting.cpp:346
>      RefPtr<ImageData>
srcImageData(in->resultImage()->getUnmultipliedImageData(effectDrawingRect));

change srcImageData(...) to srcImageData = ... please

> WebCore/platform/graphics/filters/FELighting.h:57
> +    struct IntPair {
> +	   int normalX;
> +	   int normalY;
> +    };

Do you really see a noticeable performance win compared to IntPoint?