Bug 106159 - Remove the bitmap SVG image cache
Summary: Remove the bitmap SVG image cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on: 113420
Blocks: 99481 104189 108999
  Show dependency treegraph
 
Reported: 2013-01-04 18:31 PST by Philip Rogers
Modified: 2013-03-27 16:02 PDT (History)
20 users (show)

See Also:


Attachments
Remove the bitmap SVG image cache (39.52 KB, patch)
2013-02-08 21:24 PST, Philip Rogers
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Update (43.04 KB, patch)
2013-02-09 18:29 PST, Philip Rogers
no flags Details | Formatted Diff | Diff
Remove ChangeLog asterisks because the stylebot does not like them. (43.04 KB, patch)
2013-02-09 18:36 PST, Philip Rogers
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Update per reviewer comments (40.62 KB, patch)
2013-02-11 15:55 PST, Philip Rogers
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Add a file someone forgot to include in the previous patch. (46.86 KB, patch)
2013-02-11 16:23 PST, Philip Rogers
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Add tests for newly-fixed bugs (50.69 KB, patch)
2013-02-11 20:34 PST, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2013-01-04 18:31:18 PST
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.
Comment 1 Philip Rogers 2013-02-08 21:24:06 PST
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).
Comment 2 WebKit Review Bot 2013-02-08 21:33:00 PST
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.
Comment 3 Early Warning System Bot 2013-02-08 21:36:13 PST
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
Comment 4 Early Warning System Bot 2013-02-08 21:37:47 PST
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
Comment 5 EFL EWS Bot 2013-02-08 21:39:43 PST
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
Comment 6 WebKit Review Bot 2013-02-08 21:40:07 PST
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
Comment 7 kov's GTK+ EWS bot 2013-02-08 21:47:19 PST
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
Comment 8 Peter Beverloo (cr-android ews) 2013-02-08 21:47:44 PST
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
Comment 9 WebKit Review Bot 2013-02-09 12:01:53 PST
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
Comment 10 Philip Rogers 2013-02-09 18:29:27 PST
Created attachment 187452 [details]
Update

This patch updates the other build systems, fixes a linux compile error, and does some minor cleanup.
Comment 11 WebKit Review Bot 2013-02-09 18:32:53 PST
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.
Comment 12 Philip Rogers 2013-02-09 18:36:11 PST
Created attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.
Comment 13 Philip Rogers 2013-02-09 19:25:35 PST
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
Comment 14 WebKit Review Bot 2013-02-09 20:31:58 PST
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
Comment 15 Stephen Chenney 2013-02-11 07:59:22 PST
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?
Comment 16 Philip Rogers 2013-02-11 15:55:27 PST
Created attachment 187705 [details]
Update per reviewer comments
Comment 17 Philip Rogers 2013-02-11 16:06:12 PST
(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.
Comment 18 Early Warning System Bot 2013-02-11 16:10:29 PST
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
Comment 19 Philip Rogers 2013-02-11 16:23:07 PST
Created attachment 187714 [details]
Add a file someone forgot to include in the previous patch.
Comment 20 WebKit Review Bot 2013-02-11 19:29:18 PST
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
Comment 21 Philip Rogers 2013-02-11 20:34:12 PST
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
Comment 22 Tim Horton 2013-02-12 05:11:13 PST
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
Comment 23 Philip Rogers 2013-02-12 13:36:24 PST
(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.
Comment 24 WebKit Review Bot 2013-02-13 11:45:58 PST
Comment on attachment 187765 [details]
Add tests for newly-fixed bugs

Clearing flags on attachment: 187765

Committed r142765: <http://trac.webkit.org/changeset/142765>
Comment 25 WebKit Review Bot 2013-02-13 11:46:07 PST
All reviewed patches have been landed.  Closing bug.