WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
77237
REGRESSION(99539): SVG <img> disregards page scale and device scale
https://bugs.webkit.org/show_bug.cgi?id=77237
Summary
REGRESSION(99539): SVG <img> disregards page scale and device scale
Tim Horton
Reported
2012-01-27 12:52:05 PST
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.
Attachments
repro
(252 bytes, text/html)
2012-01-27 12:52 PST
,
Tim Horton
no flags
Details
patch
(88.17 KB, patch)
2012-05-01 19:43 PDT
,
Tim Horton
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-04
(5.65 MB, application/zip)
2012-05-01 20:30 PDT
,
WebKit Review Bot
no flags
Details
patch + smfr's changes
(112.20 KB, patch)
2012-05-02 16:43 PDT
,
Tim Horton
simon.fraser
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ec2-cr-linux-01
(5.78 MB, application/zip)
2012-05-02 18:17 PDT
,
WebKit Review Bot
no flags
Details
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2012-01-27 12:53:20 PST
<
rdar://problem/10767413
>
Tim Horton
Comment 2
2012-01-27 12:53:55 PST
This also breaks SVG-in-<img> scaling correctly during fluid zoom in Safari.
Tim Horton
Comment 3
2012-02-28 00:33:10 PST
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
Comment 4
2012-02-28 00:45:53 PST
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
Comment 5
2012-03-28 12:53:43 PDT
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
Comment 6
2012-03-28 13:11:33 PDT
> 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
Comment 7
2012-03-28 15:32:39 PDT
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
Comment 8
2012-03-28 15:58:01 PDT
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
Comment 9
2012-03-29 02:38:54 PDT
(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
Comment 10
2012-03-29 03:07:47 PDT
(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)
Comment 11
2012-03-29 08:52:40 PDT
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
Comment 12
2012-03-29 09:37:17 PDT
(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
Comment 13
2012-03-29 12:40:34 PDT
(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
Comment 14
2012-04-30 15:09:04 PDT
(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
Comment 15
2012-05-01 18:54:07 PDT
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
Comment 16
2012-05-01 19:43:34 PDT
Created
attachment 139730
[details]
patch
WebKit Review Bot
Comment 17
2012-05-01 20:29:49 PDT
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
Comment 18
2012-05-01 20:30:08 PDT
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 19
2012-05-01 20:48:32 PDT
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 20
2012-05-02 00:37:20 PDT
Comment on
attachment 139730
[details]
patch Looks good to me - you should resolve Simons concerns first though :-)
Tim Horton
Comment 21
2012-05-02 16:43:00 PDT
Created
attachment 139912
[details]
patch + smfr's changes
WebKit Review Bot
Comment 22
2012-05-02 16:45:43 PDT
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
Comment 23
2012-05-02 16:48:31 PDT
(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 24
2012-05-02 18:17:00 PDT
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
Comment 25
2012-05-02 18:17:07 PDT
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
Comment 26
2012-05-03 01:41:42 PDT
(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
Comment 27
2012-05-03 13:03:13 PDT
(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
Comment 28
2012-05-03 13:31:47 PDT
Landed in
http://trac.webkit.org/changeset/116001
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