Bug 106159 – Remove the bitmap SVG image cache

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

[Philip Rogers]

WK71368 introduced a change to cache rendered image buffers instead of dom&render trees in the SVG image cache. This approach has several downsides:
1) Memory - image buffers are big
2) Prevents viewport rendering - WK104693 is an example of this since we cannot viewport render a portion of an enormous svg image.
3) Introduces an unnecessary blit for every image.

We should revert this change and cache something else, likely the dom and render tree for the inner SVG document.

[Philip Rogers]

Created attachment 187407 [details]
Remove the bitmap SVG image cache

Caching the SVG dom/tree at specific sizes turned out to not be the right approach after all.

I'm attaching a first-pass at removing the bitmap SVG image cache entirely and instead directly rendering SVG content. With this applied, we pass both the IE Chalkboard demo and all our layout tests (except minor text aliasing).

[WebKit Review Bot]

Attachment 187407 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/svg/graphics/SVGImage.cpp', u'Source/WebCore/svg/graphics/SVGImage.h', u'Source/WebCore/svg/graphics/SVGImageCache.cpp', u'Source/WebCore/svg/graphics/SVGImageCache.h', u'Source/WebCore/svg/graphics/SVGImageForContainer.cpp', u'Source/WebCore/svg/graphics/SVGImageForContainer.h']" exit_code: 1
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

Attachment 187407 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16476040

