[webkit-reviews] review denied: [Bug 42273] Convolution computation causes bad alpha channel values : [Attachment 61614] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 42273: Convolution computation causes bad alpha channel values
https://bugs.webkit.org/show_bug.cgi?id=42273

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
LayoutTests/ChangeLog:5
 +	    Bug https://bugs.webkit.org/show_bug.cgi?id=42273
Please rerun, prepare-ChangeLog --bug=42273. It generates the correct template
to be used in WebKit ChangeLogs.
It will list the bug title, that's currently missing here, and leads to
confusion.

WebCore/ChangeLog:5
 +	    Bug https://bugs.webkit.org/show_bug.cgi?id=42273
Ditto.

WebCore/platform/graphics/skia/SkiaUtils.cpp:89
 +	    // A 0 alpha when there are non-zero R, G, or B channels is an 
I'd use "A zero alpha value" instead of "A 0 alpha".

WebCore/platform/graphics/skia/SkiaUtils.cpp:90
 +	    // invalid premultiplid color (since all channels should have been
Typo, premultiplied.

WebCore/platform/graphics/skia/SkiaUtils.cpp:91
 +	    // multiplied by 0 if a=0). Assert(false) to catch in debug mode.
I don't think you need to mention that you're asserting, pretty clear from the
line below :-)

WebCore/platform/graphics/skia/SkiaUtils.cpp:93
 +	    // In production, return 0 to protect against divide by 0.
rephrase, to protect against division by zero.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:207
 +  ALWAYS_INLINE void setDestinationPixels(CanvasPixelArray *image, int
*pixel, float *totals, float divisor, float bias, CanvasPixelArray *src)
Style issues: the star * goes next to the type. I'd suggest to use int& pixel,
to avoid, using (*pixel)++, see the next comments.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:209
 +	for (int i = 0; i < 3 + !preserveAlphaValues; ++i)
I hope this compiles on Windows, it's sometimes picky about these contructs, to
be safe, I'd use: "3 + (preserveAlphaValues ? 0 : 1)". That's also used in
other parts of the code.


WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:214
 +	    image->set((*pixel)++, clampRGBAValue(totals[0], alpha));
Okay, this should really read: image->set(pixel++, clampRGBAValue(....)), just
pass the pixel int by reference, that's faster and avoids the unnecessary
dereference operator usage.

I still think the function could be more readable, suggestion:

template<bool preserveAlphaValues>
ALWAYS_INLINE void setDestinationPixels(CanvasPixelArray* image, int& pixel,
float* totals, float divisor, float bias, CanvasPixelArray* src)
{
    unsigned char maxAlpha = 255;
    if (!preserveAlphaValues)
	maxAlpha = clampRGBAValue(totals[3]);

    for (int i = 0; i < 3; ++i)
	image->set(pixel++, clampRGBAValue(totals[i], maxAlpha);

    image->set(pixel + 1, preserveAlphaValues ? src->get(pixel) : alpha);
    ++pixel;
}

Note that I've decided to not use the "image->set(pixel++ ? src->get(pixel) :
alpha)" approach, as I think it's hard to read, better be explicit than
confusing.
What do you think?

Other than that, it's a nice patch! Looking forward to the next one.