[webkit-reviews] review denied: [Bug 110193] OpenCL implementation of FEMerge filter. : [Attachment 189025] proposed patch

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has denied Tamas Czene
<tczene at inf.u-szeged.hu>'s request for review:
Bug 110193: OpenCL implementation of FEMerge filter.
https://bugs.webkit.org/show_bug.cgi?id=110193

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

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
Nice work, mostly style comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=189025&action=review


> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:59
> +	   , m_copy(0)

m_mergeCopyOperation

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:99
> +    inline void copy(OpenCLHandle, IntSize, OpenCLHandle, IntRect);

applyFEMergeCopy

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:42
> +    float4 destinationPixel = sourcePixel + destinationPixel * (1 -
sourcePixel.w);

What is the purpose of this line? Especially as this is a copy operation? The
sourcePixel should be renamed to destinationPixel, and no need for any
modifications.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:48
> +    int2 coord = (int2) (get_global_id(0), get_global_id(1));

destinationCoord

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:57
> +
> +    destinationPixel = sourcePixelA + destinationPixel * (1 -
sourcePixelA.w);
> +
> +    destinationPixel = sourcePixelB + destinationPixel * (1 -
sourcePixelB.w);

We don't need that many empty lines here.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:59
> +    write_imagef(previousDestination, coord, destinationPixel);

??? You write data to the previous destination? The destination should be the
target of the operation, and previousDestination should be the source.
__write_only image2d_t destination

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:138
> +    OpenCLHandle previousDestination =
context->createOpenCLImage(absolutePaintRect().size());
> +    context->fill(previousDestination, absolutePaintRect().size(),
Color(0.0f, 0.0f, 0.0f, 0.0f));

OpenCLHandle previousDestination = destination;
destination = context->createOpenCLImage(absolutePaintRect().size());

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:153
> +	   context->applyFEMerge(destination, previousDestination,
absolutePaintRect().size(), sourceA, relativeSourceRectA, sourceB,
relativeSourceRectB);

This does not match to __kernel void merge. The applyFEMerge should have to
same order of arguments.