This patch contains the OpenCL implementation of FEMerge filter.
Created attachment 189025 [details] proposed patch
Comment on attachment 189025 [details] proposed patch 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.
Created attachment 189075 [details] proposed patch
Created attachment 189262 [details] proposed patch
Comment on attachment 189262 [details] proposed patch Few more comments: View in context: https://bugs.webkit.org/attachment.cgi?id=189262&action=review > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:39 > +__kernel void copy(__write_only image2d_t destination, __read_only image2d_t source, int x, int y, int width, int height) width, height is unused argument. > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:77 > +inline void FilterContextOpenCL::applyFEMergeCopy(OpenCLHandle destination, IntSize destinationSize, OpenCLHandle source, IntRect relativeSourceRect) Use references such as IntSize& > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:85 > + kernel.addArgument(relativeSourceRect.width()); > + kernel.addArgument(relativeSourceRect.height()); Ditto. > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:124 > + IntRect relativeSourceRect(in->absolutePaintRect()); use IntPoint when possible.
Created attachment 189307 [details] proposed patch
Comment on attachment 189307 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=189307&action=review > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:39 > +const sampler_t sampler = CLK_NORMALIZED_COORDS_FALSE | CLK_ADDRESS_CLAMP | CLK_FILTER_NEAREST; > +__kernel void copy(__write_only image2d_t destination, __read_only image2d_t source, int x, int y) Newline after the sampler. > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:77 > +inline void FilterContextOpenCL::applyFEMergeCopy(OpenCLHandle destination, IntSize destinationSize, OpenCLHandle source, IntPoint &relativeSourcePoint) The & sign should be pert of the type: IntPoint& relativeSourcePoint IntSize& destinationSize > Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEMerge.cpp:87 > +inline void FilterContextOpenCL::applyFEMerge(OpenCLHandle destination, OpenCLHandle previousDestination, OpenCLHandle sourceA, OpenCLHandle sourceB, IntSize destinationSize, IntPoint &relativeSourcePointA, IntPoint &relativeSourcePointB) Ditto.
Created attachment 189468 [details] proposed patch
Comment on attachment 189468 [details] proposed patch r=me
Comment on attachment 189468 [details] proposed patch Clearing flags on attachment: 189468 Committed r143578: <http://trac.webkit.org/changeset/143578>
All reviewed patches have been landed. Closing bug.