Rendering SVGs through img elements is slow
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.
Created attachment 53909 [details] Patch
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.
(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?
Created attachment 53915 [details] Updated patch with coding style fixes
(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?
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 :-)
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.
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.
Any news on this bug Simon?
(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 :)
Changed component to SVG, so it shows up in my all-svg-bugs search.
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?
Agreed, this is probably not worth any further work. We can always re-open it :)
(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. :-)
(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.