[Webkit-unassigned] [Bug 99829] OpenCL version of SourceAlpha, SourceGraphics and FETurbulence filter effects

[bugzilla-daemon at webkit.org]

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


Matt Arsenault <arsenm2 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |arsenm2 at gmail.com




--- Comment #16 from Matt Arsenault <arsenm2 at gmail.com>  2012-11-23 01:15:29 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=175437&action=review

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:141
> +

These look like they don't account for the possibility that the kernel will fail to build. If the build fails, you'll be setting arguments / trying to run a null kernel

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:32
> +#include "CL/cl.h"

On OS X This is under <OpenCL/cl.h>

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:104
> +            {
> +                clFinish(m_context->m_commandQueue);
> +                clEnqueueNDRangeKernel(m_context->m_commandQueue, m_kernel, 2, 0, m_globalSize, 0, 0, 0, 0);
> +            }

These should really be checked. These are pretty likely to fail.

I also don't approve of using the default group size since that tends to not perform well (although choosing an appropriate size is unfortunately somewhat difficult)

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:58
> +    return a + t * (b - a);

return mad(b - a, t, a);?

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:86
> +

I suspect that a lot of your indexing calculations are candidates for mad24() if you know your sizes require < 24 bits.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:114
> +    StitchData stitchData;
> +    stitchData.width = 0;
> +    stitchData.wrapX = 0;
> +    stitchData.height = 0;
> +    stitchData.wrapY = 0;

How about StitchData stitchData = { 0, 0, 0, 0 };?

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:126
> +            if (baseFrequencyX / lowFrequency < highFrequency / baseFrequencyX)
> +                baseFrequencyX = lowFrequency;
> +            else
> +                baseFrequencyX = highFrequency;

For conditional assignments like this it would probably be better to use ternary operator or select() function.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:135
> +            if (baseFrequencyY / lowFrequency < highFrequency / baseFrequencyY)
> +                baseFrequencyY = lowFrequency;
> +            else
> +                baseFrequencyY = highFrequency;
> +        }

Same.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:161
> +        if (type == 1) {
> +            turbulenceFunctionResult.x += noise2D(redComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles) / ratio;
> +            turbulenceFunctionResult.y += noise2D(greenComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles) / ratio;
> +            turbulenceFunctionResult.z += noise2D(blueComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles) / ratio;
> +            turbulenceFunctionResult.w += noise2D(alphaComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles) / ratio;
> +        } else {
> +            turbulenceFunctionResult.x += fabs(noise2D(redComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles)) / ratio;
> +            turbulenceFunctionResult.y += fabs(noise2D(greenComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles)) / ratio;
> +            turbulenceFunctionResult.z += fabs(noise2D(blueComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles)) / ratio;
> +            turbulenceFunctionResult.w += fabs(noise2D(alphaComponent, latticeSelector, stitchData, noiseVectorX, noiseVectorY, stitchTiles)) / ratio;
> +        }

Move the common noise2D part ahead of the if, and then instead of the if do something like turbuenceFunctionResult += (type == 1) ? result : fabs(result)

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:169
> +            stitchData.wrapX = 2 * stitchData.wrapX - s_perlinNoise;

How about stitchData.wrapX = mad(2, stitchData.wrapX, -s_perlineNoise)?

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:176
> +        turbulenceFunctionResult = turbulenceFunctionResult * 0.5f + 0.5f;

mad(0.5f, turbulenceFunctionResult, 0.5f)?

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:223
> +    kernel.run();

Where do you wait for the kernel to complete? The only clFinish/clWaitForEvents I see is before a clEnqueueNDRangeKernel

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