[Webkit-unassigned] [Bug 42273] Convolution computation causes bad alpha channel values

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61614|review+                     |review-
               Flag|                            |




--- Comment #18 from Nikolas Zimmermann <zimmermann at kde.org>  2010-07-15 00:52:59 PST ---
(From update of attachment 61614)
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.

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