942364 - Bottom edge of SVG cut off

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

Attachment: Screenshot Aurora27.0a2 and Nightly28.0a1

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/dbf94e314cde
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131122030202

When I tested Bug 942174, I found a plroblem  in Nightly280a1

Steps To Reproduce:
1. Open attachment 8336827 [details] of Bug 942174

Actual Results:
Bottom edge of SVG cut off

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/4c2e31c6d4ef
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118182715
Bad:
http://hg.mozilla.org/mozilla-central/rev/5236947f9090
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118183211
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c2e31c6d4ef&tochange=5236947f9090

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3d3a8f13aa66
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131117233137
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/99108bac6f2d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118003137
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3d3a8f13aa66&tochange=99108bac6f2d

Regressed by: Bug 923341

Comment 2

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

Taking this but I'm not starting work immediately. Matt, if it's clear to you what's going on here, feel free to jump in.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Seth is this in your queue for the next few weeks?  It's a new regression in FF28 so it would be great to get something landed to fix this before ever shipping it to users.

Comment 4

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

I'll make a note to take a look at it on Friday. I just finished a big chunk of feature work so now I have some time for fixing bugs and regressions.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Unfortunately we're out of time for FF28 cycle, let's see if this can get into Aurora 29 in the coming week or so.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Seth, did you find the time to work on this? We are in middle of the beta cycle for 29. Thanks

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Untracking for future releases, we can take a low risk uplift when ready but there's no major user complaints coming in so we're not going to apply a ton of pressure on you to get this done above other things.

Comment 8

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

I should probably post in this bug more often. I'm actually working on this now.

Comment 9

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

Note that we're probably not getting many user complaints about this because it's actually kinda hard to reproduce, at least from what I've seen so far. On both high-DPI and low-DPI machines, I only observe it when the image in the STR is zoomed out as far as possible. Definitely not worth tracking based on what we've seen so far.

Comment 10

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

Attachment: Self-contained, simplified test-case.

This is a simplified testcase that demonstrates the problem when you zoom out as far as possible.

I could've simplified it a bit further by removing shape-rendering="crispEdges", but while it's still reproducible that way it strains your eyes a bit too much for my taste.

Comment 12

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

I marked bug 1004067 as a duplicate of this bug. If anyone in the future is debugging a regression of this issue, though, it's worth noting that bug 1004067 also has a very good testcase.

Comment 13

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

Attachment: Add reftests

This patch adds reftests for this issue. They're marked as random, here, since
after all they don't actually pass right now. The reftest patch for bug 1043560
will remove the random annotation.

Comment 14

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

