Bug 70350

Summary: Make FilterEffect::apply() independent of image data management
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Dirk Schulze <krit>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dglazkov, dino, senorblanco, simon.fraser, thorton, webkit.review.bot, zherczeg, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70099    
Attachments:
Description Flags
Patch
none
Patch
none
Patch zherczeg: review+

Dirk Schulze
Reported 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().
Attachments
Patch (34.89 KB, patch)
2011-10-18 12:49 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
Patch (35.17 KB, patch)
2011-10-18 13:11 PDT, Dirk Schulze 		no flags
DetailsFormatted DiffDiff
Patch (29.66 KB, patch)
2011-10-19 05:20 PDT, Dirk Schulze 		zherczeg: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dirk Schulze
Comment 1 2011-10-18 12:49:48 PDT
Created attachment 111485 [details] Patch
WebKit Review Bot
Comment 2 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
Gyuyoung Kim
Comment 3 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
Dirk Schulze
Comment 4 2011-10-18 13:11:14 PDT
Created attachment 111490 [details] Patch
Zoltan Herczeg
Comment 5 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.
Dirk Schulze
Comment 6 2011-10-19 05:20:26 PDT
Created attachment 111597 [details] Patch
Zoltan Herczeg
Comment 7 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.
Dirk Schulze
Comment 8 2011-10-19 06:27:08 PDT
Committed r97853: <http://trac.webkit.org/changeset/97853>
Simon Fraser (smfr)
Comment 9 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.
Dirk Schulze
Comment 10 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'.
Note You need to log in before you can comment on or make changes to this bug.