Closed Bug 942364 Opened 6 years ago Closed 5 years ago

Bottom edge of SVG cut off

Categories

(Core :: ImageLib, defect)

28 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox27 --- unaffected
firefox28 + wontfix
firefox29 + wontfix
firefox30 - wontfix
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- verified

People

(Reporter: alice0775, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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
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
Blocks: 923341
Component: Layout → ImageLib
OS: Windows 7 → All
Assignee: nobody → seth
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.
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.
Flags: needinfo?(seth)
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.
Flags: needinfo?(seth)
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.
Seth, did you find the time to work on this? We are in middle of the beta cycle for 29. Thanks
Flags: needinfo?(seth)
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.
I should probably post in this bug more often. I'm actually working on this now.
Flags: needinfo?(seth)
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.
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.
Depends on: DrawAPIRefactor
Duplicate of this bug: 1004067
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.
Attached patch Add reftests (obsolete) — Splinter Review
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.
Attachment #8463632 - Flags: review?(dholbert)
(To be clear, bug 1043560 resolves this issue.)
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.
Attachment #8463632 - Flags: review?(dholbert) → review-
> 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)")
Thanks for the review!

I'll address these issues. Using <embed> didn't occur to me here; I'll give it a shot.
Attached patch Add reftests (obsolete) — Splinter Review
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.
Attachment #8464172 - Flags: review?(dholbert)
Attachment #8463632 - Attachment is obsolete: true
Attached patch Add reftestsSplinter Review
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.
Attachment #8464172 - Attachment is obsolete: true
This was fixed by bug 1037713.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
QA Whiteboard: [good first verify]
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.
Thank you Ikram! Marking as verified based on your results.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [good first verify][bugday-20141119]
You need to log in before you can comment on or make changes to this bug.