[Webkit-unassigned] [Bug 49907] Better result passing in filter primitives

[bugzilla-daemon at webkit.org]

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


Nikolas Zimmermann <zimmermann at kde.org> changed:

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




--- Comment #15 from Nikolas Zimmermann <zimmermann at kde.org>  2010-11-29 12:14:56 PST ---
(From update of attachment 74995)
View in context: https://bugs.webkit.org/attachment.cgi?id=74995&action=review

I love the general idea of this patch! Some comments below, Dirk might have additional ones:

> WebCore/ChangeLog:8
> +        SVN filter primitives can use the output of other filters. The

s/SVN/SVG/

> WebCore/ChangeLog:11
> +        primitive result was converted to image buffer before, which

s/result was/results were/, s/buffer/buffers/

> WebCore/ChangeLog:15
> +        this patch. All apply() methods updated according to this new

s/updated/are updated/

> WebCore/platform/graphics/filters/FEBlend.cpp:-135
> -    resultImage()->putPremultipliedImageData(imageData.get(), imageRect, IntPoint());

Ah, this is correct, as you're using the resultImage instead of creating a new one.

> WebCore/platform/graphics/filters/FEComponentTransfer.cpp:172
> +    in->asUnmultipliedImage(imageData, drawingRect);

Hm, why is the return value ignored?
(Ignore this, I was confused as there are two variants of asUnmultipliedImage, this one returns void.... see below)

> WebCore/platform/graphics/filters/FilterEffect.cpp:84
> +    m_imageBufferResult = ImageBuffer::create(m_absolutePaintRect.size(), ColorSpaceLinearRGB);
> +    IntRect dst(IntPoint(), m_absolutePaintRect.size());

line 84: s/dst/destinationRect/

> WebCore/platform/graphics/filters/FilterEffect.cpp:96
> +    return imageData;

A call to .release() is missing here!

> WebCore/platform/graphics/filters/FilterEffect.cpp:103
> +    return imageData;

Ditto.

> WebCore/platform/graphics/filters/FilterEffect.cpp:106
> +inline void FilterEffect::copyImageBytes(ImageData* src, ImageData* dst, const IntRect& rect)

Avoid abbrevations, while it is convienent here, I'd prefer source and destination.

> WebCore/platform/graphics/filters/FilterEffect.cpp:113
> +    int originx = rect.x();

I'd prefer originX or xOrigin here.

> WebCore/platform/graphics/filters/FilterEffect.cpp:114
> +    int destx = 0;

Same here, destX or xDest (I'd prefer the latter ;-)

> WebCore/platform/graphics/filters/FilterEffect.cpp:119
> +    int endx = rect.x() + rect.width();

rect.right() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:129
> +    int endy = rect.y() + rect.height();

rect.bottom() ?

> WebCore/platform/graphics/filters/FilterEffect.cpp:137
> +    int dstScanline = rect.width() * 4;
> +    int srcScanline = m_absolutePaintRect.width() * 4;
> +    unsigned char *dstPixel = dst->data()->data()->data() + (((desty * rect.width()) + destx) * 4);
> +    unsigned char *srcPixel = src->data()->data()->data() + (((originy * m_absolutePaintRect.width()) + originx) * 4);

unsigned char* destinationPixel,  and one useless pair of braces at the end. Same for sourcePixel.
dstScanline should be "destinationScanline", same for srcScanline.

> WebCore/platform/graphics/filters/FilterEffect.cpp:143
> +        originy++;

I'd prefer ++originy.

> WebCore/platform/graphics/filters/FilterEffect.cpp:151
> +    if (!m_unmultipliedImageResult) {

Can you rewrite this with a early return for m_unmultipliedImageResult=true.

> WebCore/platform/graphics/filters/FilterEffect.cpp:159
> +            unsigned char* src = m_premultipliedImageResult->data()->data()->data();
> +            unsigned char* dst = m_unmultipliedImageResult->data()->data()->data();
> +            unsigned char* end = src + (m_absolutePaintRect.width() * m_absolutePaintRect.height() * 4);

Again, src -> source, dst -> destination etc.

> WebCore/platform/graphics/filters/FilterEffect.cpp:184
> +    if (!m_premultipliedImageResult) {

Same comments as abobe, early return style please, and variable renamings.

> WebCore/platform/graphics/filters/FilterEffect.h:58
> +    PassRefPtr<ImageData> asUnmultipliedImage(const IntRect&);
> +    PassRefPtr<ImageData> asPremultipliedImage(const IntRect&);
> +    void asUnmultipliedImage(ImageData* dst, const IntRect&);
> +    void asPremultipliedImage(ImageData* dst, const IntRect&);

The naming of these methods is misleading, I stared at 172     in->asUnmultipliedImage(imageData, drawingRect); before, wondering why you ignore the returned PassRefPtr<ImageData> until I realized there are two variants of the same method, one which returns void and the other with the PassRefPtr.

Can you rename the last two methods, to something like "convertToXXXImage" or maybe even a better name..
Also the "dst" parameter name is not needed.

> WebCore/platform/graphics/filters/FilterEffect.h:132
> +    inline void copyImageBytes(ImageData* src, ImageData* dst, const IntRect&);

s/src/source/ s/dst/destination/

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