Bug 71368 – Switch SVGImage cache to store ImageBuffers instead of whole SVGImages, including a DOM/Render tree

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

[Nikolas Zimmermann]

The current approach of having N-SVGImages, including N views/dom/render trees is tricky, as we have multiple SVGImageChromeClients whose invalidateContentsAndWindow() calls all inform the same ImageObserver, the CachedImage. Any change of any of the documents in the SVGImage cache cause imageChanged() calls in RenderImage, leading to races.

Another side-effect of using multiple SVGImages, is that animations restart each time the document is zoomed.
This is very likely to cause of the regressions, see bug 71226 & 71253, that bug 47156 caused.

I've now switched this again to use a single SVGImage, but cache ImageBuffers at different sizes and create Images from them, if needed.
If a resource is now shared by eg. two consumers (say <html:img> and background-image of a <div>), and if it contains animations following happens now: the FrameView::layout() thats triggered from the embedded documents layout timer, marks the ImageBuffer in the CachedImage dirty and RenderImage::imageChanged() / RenderBox::imageChanged() both trigger repaints of themselves in the host document). Once the host document repaints,  the ImageBuffers are cleared and repainted using SVGImage::drawSVGToImageBuffer(), which takes a snapshot of the current frame.  This doesn't cause any recreation of ImageBuffers and/or SVGImages anymore.

This will also cleanup the code a lot. Patch will come soon.

[Nikolas Zimmermann]

Created attachment 113764 [details]
Patch v1

This should fix the SVGImage repainting regressions that chromium/Qt see and also speedup SVGImage painting a lot.
Uploading early to get EWS feedback...

[Gyuyoung Kim]

Comment on attachment 113764 [details]
Patch v1

Attachment 113764 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10334386

[WebKit Review Bot]

Comment on attachment 113764 [details]
Patch v1

Attachment 113764 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10330327

New failing tests:
svg/as-background-image/svg-as-background-4.html
svg/as-image/img-preserveAspectRatio-support-1.html
css2.1/20110323/background-intrinsic-004.htm
svg/as-image/img-preserveAspectRatio-support-2.html
svg/as-background-image/svg-as-background-2.html
css2.1/20110323/background-intrinsic-005.htm
svg/as-background-image/svg-as-background-with-relative-size.html
svg/as-image/animated-svg-as-image.html
svg/as-border-image/svg-as-border-image.html
svg/as-background-image/svg-background-partial-redraw.html
svg/as-background-image/svg-as-background-1.html
svg/as-background-image/svg-as-background-3.html
svg/as-image/animated-svg-as-image-same-image.html
svg/as-image/animated-svg-as-image-no-fixed-intrinsic-size.html
svg/as-background-image/background-image-preserveaspectRatio-support.html

[Nikolas Zimmermann]

Created attachment 113785 [details]
Patch v2

Attempt to fix win/efl builds. Fix for win: copy forwarding headers from svg/graphics. Fix for efl: add svg/graphics to WebKit include dirs, as CachedImage.h includes SVGImageCache.h. Add some more information to the ChangeLog, to aid in reviewing.

I'm not sure about the failures cr-linux EWS reports. It passes pixel tests w/o problems on Mac 10.6.
Can someone get these results for me? I'd be happy to check them first.

[Adam Barth]

Have you tried building and running the Chromium port yourself (on Mac)?  Just pass --chromium to update-webkit, build-webkit, and new-run-webkit-tests.

[Nikolas Zimmermann]

