Bug 48174 - Filter example Chiseled from SVG Wow! is slow
Summary: Filter example Chiseled from SVG Wow! is slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://svg-wow.org/filterEffects/chis...
Keywords:
Depends on: 44528 48212
Blocks: 68469 26389
  Show dependency treegraph
 
Reported: 2010-10-22 23:59 PDT by Dirk Schulze
Modified: 2014-05-12 05:54 PDT (History)
4 users (show)

See Also:


Attachments
shark report (624.40 KB, text/plain)
2010-10-23 00:03 PDT, Dirk Schulze
no flags Details
shark report as txt (34.12 KB, text/plain)
2010-10-23 02:36 PDT, Dirk Schulze
no flags Details
FEGaussianBlur with maximal size 4100x4100 (+ blur radius) (254 bytes, image/svg+xml)
2010-10-23 23:01 PDT, Dirk Schulze
no flags Details
Patch (21.26 KB, patch)
2010-10-24 01:34 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (23.12 KB, patch)
2010-10-24 05:00 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (22.33 KB, patch)
2010-10-24 09:52 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-10-22 23:59:03 PDT
Filter example chiseled from SVG Wow! is slow. Adding a shark report soon.
Comment 1 Dirk Schulze 2010-10-23 00:03:48 PDT
Created attachment 71625 [details]
shark report

shark report
Comment 2 Dirk Schulze 2010-10-23 02:36:18 PDT
Created attachment 71632 [details]
shark report as txt
Comment 3 Dirk Schulze 2010-10-23 23:01:18 PDT
Created attachment 71671 [details]
FEGaussianBlur with maximal size 4100x4100 (+ blur radius)

I checked the performance on this example. Replacing CanvasPixelArray* with WTF::ByteStream* increased the rendering time from 31s to 21s on my machine. Still slow but an increase of 30% (taking unsigned char*, like in bug 44528, didn't make a difference). Niko thought about fixing the style violation in platform/graphics/ImageBuffer::get/putImageData (RefPtr ImageData -> RefPtr CanvasPixelArray -> RefPtr ByteArray ) to increase the rendering speed as well. I agree him, but don't see that much performance win for now (less than < 2% of the time is spend in this functions + getting the ByteArray from ImageData).

I'd suggest to replace CanvasPixelArray* by WTF::ByteArray* on all filter effects + SVG masker in a first step. After that we spend the most time in calculating the position of all necessary pixels for the blurring process than in anything else. This needs further investigation on feGaussianBlur. Ariya was working on that for ContextShadow, maybe we can back-port his algorithm from ContextShadow to FEGaussianBlur (at least in parts) and look  what happens.

The problem for lightning is of course CanvasPixelArray again, but also the normalization of the FloatPoint3D. Maybe Zoltan can take a look at this?
Comment 4 Dirk Schulze 2010-10-24 01:34:07 PDT
Created attachment 71675 [details]
Patch
Comment 5 Nikolas Zimmermann 2010-10-24 04:14:35 PDT
Comment on attachment 71675 [details]
Patch

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

> WebCore/platform/graphics/filters/FEBlend.cpp:104
> -    RefPtr<CanvasPixelArray> srcPixelArrayA(in->resultImage()->getPremultipliedImageData(effectADrawingRect)->data());
> +    RefPtr<WTF::ByteArray> srcPixelArrayA(in->resultImage()->getPremultipliedImageData(effectADrawingRect)->data()->data());

Discussed on IRC. This is the not right approach, CanvasPixelArray has to be stored in a RefPtr<> somewhere.
The performance increase that Dirk sees, is not calling canvasPixelArray->get(). It forward to call to the ByteArray, but CanvasPixelArray stores a RefPtr<ByteArray>. The slowness is because of calling operator-> on the RefPtr.

The best way to fix this now, is to leave RefPtr<CanvasPixelArray> as is, and store "ByteArray* srcByteArrayA = srcPixelArrayA->data()" in a local variable. That gives the same benefit.
Comment 6 Dirk Schulze 2010-10-24 05:00:38 PDT
Created attachment 71679 [details]
Patch
Comment 7 Nikolas Zimmermann 2010-10-24 05:08:15 PDT
Comment on attachment 71679 [details]
Patch

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

> WebCore/platform/graphics/filters/FEBlend.cpp:38
>  
> +using WTF::ByteArray;

This is wrong, it belongs on the bottom of wtf/ByteArray.h.

> WebCore/platform/graphics/filters/FEBlend.cpp:108
> +    RefPtr<CanvasPixelArray> srcCanvasPixelArrayA(in->resultImage()->getPremultipliedImageData(effectADrawingRect)->data());
> +    ByteArray* srcPixelArrayA(srcCanvasPixelArrayA->data());

This is dangerous.
RefPtr<ImageData> srcImageDataA = in->resultImage()->getPremultipliedImageData(effectADrawingRect);
ByteArray* srcPixelArrayA(srcImageDataA->data()->data());

That would be a safe pattern. The ImageData is refcounted, it's contained CanvasPixelArray too, and it's contained ByteArray as well.
You only need to hold the ImageData in a RefPtr, then you can safely access the ByteArray pointer.

This affects multiple places, please look through them first.
Comment 8 Dirk Schulze 2010-10-24 09:52:31 PDT
Created attachment 71691 [details]
Patch
Comment 9 Nikolas Zimmermann 2010-10-24 10:58:50 PDT
Comment on attachment 71691 [details]
Patch

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

r=me, but please use assignment operators everywhere.

> JavaScriptCore/ChangeLog:19
> +2010-10-24  Dirk Schulze  <krit@webkit.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Filter example Chiseled from SVG Wow! is slow
> +        https://bugs.webkit.org/show_bug.cgi?id=48174
> +
> +        * wtf/ByteArray.h:

ChangeLog contains double entries.

> WebCore/platform/graphics/filters/FEBlend.cpp:106
> +    ByteArray* srcPixelArrayA(srcImageDataA->data()->data());

Please use the assignment operator: ByteArray* srcPixelArrayA = srcImage...
Comment 10 Dirk Schulze 2010-10-24 12:25:30 PDT
Committed r70421: <http://trac.webkit.org/changeset/70421>
Comment 11 Dirk Schulze 2010-10-24 12:39:05 PDT
The test is still slow, let the bug open and use it as master bug for more perf improvements.
Comment 12 Eric Seidel (no email) 2010-12-14 01:50:59 PST
Comment on attachment 71691 [details]
Patch

Seems we should file a new bug as a master.  Obsoleting this patch since it was landed.
Comment 13 Dirk Schulze 2011-04-11 23:57:53 PDT
We store primitve results that did not change. This made the test rapidly faster. Closing the bug now.