Last Comment Bug 600207 - SVG-as-image is fuzzy/pixelated when scaled or printed, when we trigger the tiling codepath
: SVG-as-image is fuzzy/pixelated when scaled or printed, when we trigger the t...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 39 votes (vote)
: mozilla24
Assigned To: Seth Fowler [:seth] [:s2h]
:
:
Mentors:
: 603093 650490 720954 775483 792530 828834 853324 858357 869269 896424 (view as bug list)
Depends on: 276431 859377 890009
Blocks: 669923 win-hidpi 821188 603093 619500 862323 885939
  Show dependency treegraph
 
Reported: 2010-09-28 07:36 PDT by Alex
Modified: 2014-09-21 06:12 PDT (History)
55 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
24+


Attachments
Result of Firefox (31.94 KB, application/pdf)
2010-09-28 07:38 PDT, Alex
no flags Details
Result of Safari (26.31 KB, application/pdf)
2010-09-28 07:38 PDT, Alex
no flags Details
(SVG for use in testcase) (12.36 KB, image/svg+xml)
2010-09-28 16:26 PDT, Daniel Holbert [:dholbert]
no flags Details
testcase 1 (310 bytes, text/html)
2010-09-28 16:29 PDT, Daniel Holbert [:dholbert]
no flags Details
(svg for use in testcase 2) (502 bytes, image/svg+xml)
2010-10-20 22:33 PDT, Daniel Holbert [:dholbert]
no flags Details
testcase 2 (interactive) (493 bytes, text/html)
2010-10-20 22:47 PDT, Daniel Holbert [:dholbert]
no flags Details
(sorry, wrong patch) (1.11 KB, patch)
2010-10-21 13:33 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
wip fix (5.14 KB, patch)
2010-10-21 13:34 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
Another sub-testcase (289 bytes, image/svg+xml)
2011-04-06 00:39 PDT, Marek Raida
no flags Details
Another testcase - pixelated scaling is on this example much more visible (221 bytes, text/html)
2011-04-06 00:42 PDT, Marek Raida
no flags Details
wip fix (4.57 KB, patch)
2012-02-21 14:52 PST, Lazar Sumar [:nonsensickle]
no flags Details | Diff | Splinter Review
SVG file from Another sub-testcase (524149), but with viewBox. (297 bytes, image/svg+xml)
2012-05-05 10:43 PDT, Jack
no flags Details
Screenshot of 621320 altered image in firefox 12 (82.69 KB, image/png)
2012-05-05 11:16 PDT, Jack
no flags Details
Testcase for altered svg (478 bytes, text/html)
2012-05-05 11:19 PDT, Jack
no flags Details
The exact same SVG background, times 3. (5.29 KB, image/png)
2013-03-18 05:04 PDT, Robert Lidberg
no flags Details
svg scaling bug example file (3.33 KB, image/svg+xml)
2013-03-27 17:34 PDT, Elmo Allén
no flags Details
svg scaling bug example html (508 bytes, text/plain)
2013-03-27 17:35 PDT, Elmo Allén
no flags Details
svg scaling bug example html (783 bytes, text/html)
2013-03-27 17:36 PDT, Elmo Allén
no flags Details
svg scaling bug example html with css background (858 bytes, text/html)
2013-03-27 17:48 PDT, Elmo Allén
no flags Details
Blurry SVG in Firefox 20 (21.13 KB, image/jpeg)
2013-04-04 19:22 PDT, spizder
no flags Details
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling. (6.39 KB, patch)
2013-06-06 23:29 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling. (9.71 KB, patch)
2013-06-07 12:59 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling. (10.57 KB, patch)
2013-06-10 14:04 PDT, Seth Fowler [:seth] [:s2h]
dholbert: review+
Details | Diff | Splinter Review
(Part 2) - Tests for SVG image scaling in the presence of page zoom. (3.70 KB, patch)
2013-06-10 14:06 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 2) - Tests for SVG image scaling in the presence of page zoom. (3.70 KB, patch)
2013-06-10 14:17 PDT, Seth Fowler [:seth] [:s2h]
dholbert: review+
Details | Diff | Splinter Review
testcase w/ "transform: scale(4)" (255 bytes, text/html)
2013-06-11 15:37 PDT, Daniel Holbert [:dholbert]
no flags Details
(Part 2) - Tests for SVG image scaling in the presence of page zoom. (4.87 KB, patch)
2013-06-12 18:56 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review
(Part 2) - Tests for SVG image scaling in the presence of page zoom. (6.58 KB, patch)
2013-06-12 22:17 PDT, Seth Fowler [:seth] [:s2h]
no flags Details | Diff | Splinter Review

Description Alex 2010-09-28 07:36:07 PDT
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100927 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b7pre) Gecko/20100927 Firefox/4.0b7pre

When a page including an SVG image via an <img> tag is printed, the SVG image is rasterized during the print output, this differs to the <object> tag, which keeps as much vector data as possible (filters, etc.) when printing.

The resulting rasterized images are created according to the screen PPI, not the printer DPI, causing the image to become blocky/blurry on the output.

There is also similar rasterization occurring when using full-page zoom.

Safari doesn't suffer from this, keeping both forms as vector data.

Reproducible: Always

Steps to Reproduce:
1. Open linked document
2. Print (ideally to PDF or such)
3. View result at multiple zoom levels.
Actual Results:  
<img> included SVG image is rasterized at 96ppi or so.

