Bug 70350 - Make FilterEffect::apply() independent of image data management
Summary: Make FilterEffect::apply() independent of image data management
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 70099
  Show dependency treegraph
 
Reported: 2011-10-18 12:27 PDT by Dirk Schulze
Modified: 2011-10-19 08:31 PDT (History)
9 users (show)

See Also:


Attachments
Patch (34.89 KB, patch)
2011-10-18 12:49 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (35.17 KB, patch)
2011-10-18 13:11 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (29.66 KB, patch)
2011-10-19 05:20 PDT, Dirk Schulze
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2011-10-18 12:27:53 PDT
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().
Comment 1 Dirk Schulze 2011-10-18 12:49:48 PDT
Created attachment 111485 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-18 12:57:41 PDT
Comment on attachment 111485 [details]
Patch

Attachment 111485 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10122409
Comment 3 Gyuyoung Kim 2011-10-18 13:00:39 PDT
Comment on attachment 111485 [details]
Patch

Attachment 111485 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10141002
Comment 4 Dirk Schulze 2011-10-18 13:11:14 PDT
Created attachment 111490 [details]
Patch
Comment 5 Zoltan Herczeg 2011-10-19 01:40:55 PDT
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.
Comment 6 Dirk Schulze 2011-10-19 05:20:26 PDT
Created attachment 111597 [details]
Patch
Comment 7 Zoltan Herczeg 2011-10-19 05:53:26 PDT
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.
Comment 8 Dirk Schulze 2011-10-19 06:27:08 PDT
Committed r97853: <http://trac.webkit.org/changeset/97853>
Comment 9 Simon Fraser (smfr) 2011-10-19 07:20:56 PDT
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.
Comment 10 Dirk Schulze 2011-10-19 08:31:33 PDT
(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'.