600207 - SVG-as-image is fuzzy/pixelated when scaled or printed, when we trigger the tiling codepath

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex]

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]

Attachment: Result of Firefox

This is the PDF output of Firefox printing the test page.

Comment 2

[Alex]

Attachment: Result of Safari

And here's the output from Safari

Comment 3

[Daniel Holbert [:dholbert]]

Thanks for filing -- confirmed with a local print-to-file of the URL given.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: (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]]

Attachment: testcase 1

...and here's the testcase at the given URL (using the attached SVG file).

Comment 6

[Daniel Holbert [:dholbert]]

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

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

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

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

[Daniel Holbert [:dholbert]]

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

Attachment: (svg for use in testcase 2)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: 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]]

Wild guess: Maybe you're using CSS pixels instead of device pixels for the size of the drawable.

Comment 15

[Daniel Holbert [:dholbert]]

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

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

Attachment: (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]]

Attachment: wip fix

Comment 19

[Daniel Holbert [:dholbert]]

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]

Attachment: Another sub-testcase

Comment 21

[Marek Raida]

Attachment: 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 23

[paris]

any news to this? its a horrible bug using SVG as css backgrounds and it is unsolved for over 1 year...

Comment 24

[jmckesson]

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

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]

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 28

[Lazar Sumar [:nonsensickle]]

Attachment: wip fix

unbitrotted

Comment 29

[Jack]

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]

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]

Attachment: SVG file from Another sub-testcase (524149), but with viewBox.

Switching from width/height to viewBox makes the associated testcase work!

Comment 32

[Jack]

Attachment: 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]

Attachment: Testcase for altered svg

Comment 34

[Carl Brown]

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 36

[Daniel Holbert [:dholbert]]

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 39

[Jörn Berkefeld]

2 years and 3 months... *push*

Comment 40

[Robert Lidberg]

Attachment: The exact same SVG background, times 3.

Comment 42

[Ian Storm Taylor]

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]

Ian, note that the workaround #c29 works, it's just a little awkward to implement.

Comment 44

[Ian Storm Taylor]

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]

Attachment: svg scaling bug example file

Comment 46

[Elmo Allén]

Attachment: svg scaling bug example html

Comment 47

[Elmo Allén]

Attachment: svg scaling bug example html

Comment 48

[Elmo Allén]

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]

Attachment: 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]

The attachment 730458 [details] shows the same effect, but also on the max zoom level now.

Comment 52

[spizder]

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]

Attachment: Blurry SVG in Firefox 20

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jet, this is a pretty bad bug you might want to find an owner for.

Comment 56

[David]

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

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 (inactive)]

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

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

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

(Of course, that patch is quite old, but I want to document my concern in any case.)

Comment 62

[Daniel Holbert [:dholbert]]

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

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 66

[costa]

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]

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

[Ariane Bouchard]

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

Attachment: (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]]

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

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

Attachment: (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]

Are you just correcting the scale, or will this patch produce pixel perfect rotations/shears?

Comment 74

[Seth Fowler [:seth] [:s2h]]

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 [:longsonr]]

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

Heh, quite true. Will reupload.

Comment 77

[Seth Fowler [:seth] [:s2h]]

Attachment: (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]]

Attachment: (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]]

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

Attachment: (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]]

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

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

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

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

[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]]

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

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

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

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

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

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

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

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

Attachment: 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]]

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

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

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

Attachment: (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]]

Heh, "review complaints" is probably not the best term. =) "Review issues".

Comment 100

[Daniel Holbert [:dholbert]]

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

Attachment: (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 102

[Seth Fowler [:seth] [:s2h]]

Looks OK. Pushed in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/50d93261dbb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e0a788b159

Comment 103

[Daniel Holbert [:dholbert]]

Awesome! Thanks again for taking this on, Seth -- I'm really happy to see this fixed.

Comment 104

[Seth Fowler [:seth] [:s2h]]

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 105

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/50d93261dbb8
https://hg.mozilla.org/mozilla-central/rev/57e0a788b159

Comment 106

[Jared Wein [:jaws] (please needinfo? me)]

Setting relnote-firefox to ? should be all that's needed.

Comment 107

[Seth Fowler [:seth] [:s2h]]

Thanks, Jared.

Comment 108

[Jörn Berkefeld]

Thank you guys

Comment 109

[Lev Solntsev]

Just wondering: any chance for Bug 619500 - SVG as border-image does not scale to element?

Comment 110

[Alex]

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

Can be added to the developer section as it impacts web dev with perf improvements and makes them happy.

Comment 113

[Allan H]

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

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.

Comment 115

[dotnetCarpenter]

I'm not sure, but this bug seems to either have re-appeared or the fix is not thorough enough. In https://bugzilla.mozilla.org/show_bug.cgi?id=1462659 I have demonstrated fuzziness when scaling an HTML container containing SVGs. And it seems remarkable similar to what is described here.

I tried to see attachment https://bug600207.bmoattachments.org/attachment.cgi?id=524150 in Firefox 61 but the test-case fail to load due to CSP errors and no background is visible.

Comment 116

[Daniel Holbert [:dholbert]]

(In reply to dotnetCarpenter from comment #115)
> I tried to see attachment
> https://bug600207.bmoattachments.org/attachment.cgi?id=524150 in Firefox 61
> but the test-case fail to load due to CSP errors and no background is
> visible.

testcase 1 and testcase 2 are still effective testcases for this bug, and they render crisply for me, when zoomed/clicked (the steps that originally made them fuzzy).
testcase 1: https://bug600207.bmoattachments.org/attachment.cgi?id=479223
testcase 2: https://bug600207.bmoattachments.org/attachment.cgi?id=484973

So I think we should still consider this bug fixed (i.e. it didn't regress), and we should keep discussion of similar-seeming issues to newer bugs.  (Thanks for filing that new bug BTW!)

Comment 117

[dotnetCarpenter]

(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #116)

You're right. It even work when using when using CSS scale. Adding `transform-style: preserve-3d;` to the body will make them fuzzy though, but the discussion for this is better to have in https://bugzilla.mozilla.org/show_bug.cgi?id=1462659