The FEMorphology filter of SVG can consume lots of resources if it is applied to a large area. The computation can be distributed to multiple cores if the architecture supports.
Created attachment 97979 [details] Apply the ParallelJobs support to FEMorphology
Comment on attachment 97979 [details] Apply the ParallelJobs support to FEMorphology View in context: https://bugs.webkit.org/attachment.cgi?id=97979&action=review > Source/WebCore/ChangeLog:1 > +2011-06-15 Piroska András <Piroska.András@stud.u-szeged.hu> Your email address is wrong! I guess you want to write Piroska.Andras@... ;)
Zoltan, can you give an informal review?
Comment on attachment 97979 [details] Apply the ParallelJobs support to FEMorphology View in context: https://bugs.webkit.org/attachment.cgi?id=97979&action=review > Source/WebCore/ChangeLog:10 > + cores if the architecture supports. You should mention the gain somewhere (and the benchmark if it is publicly availible). > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:110 > + const int rowByteWidth = width * 4; Strange name for me. I liked the effectWidth better, or maybe scanlineWidth? > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:115 > + yEnd = height; I would not prefer these "ifs" here, why not simply pass these values as arguments? > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:119 > + int extStartY = max(0, y - radiusY); We don't use abbreviations is WebKit, keep the original name or make it extremaStartY > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:120 > + int extEndY = min(height - 1, y + radiusY); Ditto. > Source/WebCore/platform/graphics/filters/FEMorphology.h:61 > + }; It seems to me that radiusX and radiusY are constants, so they should go here, and not passed as arguments to platformApply* . > Source/WebCore/platform/graphics/filters/FEMorphology.h:64 > + static const int s_minimalArea = 300 * 300; We should mention here that this is empirical data. > Source/WebCore/platform/graphics/filters/FEMorphology.h:78 > + void platformApplyGeneric(PaintingData*, const int radiusX, const int radiusY, const int yStart=-1, const int yEnd=-1); The above two should be inline function.
Created attachment 98343 [details] Apply the ParallelJobs support to FEMorphology
Created attachment 98477 [details] Apply the ParallelJobs support to FEMorphology The FEMorphology filter of SVG can consume lots of resources if it is applied to a large area. The computation can be distributed to multiple cores if the architecture supports. The average performance progression is 20-25% on dual-core machines.
Comment on attachment 98477 [details] Apply the ParallelJobs support to FEMorphology View in context: https://bugs.webkit.org/attachment.cgi?id=98477&action=review > Source/WebCore/platform/graphics/filters/FEMorphology.h:73 > + static inline void platformApplyWorker(PlatformApplyParameters*); This function cannot be inline, since called by pointer. Probably the compiler realises this anyway, but still it would be better to remove it. > Source/WebCore/platform/graphics/filters/FEMorphology.h:76 > + inline void platformApply(PaintingData*, const int radiusX, const int radiusY); Ok, this is still bad. Maybe I was not clear last time. platformApply should get only 1 parameter: PaintingData* radiusX and radiusY should go to PaintingData as the width and height The calculation should be paintingData.radiusX = min(effectDrawingRect.width() - 1, radiusX);, since otherwise the plain radiusX would be an unused parameter. Drop these lines: int radiusX = static_cast<int>(floorf(filter()->applyHorizontalScale(m_radiusX))); and radiusY = ... I am not even sure this is correct...
Created attachment 99048 [details] Apply the ParallelJobs support to FEMorphology
(In reply to comment #8) > Created an attachment (id=99048) [details] > Apply the ParallelJobs support to FEMorphology I think the patch is OK now.
Comment on attachment 99048 [details] Apply the ParallelJobs support to FEMorphology View in context: https://bugs.webkit.org/attachment.cgi?id=99048&action=review I just have a snippet, a style issue. r- because you don't have commit rights and can't change it before landing. > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:183 > + param.endY = (job) ? (currentY) : (paintingData->height); Can you omit the braces here? They are superfluous and not webkit style.
Created attachment 99242 [details] Apply the ParallelJobs support to FEMorphology
Comment on attachment 99242 [details] Apply the ParallelJobs support to FEMorphology r=me. Great job!
Comment on attachment 99242 [details] Apply the ParallelJobs support to FEMorphology Clearing flags on attachment: 99242 Committed r90091: <http://trac.webkit.org/changeset/90091>
All reviewed patches have been landed. Closing bug.
This change broke the following tests on Apple Mac, Chromium-win, Chromium-mac, Chromium-linux. svg/W3C-SVG-1.1/filters-morph-01-f.svg svg/filters/filterRes.svg Especially, the result of filterRes.svg looks significantly different to me. It this expected?
Created attachment 99256 [details] filterRes.svg result on Apple Mac 10.6
(In reply to comment #16) > Created an attachment (id=99256) [details] > filterRes.svg result on Apple Mac 10.6 I examine this difference, but need a little time
(In reply to comment #17) > (In reply to comment #16) > > Created an attachment (id=99256) [details] [details] > > filterRes.svg result on Apple Mac 10.6 > > I examine this difference, but need a little time I created a bug / patch with the diff: https://bugs.webkit.org/show_bug.cgi?id=63692 I'll disable these two tests in chromium now so I can roll WebKit there. Please let me know when you have a fix so I can re-enable these tests.
(In reply to comment #16) > Created an attachment (id=99256) [details] > filterRes.svg result on Apple Mac 10.6 Thanks, I will fix it
Comment on attachment 99242 [details] Apply the ParallelJobs support to FEMorphology View in context: https://bugs.webkit.org/attachment.cgi?id=99242&action=review > Source/WebCore/platform/graphics/filters/FEMorphology.cpp:122 > + unsigned char columnExtrema = srcPixelArray->get(yStart * effectWidth + 4 * x + clrChannel); There was a typo in this line. The extremaStartY should have been used here instead of yStart. I guess it was a typo, because you used extrameStartY in a right way below. Anyway, the fix is landed in r90105 and the bug 63692 is closed as well.