Bug 37905 – Rendering SVGs through img elements is slow

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

[Simon Hausmann]

Rendering SVGs through img elements is slow

[Simon Hausmann]

The img element is used for bitmap images in 99% of all cases. The rendering of bitmap images is very fast.

When using the img element with svg documents as source, the rendering is slower because WebKit repaints the SVG every time the img is painted.

An obvious optimization is to cache the rendered SVG and update the cache as the SVG contents changes.

[Simon Hausmann]

Created attachment 53909 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 53909 [details]
Patch


> +    if (s.width() * s.height() < 1024*1024) {

I guess this is the why you dont pass the coding style. Space needed around the *. That needs fixing.

> Image* cachedImage = svgAsCachedImage();

so svgAsCachedImage creates a cached representation and stores it internally? Maybe use a better name.

Maybe you could merge the two lines:

182         Image* cachedImage = svgAsCachedImage();
 183         if (cachedImage) {

Apart from that looks good.

[Simon Hausmann]

(In reply to comment #3)
> (From update of attachment 53909 [details])
> 
> > +    if (s.width() * s.height() < 1024*1024) {
> 
> I guess this is the why you dont pass the coding style. Space needed around the
> *. That needs fixing.
> 
> > Image* cachedImage = svgAsCachedImage();
> 
> so svgAsCachedImage creates a cached representation and stores it internally?
> Maybe use a better name.

How about toCachedImage()?

> Maybe you could merge the two lines:
> 
> 182         Image* cachedImage = svgAsCachedImage();
>  183         if (cachedImage) {

That's a good idea.

> Apart from that looks good.

Thanks for the review.

Dirk, any thoughts from your side about this optimization in general?

[Simon Hausmann]

Created attachment 53915 [details]
Updated patch with coding style fixes

[Dirk Schulze]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 53909 [details] [details])
> > 
> > > +    if (s.width() * s.height() < 1024*1024) {
Maybe we can save the constant multiplication, by taking the product of 1024^2 with a short comment?
Also what happens on zooming a page? Get the SVG redrawn, or is the cached image just scaled?

[Nikolas Zimmermann]

I'd also like to hear about Dirks question :-)

And I think the 1024*1024 constant should be moved to a "static const unsigned int kMaxCachedImageSize" on top of file, this way the multiplication can stay as is, and it's slightly cleaner then keeping around magic values in the code :-)

[Benjamin Poulain]

Comment on attachment 53915 [details]
Updated patch with coding style fixes

> +    if (s.width() * s.height() < 1024 * 1024) {

Here, you could also check if m_frameCache is valid. If SVGImage::nativeImageForCurrentFrame() has been called, the image is in cache even if the size exceed 1024 * 1024.

[Eric Seidel (no email)]

Comment on attachment 53915 [details]
Updated patch with coding style fixes

Woh:
 246     // Don't delete the frame cache right away as someone might still hold
 247     // a pointer to the NativeImagePtr returned by nativeImageForCurrentFrame.
 248     m_frameCacheDirty = true;
That sounds really complicated and scary.

This is what ref counting was invented to solve.  Not understanding when things get deleted is badnews bears.

[Dirk Schulze]

Any news on this bug Simon?

[Simon Hausmann]

(In reply to comment #10)
> Any news on this bug Simon?

I was going to implement the proposed changes, but I'm currently busy doing release monkey work. I hope to get back to finishing this patch after the release, unless someone beats me to it :)

[Nikolas Zimmermann]

Changed component to SVG, so it shows up in my all-svg-bugs search.

[Benjamin Poulain]

Simon, I updated the patch today and explored how to get it right in the different use cases.

I think we cannot cache the svg rendered. The problem is the transforms of the view, and then the CSS transforms. If we scale either the QGraphicsWebView or a parent element with CSS, the SVG is not rendered precisely with the patch.

We can probably do some hackery around the GraphicsContext's transform but I am not sure that is worth it. What is your opinion?

[Simon Hausmann]

Agreed, this is probably not worth any further work. We can always re-open it :)

[Nikolas Zimmermann]

(In reply to comment #14)
> Agreed, this is probably not worth any further work. We can always re-open it :)

For what its worth: We have this caching mechanism for bitmap backed copies of an SVGImage in trunk: SVGImageCache. It's there since some months. So We don't need to close this as WONTFIX, but rather FIXED. :-)

[Tim Horton]

(In reply to comment #15)
> (In reply to comment #14)
> > Agreed, this is probably not worth any further work. We can always re-open it :)
> 
> For what its worth: We have this caching mechanism for bitmap backed copies of an SVGImage in trunk: SVGImageCache. It's there since some months. So We don't need to close this as WONTFIX, but rather FIXED. :-)

Except, as per https://bugs.webkit.org/show_bug.cgi?id=77237, SVGImageCache has the same bug that Benjamin noted here:

> I think we cannot cache the svg rendered. The problem is the transforms of the view, and then the CSS transforms. If we scale either the QGraphicsWebView or a parent element with CSS, the SVG is not rendered precisely with the patch.