WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
98403
SVG image cache is very broken
https://bugs.webkit.org/show_bug.cgi?id=98403
Summary
SVG image cache is very broken
Branimir Lambov
Reported
2012-10-04 06:39:16 PDT
There's a special cache for SVG images in the webkit code, SVGImageCache.cpp, which is supposed to cache the image as one or more bitmaps so that it can be quickly redrawn. This cache is broken in many ways: * It usually does not apply at all, because it relies on a call to set dimensions that it never receives because it is normally made before the cache is constructed (see CachedImage::setContainerSizeForRenderer). This masks the other problems with the cache as it is hard (but possible) to engage the cache. * The cache does not take SVG/CSS transformations into account when calculating the cached image scale, which may result in images that are way too big or way too small, in both cases resulting in very ugly rendering. * The cache re-renders all cached content on all image changes rather than waiting for a renderer to actually request cached data, resulting in wasteful and unnecessary pauses. * The cache is per-renderer, which is too wasteful and practically defeats its purpose.
Attachments
Example
(43.98 KB, application/zip)
2012-10-04 06:57 PDT
,
Branimir Lambov
no flags
Details
Proposed patch
(
deleted
)
2012-10-19 08:44 PDT
,
Branimir Lambov
no flags
Details
Formatted Diff
Diff
Patch
(75.48 KB, patch)
2012-10-19 09:00 PDT
,
Branimir Lambov
no flags
Details
Formatted Diff
Diff
Patch
(75.41 KB, patch)
2012-10-19 09:06 PDT
,
Branimir Lambov
kling
: review-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Branimir Lambov
Comment 1
2012-10-04 06:57:17 PDT
Created
attachment 167096
[details]
Example The attached file illustrates some of the problems on Chrome Linux: * When the file is opened, usually the cache does not engage and 26 copies of the SVG image are all re-drawn every frame. Performance is very bad, images look good. * Hitting refresh sometimes causes the page to load with the cache activated. When this happens: * * The page does not show for a few seconds while 26 copies of the SVG file are rendered at a resolution that is 20x20 times higher than required; * * After that the animation is smooth, but the images get scaled from the wrong resolution and look bad (non-antialiased). On Safari getting the cache activated also results in practically frozen image as it tries to do a smooth scale from the absurdly high resolution. (Sorry for the ZIP file, Chrome and Firefox both crash when attempting to upload this unzipped)
Branimir Lambov
Comment 2
2012-10-08 01:44:48 PDT
I am working on a patch to fix this.
Philip Rogers
Comment 3
2012-10-08 10:21:31 PDT
Agreed on all points! I'm looking forward to this fix :)
Branimir Lambov
Comment 4
2012-10-19 08:44:27 PDT
Created
attachment 169629
[details]
Proposed patch
Alexey Proskuryakov
Comment 5
2012-10-19 08:53:09 PDT
Did you intend to submit this patch for review? Please mark it "review?" if so by clicking Details link to the right of the patch.
Branimir Lambov
Comment 6
2012-10-19 09:00:55 PDT
Created
attachment 169630
[details]
Patch
Branimir Lambov
Comment 7
2012-10-19 09:06:53 PDT
Created
attachment 169631
[details]
Patch
Branimir Lambov
Comment 8
2012-10-19 09:16:48 PDT
How can I get more information about why the bots fail to apply the patch?
Andreas Kling
Comment 9
2012-10-19 09:21:31 PDT
Comment on
attachment 169631
[details]
Patch This should be split up into multiple patches to make it clear which changes fix which bugs.
Antti Koivisto
Comment 10
2012-10-19 09:27:58 PDT
Nikolas should take a look.
Branimir Lambov
Comment 11
2012-10-19 09:35:49 PDT
Splitting is going to be very difficult if at all possible. Apart from the change to SVGPreserveAspectRatio.cpp, this is in fact a single patch that replaces a broken component which causes a number of seemingly unrelated problems. I can extract the change to SVGPreserveAspectRatio into a separate patch which blocks this, but I cannot split it further without breaking something or losing existing functionality (however broken).
Dirk Schulze
Comment 12
2012-10-19 10:07:11 PDT
Comment on
attachment 169631
[details]
Patch I am surprised that preserve aspect ratio code needs fixes. We had quite some tests for it, but mainly inline code, with the exception of one test on feImage. Did you run pixel tests with the changes?
Branimir Lambov
Comment 13
2012-10-19 11:02:19 PDT
Yes, I did run pixel tests, and the only two that were affected (with the SVGImageCache code) were svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html which are now rendered properly, in agreement with what the other browsers do. The code would run fine if the source and destination locations were 0, 0, or if a method other than 'slice' was used. I need it to render <img> HTML tags in RenderImage.cpp, where the destination location is practically never 0, 0.
Nikolas Zimmermann
Comment 14
2012-10-21 07:46:36 PDT
Thanks Branimir, it's great to see someone revisiting this code. Can you update the patch, so we see if it builds everywhere? Andreas, are you convinced that this is hard to split up?
Philip Rogers
Comment 15
2013-02-19 15:50:34 PST
Over the past couple months I have fixed most of these issues, so this bug is no longer valid. - Replaces the broken SVGImageCache class with two separate maps, dealing with container sizes and cached bitmaps separately. - Makes sure CachedImage stores container size requests made before the image is loaded. - Wraps relative size SVG images into an ImageWithContainerSize object which takes care to assign the container size immediately before drawing. - Changes SVGImage to implement drawPattern instead of nativeImageForCurrentFrame to enable SVG patterns to be drawn at the correct scale.
http://trac.webkit.org/changeset/140722
- Preserve container size requests across image loads
http://trac.webkit.org/changeset/142765
- Replace SVG bitmap cache with directly-rendered SVG
http://trac.webkit.org/changeset/143257
- Fix scaling of tiled SVG backgrounds on high-dpi displays
http://trac.webkit.org/changeset/141637
- Change hasAlpha to isKnownToBeOpaque and correct the return value for SVG images.
http://trac.webkit.org/changeset/141303
- Track scale and zoom together when drawing SVG images
http://trac.webkit.org/changeset/139484
- Skip CachedImage::CreateImage if we don't have image data
http://trac.webkit.org/changeset/138976
- Clear pending container size requests as early as possible
http://trac.webkit.org/changeset/138958
- Refactor client removal in CachedResource::switchClientsToRevalidatedResource
http://trac.webkit.org/changeset/137981
- Queue container size requests while images are loading. - Implements caching of SVG images as bitmaps rendered at the correct scale. - Makes sure all bitmap caching takes world transforms into account. N/A - replaced with directly-rendered SVG. - Fixes 'slice' SVG aspect ratio calculations which were using the wrong offset. Cleaned up and landed Branimir's patch:
http://trac.webkit.org/changeset/143389
- Fixes incorrect zooming of fixed size SVG images. TODO(pdr):
https://bugs.webkit.org/show_bug.cgi?id=110272
Piotr Janik
Comment 16
2013-05-29 09:35:06 PDT
I'm not familiar with WebKit / Safari release cycle, so can I ask when we should expect fixes mentioned by Philip to show up in stable Safari release? In general I work on application using JS and SVG animations pretty heavily:
http://lab.dev.concord.org/interactives.html#interactives/sam/DNA-to-proteins/3-modeling-translation.json
If you open this page in Safari 6.0.4 and click "Transcribe" or "Translate" you will notice that the animation isn't smooth and various objects (<image> tags) show up with a long delay or even don't show up at all (during transcription - red nucleotides' backbones, during translation - tRNAs => "green forks" carrying amino acids). Not sure if these problems are related to this bug, but it seems to me that they can. In WebKit Nightly everything works much better, however occasionally there are still problems with tRNA images ("green forks" during translation, they appear after long delay). Everything works fine and smooth in Chrome. Do you think these problems can be connected with this particular bug?
Alexey Proskuryakov
Comment 17
2013-05-29 10:35:29 PDT
We do not comment on vendors' release cycles, sorry.
Philip Rogers
Comment 18
2013-05-29 10:55:12 PDT
As AP mentioned, the release cycles are unfortunately not public. If your page works in WebKit nightly though, there's a pretty good chance it will be in the next Safari release. For your second issue that's still present in WebKit nightly, do you mind creating a minimized testcase and filing a separate bug about it? I'm happy to look into it.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug