[webkit-reviews] review granted: [Bug 99829] OpenCL version of SourceAlpha, SourceGraphics and FETurbulence filter effects : [Attachment 175437] proposed patch

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has granted Tamas Czene
<tczene at inf.u-szeged.hu>'s request for review:
Bug 99829: OpenCL version of SourceAlpha, SourceGraphics and FETurbulence
filter effects
https://bugs.webkit.org/show_bug.cgi?id=99829

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

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
Thanks for all comments, and thanks for the patch. r=me with a few comments:

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


> Source/WebCore/ChangeLog:9
> +	   This patch contains the OpenCL implementation of SourceAlpha,
SourceGraphic, FETurbulence and a simple OpenCL environment.
> +	   This is only included in Qt linux build.

Please mention here the speedup. And a little more verbose about the patch.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:38
> +    static bool initFailed = false;

This name should int alreadyInitialized

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:52
> +    size_t deviceBufferSize = -1;

Here deviceBufferSize is unsigned...

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:67
> +    if (deviceBufferSize <= 0)

And you compare it with <= 0. I think you could simply set it to 0, and change
this comparison to !deviceBufferSize.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:70
> +    devices = reinterpret_cast<cl_device_id
*>(fastMalloc(deviceBufferSize));

cl_device_id* without space

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:120
> +void FilterContextOpenCL::openCLTransformColorSpace(OpenCLHandle& source,
IntRect sourceSize, ColorSpace srcColorSpace, ColorSpace dstColorSpace)

This is just a note, but perhaps you could try sometimes the performance of
doing the computation in the kernel without passing an array.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLHandle.h:30
> +    OpenCLHandle() : m_openclMemory(0) { }

openCLImage and m_openclMemory names are not consistent. Use openCL everywhere
(there are some other identifiers where this renaming is needed).