Summary: | Remove the bitmap SVG image cache | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philip Rogers <pdr> | ||||||||||||||
Component: | SVG | Assignee: | Philip Rogers <pdr> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cricri.bug, dglazkov, d-r, eric, fmalita, gtk-ews, gyuyoung.kim, japhet, m.goleb+bugzilla, peter+ews, rakuco, reed, schenney, shezbaig.wk, thorton, tomhudson, webkit-ews, webkit.review.bot, xan.lopez, zimmermann | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 420+ | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 113420 | ||||||||||||||||
Bug Blocks: | 108999, 99481, 104189 | ||||||||||||||||
Attachments: |
|
Description
Philip Rogers
2013-01-04 18:31:18 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).
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 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 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 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 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 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 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 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 Created attachment 187452 [details]
Update
This patch updates the other build systems, fixes a linux compile error, and does some minor cleanup.
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.
Created attachment 187453 [details]
Remove ChangeLog asterisks because the stylebot does not like them.
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 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 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? Created attachment 187705 [details]
Update per reviewer comments
(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 on attachment 187705 [details] Update per reviewer comments Attachment 187705 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/16495411 Created attachment 187714 [details]
Add a file someone forgot to include in the previous patch.
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 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 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 (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 on attachment 187765 [details] Add tests for newly-fixed bugs Clearing flags on attachment: 187765 Committed r142765: <http://trac.webkit.org/changeset/142765> All reviewed patches have been landed. Closing bug. |