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

RESOLVED FIXED in mozilla24

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Alex, Assigned: seth)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -, relnote-firefox 24+)

Details

Attachments

(19 attachments, 9 obsolete attachments)

31.94 KB, application/pdf
Details
26.31 KB, application/pdf
Details
12.36 KB, image/svg+xml
Details
310 bytes, text/html
Details
502 bytes, image/svg+xml
Details
493 bytes, text/html
Details
289 bytes, image/svg+xml
Details
221 bytes, text/html
Details
297 bytes, image/svg+xml
Details
82.69 KB, image/png
Details
478 bytes, text/html
Details
5.29 KB, image/png
Details
3.33 KB, image/svg+xml
Details
783 bytes, text/html
Details
858 bytes, text/html
Details
21.13 KB, image/jpeg
Details
10.57 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
255 bytes, text/html
Details
6.58 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 479053 [details]
Result of Firefox

This is the PDF output of Firefox printing the test page.
(Reporter)

Comment 2

7 years ago
Created attachment 479054 [details]
Result of Safari

And here's the output from Safari
Thanks for filing -- confirmed with a local print-to-file of the URL given.
Status: UNCONFIRMED → NEW
Depends on: 276431
Ever confirmed: true
Assignee: nobody → dholbert
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
Created attachment 479223 [details]
testcase 1

...and here's the testcase at the given URL (using the attached SVG file).
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.)
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
}
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.
(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|)
blocking2.0: ? → betaN+
Blocks: 603093
Duplicate of this bug: 603093
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.
Summary: SVG loaded via <img> tag is rasterized at low resolution when printed → SVG-as-image is fuzzy/pixelated at particular zoomlevels/sizes, or when printed
Created attachment 484972 [details]
(svg for use in testcase 2)
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.
Summary: SVG-as-image is fuzzy/pixelated at particular zoomlevels/sizes, or when printed → SVG-as-image is fuzzy/pixelated when zoomed or printed, when we trigger the tiling codepath
Wild guess: Maybe you're using CSS pixels instead of device pixels for the size of the drawable.
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...
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.
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.
Created attachment 485129 [details] [diff] [review]
wip fix
Attachment #485128 - Attachment description: wip fix → (sorry, wrong patch)
Attachment #485128 - Attachment is obsolete: true
Whiteboard: [softblocker](?)
blocking2.0: betaN+ → -
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.

Updated

7 years ago
Whiteboard: [softblocker](?)
Blocks: 619500

Comment 20

7 years ago
Created attachment 524149 [details]
Another sub-testcase

Comment 21

7 years ago
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...
Duplicate of this bug: 650490

Updated

6 years ago
Blocks: 669923

Comment 23

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

Comment 24

6 years ago
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.
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.)
Assignee: dholbert → nobody

Comment 26

6 years ago
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:(
Assignee: nobody → lsumar

Updated

6 years ago
Duplicate of this bug: 720954
Created attachment 599378 [details] [diff] [review]
wip fix

unbitrotted
Attachment #485129 - Attachment is obsolete: true

Comment 29

5 years ago
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

5 years ago
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

5 years ago
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

5 years ago
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

5 years ago
Created attachment 621327 [details]
Testcase for altered svg

Updated

5 years ago
Attachment #621327 - Attachment mime type: text/plain → text/html

Comment 34

5 years ago
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.
Blocks: 775483
No longer blocks: 775483
Duplicate of this bug: 775483
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.
Summary: SVG-as-image is fuzzy/pixelated when zoomed or printed, when we trigger the tiling codepath → SVG-as-image is fuzzy/pixelated when scaled or printed, when we trigger the tiling codepath

Updated

5 years ago
Duplicate of this bug: 792530
Blocks: 820679

Updated

5 years ago
Duplicate of this bug: 828834

Comment 39

5 years ago
2 years and 3 months... *push*

Updated

5 years ago

Comment 40

5 years ago
Created attachment 726132 [details]
The exact same SVG background, times 3.

Updated

5 years ago
Duplicate of this bug: 853324

Comment 42

5 years ago
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

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

Comment 44

5 years ago
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

5 years ago
Created attachment 730438 [details]
svg scaling bug example file

Comment 46

5 years ago
Created attachment 730439 [details]
svg scaling bug example html

Comment 47

5 years ago
Created attachment 730440 [details]
svg scaling bug example html
Attachment #730439 - Attachment is obsolete: true

Updated

5 years ago
Attachment #730440 - Attachment mime type: text/plain → text/html

Comment 48

5 years ago
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

5 years ago
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.

Updated

5 years ago
Attachment #730458 - Attachment mime type: text/plain → text/html

Comment 50

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

Updated

4 years ago
Duplicate of this bug: 858357

Comment 52

4 years ago
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

4 years ago
Created attachment 733705 [details]
Blurry SVG in Firefox 20
Jet, this is a pretty bad bug you might want to find an owner for.
Assignee: bugzilla → bugs

Updated

4 years ago
Duplicate of this bug: 862323

Comment 56

4 years ago
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?
I don't think it's related. I'll un-dupe. Let's take further discussion about it over to that bug.

Comment 58

4 years ago
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.
Assignee: bugs → jwatt
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)
Depends on: 859377
(Assignee)