Expected Results:  
SVG image is kept in it's vector state.
Comment 1 Alex 2010-09-28 07:38:08 PDT
Created attachment 479053 [details]
Result of Firefox

This is the PDF output of Firefox printing the test page.
Comment 2 Alex 2010-09-28 07:38:41 PDT
Created attachment 479054 [details]
Result of Safari

And here's the output from Safari
Comment 3 Daniel Holbert [:dholbert] 2010-09-28 08:28:45 PDT
Thanks for filing -- confirmed with a local print-to-file of the URL given.
Comment 4 Daniel Holbert [:dholbert] 2010-09-28 16:26:45 PDT
Created attachment 479222 [details]
(SVG for use in testcase)

Just so we have a copy of this testcase in bugzilla -- here's the SVG content used in the given URL.
Comment 5 Daniel Holbert [:dholbert] 2010-09-28 16:29:26 PDT
Created attachment 479223 [details]
testcase 1

...and here's the testcase at the given URL (using the attached SVG file).
Comment 6 Daniel Holbert [:dholbert] 2010-09-28 17:21:41 PDT
I can also see this bug at various zoom-levels. It's particularly pronounced if I zoom in the maximum amount, and then back off by 1 step. (either with mousewheel or Ctrl +/-)

From an initial inspection in GDB, this is happening because we trigger the "doTile" case in gfxUtils::DrawPixelSnapped...
>375     PRBool doTile = !aImageRect.Contains(aSourceRect);
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#375
...because aSourceRect (called "sourceRect" in VectorImage::Draw) can easily have some floating-point error, since it's initialized using a transform:
>480   gfxRect sourceRect = aUserSpaceToImageSpace.Transform(aFill);
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/VectorImage.cpp#480

In contrast, aImageRect (called imageRect in VectorImage::Draw) is an exact pixel-value, since it's initialized based on the (exact-pixel-valued) viewport size.

In our testcase, sourceRect and imageRect correspond to the same rectangle -- sized based on the image dimensions -- and so we're supposed to pass the "Contains" check and end up with doTile = PR_FALSE.  But when sourceRect has some float error, we (barely) fail the Contains check, and doTile gets turned on, and we end up with a fuzzy picture.  (If I manually turn doTile off in GDB, that fixes this bug.)
Comment 7 Daniel Holbert [:dholbert] 2010-09-28 17:24:14 PDT
In particular -- with this bug's testcase, zoomed all the way and then backed off with Ctrl-, I get these values in gfxUtils::DrawPixelSnapped:
(gdb) p aImageRect.size
$26 = {
  width = 169, 
  height = 48
}
(gdb) p aSourceRect.size
$27 = {
  width = 169.04999999999998, 
  height = 47.949999999999996
}
Comment 8 Daniel Holbert [:dholbert] 2010-09-28 17:35:13 PDT
Up one level from VectorImage::Draw, we're in DrawImageInternal, which has
  aFill.width = 10140 = 169 * 60
So at 60 app units per dev pixel (the default), the math works out nicely.

However, in the zoomed situation described in Comment 7, we've got 21 app units per dev pixel, so we end up with: 
  drawingParams.mFillRect.size.width = round(10140/21) = 483
(rounded from 482.857)

This rounding (in DrawImageInternal) is where information is lost.

