Bug 77237 – REGRESSION(99539): SVG <img> disregards page scale and device scale

• Component: SVG
• Status: RESOLVED FIXED

[Tim Horton]

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.

[Radar WebKit Bug Importer]

<rdar://problem/10767413>

[Tim Horton]

This also breaks SVG-in-<img> scaling correctly during fluid zoom in Safari.

[Tim Horton]

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.

[Tim Horton]

Simply scaling the size of the cached bitmap doesn't quite cut it either, in the case where the image has no explicit size.

[Tim Horton]

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?

[Tim Horton]

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

[Tim Horton]

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

[Tim Horton]

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.

[Nikolas Zimmermann]

(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?

[Nikolas Zimmermann]

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

[Simon Fraser (smfr)]

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?

[Tim Horton]

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

[Nikolas Zimmermann]

(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!

[Tim Horton]

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

[Tim Horton]

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.

[Tim Horton]

Created attachment 139730 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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.

[Nikolas Zimmermann]

Comment on attachment 139730 [details]
patch

Looks good to me - you should resolve Simons concerns first though :-)

[Tim Horton]

Created attachment 139912 [details]
patch + smfr's changes

[WebKit Review Bot]

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.

[Tim Horton]

(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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nikolas Zimmermann]

(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?

[Tim Horton]

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

[Tim Horton]

Landed in http://trac.webkit.org/changeset/116001