(To be clear, bug 1043560 resolves this issue.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8463632 [details] [diff] [review]
Add reftests

>diff --git a/image/test/reftest/downscaling/downscale-svg-1a-ref.html b/image/test/reftest/downscaling/downscale-svg-1a-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/downscaling/downscale-svg-1a-ref.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html>
>+<body style="margin: 0px">
>+  <img src="downscale-svg-1-1.0x.png" style="display: block">
>+</body>
>+</html>

Do we really need to use hardcoded PNG in the reference case here?  I don't think we do... I'll bet we could instead use <embed> with the same SVG content -- so, rendering the same SVG, but bypassing the imagelib layer entirely. (so that the reference case doesn't hit this bug, and we're still effectively testing this)

This seems to work for me, though I haven't tried it in the reftest harness:
  <embed src="black-border-rect.svg" style="display: block; width: 80px">

Unless I'm missing something, I'd rather we use this ^ instead of the hardcoded PNGs.  It should function as a better reference case than the PNGs, since it avoids hitting this bug itself due to avoiding the imagelib layer, and it should still be a pixel-perfect rendering (so hopefully minimal/no fuzziness annotations will be needed in the reftest.list file).

(I think you'd want to replace "width: 80px" with 40px, 24px, and 8px in the other reference cases, for zoom-factors 0.5, 0.3, and 0.1.)

>diff --git a/image/test/reftest/downscaling/downscale-svg-1a.html b/image/test/reftest/downscaling/downscale-svg-1a.html
>new file mode 100644
>--- /dev/null
>+++ b/image/test/reftest/downscaling/downscale-svg-1a.html
>@@ -0,0 +1,8 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html reftest-zoom="1.0">
>+<body style="margin: 0px">
>+  <div style="width: 80px; height: 80px; background: url(black-border-rect.svg) no-repeat">
>+</body>

This is missing a closing </div> (in each of the testcases in this patch, I think).

(The parser fills it in, but might as well include it, since you've got </body></html>.)

Also, these tests should be named "-1", "-2", "-3", "-4", instead of -1a.html, -1b.html, etc, since they don't share a reference case.

This (sort of) matters because we *mostly* honor the invariant that if you're viewing e.g. "downscale-svg-1c.html", you can find the reference case by adding "-ref" after the number (and stripping off any variant suffix like "a","b","c"). No need to look at the reference case -- the naming is predictable.  But the naming here violates that assumption (right now).  So, probably better to just be consistent with the established practice and give these tests different numbers.

(I know the tests are extremely related, which makes them feel like they should share a number.  But they do share the rest of their prefix, after all, which does indicate their relatedness.)

>+# RUN TESTS NOT AFFECTED BY HIGH QUALITY DOWNSCALING:
>+# =================================================
>+random == downscale-svg-1a.html downscale-svg-1a-ref.html
>+fuzzy(93,256) random == downscale-svg-1b.html downscale-svg-1b-ref.html
>+fuzzy(93,144) random == downscale-svg-1c.html downscale-svg-1c-ref.html
>+fuzzy(118,38) random == downscale-svg-1d.html downscale-svg-1d-ref.html
>+
> # RUN TESTS WITH HIGH QUALITY DOWNSCALING DISABLED:
> # =================================================

Nit: the "====" underlining needs two more "=" characters.

Also, "fuzzy(...) random" doesn't really make sense -- if we do need fuzzy annotations after these tests are made to be passing (and I'm hoping we won't, with the embed tweak that I mentioned above), I'd rather add it in the patch that marks the tests as passing over in bug 1043560.  (So, we go from a random (or failing) state to passing-and-fuzzy, instead of starting out random-and-fuzzy, which isn't asserting anything useful.)

Also, since <embed>/SVG rendering behavior is more predictable and platform-independent than PNG-rendering behavior, I'm hoping we can label these as either "fails" or already-passing up-front, instead of "random".  (IIUC, the randomness is due to platform-specific differences in the PNG rendering (?)).  This would function as a good sanity-check (on TBPL) that bug 1043560 *is* actually fixing things here.  Even if some of the tests have platform-specific differences and are easier to mark as 'random' for now, I'm hoping maybe one of the tests fails everywhere -- if so, it'd be great to have at least that one test start out labeled as "fails". Not a huge deal if you don't get to this part, or if it's too complicated, though.

Comment 16

[Daniel Holbert [:dholbert]]

> No need to look at the reference case -- the naming is predictable.

(Sorry, I meant to say "No need to look at the reftest.list file (to find out the name of the reference case)")

Comment 17

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

Thanks for the review!

I'll address these issues. Using <embed> didn't occur to me here; I'll give it a shot.

Comment 18

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

Attachment: Add reftests

All the review comments are addressed. <embed> was a big improvement!

I've marked all the tests as fail, but I need to push a try job to make sure that's actually true everywhere. I may have to go back and mark some of them random.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8464172 [details] [diff] [review]
Add reftests

Looks good - thanks!

Comment 20

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

Attachment: Add reftests

As discussed on IRC, I added some new tests so that we now cover all of the downscaling zoom levels you can get via the Firefox UI.

Comment 21

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

Pushed the tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9382edf1265

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9382edf1265

Comment 23

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

This was fixed by bug 1037713.

Comment 24

[Hossain Al Ikram [:ikram] (QA Contact)]

Reproduced on Nightly 2013-11-22 with STR from comment 0 and Windows 7 x64.

Verified as fixed with Firefox 34 beta 10 (Build ID: 20141117202603) on Windows 7 x64.

Comment 25

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thank you Ikram! Marking as verified based on your results.