This integer-valued rect, "drawingParams.mFillRect" (with width 483), is what gets passed to VectorImage::Draw, and later transformed (multiplied by 0.349999) to generate the barely-too-large value of aSourceRect.width.
Comment 9 Daniel Holbert [:dholbert] 2010-09-28 17:36:46 PDT
(In reply to comment #8)
> This rounding (in DrawImageInternal) is where information is lost.

er, technically, the rounding is in ComputeSnappedImageDrawingParameters (which DrawImageInternal calls to initialize |drawingParams|)
Comment 10 Daniel Holbert [:dholbert] 2010-10-09 09:52:43 PDT
*** Bug 603093 has been marked as a duplicate of this bug. ***
Comment 11 Daniel Holbert [:dholbert] 2010-10-09 09:55:39 PDT
See also attachment 482053 [details] from duplicate bug 603093.  It has multiple scaled versions of the same SVG image (as a CSS background), and some of the versions are fuzzy at each Firefox full-page-zoom level.
Comment 12 Daniel Holbert [:dholbert] 2010-10-20 22:33:46 PDT
Created attachment 484972 [details]
(svg for use in testcase 2)
Comment 13 Daniel Holbert [:dholbert] 2010-10-20 22:47:49 PDT
Created attachment 484973 [details]
testcase 2 (interactive)

This testcase shows that this bug actually gets triggered whenever we hit the image-tiling code path, for almost all non-100% zoomlevels.

This testcase has a div with a CSS background.  If you click it, it alternates between being the exact size of the image (no tiling) and slightly larger (tiling needed).  The tiling-needed case has noticeably fuzzier edges at all zoomlevels.   (except for the "unlucky" zoomlevels where we're already fuzzy in the no-tiling case, due to the rounding bug described in comment 8)

So the real bug here is that the tiling codepath is producing fuzzy output for some reason.
Comment 14 Markus Stange [:mstange] 2010-10-21 03:27:49 PDT
Wild guess: Maybe you're using CSS pixels instead of device pixels for the size of the drawable.
Comment 15 Daniel Holbert [:dholbert] 2010-10-21 12:13:43 PDT
I am, but even if I scale up the drawable's size (in gdb) to use device pixels, the bug remains.

I think the problem is actually that the static function CreateSamplingRestrictedDrawable() (in gfxUtils.cpp) creates a temporary surface whose size is in "image space" pixels.

In the latest testcase, regardless of zoom-level, we hit CreateSamplingRestrictedDrawable with |aSourceRect| = { 101, 100 }.  That parameter is pretty much directly copied into |size|, which is used to create an offscreen surface that we end up tiling.

Once we've created that surface of size 101x100, we've committed to rasterizing at a not-zoomed resolution, and we're stuck being fuzzy.

So, at least for SVG images, we need to get that temporary surface's size to take our zoom level into account.  That might be possible by scaling up aSourceRect & friends, somewhere before this point...
Comment 16 Daniel Holbert [:dholbert] 2010-10-21 12:30:59 PDT
Ah, I'm passing CSS pixels to Draw's |aViewportSize| parameter right now, but if that were in dev pixels, that would probably help.  (That's what's used to size the gfxDrawable & aSourceRect & friends, so it should address both comment 14 & comment 15.)

From tweaking in GDB, it looks like that gets us to paint at the right resolution, but we zoom twice as much as we should, so I probably just need to counterbalance that change somewhere.
Comment 17 Daniel Holbert [:dholbert] 2010-10-21 13:33:38 PDT
Created attachment 485128 [details] [diff] [review]
(sorry, wrong patch)

This fixes this bug for the attached testcases, but it's not perfect yet.

Issues:
 - it only works when <img> size & <div> size matches the <svg> image's reported size.  (in particular, it does *not* fix https://bugzilla.mozilla.org/attachment.cgi?id=482053 , where the size of the object using it is significantly larger than the svg document's height/width)
 - it breaks full-page-zoom for SVG images that lack a viewBox.  With this patch, we give them a larger region to draw into, and they draw into it at their normal 100% size (as they think they're supposed to).  Without the patch, we give them a normal-size region, and they end up drawing larger (without knowing it) due to the gfxContext's transform.
Comment 18 Daniel Holbert [:dholbert] 2010-10-21 13:34:44 PDT
Created attachment 485129 [details] [diff] [review]
wip fix
Comment 19 Daniel Holbert [:dholbert] 2011-01-18 15:28:56 PST
Just spoke with roc, and we agree that this shouldn't block.

I've been working on it on and off for a bit, and it basically requires that we rework how gfxDrawables behave in some serious ways (in particular gfxSurfaceDrawable and gfxCallbackDrawable -- getting them to be more zoomlevel-aware (and create larger temporary to-be-tiled surfaces for SVG, but not for other types of tiled drawables).

Given the following...
 (a) this bug only happens when we hit the image-tiling codepath (though sadly that includes some non-tiling use cases, because of rounding errors from zoom-level)
 (b) it's not a regression but rather a limitation in a new feature
 (c) reworking gfxDrawable behavior is a bit risky, this late in the game
... it seems reasonable to postpone this bug for fixing post-Firefox-4.
Comment 20 Marek Raida 2011-04-06 00:39:41 PDT
Created attachment 524149 [details]
Another sub-testcase
Comment 21 Marek Raida 2011-04-06 00:42:33 PDT
Created attachment 524150 [details]
Another testcase - pixelated scaling is on this example much more visible

and I would personally more than welcome to have this working => it would be great to draw in SVG small logo and then stretch it in HQ on main page background => right know I wouldn't dare to do this yet...
Comment 22 Daniel Holbert [:dholbert] 2011-04-16 11:56:02 PDT
*** Bug 650490 has been marked as a duplicate of this bug. ***
Comment 23 paris 2011-12-23 02:06:19 PST
any news to this? its a horrible bug using SVG as css backgrounds and it is unsolved for over 1 year...
Comment 24 jmckesson 2011-12-24 21:29:02 PST
We have had **5** releases since Firefox 4 and it's still there. It may not be blocking of a release, but it's stupid that the FF developers have basically forgotten about this.
Comment 25 Daniel Holbert [:dholbert] 2011-12-24 23:01:05 PST
No news to report.  (If there were news, you'd have seen it here. :))

Unassigning, as I've been focusing on implementing other features (css flexbox in particular) and am not actively working on this.  I haven't forgotten about it, and I still would like to fix this, though if someone else gets to it first I'd be happy with that as well. :)

(This is non-trivial, as noted in comment 19.  The basic problem is that the existing infrastructure used for drawing tiled background images (gfxDrawable) was designed with raster graphics in mind, so IIRC it assumes that it can rasterize to a temporary surface very early on, at a place in the code that doesn't have easy access to our ultimate zoom-level.  And then later, that already-rasterized surface is scaled to the right level, which causes blurring, which is expected for raster graphics, but not for SVG.)
Comment 26 paris 2011-12-25 08:48:18 PST
thanks for your feedback. i agree it sounds like this bug is not easy to fix but i also think this is not a trivial bug. with this bug it is just not possible to use background SVG images because of zoom problems and its not possible to use svg backgrounds in combination with background-size because this causes the same issue:(
Comment 27 Robert Longson 2012-01-25 02:31:50 PST
*** Bug 720954 has been marked as a duplicate of this bug. ***
Comment 28 Lazar Sumar [:nonsensickle] 2012-02-21 14:52:05 PST
Created attachment 599378 [details] [diff] [review]
wip fix

unbitrotted
Comment 29 Jack 2012-05-05 06:44:58 PDT
From what I see on my firefox, the problem has been mis-represented.

Looking at svg files above, the problem disappears if I *replace* width and height attributes with a viewBox="0 0 width height".  Then magically all fuzziness disappears :D.

For some reason if firefox sees a width or a height on the svg element, even if it contains a viewBox, it tries to scale it as if it were a raster image of dimensions given by width and height.
Comment 30 Carl Brown 2012-05-05 08:25:25 PDT
Jack, what units did you use in the viewBox attribute? viewBox="0 0 100 100" has a different outcome than "0 0 100px 100px".

But even without height and width attributes, fuzziness remains. It just appears at zoom levels different from those that lead to the bug using svg files with width and height attributes. Maybe this has something to do with the rounding errors that lead to tiling codepath, mentioned in comment 13.



Are there any news on this bug? It becomes more and more important since SVG files are the only reliable way to meet the emergence of high-resolution displays. And it has been unsolved for more than one and a half year.
Comment 31 Jack 2012-05-05 10:43:12 PDT
Created attachment 621320 [details]
SVG file from Another sub-testcase (524149), but with viewBox.

Switching from width/height to viewBox makes the associated testcase work!
Comment 32 Jack 2012-05-05 11:16:31 PDT
Created attachment 621323 [details]
Screenshot of 621320 altered image in firefox 12

So in answer to your question, it was the former, the viewBox without units.  I.e. viewbox="0 0 100 62", see attachment 621320 [details].

Here I attach a screenshot showing the modified svg file in attachment 621320 [details]. the crystal clear lines in firefox 12, though I have tested in previous versions of firefox and also the result looks the same, i.e. good.

I would find it interesting to see a testcase of it not working at other zoom levels, as my experience with firefox has always to code svg files like this and I have never seen a problem.

I shall upload testcase from before pointing to my svg for easy reference.
Comment 33 Jack 2012-05-05 11:19:40 PDT
Created attachment 621327 [details]
Testcase for altered svg
Comment 34 Carl Brown 2012-05-05 11:37:09 PDT
Thank you for your reply. You are right, as seen in your example, switching to viewBox seems to fix the issue in the case of CSS backgrounds. That is very interesting.

I had only tested it using the html img tag to unclude the svg, like in testcase 1 (attachment 479223 [details]) and zooming using ctrl + mouse wheel. Modifying the svg as you did doesn’t seem to help in that case.
Comment 35 Daniel Holbert [:dholbert] 2012-07-19 12:34:12 PDT
*** Bug 775483 has been marked as a duplicate of this bug. ***
Comment 36 Daniel Holbert [:dholbert] 2012-09-14 11:45:33 PDT
FWIW, this currently affects the navbar at the top of https://www.apple.com/ (if you zoom or print that page).  That uses https://www.apple.com/global/nav/images/globalnav_text.svg as an SVG image sprite, for the stylized text on the navbar.
Comment 37 Robert Longson 2012-09-19 12:50:33 PDT
*** Bug 792530 has been marked as a duplicate of this bug. ***
Comment 38 Robert Longson 2013-01-10 08:36:12 PST
*** Bug 828834 has been marked as a duplicate of this bug. ***
Comment 39 Jörn Berkefeld 2013-01-10 09:12:15 PST
2 years and 3 months... *push*
Comment 40 Robert Lidberg 2013-03-18 05:04:51 PDT
Created attachment 726132 [details]
The exact same SVG background, times 3.
Comment 41 Mardeg 2013-03-20 23:12:57 PDT
*** Bug 853324 has been marked as a duplicate of this bug. ***
Comment 42 Ian Storm Taylor 2013-03-25 21:58:18 PDT
SVG is pretty much unusable for CSS backgrounds because of this bug, because not being able to scale SVG is the most ironic thing ever.

2.5 years, any update?
Comment 43 Philip Rogers 2013-03-25 22:01:31 PDT
Ian, note that the workaround #c29 works, it's just a little awkward to implement.
Comment 44 Ian Storm Taylor 2013-03-25 22:41:49 PDT
Thanks :) but yeah I'm working with lots of SVG files, that are potentially made by others, so it doesn't work well for me.
Comment 45 Elmo Allén 2013-03-27 17:34:26 PDT
Created attachment 730438 [details]
svg scaling bug example file
Comment 46 Elmo Allén 2013-03-27 17:35:18 PDT
Created attachment 730439 [details]
svg scaling bug example html
Comment 47 Elmo Allén 2013-03-27 17:36:51 PDT
Created attachment 730440 [details]
svg scaling bug example html
Comment 48 Elmo Allén 2013-03-27 17:44:45 PDT
It seems not to be working for me. Comment #34 shows the problem as well, on the img element. The object element is rendered sharp on every zoom level.

I also created an attachment of another svg file not working. On 100 % zoom, all the five different img's of the same file (at different width and height) are sharp. When zooming in, different images become fuzzy at different times. At max zoom in level, they are all sharp, as well as on some zoom levels on the way between.

One mouse wheel notch back from the max zoom, images 2, 3, and 5 show clearly pixelating artifacts.

No effect whether the hardware acceleration is on or off. No effect whether the width and height attributes are present in the svg (in the attachment, they are not).

I have also tried using it as css background. No difference, if the width and height attributes are set or not.
Comment 49 Elmo Allén 2013-03-27 17:48:00 PDT
Created attachment 730458 [details]
svg scaling bug example html with css background

This is an html wrapper for the svg file (attachment 730438 [details]), but using css background property.
Comment 50 Elmo Allén 2013-03-27 17:49:36 PDT
The attachment 730458 [details] shows the same effect, but also on the max zoom level now.
Comment 51 Alice0775 White 2013-04-04 18:56:42 PDT
*** Bug 858357 has been marked as a duplicate of this bug. ***
Comment 52 spizder 2013-04-04 19:21:19 PDT
2.5 years and 16 releases since FF 4, and the bug is still there.

Makes it pretty much impossible to use background image SVG in production.
Comment 53 spizder 2013-04-04 19:22:23 PDT
Created attachment 733705 [details]
Blurry SVG in Firefox 20
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-05 04:53:08 PDT
Jet, this is a pretty bad bug you might want to find an owner for.
Comment 55 Robert Longson 2013-04-16 06:53:45 PDT
*** Bug 862323 has been marked as a duplicate of this bug. ***
Comment 56 David 2013-04-18 16:01:31 PDT
I did open the ticket 862323 and was closed as duplicate, what happens is that embedded svg images appear blurry after changing the viewbox values on the root svg, you can see an example on the follow URL.

http://jsfiddle.net/davidaco/YMWha/17/

This error was not happening before release 20, 

is this error related to this ticket?
Comment 57 Daniel Holbert [:dholbert] 2013-04-18 16:11:48 PDT
I don't think it's related. I'll un-dupe. Let's take further discussion about it over to that bug.
Comment 58 Jet Villegas (:jet) 2013-04-19 03:21:31 PDT
To jwatt for a look. Try saving off the zoom-level (gfxMatrix::ScaleFactors) when gfxDrawable rasterizes a SVG image, and replace the bitmap at the correct zoom-level when we detect a mis-match.
Comment 59 Daniel Holbert [:dholbert] 2013-04-19 15:26:36 PDT
From talking to seth in IRC just now, it sounds like his work in bug 859377 may change what happens under-the-hood here.  I'm marking this as depending on that bug, since the patch here may end up benefiting from bug 859377's changes. (or alternately, may collide with bug 859377's changes)
Comment 60 Seth Fowler [:seth] [:s2h] 2013-04-19 15:51:15 PDT
Comment on attachment 599378 [details] [diff] [review]
wip fix

Review of attachment 599378 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like the wrong layer for this change to me; am I missing something? Generally if you are checking imgIContainer::GetType() outside of imagelib, you are probably (though not definitely) doing something wrong.
Comment 61 Seth Fowler [:seth] [:s2h] 2013-04-19 15:52:12 PDT
(Of course, that patch is quite old, but I want to document my concern in any case.)
Comment 62 Daniel Holbert [:dholbert] 2013-04-19 16:04:08 PDT
(In reply to Seth Fowler [:seth] from comment #60)
> This looks like the wrong layer for this change to me; am I missing
> something? Generally if you are checking imgIContainer::GetType() outside of
> imagelib, you are probably (though not definitely) doing something wrong.

I think it makes sense to check imgIContainer::GetType() here.  The basic problem is explained in comment 15, but I'll rephrase it here, a little clearer, perhaps.

Basically: we're making a gfxDrawable for our image, and if we detect that it's going to end up tiling (even just a tiny bit), then that makes us create a gfxSurfaceDrawable with a temporary surface.  The question is, how big do we want that surface to be?

Currently, we make that temporary surface the image's intrinsic size, in CSS pixels, and then if we're zoomed or HiDPI, it may get scaled when we draw it. This behavior makes sense for raster images (because we can't do anything better), but for SVG images, we'd prefer to make a *larger* temporary surface using the actual device-pixel resolution that we're going to end up painting with.  That's why there's that "if (isSVG)" check in the wip patches that've been attached so far.

(If it'd make you feel better, we could drop the GetType() checks and just unconditionally use a larger surface, at device-pixel resolution, but it'd probably be a waste for raster images, which are the common case.)
Comment 63 Seth Fowler [:seth] [:s2h] 2013-04-19 18:06:15 PDT
Right, that all makes sense, but I stand by the view that the image itself should be the thing to compute this. That is, an Image object should know what size is appropriate for drawing itself. Clearly to support this we'd need to modify the API a bit, and we'd need to think about what that should look like, but I'm really against testing for a specific type of image when it can be avoided, and I'm not convinced it can't in this case.

(Note that I do not think any solution that wastes memory is a good one.)
Comment 64 Mardeg 2013-04-25 16:16:57 PDT
*** Bug 865903 has been marked as a duplicate of this bug. ***
Comment 65 Daniel Holbert [:dholbert] 2013-05-07 22:29:10 PDT
*** Bug 869269 has been marked as a duplicate of this bug. ***
Comment 66 costa 2013-05-08 07:36:33 PDT
I don't know if this is helpful at all, but svg is crisp and lovely in Nightly 20.0a1 (2013-01-06).

Here's my example: http://innovationbound.nodejitsu.com/advancedProblemSolvingSkills.html#/problems-challenges. Press right arrow to navigate through the presentation zooming to various levels with CSS.
Comment 67 Greg Edwards 2013-05-08 07:46:32 PDT
I can replicate with your URL: http://puu.sh/2Pirs.jpg
And I can still replicate with the Apple header.
Keep in mind that the bug is finnicky and suddenly fixes itself at certain zoom levels.
Comment 68 azma 2013-05-11 10:56:17 PDT
The bug doesn't only appear when the user zooms or prints the page. On my mobile website in development, without zooming or anything - in short, in the basic, standard size - the scissors background-image is blurry. I think as soon as you use an SVG image in a size different than the size the SVG file was originally created in, smaller or larger, it shows up wrong. In this case, the SVG image is larger than what is shown on the screen... which is strange, because as far as I know, scaling down a PNG works much better than this.

http://i.imgur.com/q1c2Wf4.png
Comment 69 Seth Fowler [:seth] [:s2h] 2013-06-06 23:29:23 PDT
Created attachment 759626 [details] [diff] [review]
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling.

Here's a proposed patch. This resolves every test case I've tried so far, and stands up to the SVG reftests.

Here's a summary: when we hit the tiling codepath in DrawPixelSnapped and rasterize to a temporary surface, that surface is not sized with the scale of the current transform taken into account. Rather than scale the temporary surface in all cases (which would hurt perf for raster images), we bake the scale into the draw parameters in VectorImage::Draw and remove it from the transform altogether. This means that the temporary surface created on the tiling codepath is exactly the same size as what we intend to ultimately draw to the screen, eliminating the fuzziness. Other than a minor adjustment to nsSVGImageFrame, no callsites have to be updated; the fix does its work from inside VectorImage.
Comment 70 Seth Fowler [:seth] [:s2h] 2013-06-06 23:34:20 PDT
Here's a try job. There will be a part 2 with some tests as well, but it's getting late so I'm going to come back to that later.

https://tbpl.mozilla.org/?tree=Try&rev=560416800dde
Comment 71 Seth Fowler [:seth] [:s2h] 2013-06-06 23:37:46 PDT
Incidentally, I'd like to thank everyone who contributed test cases to this bug. There are some really good ones here that made it a _lot_ easier to figure out what was going on.
Comment 72 Seth Fowler [:seth] [:s2h] 2013-06-07 12:59:49 PDT
Created attachment 759902 [details] [diff] [review]
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling.

OK, all the failures in the try job above were tests that used to fail passing. I've updated the appropriate reftest.list files. Not submitting a new try job since there's no actual code changes.
Comment 73 Greg Edwards 2013-06-07 13:35:23 PDT
Are you just correcting the scale, or will this patch produce pixel perfect rotations/shears?
Comment 74 Seth Fowler [:seth] [:s2h] 2013-06-07 15:23:07 PDT
The scaling was the source of the issue. Rotations and shears should just work. (I can confirm this in this case of rotations; I don't have a test case for shears.)
Comment 75 Robert Longson 2013-06-07 15:59:25 PDT
These lines can go...

> # We don't really care about the failures for this
> # extreme edge case (the test exists more to test for safety against division by
> # zero), so there is no bug has been filed to fix it, although a patch would
> # probably be accepted.
> # They're still marked as failing though, rather than 'load', since
> # we want to know if they start working when we upgrade to Azure.
Comment 76 Seth Fowler [:seth] [:s2h] 2013-06-07 16:00:11 PDT
Heh, quite true. Will reupload.
Comment 77 Seth Fowler [:seth] [:s2h] 2013-06-10 14:04:17 PDT
Created attachment 760573 [details] [diff] [review]
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling.

Updated comments and whitespace in the reftest.list files as suggested in comment 75. No code changes, so no new try job.
Comment 78 Seth Fowler [:seth] [:s2h] 2013-06-10 14:06:32 PDT
Created attachment 760575 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

These tests check for regression of this bug on both zoom-in and zoom-out. Verified locally. Need to send out another try job, though.
Comment 79 Seth Fowler [:seth] [:s2h] 2013-06-10 14:07:38 PDT
Here's a try job for the whole patch stack, including the tests.

https://tbpl.mozilla.org/?tree=Try&rev=c50507e3d516
Comment 80 Seth Fowler [:seth] [:s2h] 2013-06-10 14:17:42 PDT
Created attachment 760586 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

Switched the SVG circle used in the reftests from red to green since it does not indicate failure.
Comment 81 Daniel Holbert [:dholbert] 2013-06-10 14:27:59 PDT
Comment on attachment 760573 [details] [diff] [review]
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling.

Thanks for taking this!

>Bug 600207 (Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling. r=dholbert

s/SVGs/SVG images/

>-skip-if(B2G) == svg-border-image-repaint-1.html svg-border-image-repaint-1-ref.html
>+skip-if(B2G) fuzzy(2,1) == svg-border-image-repaint-1.html svg-border-image-repaint-1-ref.html

Was this fuzzy on all platforms, or just one? If just one, please scope it just to that platform.  If it's multi-platform, please file a followup and annotate this line with the followup bug number.  (It'd be useful to note which pixel is fuzzy.)
Comment 82 Daniel Holbert [:dholbert] 2013-06-10 14:31:56 PDT
Comment on attachment 760586 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

>+++ b/layout/reftests/svg/as-image/zoom/reftest.list
>+# Ensure that zooming doesn't result in fuzzy images. (Bug 600207.)

Maybe "Ensure that zooming doesn't make scaled SVG images fuzzy"

I wouldn't bother with the bug number. That comment should be sufficient, and if someone really wants to know more, they can look at the hg blame.

r=me on the tests; I want to look at the code-patch a little more (and do a quick GDB session to make sure I know what's going on). r+ on that part likely coming later today (with comment 81 addressed)
Comment 83 Seth Fowler [:seth] [:s2h] 2013-06-10 14:39:27 PDT
(In reply to Daniel Holbert [:dholbert] (traveling June 7-12, w/ mixed availability) from comment #81)
> Was this fuzzy on all platforms, or just one? If just one, please scope it
> just to that platform.  If it's multi-platform, please file a followup and
> annotate this line with the followup bug number.  (It'd be useful to note
> which pixel is fuzzy.)

TBH I'm not sure if this applies to all platforms, though I strongly suspect it does. I'll file a bug based on the results of this try job:

https://tbpl.mozilla.org/?tree=Try&rev=47ec4045a096
Comment 84 Daniel Holbert [:dholbert] 2013-06-10 14:43:01 PDT
Comment on attachment 760586 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

Actually, sorry -- several nits on the tests:

Firstly, prepend them all with a doctype and CC0 copyright header, per
 https://www.mozilla.org/MPL/headers/

<!DOCTYPE html>
<!-- Any copyright is dedicated to the Public Domain.
   - http://creativecommons.org/publicdomain/zero/1.0/ -->

Secondly:
>+++ b/layout/reftests/svg/as-image/zoom/img-fuzzy-zoomIn-1-ref.html
>@@ -0,0 +1,23 @@
>+<html reftest-zoom="1.0">

Drop the reftest-zoom from the reference cases.  (It has no effect since it's 1.0 -- and even if it did have an effect, that'd be bad because we want references to be basically "dumb" static HTML)


>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/as-image/zoom/img-fuzzy-zoomIn-1.html
>@@ -0,0 +1,23 @@
>+<html reftest-zoom="0.9">

The test naming is reversed.  "zoomIn" actually has zooming *out* applied (i.e. the circle is smaller when the zoom is applied), and vice versa.  So -- please swap the test names.
Comment 85 Daniel Holbert [:dholbert] 2013-06-10 15:03:58 PDT
[aside: I think that some other patch must've improved things here since this was filed; the only testcase that *unmistakably/grossly* demonstrates this now, AFAICT, is attachment 524150 [details], in my linux nightly. (and attachment 730458 [details] a bit, but somewhat less obviously.)]
Comment 86 Seth Fowler [:seth] [:s2h] 2013-06-10 15:05:46 PDT
If you navigate through http://innovationbound.nodejitsu.com/advancedProblemSolvingSkills.html#/problems-challenges things are insanely ugly at some points without this patch applied.
Comment 87 Seth Fowler [:seth] [:s2h] 2013-06-10 16:48:36 PDT
So the new tests are currently failing on all platforms that have finished so far. Pretty frustrating as they work perfectly locally. I'm developing on a high DPI screen, which probably has something to do with this.
Comment 88 Daniel Holbert [:dholbert] 2013-06-10 21:29:12 PDT
So at least in the case of the testcase in attachment 524150 [details], I believe we're succeeding where we previously failed due to |doTile| now ending up 'false' in DrawPixelSnapped (which is sufficient to make this work).  So we're succeeding by avoiding the tiling codepath, rather than by fixing the tiling codepath.

Are there cases where we still have |doTile| == true where we end up un-fuzzy where we used to be fuzzy?
Comment 89 Daniel Holbert [:dholbert] 2013-06-11 11:43:43 PDT
(In reply to Seth Fowler [:seth] from comment #86)
> If you navigate through
> http://innovationbound.nodejitsu.com/advancedProblemSolvingSkills.html#/
> problems-challenges things are insanely ugly at some points without this
> patch applied.

(Not on my linux laptop, w/ a recent nightly - looks pretty smooth to me over here.  Maybe the issues at that site are retina-display dependent?)
Comment 90 Daniel Holbert [:dholbert] 2013-06-11 12:11:04 PDT
(In reply to Daniel Holbert [:dholbert] (traveling June 7-12, w/ mixed availability) from comment #85)
> [aside: I think that some other patch must've improved things here since
> this was filed

Using (zoomed) attachment 479223 [details] as a testcase, I narrowed the improvement to this range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e3a4ebcf067&tochange=8f9ba85eb61c
and in that range, it looks like bug 786064 probably helped us out here.
Comment 91 Daniel Holbert [:dholbert] 2013-06-11 15:20:31 PDT
(In reply to Daniel Holbert [:dholbert] (traveling June 7-12, w/ mixed availability) from comment #88)
> Are there cases where we still have |doTile| == true where we end up
> un-fuzzy where we used to be fuzzy?

[answering my own question, after digging a bit more]: Yes, there are cases -- e.g. attachment 484973 [details], the "interactive testcase" (though you have to trigger a full-window-repaint by e.g. alt-tabbing, or else the div doesn't get invalidated there.)

So: I was initially concerned that the patch might be fixing a related-but-different bug, but now I think it does actually fix this original bug. (nice!)
Comment 92 Seth Fowler [:seth] [:s2h] 2013-06-11 15:33:48 PDT
So http://jsfiddle.net/gLWE9/1/embedded/result/ is a pretty good test case. I can easily make it look horrible by zooming in and out on current Nightly. Looks fine with the patch in this bug.
Comment 93 Seth Fowler [:seth] [:s2h] 2013-06-11 15:35:27 PDT
(Basically bug 786064 helps us trigger the tiling codepath in fewer cases, but the problem is still as bad as ever in cases where we still tile, such as when there's actual tiling going on.)
Comment 94 Daniel Holbert [:dholbert] 2013-06-11 15:37:48 PDT
Created attachment 761196 [details]
testcase w/ "transform: scale(4)"

As shown by this testcase (which renders as fuzzy for me), you can trigger this bug with "transform: scale(4)" on an element with a tiled background -- no need for full-page-zoom.

Probably worth including another reftest that, like this testcase:
 a) ...uses a tiled background, to be sure we're actually triggering the tiling code path.
 b) ...uses "transform: scale" instead of reftest-zoom, since it's another (and more easy-to-quickly-test-by-loading-the-file) way to trigger this.
Comment 95 Daniel Holbert [:dholbert] 2013-06-11 15:41:48 PDT
oops, I midaired you when I attached that new testcase.  Cool, sounds like we're on the same page.

> As shown by this testcase (which renders as fuzzy for me),

(To be clear: it looks fuzzy in nightly, but it looks crisp with the patch.)
Comment 96 Daniel Holbert [:dholbert] 2013-06-11 22:56:56 PDT
Comment on attachment 760573 [details] [diff] [review]
(Part 1) - Avoid fuzzy SVGs on the tiling path by matrix twiddling.

OK -- r=me on the code part.

RE the test part, r=me stands with comment 84 & comment 94 addressed (and with the test passing reliably.  Feel free to twiddle the numbers if it helps make the already-attached reftests pass more reliably, too, as long as they still fail pre-patch.)
Comment 97 Seth Fowler [:seth] [:s2h] 2013-06-12 13:20:28 PDT
Thanks for the review, Daniel. I'll try to get in front of a standard-DPI display today to figure out the test issue; probably some numbers need to be tweaked. I'll make the changes you've requested too.
Comment 98 Seth Fowler [:seth] [:s2h] 2013-06-12 18:56:53 PDT
Created attachment 761832 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

OK, I've addressed all the review complaints. The tests should be more robust now. They all use tiling to ensure that we always hit the appropriate code path, which I think should take care of the difference across platforms and DPIs. I've also added test variations that use |transform|. If this looks good on try, I think this should be ready to check in.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=eeb9299beee9
Comment 99 Seth Fowler [:seth] [:s2h] 2013-06-12 18:58:12 PDT
Heh, "review complaints" is probably not the best term. =) "Review issues".
Comment 100 Daniel Holbert [:dholbert] 2013-06-12 22:12:02 PDT
Looks like the new test-patch is just missing the new reftest files, for the transform reftest. (You probably forgot to 'hg add' them before qrefing)
Comment 101 Seth Fowler [:seth] [:s2h] 2013-06-12 22:17:17 PDT
Created attachment 761879 [details] [diff] [review]
(Part 2) - Tests for SVG image scaling in the presence of page zoom.

It'd probably help if I included the new tests in the patch. Sigh.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=6f6efbb17c6f
Comment 103 Daniel Holbert [:dholbert] 2013-06-13 11:30:18 PDT
Awesome! Thanks again for taking this on, Seth -- I'm really happy to see this fixed.
Comment 104 Seth Fowler [:seth] [:s2h] 2013-06-13 12:18:59 PDT
Glad to do it. =)

I suspect that between bug 786064 and this one, we have something worth putting in the release notes. ("Major SVG rendering improvements" or something.)

Alex, it was suggested to me that you might the person to talk to about this - not sure what the process is here.
Comment 106 Jared Wein [:jaws] (please needinfo? me) 2013-06-13 12:51:45 PDT
Setting relnote-firefox to ? should be all that's needed.
Comment 107 Seth Fowler [:seth] [:s2h] 2013-06-13 12:56:41 PDT
Thanks, Jared.
Comment 108 Jörn Berkefeld 2013-06-13 13:26:16 PDT
Thank you guys
Comment 109 Lev Solntsev 2013-06-13 13:33:31 PDT
Just wondering: any chance for Bug 619500 - SVG as border-image does not scale to element?
Comment 110 Alex 2013-06-13 20:03:14 PDT
It's great to see this fixed, now I can try convincing my web developer friends to switch to SVG images going forwards.
Comment 111 bhavana bajaj [:bajaj] 2013-06-21 13:13:01 PDT
Can be added to the developer section as it impacts web dev with perf improvements and makes them happy.
Comment 112 Alice0775 White 2013-07-22 12:59:11 PDT
*** Bug 896424 has been marked as a duplicate of this bug. ***
Comment 113 Allan H 2014-04-29 15:17:48 PDT
This bug seems to have reappeared in recent versions of firefox. It was fixed in firefox 25, but occurs in firefox 27 and 29. The bottom image in testcase1 above becomes blurry when printed for me, as before.
Comment 114 Daniel Holbert [:dholbert] 2014-04-29 15:30:33 PDT
Darn -- thanks for the heads-up about that. I can confirm your report -- printed versions of Testcase 1 are fuzzy in Firefox 27 and beyond.  (The non-printing-dependent zooming aspects of this bug do seem to still be fixed, though).

Rather than picking up the discussion again here, I filed a new bug to cover this: bug 1003505.

Note You need to log in before you can comment on or make changes to this bug.