933057 - SVG <image> tags displaying wrong size, particularly when zoomed

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Moore]

Attachment: test.zip

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025150754

Steps to reproduce:

This works correctly in Firefox 24, but breaks in 25.  It also is appearing in Aurora at the moment.

In inline SVG code in an HTML document, include another SVG document via an <image> tag.

Attached is an example of a 100x100 image being included, with comparison to a 100px wide div.


Actual results:

Included image does not appear at the correct size.  It appears to have an automatic scale or crop applied to it.


Expected results:

The image size is correct in Firefox 24, Chrome 30, and IE 10.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reporter's unzipped testcase

Here's the testcase's HTML file, with the external SVG image referenced via a data URI tag so that it's all included.

I can't reproduce the reported behavior -- the image is 100px wide for me

xdhmoore, can you confirm whether you see the bug in this version?

(I can't reproduce, nor do I see the bug in a local version from the zip file; Firefox 24 and 25 and trunk all behave the same for me. I'm on Linux, though; might be a windows-specific thing)

Comment 2

[mjh563]

I can reproduce this when the page is zoomed in. It looks like a regression in Firefox 25.

Steps to reproduce:
Load the testcase, then zoom the page by pressing Ctrl-+ a few times.

Expected result:
Both coloured blocks get zoomed.

Actual result:
Only the blue block is zoomed. The red block remains its original size.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to mjh563 from comment #2)
> I can reproduce this when the page is zoomed in. It looks like a regression
> in Firefox 25.