Comment 60

4 years ago
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.
(Assignee)

Comment 61

4 years ago
(Of course, that patch is quite old, but I want to document my concern in any case.)
(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.)
(Assignee)

Comment 63

4 years ago
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.)

Updated

4 years ago
Duplicate of this bug: 865903
Duplicate of this bug: 869269

Comment 66

4 years ago
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

4 years ago
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

4 years ago
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
(Assignee)

Comment 69

4 years ago
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.
Attachment #759626 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Assignee: jwatt → seth
(Assignee)

Comment 70

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #599378 - Attachment is obsolete: true
(Assignee)

Comment 71

4 years ago
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.
(Assignee)

Comment 72

4 years ago
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.
Attachment #759902 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #759626 - Attachment is obsolete: true
Attachment #759626 - Flags: review?(dholbert)

Comment 73

4 years ago
Are you just correcting the scale, or will this patch produce pixel perfect rotations/shears?
(Assignee)

Comment 74

4 years ago
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

4 years ago
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.
(Assignee)

Comment 76

4 years ago
Heh, quite true. Will reupload.
(Assignee)

Comment 77

4 years ago
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.
Attachment #760573 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #759902 - Attachment is obsolete: true
Attachment #759902 - Flags: review?(dholbert)
(Assignee)

Comment 78

4 years ago
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.
Attachment #760575 - Flags: review?(dholbert)
(Assignee)

Comment 79

4 years ago
Here's a try job for the whole patch stack, including the tests.

https://tbpl.mozilla.org/?tree=Try&rev=c50507e3d516
(Assignee)

Comment 80

4 years ago
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.
Attachment #760586 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #760575 - Attachment is obsolete: true
Attachment #760575 - Flags: review?(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 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)
Attachment #760586 - Flags: review?(dholbert) → review+
(Assignee)

Comment 83

4 years ago
(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 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.
[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.)]
(Assignee)

Comment 86

4 years ago
If you navigate through http://innovationbound.nodejitsu.com/advancedProblemSolvingSkills.html#/problems-challenges things are insanely ugly at some points without this patch applied.
(Assignee)

Comment 87

4 years ago
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.
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?
(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?)
(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.
(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!)
(Assignee)

Comment 92

4 years ago
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.
(Assignee)

Comment 93

4 years ago
(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.)
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.
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 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.)
Attachment #760573 - Flags: review?(dholbert) → review+
(Assignee)

Comment 97

4 years ago
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.
(Assignee)

Comment 98

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #760586 - Attachment is obsolete: true
(Assignee)

Comment 99

4 years ago
Heh, "review complaints" is probably not the best term. =) "Review issues".
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)
(Assignee)

Comment 101

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #761832 - Attachment is obsolete: true
(Assignee)

Comment 102

4 years ago
Looks OK. Pushed in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/50d93261dbb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e0a788b159
Awesome! Thanks again for taking this on, Seth -- I'm really happy to see this fixed.
Status: NEW → ASSIGNED
Flags: in-testsuite+
(Assignee)

Comment 104

4 years ago
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.
Flags: needinfo?(akeybl)
https://hg.mozilla.org/mozilla-central/rev/50d93261dbb8
https://hg.mozilla.org/mozilla-central/rev/57e0a788b159
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Setting relnote-firefox to ? should be all that's needed.
relnote-firefox: --- → ?
Flags: needinfo?(akeybl)
(Assignee)

Comment 107

4 years ago
Thanks, Jared.

Comment 108

4 years ago
Thank you guys

Comment 109

4 years ago
Just wondering: any chance for Bug 619500 - SVG as border-image does not scale to element?
(Reporter)

Comment 110

4 years ago
It's great to see this fixed, now I can try convincing my web developer friends to switch to SVG images going forwards.
Can be added to the developer section as it impacts web dev with perf improvements and makes them happy.
(Assignee)

Updated

4 years ago
Blocks: 885939
Blocks: 862323

Updated

4 years ago
relnote-firefox: ? → 24+
Depends on: 890009

Updated

4 years ago
Duplicate of this bug: 896424

Updated

4 years ago
Blocks: 821188

Comment 113

3 years ago
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.
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.
You need to log in before you can comment on or make changes to this bug.