Bug 110193 - OpenCL implementation of FEMerge filter.
Summary: OpenCL implementation of FEMerge filter.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 70099
  Show dependency treegraph
 
Reported: 2013-02-19 01:42 PST by Tamas Czene
Modified: 2013-02-21 02:44 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (11.29 KB, patch)
2013-02-19 01:44 PST, Tamas Czene
zherczeg: review-
zherczeg: commit-queue-
Details | Formatted Diff | Diff
proposed patch (11.35 KB, patch)
2013-02-19 06:27 PST, Tamas Czene
no flags Details | Formatted Diff | Diff
proposed patch (11.25 KB, patch)
2013-02-20 00:34 PST, Tamas Czene
zherczeg: review-
zherczeg: commit-queue-
Details | Formatted Diff | Diff
proposed patch (11.21 KB, patch)
2013-02-20 06:34 PST, Tamas Czene
no flags Details | Formatted Diff | Diff
proposed patch (11.21 KB, patch)
2013-02-21 00:49 PST, Tamas Czene
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tamas Czene 2013-02-19 01:42:28 PST
This patch contains the OpenCL implementation of FEMerge filter.
Comment 1 Tamas Czene 2013-02-19 01:44:55 PST
Created attachment 189025 [details]
proposed patch
Comment 2 Zoltan Herczeg 2013-02-19 02:49:15 PST
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.
Comment 3 Tamas Czene 2013-02-19 06:27:04 PST
Created attachment 189075 [details]
proposed patch
Comment 4 Tamas Czene 2013-02-20 00:34:06 PST
Created attachment 189262 [details]
proposed patch
Comment 5 Zoltan Herczeg 2013-02-20 01:46:27 PST
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.
Comment 6 Tamas Czene 2013-02-20 06:34:35 PST
Created attachment 189307 [details]
proposed patch
Comment 7 Zoltan Herczeg 2013-02-20 06:48:30 PST
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.
Comment 8 Tamas Czene 2013-02-21 00:49:28 PST
Created attachment 189468 [details]
proposed patch
Comment 9 Zoltan Herczeg 2013-02-21 02:34:05 PST
Comment on attachment 189468 [details]
proposed patch

r=me
Comment 10 WebKit Review Bot 2013-02-21 02:44:47 PST
Comment on attachment 189468 [details]
proposed patch

Clearing flags on attachment: 189468

Committed r143578: <http://trac.webkit.org/changeset/143578>
Comment 11 WebKit Review Bot 2013-02-21 02:44:51 PST
All reviewed patches have been landed.  Closing bug.