791675 - SVG polyline does not render, when moving start by 1px

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Martin Egge]

Attachment: broken.svg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

I tried to view two SVG files in Firefox 15.0.1.


Actual results:

First file (ok.svg) is rendered as expected. Second file (broken.svg) does not show anything.


Expected results:

Both SVG should be rendered correctly.

Comment 1

[Martin Egge]

Attachment: File is rendered correctly

Comment 2

[Martin Egge]

Both files are rendered correctly with FF 12.0. Things are broken since FF 13.0

Comment 3

[Martin Egge]

Attachment: ok.svg

Comment 4

[Robert Longson [:longsonr]]

Martin, would you be willing to fix and exact regression day?

https://quality.mozilla.org/docs/bugzilla/guide-to-triaging-bugs-for-firefox/finding-a-regression-window/

type about:buildconfig in the address bar in the last good and subsequent first bad build and post the Built from link please.

Since it worked in 12 and fails in 13 you're probably looking for something between 2012-01-31 and 2012-03-13

Comment 5

[Martin Egge]

The regression day ist 2012-02-11.

Everything ist fine with http://hg.mozilla.org/mozilla-central/rev/fb81c9a433e4 and broken with http://hg.mozilla.org/mozilla-central/rev/d71dab82fff4

Comment 6

[Robert Longson [:longsonr]]

So something in here: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fb81c9a433e4&tochange=d71dab82fff4

which points to bug Bug 614732, Bug 725903 or Bug 725897

Comment 7

[Jonathan Watt [:jwatt]]

From bug 614732, that would be the "Kill covered regions" patch specifically (a439c947dc99). That also seems like the most likely one to cause a regression of this type.

Unfortunately this is the range that I couldn't get to build on my Mac. Robert, I don't suppose you still have those builds you created for the other bug in this range, do you?

Comment 8

[Robert Longson [:longsonr]]

No, but I guess I can compile them again.

Comment 9

[Robert Longson [:longsonr]]

bug 614732 is the winner.

a439c947dc99	broken
6d5192687c91	OK.

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #9)
> a439c947dc99	broken
> 6d5192687c91	OK.

Thanks!

Comment 11

[Jonathan Watt [:jwatt]]

So this seems to come down to the way we compute bounds for elements, and then skip painting them if the bounds don't overlap the dirty area.

In a439c947dc99, nsSVGPathGeometryFrame::UpdateCoveredRegion was changed to pass gfxMatrix() instead of GetCanvasTM() to the GetBBoxContribution() call. Passed this identity matrix, GetBBoxContribution() returns broken results for large coordinate values such as those found in the broken testcase because it gets broken results when it calls gfxContext::GetUserPathExtent (presumably because of cairo fixed point rounding).

Although UpdateCoveredRegion no longer exists in the current code, we still have the same problem in ReflowSVG today, since it uses GetBBoxContribution() to calculate mRect, and the broken mRect means we have a broken visual overflow rect, which means we don't overlap the dirty area, which means we don't paint.

Maybe Azure backed gfx will mean we'll get non-broken results back from gfxContext::GetUserPathExtent, but we'll still have reduced limits in comparison to other SVG implementations due to the fact that mRect uses integers, not floats.

Comment 12

[Robert Longson [:longsonr]]

Might be worth looking into going back to passing GetCanvasTM() to the GetBBoxContribution() and then multipling that by the inverse of the CanvasTM()

non-scaling-stroke may be a problem. Might have to see whether there's a stroke and whether it's non-scaling and not do the above if it is.

Comment 13

[Jonathan Watt [:jwatt]]

That's a possibility I guess, although it would mean we would be using bounds-of-bounds, resulting in overinvalidation and extra overpainting. (It would be good to have a way to tell if we overflowed so we could avoid that if possible.) There's also the limits imposed by mRect, so it may not actually buy us much.

Comment 14

[Robert Longson [:longsonr]]

Not sure I follow the first part. We'd still store the same mRect in most cases it would just be calculated as invTM * TM * bbox rather than bbox.

Not sure why cairo is throwing a wobbly as the polyline values are less than 2^24.

Comment 15

[Robert Longson [:longsonr]]

> Not sure I follow the first part. We'd still store the same mRect in most
> cases it would just be calculated as invTM * TM * bbox rather than bbox.

I see now. We'd get the bbox of the rotated shape and then when we rotate that we're not going to get the original shape back.

Comment 16

[Jonathan Watt [:jwatt]]

