Bug 70350 – Make FilterEffect::apply() independent of image data management

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

FilterEffect::apply() needs to be independent from the image data management in FilterEffect. This allows implementing effective hardware accelerated alternatives, like OpenGL, CI or OpenCL based filters, to the software rendering. The software rendering code will be used as fallback and moves to platformApplyGeneric().

[Dirk Schulze]

Created attachment 111485 [details]
Patch

[WebKit Review Bot]

Comment on attachment 111485 [details]
Patch

Attachment 111485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10122409

[Gyuyoung Kim]

Comment on attachment 111485 [details]
Patch

Attachment 111485 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10141002

[Dirk Schulze]

Created attachment 111490 [details]
Patch

[Zoltan Herczeg]

Comment on attachment 111490 [details]
Patch

The patch looks good, but I am thinking of the following:

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

> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:110
> +inline void FEGaussianBlur::platformApplyGenericBlur(ByteArray* srcPixelArray, ByteArray* tmpPixelArray, unsigned kernelSizeX, unsigned kernelSizeY, IntSize& paintSize)

We need a better naming here, since we should append Blur to the Neon function as well to avoid naming inconsistency.

I would prefer mention that it is a software based option and rename it to: platformApplySoftwareRendering(...) or platformApplySoftwareRender(...) or something similar.

And do this for all filter effects of course.

[Dirk Schulze]

Created attachment 111597 [details]
Patch

[Zoltan Herczeg]

Comment on attachment 111597 [details]
Patch

r=me with the followings:

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

> Source/WebCore/platform/graphics/filters/FEMorphology.cpp:199
>      if (hasResult())
>          return;

Delete hasResult() here

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:92
> +bool FilterEffect::hasResult() const
> +{
> +    // This function needs platform specific checks, if the memory managment is not done by FilterEffect.
> +    return m_imageBufferResult || m_unmultipliedImageResult || m_premultipliedImageResult;
> +}

I think this is still need to be inline. Mush faster.

[Dirk Schulze]

Committed r97853: <http://trac.webkit.org/changeset/97853>

[Simon Fraser (smfr)]

I don't like the platformApplySoftware() software name. First, what software is being applied?

But my main objection is that the software/hardware distinction is fuzzy. Does "running on the GPU" imply hardware? Is that really so different from running on the CPU?. If you start using OpenCL, you may not even know what's happening behind the scenes.

[Dirk Schulze]

(In reply to comment #9)
> I don't like the platformApplySoftware() software name. First, what software is being applied?
> 
> But my main objection is that the software/hardware distinction is fuzzy. Does "running on the GPU" imply hardware? Is that really so different from running on the CPU?. If you start using OpenCL, you may not even know what's happening behind the scenes.

No, there is no opposite like "platformApplyHardware()". Software stands for software rendering here. Valid names for other implementations could be 'platformApplyOpenGL', 'platformApplyOpenCL' or 'platformApplyCIFilter'.