Bug 63064 – Apply the ParallelJobs support to FEMorphology

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

[Andras Piroska]

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.

[Andras Piroska]

Created attachment 97979 [details]
Apply the ParallelJobs support to FEMorphology

[Gabor Loki]

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
> [phone]  Piroska András  <Piroska.Andrá[e-mail]>

Your email address is wrong! I guess you want to write Piroska.Andras@... ;)

[Nikolas Zimmermann]

Zoltan, can you give an informal review?

[Zoltan Herczeg]

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.

[Andras Piroska]

Created attachment 98343 [details]
Apply the ParallelJobs support to FEMorphology

[Andras Piroska]

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.

[Zoltan Herczeg]

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...

[Andras Piroska]

Created attachment 99048 [details]
Apply the ParallelJobs support to FEMorphology

[Zoltan Herczeg]

(In reply to comment #8)
> Created an attachment (id=99048) [details]
> Apply the ParallelJobs support to FEMorphology

I think the patch is OK now.

[Dirk Schulze]

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.

[Andras Piroska]

Created attachment 99242 [details]
Apply the ParallelJobs support to FEMorphology

[Dirk Schulze]

Comment on attachment 99242 [details]
Apply the ParallelJobs support to FEMorphology

r=me. Great job!

[WebKit Review Bot]

Comment on attachment 99242 [details]
Apply the ParallelJobs support to FEMorphology

Clearing flags on attachment: 99242

Committed r90091: <http://trac.webkit.org/changeset/90091>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

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?

[Kent Tamura]

Created attachment 99256 [details]
filterRes.svg result on Apple Mac 10.6

[Andras Piroska]

(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

[Marcus Bulach]

(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.

[Andras Piroska]

(In reply to comment #16)
> Created an attachment (id=99256) [details]
> filterRes.svg result on Apple Mac 10.6

Thanks, I will fix it

[Gabor Loki]

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.