(I see that too, but that's not a regression -- e.g. I get that same behavior in old Firefox 20 & 23 nightlies that I have lying around.)

Comment 4

[Daniel Holbert [:dholbert]]

I filed bug 933130 on comment 2 / 3, since it sounds like what the author described in comment 0 here is different. (reportedly a recent regresion, & no mention of full-page zoom)

Comment 5

[mjh563]

xdhmoore, can you confirm whether the bug you're reporting only happens when the page is zoomed in, or does it also happen at the default zoom level?

Comment 6

[mjh563]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (I see that too, but that's not a regression -- e.g. I get that same
> behavior in old Firefox 20 & 23 nightlies that I have lying around.)

Right. It works as expected in Firefox 24 release version though.

Comment 7

[Daniel Moore]

Attachment: screenshots.jpg

2 screenshots of what I am seeing for the attached test case in FF 24 and FF 25.

Comment 8

[Daniel Moore]

(In reply to mjh563 from comment #5)
> xdhmoore, can you confirm whether the bug you're reporting only happens when
> the page is zoomed in, or does it also happen at the default zoom level?

It does happen at the default zoom level, and I am seeing it in the data url version as well.  It may be only a Windows issue.  It is working correctly on Aurora (26?) in Mac OSX. I added a file with 2 combined screenshots of what I am seeing.

Comment 9

[Daniel Holbert [:dholbert]]

Whoah, you're right. I can confirm that it works in release build from:
 https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/24.0/linux-x86_64/en-US/

So this appears to be something we briefly fixed (possibly with bug 600207?) and then re-broke (probably with one of the followups for bug 600207, maybe bug 885939?)

(Note also that the simpler testcase in my spin-off bug 933130 _is_ broken in v24 (albeit in a different way from how it's broken on nightly); so I'm leaving that bug as being spun off for now, since it may be revealing a subtly different issue.)

(In reply to xdhmoore from comment #8)
> It does happen at the default zoom level, and I am seeing it in the data url
> version as well.  It may be only a Windows issue.

Your bad-rendering screenshot matches what I (and I suspect mjh563) see when zoomed in on Linux, so I'm guessing we're talking about the same issue. (though maybe your zoom levels don't map to our zoom levels for some reason)  I'm adding "zoom" to the bug title, since that seems to help at least.

Comment 10

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/f5b6b5523719
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131003073923
Bad:
http://hg.mozilla.org/mozilla-central/rev/51b36c5fd45f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131003080100
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f5b6b5523719&tochange=51b36c5fd45f


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/922eb17b8a44
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131002174014
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3a4efc8623c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131002190604
http://hg.mozilla.org/integration/mozilla-inbound/rev/2ec8fcf012bd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131003015316
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=922eb17b8a44&tochange=c3a4efc8623c

Regressed by:
	85cb12480a3e	Seth Fowler — Bug 920621 (Part 1) - Correct for full page zoom in nsSVGImageFrame. (Partial revert of bug 600207.) r=dholbert

Comment 11

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

There are some other things ahead of this in my queue but I'll go ahead and take this.

Comment 13

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

Attachment: (Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

Here's the patch. This fixes all of the test cases in this bug and doesn't regress bug 920621. Given how subtle this stuff can be, though, I'm holding off on r? until I'm sure try looks good.

https://tbpl.mozilla.org/?tree=Try&rev=a8b3ba4b4c3c

Comment 14

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

Comment on attachment 828363 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

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

::: layout/base/nsLayoutUtils.cpp
@@ +4333,5 @@
>    nsIntSize imageSize;
>    if (aImage->GetType() == imgIContainer::TYPE_VECTOR) {
> +    // SVG images need to know the size of the region into which they are
> +    // being drawn (the viewport size), so we compute that here. The same size
> +    // is used to compute the destination rect.

The hard part of fixing these zoom bugs is figuring out which value is actually wrong, since frequently multiple approaches will work for a single bug - you can either scale down A or scale up B, for example, and both produce correct results. You then later learn that scaling up B broke some other case we don't have a test for. =)

Splinter doesn't show enough context to discuss this as nicely as I'd like, but after a lot of time in gdb my conclusion is that the wrong value in this recent crop of zoom bugs is |imageSize|, here in DrawSingleImage. This value eventually becomes |aViewportSize| in VectorImage::Draw, and you'd think the solution would simply be to scale it there as we do with the other drawing parameters, but that doesn't work. DrawSingleImage (via code in functions like GetWholeImageDestination) bakes the value of |imageSize|/|aViewportSize| into the other drawing parameters. By the time you get to VectorImage::Draw, just scaling |aViewportSize| is not enough to correct the problem.

I believe this patch is the correct fix to this immediate issue, but the real long term fix is probably to restructure how DrawSingleImage and friends work to move a bit more of the intelligence into imgIContainer::Draw. I've said it many times by now and I still think it's true: checking imgIContainer::GetType and behaving differently depending on the result is a serious code smell, and it indicates that our abstractions are wrong. That is, in my opinion, definitely the case here.

Comment 15

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

To be clear: when I say 'long term fix', the thing I'm trying to fix is how easy it is to reason about this code and fix future bugs.

Comment 16

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

Attachment: (Part 2) - Add tests for scaling explicitly sized svg:image elements in inline SVGs.

Here are the new tests. The 'd' variations actually test this case, while the 'c' variations are added for completeness. It's a shame that such a simple variation on the existing tests would have caught this. =\

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 828407 [details] [diff] [review]
(Part 2) - Add tests for scaling explicitly sized svg:image elements in inline SVGs.

>diff --git a/layout/reftests/svg/image/image-svg-inline-zoom-in-01c.html b/layout/reftests/svg/image/image-svg-inline-zoom-in-01c.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/svg/image/image-svg-inline-zoom-in-01c.html

r=me, with one optional improvement: 

I noticed that these "c" and "d" files are nearly identical to their "a" variants (just a different image src, and in some cases px units instead of %)

So ideally, the new files should be created with "hg cp", e.g.
  hg cp image-svg-inline-zoom-in-01a.html image-svg-inline-zoom-in-01c.html

That way, this changeset would highlight the "new and interesting" bits of these tests, and share the "a" variant's hg blame for the unchanged lines.

Doesn't matter too much, since it's just 4 files and the files are pretty short anyway. But if you feel like rewriting the patch to use "hg cp", bug 921761 comment 17 has some steps that you could follow.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 828363 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.


>+++ b/layout/base/nsLayoutUtils.cpp
>   nsIntSize imageSize;
>   if (aImage->GetType() == imgIContainer::TYPE_VECTOR) {
>-    imageSize.width  = nsPresContext::AppUnitsToIntCSSPixels(aDest.width);
>-    imageSize.height = nsPresContext::AppUnitsToIntCSSPixels(aDest.height);
>+    // SVG images need to know the size of the region into which they are
>+    // being drawn (the viewport size), so we compute that here. The same size
>+    // is used to compute the destination rect.
>+    nsIntSize destSize(nsPresContext::AppUnitsToIntCSSPixels(aDest.width),
>+                       nsPresContext::AppUnitsToIntCSSPixels(aDest.height));
>+
>+    // Determine the page zoom that's currently applied.
>+    nscoord appUnitsPerDevPx = aRenderingContext->AppUnitsPerDevPixel();
>+    gfxFloat pageZoomFactor =
>+      nsPresContext::AppUnitsToFloatCSSPixels(appUnitsPerDevPx);
>+    if (pageZoomFactor == 0) {
>+      pageZoomFactor = 1.0;
>+    }
>+
>+    // Rescale the region to account for the page zoom.
>+    imageSize.width = destSize.width / pageZoomFactor;
>+    imageSize.height = destSize.height / pageZoomFactor;

So IIUC, this patch is changing imageSize from being in CSS pixels to now being in *dev pixels*, for SVG images.

The patch currently takes us through CSS pixels as an intermediate, but I think (?) we can skip that step.

Isn't the above code equivalent to just:
 imageSize.width = appUnitsPerDevPx * aDest.width;
 imageSize.height = appUnitsPerDevPx * aDest.height;
or something like that?

Comment 19

[Daniel Holbert [:dholbert]]

(i.e. I don't think we need to bother with pageZoomFactor or destSize)

Comment 20

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

Thanks for the review, Daniel!

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Isn't the above code equivalent to just:
>  imageSize.width = appUnitsPerDevPx * aDest.width;
>  imageSize.height = appUnitsPerDevPx * aDest.height;
> or something like that?

Hah, yes! Absolutely. I got too stuck on the "page zoom" idea. I'll update the patch.

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #20)
> Thanks for the review, Daniel!

Of course -- thanks for investigating & fixing! :)

So, one other thing... I'm a little uneasy about how we're using "imageSize" here. It used to be exclusively in CSS pixels, but now it might either be dev pixels or CSS pixels depending on the image type.

We pass it to DrawImageInternal (which isn't clear about its expectations, but presumably expects it to be in CSS pixels). And then *that* passes it to imgIContainer::Draw as the "aViewportSize" arg, and the Draw documentation does say that it's expected to be in CSS pixels:
 http://mxr.mozilla.org/mozilla-central/source/image/public/imgIContainer.idl#206

Maybe that documentation should be updated?

Or, maybe we should keep things in terms of CSS pixels, and do this scaling inside of Draw()?  (We sort of have access to appUnitsPerDevPixel there, via aUserSpaceToImageSpace -- not sure how easy it is to tease out of that, though.)

Comment 22

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

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Or, maybe we should keep things in terms of CSS pixels, and do this scaling
> inside of Draw()?  (We sort of have access to appUnitsPerDevPixel there, via
> aUserSpaceToImageSpace -- not sure how easy it is to tease out of that,
> though.)

The problem is that other drawing parameters also get screwed up. =\ I'm not sure trying to infer this in there is the right thing, anyway. IMO the solution in this patch is the right short-term solution and a bigger change is the right long-term solution.

Let's back up a bit.

In bug 600207 and related bugs, the big change that happened was that we started drawing SVG images internally as if their size was perfect for the rect they were being drawn into - the goal was to avoid any scaling. So internally, the 'size' of the SVG image is already "destination area in dev pixels". What's caused these recent zooming bugs is that certain calculations are done outside of VectorImage with a different perception for what the size of the image is. This patch brings the code in DrawSingleImage into harmony with what's already happening in VectorImage::Draw. Now DrawSingleImage, DrawImageInternal, and VectorImage::Draw all perceive the SVG image to be the same size.

So I don't think that this is a coordinate space issue, necessarily; it's more that we are choosing an appropriate size for the image to get the results we want. That convenient size can be viewed as being in CSS pixels if we wish - we just _choose_ the size using a calculation involving device pixels.

A reasonable response to this would be 'that's an ugly hack', and I _totally_ agree. However, in order to avoid the hack, I think what we need to do is move some of the logic of DrawSingleImage/DrawImageInternal/etc. into imgIContainer::Draw, so we can stop trying to infer things from the drawing parameters we're given and start just directly making the calculations we need. This requires some thought as to what should move and what should say, what API changes might be desirable, etc. In the end this will produce better results (in particular, it should allow the removal of some of the ugly math in VectorImage::Draw and friends) but it's a larger project, and I think we should go ahead and fix this bug before tackling it.

Comment 23

[Daniel Holbert [:dholbert]]

Thanks for the background & summary. I agree that this is probably the right short term solution.  I just wanted to make sure I understood the change & that this wasn't making us violate our assumptions. Fortunately (after some more thought), I think it's fine & I'm just misreading the assumptions.

(In reply to Seth Fowler [:seth] from comment #22)
> we are choosing an appropriate size for the image to get the
> results we want. That convenient size can be viewed as being in CSS pixels
> if we wish

This is the key part - now that I'm grocking this, the patch makes more sense.

Rephrasing what you said: basically, DrawSingleImage is used to dealing with raster images (which have an intrinsic width & height in pixels), and to fit in, we have to behave as if we also have a particular size in pixels. We could pick any size (and end up being really high-res or really low-res), but the size that makes the most sense is the size of the area we're being rendered into, converted into dev pixels. That lets us emulate a raster image that is exactly the right resolution for the screen.

Comment 24

[Daniel Holbert [:dholbert]]

It'd be good to include a comment that gets the point of Comment 23 across, since (at least for me) this code made a lot more sense once I realized that. e.g.:

 // The idea here is to emulate a raster image that's perfectly sized such that
 // each pixel in the image maps exactly to a pixel on-screen.
 imageSize.width  = [convert aDest.width into dev pixels];
 imageSize.height = [convert aDest.height into dev pixels];

(This is essentially r+ with comment 20 addressed and with a comment along those ^ lines.)

Comment 25

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

I think that's a great summary, and I'll update the comment to include it.

Comment 26

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

Attachment: (Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

All the review comments should now be addressed.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 829578 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

>-    imageSize.width  = nsPresContext::AppUnitsToIntCSSPixels(aDest.width);
>-    imageSize.height = nsPresContext::AppUnitsToIntCSSPixels(aDest.height);
>+    // We draw vector images at a size that emulates a raster image which is
>+    // perfectly sized for the destination rect: each pixel in the image maps
>+    // exactly to a single pixel on-screen.
>+    nscoord appUnitsPerDevPx = aRenderingContext->AppUnitsPerDevPixel();
>+    imageSize.width = aDest.width / appUnitsPerDevPx;
>+    imageSize.height = aDest.height / appUnitsPerDevPx;


This should use NSAppUnitsToIntPixels()  (like we use internally to implement AppUnitsToIntCSSPixels). This handles rounding issues, so that e.g. 59 app units == 1 pixel instead of 0 pixels.

Defined here:
  http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsCoord.h#369

>   } else {
>+    // We draw raster images at their intrinsic size.

This is misleading. We really might be drawing the image at any multiple of its intrinsic size (depending on aDest and maybe other factors), right?

If you can think of a way to rephrase this to clear up that ambiguity, please do; otherwise, I'd rather just drop this comment.

r=me with the above.

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #27)
> This should use NSAppUnitsToIntPixels()

So e.g. this should be:
  imageSize.width = NSAppUnitsToIntPixels(aDest.width, appUnitsPerDevPx);

Comment 29

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

Thanks for the quick review! I'll make those changes.

(In reply to Daniel Holbert [:dholbert] from comment #27)
> This is misleading. We really might be drawing the image at any multiple of
> its intrinsic size (depending on aDest and maybe other factors), right?

The problem here is the verb 'draw'. I'll rephrase.

Comment 30

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

Attachment: (Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

Final version of the patch.

Comment 31

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8255ebcdfe7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/edbf63c3afe2

Comment 32

[Daniel Holbert [:dholbert]]

Yup, that's clearer. Thanks!

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8255ebcdfe7e
https://hg.mozilla.org/mozilla-central/rev/edbf63c3afe2

Comment 34

[Daniel Moore]

Awesome.  Thanks!

Comment 35

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

(In reply to Daniel Moore from comment #34)
> Awesome.  Thanks!

Thanks for reporting the bug! Clean testcases like the one you supplied _really_ help!

Comment 36

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

Needinfo'ing myself to remind myself to request uplift to Aurora and Beta in a couple of days, once the patch spends some time cooking on m-c.

Comment 37

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

Comment on attachment 829597 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 885939 (Although this only briefly ever worked in the past, after being temporarily fixed by bug 600207 - the bug has been present for a very long time, in many release versions.)
User impact if declined: SVG images will render wrong. There is significant potential for breakage of sites that use SVG heavily.
Testing completed (on m-c, etc.): On m-c for 6 days with no issues.
Risk to taking this patch (and alternatives if risky): Since this changes how SVG images are rendered - in particular, how they are scaled - there could be another regression that we don't have test coverage for. Any fix for the bug is going to have this problem, so I don't see a meaningful alternative other than not fixing the bug.
String or IDL/UUID changes made by this patch: None.

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 829597 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

Let's let this bake on Aurora, there's no rush to fix a longstanding bug this late into the beta cycle.

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] [:lsblakk] from comment #38)
> Let's let this bake on Aurora, there's no rush to fix a longstanding bug
> this late into the beta cycle.

To clarify: while this *was* a longstanding bug, we actually fixed it in Firefox 24 (per comment 9), and then we re-broke without noticing it in Firefox 25 and later.

So this is technically a regression with respect to Firefox 24.

(needinfo'ing lsblakk in case that changes her approval calculus in comment 38.)

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 829597 [details] [diff] [review]
(Part 1) - Correct scaling of SVG images in nsLayoutUtils::DrawSingleImage.

Ah, that does affect it.  We can get this into the Thursday beta.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6600291b61b0
https://hg.mozilla.org/releases/mozilla-aurora/rev/ebe9766a0fb6

https://hg.mozilla.org/releases/mozilla-beta/rev/64791f0d76bf
https://hg.mozilla.org/releases/mozilla-beta/rev/a1d2983c6f12

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/64791f0d76bf
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a1d2983c6f12

Comment 43

[Daniel Moore]

Looks like this also fixes https://bugzilla.mozilla.org/show_bug.cgi?id=687092, unless I am mistaken.

Comment 44

[Bogdan Maris, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on Firefox 26 beta 7 (buildID: 20131122094025).

Comment 45

[u279076]

[qa-] since this has in-testsuite coverage and it's already been verified on release.