Bug 98403

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)

I am working on a patch to fix this.

Agreed on all points! I'm looking forward to this fix :)

Created attachment 169629 [details] Proposed patch

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.

Created attachment 169630 [details] Patch

Created attachment 169631 [details] Patch

How can I get more information about why the bots fail to apply the patch?

Comment on attachment 169631 [details] Patch This should be split up into multiple patches to make it clear which changes fix which bugs.

Nikolas should take a look.

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

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?

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.

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?

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

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?

We do not comment on vendors' release cycles, sorry.

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.