Improve FEGaussianBlur algorithm to access image buffer memory directly

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alejandro G. Castro]

Currently the FEGaussianBlur uses CanvasPixelArrays objects to handle pixel operations, this is very slow, we need to add support to get and handle image buffer memory directly, avoiding copies if possible.

[Alejandro G. Castro]

Created attachment 65282 [details]
Proposed patch

[Eric Seidel (no email)]

Attachment 65282 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3768598

[Alejandro G. Castro]

Created attachment 65284 [details]
Proposed patch

[WebKit Review Bot]

Attachment 65284 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3790427

[Alejandro G. Castro]

Created attachment 65314 [details]
Proposed patch

[Alejandro G. Castro]

After talking to dhyatt I'm going review the patch and create begin/end functions instead of data and dirty.

[Alejandro G. Castro]

Created attachment 65698 [details]
Proposed patch

Replaced data/makeDirty with beginAccessData/endAccessData and reviewed some of the port functions to avoid a copy, probably someone of each port should check those functions are correct for each technology.

[Dirk Schulze]

Comment on attachment 65698 [details]
Proposed patch

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

Have you tried to use the original code, but take the WTF::ByteArray instead of the CanvasPixelArray? I'm interested in how much perf win this could bring. Surely not as much as your code, but still. Also it is not needed here to mark the surface as dirty. This is only necessary, if you want to access the context after pixel manipulations. That isn't the case here or on any other filter effect result.
You also ignored, that some platforms may use unpremultiplied colors. Would be great if we can find a way to manage pixel access with premultiplied and unpremultiplied colors, that would be a win for the other filter effects as well.

I know that you were working on that to make CSS shadows faster on Cairo. But it would still be useful for SVG filter effects (that are supported by WebKitGtk as well).

> WebCore/platform/graphics/wx/ImageBufferWx.cpp:72
> +     notImplemented();

indention wrong.

[Alejandro G. Castro]

Thanks for the review krit.

(In reply to comment #8)
> (From update of attachment 65698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=65698&action=review
> 
> Have you tried to use the original code, but take the WTF::ByteArray instead of the CanvasPixelArray? I'm interested in how much perf win this could bring. Surely not as much as your code, but still. Also it is not needed here to mark the surface as dirty. This is only necessary, if you want to access the context after pixel manipulations. That isn't the case here or on any other filter effect result.
> You also ignored, that some platforms may use unpremultiplied colors. Would be great if we can find a way to manage pixel access with premultiplied and unpremultiplied colors, that would be a win for the other filter effects as well.
> 

Yeah, you are right, I'll try to check if there is any option and check the performance in that case.

[Dirk Schulze]

(In reply to comment #9)
> Thanks for the review krit.
> 
> (In reply to comment #8)
> > (From update of attachment 65698 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=65698&action=review
> > 
> > Have you tried to use the original code, but take the WTF::ByteArray instead of the CanvasPixelArray? I'm interested in how much perf win this could bring. Surely not as much as your code, but still. Also it is not needed here to mark the surface as dirty. This is only necessary, if you want to access the context after pixel manipulations. That isn't the case here or on any other filter effect result.
> > You also ignored, that some platforms may use unpremultiplied colors. Would be great if we can find a way to manage pixel access with premultiplied and unpremultiplied colors, that would be a win for the other filter effects as well.
> > 
> 
> Yeah, you are right, I'll try to check if there is any option and check the performance in that case.

I already checked and tested ByteArray* an unsgined char* and couldn't see a difference in performance. I took the attached SVG on bug 48174 as reference. On Mac put/ and getImageData just take 0.8% of the time, GaussianBlur algo itself took 96% of the time. On my machine I could reduce the rendering time from 31s to 21s just by eliminating the -> operators and accessing the pixel data via ByteArray instead of CanvasPixelArray. 21s is still to much, but I don't see huge improvement by implementing direct pixel data access on ImageBuffer right now. Even so I'd support it!

Do you plan to investigate on this more?

[Dirk Schulze]

(In reply to comment #10)
> Do you plan to investigate on this more?

Oh, I forgot to mention that the patch that makes use of ByteArray on FEGaussianBlur is already in trunk.

[Alejandro G. Castro]

(In reply to comment #10)
>
> [...]
>
> I already checked and tested ByteArray* an unsgined char* and couldn't see a difference in performance. I took the attached SVG on bug 48174 as reference. On Mac put/ and getImageData just take 0.8% of the time, GaussianBlur algo itself took 96% of the time. On my machine I could reduce the rendering time from 31s to 21s just by eliminating the -> operators and accessing the pixel data via ByteArray instead of CanvasPixelArray. 21s is still to much, but I don't see huge improvement by implementing direct pixel data access on ImageBuffer right now. Even so I'd support it!
> 
> Do you plan to investigate on this more?
>

Oh, nice, good to know. If you think it makes sense I can spend time doing some profiling and check how we could do it (integrate contextshadow I guess it is an option). Althougth I'm afraid that this operation is not going to be good enough if we can not make it in specific hardware or reduce the amount of blurring area.

[Simon Fraser (smfr)]

See also bug 50881.

[Simon Fraser (smfr)]

Maybe obsoleted by the patch in bug 49907?

[Alejandro G. Castro]

(In reply to comment #14)
> Maybe obsoleted by the patch in bug 49907?

Yes, the patch must be reviewed, and not sure if after that patch the time spent in these operations are relevant at all. Anyway the option to use directly the memory buffer is open even after that patch, just need to check if it is interesting or not. Sorry for not doing the testing before, I'll try to check it soon and close the bug depending on the results, thanks for the heads up.

[Martin Robinson]

I guess we've abandoned this? Perhaps we can just close the bug?

[Alejandro G. Castro]

(In reply to comment #16)
> I guess we've abandoned this? Perhaps we can just close the bug?

I agree, thanks for the heads-up.