Bug 71368

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...

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

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

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.

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.

(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?

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.

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

(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.

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"

(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.

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

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.

Created attachment 114031 [details] Fix an obvious error leading to canvas regression This fixes a regression with the canvas-taint tests on several bots.

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

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

(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.

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