Created attachment 124354 [details] repro See attached repro; after 99539 (Niko's new SVGImageCache), the <img> referencing an SVG is rasterized and then scaled (by the -webkit-transform: scale); previously it was rendered at the correct resolution.
<rdar://problem/10767413>
This also breaks SVG-in-<img> scaling correctly during fluid zoom in Safari.
Just invalidating the cache on scale changes isn't quite sufficient, because the image is always created at the same resolution. Still, I should have a fix soon.
Simply scaling the size of the cached bitmap doesn't quite cut it either, in the case where the image has no explicit size.
The new image cache disrespects three different sources of scale: * deviceScaleFactor * pageScaleFactor * CSS transforms I have a patch that fixes the first two, but after talking to Simon, it's not clear there's a good way to fix the third one. CachedImage::setContainerSizeForRenderer (which calls into SVGImageCache::setRequestedSizeAndZoom) often gets called from outside of rendering, and trying to get the CSS transform scale there is in those cases infeasible. Niko, this is a serious regression caused by r99539. Do you have any thoughts?
> CachedImage::setContainerSizeForRenderer (which calls into SVGImageCache::setRequestedSizeAndZoom) often gets called from outside of rendering, and trying to get the CSS transform scale there is in those cases infeasible. Partially scratch that; I can do the scale entirely within lookupOrCreateBitmapImageForRenderer and still make it work (I thought I'd tried that before, but no dice), which only gets called from rendering. However, I still have no access to the graphics context at the moment to retrieve the CTM.
It seems to me that the CachedImage machinery in general, not just in the SVG case, depends on knowing the size before being rendered. This is just only a problem for SVG, because other images aren't expected to scale and/or aren't cached as bitmaps? I'm playing around with a fix; I wonder if I should go ahead and fix not respecting deviceScaleFactor and pageScaleFactor first, in a separate bug, and add in CSS transform scale later...
Niko, I'm sending this to you for now since it's a regression from your change. If you have any concrete ideas, feel free to send it back.
(In reply to comment #5) > The new image cache disrespects three different sources of scale: > > * deviceScaleFactor > * pageScaleFactor Can you enlighten me a bit about those two? I only recently discovered that ImageBuffer::create got a scale argument, though the one who introduced it made SVG pass '1' always. How does this relate to full page zoom, when zooming on using Ctrl+ <+> in Safari? What's the exact difference between device & page scale factor?
(In reply to comment #5) > * CSS transforms > > I have a patch that fixes the first two, but after talking to Simon, it's not clear there's a good way to fix the third one. CachedImage::setContainerSizeForRenderer (which calls into SVGImageCache::setRequestedSizeAndZoom) often gets called from outside of rendering, and trying to get the CSS transform scale there is in those cases infeasible. Its never used outside of rendering! The only users are RenderBoxModelObject/RenderReplaced and RenderSVGImage. That said, it should be easy to take CSS transforms into account, similar to how we handle setting -webkit-transform on SVG elements. Basically we need to transform the incoming containerSize, with the renderers transformed stored in its RenderStyle. That should work.
Are we going to potentially cache the image at lots of different scales, but only ever use one of those scales after a given time? When is the cache pruned?
(In reply to comment #9) > (In reply to comment #5) > > The new image cache disrespects three different sources of scale: > > > > * deviceScaleFactor > > * pageScaleFactor > Can you enlighten me a bit about those two? I only recently discovered that ImageBuffer::create got a scale argument, though the one who introduced it made SVG pass '1' always. > How does this relate to full page zoom, when zooming on using Ctrl+ <+> in Safari? > What's the exact difference between device & page scale factor? deviceScaleFactor is the ratio of device pixels to CSS pixels. This is only interesting on devices like the Retina display i* devices, where deviceScaleFactor=2. pageScaleFactor is an arbitrary zoom factor, used for the fluid zoom feature in Safari and probably other things (and things in other ports). Those and the full-page CSS zoom all multiply to form one big scale factor that we need to respect for any image buffers we make around WebCore.
(In reply to comment #11) > Are we going to potentially cache the image at lots of different scales, but only ever use one of those scales after a given time? When is the cache pruned? We're caching images per renderer, so if one renderer changes size or scale, the cached image for the renderer will be replaced. This is triggers by each setContainerSizeForRenderer(renderer, sizeOfImageContainer) if the sizeOfImageContainer changes, or if the renderer is not yet in the cache. (In reply to comment #12) > pageScaleFactor is an arbitrary zoom factor, used for the fluid zoom feature in Safari and probably other things (and things in other ports). > > Those and the full-page CSS zoom all multiply to form one big scale factor that we need to respect for any image buffers we make around WebCore. Good to know, there are quite a few places that don't respect this at present. Thanks for the info about those scale factors!
(In reply to comment #10) > Basically we need to transform the incoming containerSize, with the renderers transformed stored in its RenderStyle. That should work. Scaling the containerSize breaks in the case where the <svg> doesn't have an explicit size. Poking around...
Let's use this bug for page and device scale (which are easy and seem more important) and https://bugs.webkit.org/show_bug.cgi?id=85335 for CSS, which is a little harder. I have a (very simple, once I started from scratch) patch for this one, I'll post it in a bit.
Created attachment 139730 [details] patch
Comment on attachment 139730 [details] patch Attachment 139730 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12587735 New failing tests: svg/as-image/image-respects-pageScaleFactor.html svg/as-image/image-respects-deviceScaleFactor.html
Created attachment 139735 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 139730 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=139730&action=review > Source/WebCore/svg/graphics/SVGImage.cpp:186 > + IntRect scaledRect(rect); > + scaledRect.scale(scale); Does this round, or always ceil? I'd think that you'd want to scale in floating point, but ensure the buffer is big enough by rounding up. > Source/WebCore/svg/graphics/SVGImageCache.cpp:154 > + IntSize scaledSize(size); > + scaledSize.scale(scale); Ditto.
Comment on attachment 139730 [details] patch Looks good to me - you should resolve Simons concerns first though :-)
Created attachment 139912 [details] patch + smfr's changes
Attachment 139912 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Traceback (most recent call last): File "Tools/Scripts/check-webkit-style", line 48, in <module> sys.exit(CheckWebKitStyle().main()) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check self._text_file_reader.process_file(file_path=path, line_numbers=None) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 844, in process min_confidence) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 619, in dispatch min_confidence) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 593, in _create_checker checker = PNGChecker(file_path, handle_style_error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/png.py", line 43, in __init__ self._fs = host.filesystem AttributeError: 'NoneType' object has no attribute 'filesystem' If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #22) > AttributeError: 'NoneType' object has no attribute 'filesystem' > > > If any of these errors are false positives, please file a bug against check-webkit-style. /me puts on bot-hat thorton-pro:OpenSource > check-webkit-style (svgimage-scale2) Total errors found: 0 in 13 files
Comment on attachment 139912 [details] patch + smfr's changes Attachment 139912 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12579121 New failing tests: svg/as-image/image-respects-deviceScaleFactor.html
Created attachment 139931 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #24) > (From update of attachment 139912 [details]) > Attachment 139912 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12579121 > > New failing tests: > svg/as-image/image-respects-deviceScaleFactor.html Interessting, the other test you've added image-reppects-pageScaleFactor passes on chromium, matching the mac baseline. This test just times out, according to the archived results posted by the bot. Doesn't chromium support device scale factors? Why would it hang?
(In reply to comment #26) > (In reply to comment #24) > > (From update of attachment 139912 [details] [details]) > > Attachment 139912 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/12579121 > > > > New failing tests: > > svg/as-image/image-respects-deviceScaleFactor.html > > Interessting, the other test you've added image-reppects-pageScaleFactor passes on chromium, matching the mac baseline. This test just times out, according to the archived results posted by the bot. > > Doesn't chromium support device scale factors? Why would it hang? Apparently they do not! https://bugs.webkit.org/show_bug.cgi?id=70066 I will skip on non-Mac ports for now.
Landed in http://trac.webkit.org/changeset/116001