[Early Warning System Bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

Attachment 187407 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16468116

[EFL EWS Bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

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

[WebKit Review Bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

Attachment 187407 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16474052

[kov's GTK+ EWS bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

Attachment 187407 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16475040

[Peter Beverloo (cr-android ews)]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

Attachment 187407 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16478021

[WebKit Review Bot]

Comment on attachment 187407 [details]
Remove the bitmap SVG image cache

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

[Philip Rogers]

Created attachment 187452 [details]
Update

This patch updates the other build systems, fixes a linux compile error, and does some minor cleanup.

[WebKit Review Bot]

Attachment 187452 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/loader/cache/CachedImage.cpp', u'Source/WebCore/svg/graphics/SVGImage.cpp', u'Source/WebCore/svg/graphics/SVGImage.h', u'Source/WebCore/svg/graphics/SVGImageCache.cpp', u'Source/WebCore/svg/graphics/SVGImageCache.h', u'Source/WebCore/svg/graphics/SVGImageForContainer.cpp', u'Source/WebCore/svg/graphics/SVGImageForContainer.h']" exit_code: 1
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philip Rogers]

Created attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.

[Philip Rogers]

Comment on attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.

Many SVG chromium tests will fail because there are anti-aliasing differences now that we don't draw onto a buffer and then blit. Before landing, these tests will need to be rebaselined.

The following bugs are also fixed with this patch:
https://bugs.webkit.org/show_bug.cgi?id=99481
https://bugs.webkit.org/show_bug.cgi?id=104189

[WebKit Review Bot]

Comment on attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.

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

New failing tests:
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-005.htm
svg/as-border-image/svg-as-border-image.html
svg/as-image/svg-non-integer-scaled-image.html
svg/zoom/page/zoom-background-images.html
svg/as-background-image/animated-svg-as-background.html
svg/as-background-image/svg-as-background-6.html
svg/as-image/svg-image-change-content-size.xhtml
fast/writing-mode/block-level-images.html
svg/zoom/page/zoom-svg-as-background-with-relative-size-and-viewBox.html
svg/as-image/same-image-two-instances.html
svg/wicd/test-scalable-background-image1.xhtml
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-background-image/svg-as-background-4.html
svg/as-image/img-preserveAspectRatio-support-2.html
svg/as-background-image/svg-as-background-with-relative-size.html
svg/zoom/page/zoom-svg-as-background-with-relative-size.html
svg/as-image/animated-svg-as-image-no-fixed-intrinsic-size.html
svg/wicd/test-scalable-background-image2.xhtml
fast/backgrounds/size/contain-and-cover.html
svg/as-background-image/svg-as-background-2.html
svg/as-background-image/svg-as-background-5.html

[Stephen Chenney]

Comment on attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.

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

It really looks like good work to me. I have no issues with the approach.

Please address the couple of questions I have.

Also, do you have any performance numbers to share, or are test now passing that were not previously? If we are supporting things we did not previously support, and there are no tests, we should have new tests for the new support.

> Source/WebCore/svg/graphics/SVGImage.cpp:138
> +    scaledSrc.scale(1 / zoom);

Zoom is never zero, I presume. I think that's safe.

> Source/WebCore/svg/graphics/SVGImage.cpp:145
> +    draw(context, dstRect, scaledSrc, colorSpace, compositeOp, blendMode);

Can you still do layout in this method? Do you really need the disabled repaints?

> Source/WebCore/svg/graphics/SVGImageCache.cpp:73
> +// FIXME: This doesn't take into account the animation timeline.

Could you add to this by explaining what the visible effect will be? Incorrect sizing of animated SVGImage content?

> Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> +Image* SVGImageCache::lookupOrCreateImageForRenderer(const RenderObject* renderer)

This method never seems to create anything. Does the name need to be changed, or am I missing something?

[Philip Rogers]

Created attachment 187705 [details]
Update per reviewer comments

[Philip Rogers]

(In reply to comment #15)
> (From update of attachment 187453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187453&action=review
> 
> It really looks like good work to me. I have no issues with the approach.

Thank you for taking the time to review into this. It's very much appreciated as I know this was an ominous patch :)


> Please address the couple of questions I have.
> 
> Also, do you have any performance numbers to share, or are test now passing that were not previously? If we are supporting things we did not previously support, and there are no tests, we should have new tests for the new support.

I do have additional performance numbers. I have updated the changelog to include a simple viewport demo (similar to the Microsoft Chalkboard demo):
http://philbit.com/SvgImagePerformance/viewport.html:
 without patch: ~20FPS
 with patch: ~55FPS
Unfortunately this is not testable on our PerformanceTest infrastructure.

I also ensured we did not regress several other usecases (found on philbit.com/SvgImagePerformance) including multiple <img> elements referencing a single svg image, and animating tiled svg backgrounds. It is likely we will see a progression on memory performance.

> > Source/WebCore/svg/graphics/SVGImage.cpp:138
> > +    scaledSrc.scale(1 / zoom);
> 
> Zoom is never zero, I presume. I think that's safe.

This should be safe as there are similar calls in RenderLayer, but I've added an ASSERT just to be sure.

> 
> > Source/WebCore/svg/graphics/SVGImage.cpp:145
> > +    draw(context, dstRect, scaledSrc, colorSpace, compositeOp, blendMode);
> 
> Can you still do layout in this method? Do you really need the disabled repaints?

This layout ended up being unnecessary, as was disabling repaints. I have removed these calls.

Note that we still are laying out here, in setContainerSize. This layout is done on the inner SVG document which should be safe. This is similar to how it was done before this patch.

> 
> > Source/WebCore/svg/graphics/SVGImageCache.cpp:73
> > +// FIXME: This doesn't take into account the animation timeline.
> 
> Could you add to this by explaining what the visible effect will be? Incorrect sizing of animated SVGImage content?

Fixed: I reworked this comment to be more clear.

> 
> > Source/WebCore/svg/graphics/SVGImageCache.cpp:95
> > +Image* SVGImageCache::lookupOrCreateImageForRenderer(const RenderObject* renderer)
> 
> This method never seems to create anything. Does the name need to be changed, or am I missing something?

There is a bit of cruft like this that I would like to clean up in subsequent refactoring patches. I've renamed this to "imageForRenderer", as you are absolutely right that the name is misleading.

[Early Warning System Bot]

Comment on attachment 187705 [details]
Update per reviewer comments

Attachment 187705 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16495411

[Philip Rogers]

Created attachment 187714 [details]
Add a file someone forgot to include in the previous patch.

[WebKit Review Bot]

Comment on attachment 187714 [details]
Add a file someone forgot to include in the previous patch.

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

New failing tests:
css2.1/20110323/background-intrinsic-005.htm
svg/as-image/same-image-two-instances.html
css2.1/20110323/background-intrinsic-004.htm
fast/backgrounds/size/contain-and-cover.html
fast/writing-mode/block-level-images.html

[Philip Rogers]

Created attachment 187765 [details]
Add tests for newly-fixed bugs

Small update to add two reftests for the two bugs this fixes: webkit.org/b/99481 and webkit.org/b/104189

[Tim Horton]

Comment on attachment 187765 [details]
Add tests for newly-fixed bugs

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

I'm having trouble finding anything to complain about! Let's see how it goes, I guess? I'm really excited.

> Source/WebCore/svg/graphics/SVGImage.h:67
> +    friend class SVGImageForContainer;

The "ForContainer" prefix struck me as a little odd the first time I saw it, but I've quickly gotten used to it.

> Source/WebCore/svg/graphics/SVGImageForContainer.h:71
> +    void setPageScale(float pageScale) { m_pageScale = pageScale; }

This is the product of page scale and device scale, yes? Slightly confusing -- we already make this confusing enough :D

[Philip Rogers]

(In reply to comment #22)
> (From update of attachment 187765 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187765&action=review
> 
> I'm having trouble finding anything to complain about! Let's see how it goes, I guess? I'm really excited.

Thanks for the review! I am also excited.

> 
> > Source/WebCore/svg/graphics/SVGImage.h:67
> > +    friend class SVGImageForContainer;
> 
> The "ForContainer" prefix struck me as a little odd the first time I saw it, but I've quickly gotten used to it.
> 
> > Source/WebCore/svg/graphics/SVGImageForContainer.h:71
> > +    void setPageScale(float pageScale) { m_pageScale = pageScale; }
> 
> This is the product of page scale and device scale, yes? Slightly confusing -- we already make this confusing enough :D

I agree 100% here and I plan to take out most of the page scale logic in a followup patch. I've filed https://bugs.webkit.org/show_bug.cgi?id=109609 to track this work.
To summarize my plan: the page scale information can be set once, just like zoom, and we can get rid of all setters on SVGImageForContainer. When the page scale changes, users (e.g., RenderImage) already re-send container size, scale, and zoom information in setContainerSizeForRenderer, so we're covered when the user dynamically changes the page scale or device scale.


I'm going to let this sit until tomorrow AM in case there are any further comments.

[WebKit Review Bot]

Comment on attachment 187765 [details]
Add tests for newly-fixed bugs

Clearing flags on attachment: 187765

Committed r142765: <http://trac.webkit.org/changeset/142765>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.