(In reply to comment #5)
> Have you tried building and running the Chromium port yourself (on Mac)?  Just pass --chromium to update-webkit, build-webkit, and new-run-webkit-tests.
I rather hoped someone could grab it from the bots.
It's been a while since I built chromium on linux, I never tried it on mac. Can I use the same source tree that I use for building cg/mac, or will this mess up my regular builds?

[Adam Barth]

It shouldn't disturb any other builds.

The problem is that having me ssh into the bots, zip up the results, and host them on my personal web site doesn't really scale.

[WebKit Review Bot]

Comment on attachment 113785 [details]
Patch v2

Attachment 113785 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10331579

New failing tests:
svg/as-background-image/svg-as-background-4.html
svg/as-image/img-preserveAspectRatio-support-1.html
css2.1/20110323/background-intrinsic-004.htm
svg/as-image/img-preserveAspectRatio-support-2.html
svg/as-background-image/svg-as-background-2.html
css2.1/20110323/background-intrinsic-005.htm
svg/as-background-image/svg-as-background-with-relative-size.html
svg/as-image/animated-svg-as-image.html
svg/as-border-image/svg-as-border-image.html
svg/as-background-image/svg-background-partial-redraw.html
svg/as-background-image/svg-as-background-1.html
svg/as-background-image/svg-as-background-3.html
svg/as-image/animated-svg-as-image-same-image.html
svg/as-image/animated-svg-as-image-no-fixed-intrinsic-size.html
svg/as-background-image/background-image-preserveaspectRatio-support.html

[Nikolas Zimmermann]

(In reply to comment #7)
> It shouldn't disturb any other builds.
> 
> The problem is that having me ssh into the bots, zip up the results, and host them on my personal web site doesn't really scale.

Indeed build-webkit --chromium && nrwt --chromium --tolerance 0 -p works great on Mac. I could reproduce the exact list of failures that the both returned, and phew these are all marginal changes, almost not visible, reported as 0% by DRT. This is a side-effect that we're now drawing ImageBuffers on to context instead of SVG documents through RenderView & friends. Green light from chromium, doesn't break anything there.

[Antti Koivisto]

Comment on attachment 113785 [details]
Patch v2

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

r=me as this seems to be an improvement. 

I like introduction of the bitmap cache for svg images though I'm rather unhappy that this is done as regression fix instead of as no-behavior-changes performace patch.

Please think carefully if this is really the architecture you need. The FIXMEs here indicate that this still leaves major issues unsolved.

> Source/WebCore/ChangeLog:11
> +        Fix regressions/races introduced by r98852. SVGImage repainting didn't work under certain circumstences.
> +        Avoid creating multiple SVGImages and caching them for different sizes/zoom levels. Introduce SVGImageCache
> +        which holds rendered versions of the SVGImage at certain sizes/zoom levels. It holds (ImageBuffer, Image) pairs
> +        for each renderer, associated with a size and zoom level.

You should explain why and when the repainting didn't work and how this patch fixes the problem.

This patch introduces bitmap caching for SVG images, a major change. You should talk more about performance and memory implications.

> Source/WebCore/page/DragController.cpp:659
> +    // Don't use cachedImage->imageForRenderer() here as that may return BitmapImages for cached SVG Images.
> +    // Users of getImage() want access to the SVGImage, in order to figure out the filename extensions,
> +    // which would be empty when asking the cached BitmapImages.
>      return (cachedImage && !cachedImage->errorOccurred()) ?

It sounds like this code should be refactored or at least renamed (getImageForFileNameExtension()?) as it does surpising things. 

Does some test cover this?

> Source/WebCore/rendering/ImageBySizeCache.cpp:68
> -Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size, float zoom)
> +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size)

Since ImageBySizeCache now has only one client, I think it should be merged back to CSSImageGeneratorValue. It is bit too confusing as a standalone class without clear code sharing purpose.

> Source/WebCore/svg/graphics/SVGImage.cpp:157
> +    // FIXME: This doesn't work correctly with animations. If an image contains animations, that say run for 2 seconds,
> +    // and we currently have one <img> that displays us. If we open another document referencing the same SVGImage it
> +    // will display the document at a time where animations already ran - even though it has its own ImageBuffer.
> +    // We currently don't implement SVGSVGElement::setCurrentTime, and can NOT go back in time, once animations started.
> +    // There's no way to fix this besides avoiding style/attribute mutations from SVGAnimationElement.

How do you know that fixing this rather major issue won't require rearchitecting everything yet again?

> Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> +void SVGImageCache::redrawTimerFired(Timer<SVGImageCache>*)
> +{

I find it strange that a cache class has a redraw timer. Seems like someone else should take care of making the calls async.

> Source/WebCore/svg/graphics/SVGImageCache.h:50
> +    void setRequestedSizeAndZoom(const RenderObject*, const IntSize&, float zoom);
> +    void getRequestedSizeAndZoom(const RenderObject*, IntSize*, float* zoom);

Since you define a type of size/zoom pair, you should use it everywhere.

> Source/WebCore/svg/graphics/SVGImageCache.h:59
> +    struct CachedSizeAndZoom {

Word "Cached" adds no information as this type is already scoped to "SVGImageCache"

> Source/WebCore/svg/graphics/SVGImageCache.h:75
> +    struct CachedImageData {

Same here.

> Source/WebCore/svg/graphics/SVGImageCache.h:83
> +        CachedImageData(ImageBuffer* newBuffer, PassRefPtr<Image> newImage, const IntSize& newSize, float newZoom)

Could use SizeAndZoom type.

> Source/WebCore/svg/graphics/SVGImageCache.h:94
> +        float zoom;
> +        IntSize size;

again.

> Source/WebCore/svg/graphics/SVGImageCache.h:105
> +    typedef HashMap<const RenderObject*, CachedSizeAndZoom> CachedSizeAndZoomMap;
> +    typedef HashMap<const RenderObject*, CachedImageData> CachedImageDataMap;
> +
> +    SVGImage* m_svgImage;
> +    CachedSizeAndZoomMap m_cachedSizeAndZoomMap;
> +    CachedImageDataMap m_cachedImageDataMap;

Word "Cached" adds no information as this is already scoped to "SVGImageCache"

[Nikolas Zimmermann]

(In reply to comment #10)
> r=me as this seems to be an improvement. 
Thanks. 

> I like introduction of the bitmap cache for svg images though I'm rather unhappy that this is done as regression fix instead of as no-behavior-changes performace patch.
I tried to fix the regression w/o doing the bitmap cache switch, and it turned out it would need a major redesign of ImageBySizeCache, which I wanted to avoid, as when introducing the bitmap cache we wouldn't need it any longer.

> 
> Please think carefully if this is really the architecture you need. The FIXMEs here indicate that this still leaves major issues unsolved.
As discussed on IRC, fixing the FIXME requires more work on SMIL to stop SVG animations from mutating attributes/properties, which makes it impossible to reset animations and/or seek to a different time. The SVGImageCache will stay as-is though.
 
> > Source/WebCore/ChangeLog:11
> > +        Fix regressions/races introduced by r98852. SVGImage repainting didn't work under certain circumstences.
> > +        Avoid creating multiple SVGImages and caching them for different sizes/zoom levels. Introduce SVGImageCache
> > +        which holds rendered versions of the SVGImage at certain sizes/zoom levels. It holds (ImageBuffer, Image) pairs
> > +        for each renderer, associated with a size and zoom level.
> 
> You should explain why and when the repainting didn't work and how this patch fixes the problem.
> 
> This patch introduces bitmap caching for SVG images, a major change. You should talk more about performance and memory implications.
Ok. I will update it.

> 
> > Source/WebCore/page/DragController.cpp:659
> > +    // Don't use cachedImage->imageForRenderer() here as that may return BitmapImages for cached SVG Images.
> > +    // Users of getImage() want access to the SVGImage, in order to figure out the filename extensions,
> > +    // which would be empty when asking the cached BitmapImages.
> >      return (cachedImage && !cachedImage->errorOccurred()) ?
> 
> It sounds like this code should be refactored or at least renamed (getImageForFileNameExtension()?) as it does surpising things. 
> 
> Does some test cover this?
This code used cachedImage->image() before, and I made it use imageForRenderer(), this reverts it again. Other call sites don't use imageForRenderer() if they want to query the file name extension.
The testcase svg/as-image/drag-svg-as-image.html hits an assertion w/o that change.

> > Source/WebCore/rendering/ImageBySizeCache.cpp:68
> > -Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size, float zoom)
> > +Image* ImageBySizeCache::getImage(const RenderObject* renderer, const IntSize& size)
> 
> Since ImageBySizeCache now has only one client, I think it should be merged back to CSSImageGeneratorValue. It is bit too confusing as a standalone class without clear code sharing purpose.
I will do that in a follow-up patch.

> > Source/WebCore/svg/graphics/SVGImage.cpp:157
> > +    // FIXME: This doesn't work correctly with animations. If an image contains animations, that say run for 2 seconds,
> > +    // and we currently have one <img> that displays us. If we open another document referencing the same SVGImage it
> > +    // will display the document at a time where animations already ran - even though it has its own ImageBuffer.
> > +    // We currently don't implement SVGSVGElement::setCurrentTime, and can NOT go back in time, once animations started.
> > +    // There's no way to fix this besides avoiding style/attribute mutations from SVGAnimationElement.
> 
> How do you know that fixing this rather major issue won't require rearchitecting everything yet again?
I can not guarantee it unless I prototype a fix for that. Note that problem is present since the beginning of the SVGImage introduction, I just named it now :-)

> 
> > Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> > +void SVGImageCache::redrawTimerFired(Timer<SVGImageCache>*)
> > +{
> 
> I find it strange that a cache class has a redraw timer. Seems like someone else should take care of making the calls async.
I could only move the timer to CachedImage, not sure if thats an improvement.
Maybe it should be renamed to m_updateCacheTimer? or m_updateTimer?

> > Source/WebCore/svg/graphics/SVGImageCache.h:50
> > +    void setRequestedSizeAndZoom(const RenderObject*, const IntSize&, float zoom);
> > +    void getRequestedSizeAndZoom(const RenderObject*, IntSize*, float* zoom);
> 
> Since you define a type of size/zoom pair, you should use it everywhere.
Fixed.

> > Source/WebCore/svg/graphics/SVGImageCache.h:59
> > +    struct CachedSizeAndZoom {
> 
> Word "Cached" adds no information as this type is already scoped to "SVGImageCache"
Fixed.

> > Source/WebCore/svg/graphics/SVGImageCache.h:75
> > +    struct CachedImageData {
> 
> Same here.
Fixed.

> > Source/WebCore/svg/graphics/SVGImageCache.h:83
> > +        CachedImageData(ImageBuffer* newBuffer, PassRefPtr<Image> newImage, const IntSize& newSize, float newZoom)
> 
> Could use SizeAndZoom type.
Fixed.

> > Source/WebCore/svg/graphics/SVGImageCache.h:94
> > +        float zoom;
> > +        IntSize size;
> 
> again.
Fixed.

> > Source/WebCore/svg/graphics/SVGImageCache.h:105
> > +    typedef HashMap<const RenderObject*, CachedSizeAndZoom> CachedSizeAndZoomMap;
> > +    typedef HashMap<const RenderObject*, CachedImageData> CachedImageDataMap;
> > +
> > +    SVGImage* m_svgImage;
> > +    CachedSizeAndZoomMap m_cachedSizeAndZoomMap;
> > +    CachedImageDataMap m_cachedImageDataMap;
> 
> Word "Cached" adds no information as this is already scoped to "SVGImageCache"
Fixed.

[Nikolas Zimmermann]

Landed in r99539, now watching bots to rebaseline tests, once results appear.

[Nikolas Zimmermann]

A regression appeared: fast/canvas/svg-taint.html and other canvas related taint checks.
The root of this is using imageForRenderer(), instead of CachedImage. I'm fixing it.

[Nikolas Zimmermann]

Created attachment 114031 [details]
Fix an obvious error leading to canvas regression

This fixes a regression with the canvas-taint tests on several bots.

[Zoltan Herczeg]

Comment on attachment 114031 [details]
Fix an obvious error leading to canvas regression

r=me

[Nikolas Zimmermann]

Thanks Zoltan! Landed fix in r99543. Should make gtk/qt happy again.

[Nikolas Zimmermann]

(In reply to comment #16)
> Thanks Zoltan! Landed fix in r99543. Should make gtk/qt happy again.
Oops, an unused variable remained, fixed in r99544.

[Nikolas Zimmermann]

Remaining errors are probably flakey testcases, further investigation continues in 71793.
Mac/Gtk and friends work fine, so I'm closing this bug.