Sorry, missed comment 14. Yeah, so we invalidate bounds-of-bounds, and then when we come to paint we'd compound that issue by comparing the resulting over large dirty rect with visual overflow rects that are also over large due to being bounds-of-bounds.

If we had a way to tell when cairo gives us back garbage then we could special case for such elements, and we'd only get painting slowdowns in these rare, large coord cases.

Comment 17

[Robert Longson [:longsonr]]

Attachment: this is what I was thinking

It's rotation that gives us a bounds of bounds problem. If we stick to scaling then we're OK.

So we could do something along these lines couldn't we?

Comment 18

[Robert Longson [:longsonr]]

Although for images perhaps the best bet is to avoid cairo altogether and calculate the bounds from the image x, y, width and height ourselves.

Comment 19

[Martin Egge]

Are there any updates? Will the bug be fixed in future releases?

Comment 20

[INGV Milano]

+1

This bug is also related to bug 794098 which is "critical" for us.
Thanks

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #13)
> it would mean we would be using bounds-of-bounds

That's not the case for the patch.

Comment 22

[Jonathan Watt [:jwatt]]

Comment on attachment 666313 [details] [diff] [review]
this is what I was thinking

r+ with the following addressed.

Prefix the nsSVGImageFrame.cpp change with this comment:

  // We'd like to just pass the identity matrix to GeneratePath, but if
  // this frame's user space size is _very_ large/small then the extents we
  // obtain below might have overflowed or otherwise be broken. This would
  // cause us to end up with a broken mRect and visual overflow rect and break
  // painting of this frame. This is particularly noticeable if the transforms
  // between us and our nsSVGOuterSVGFrame scale this frame to a reasonable
  // size. To avoid this we sadly have to do extra work to account for the
  // transforms between us and our nsSVGOuterSVGFrame, even though the
  // overwhelming number of SVGs will never have this problem.
  // XXX Will Azure eventually save us from having to do this?

and the nsSVGPathGeometryFrame.cpp change with:

  // We'd like to just pass the identity matrix to GetBBoxContribution, but if
  // this frame's user space size is _very_ large/small then the extents we
  // obtain below might have overflowed or otherwise be broken. This would
  // cause us to end up with a broken mRect and visual overflow rect and break
  // painting of this frame. This is particularly noticeable if the transforms
  // between us and our nsSVGOuterSVGFrame scale this frame to a reasonable
  // size. To avoid this we sadly have to do extra work to account for the
  // transforms between us and our nsSVGOuterSVGFrame, even though the
  // overwhelming number of SVGs will never have this problem.
  // XXX Will Azure eventually save us from having to do this?

Comment 23

[Robert Longson [:longsonr]]

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

Comment 24

[Robert Longson [:longsonr]]

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

Comment 25

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/022244d0ca81

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/022244d0ca81

Comment 27

[Robert Longson [:longsonr]]

Attachment: patch as landed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 614732
User impact if declined: some SVG drawings which use large co-ordinates which are then scaled down will not display.
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): Worst case is that some other SVG drawings may not display but this is pretty low as this is the way the code used to work prior to bug 614732.
String or UUID changes made by this patch: none

Comment 28

[Jonathan Kew [:jfkthame]]

This seems to have caused bug 823964 (which is about the reftest-analyzer view, but presumably whatever's going wrong there could in principle affect other pages as well).

Comment 29

[Robert Longson [:longsonr]]

Comment on attachment 694752 [details] [diff] [review]
patch as landed

Probably best not to land on beta/aurora till we figure out the reftest-analyser issue.

Comment 30

[Robert Longson [:longsonr]]

Attachment: rollup patch with bug 823964 included

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 614732
User impact if declined: some SVG drawings which use large co-ordinates which are then scaled down will not display.
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): Worst case is that some other SVG drawings may not display but this is pretty low as this is the way the code used to work prior to bug 614732.
String or UUID changes made by this patch: none

Comment 31

[bhavana bajaj [:bajaj]]

Comment on attachment 695178 [details] [diff] [review]
rollup patch with bug 823964 included

Considering we are in our final beta , approving only for aurora at this time.

Comment 32

[Robert Longson [:longsonr]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee3ce57fb2f9

Comment 33

[Paul Silaghi, QA [:pauly]]

Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0b4

Comment 34

[Jonathan Watt [:jwatt]]

longsonr: just for the record, was there a particular logic for the choice of 1e-6?

Comment 35

[Robert Longson [:longsonr]]

I picked it as it was the same http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxMatrix.h#281. Other implementations of FuzzyEquals/FuzzyEqual in the codebase use this number, I don't know why though I just cargo culted it.