Closed Bug 791675 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: SVG, defect)

13 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox16 --- affected
firefox17 --- affected
firefox18 --- affected
firefox19 --- verified
firefox-esr17 --- wontfix

People

(Reporter: bugzilla, Assigned: longsonr)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 1 obsolete file)

Attached image 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.
Attached image File is rendered correctly (obsolete) —
Component: Untriaged → SVG
Product: Firefox → Core
Both files are rendered correctly with FF 12.0. Things are broken since FF 13.0
Attached image ok.svg
Attachment #661772 - Attachment is obsolete: true
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
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
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: 15 Branch → 13 Branch
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?
No, but I guess I can compile them again.
bug 614732 is the winner.

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

Thanks!
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.
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.
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.
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.
> 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.
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.
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?
Attachment #666313 - Flags: feedback?(jwatt)
Attachment #666313 - Attachment is patch: true
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.
Are there any updates? Will the bug be fixed in future releases?
+1

This bug is also related to bug 794098 which is "critical" for us.
Thanks
(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 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?
Attachment #666313 - Flags: feedback?(jwatt) → review+
Blocks: 794098
Assignee: jwatt → longsonr
https://hg.mozilla.org/mozilla-central/rev/022244d0ca81
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Attached patch patch as landedSplinter Review
[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
Attachment #694752 - Flags: approval-mozilla-beta?
Attachment #694752 - Flags: approval-mozilla-aurora?
Depends on: 823964
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 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.
Attachment #694752 - Flags: approval-mozilla-beta?
Attachment #694752 - Flags: approval-mozilla-aurora?
[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
Attachment #695178 - Flags: approval-mozilla-beta?
Attachment #695178 - Flags: approval-mozilla-aurora?
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.
Attachment #695178 - Flags: approval-mozilla-beta?
Attachment #695178 - Flags: approval-mozilla-beta-
Attachment #695178 - Flags: approval-mozilla-aurora?
Attachment #695178 - Flags: approval-mozilla-aurora+
Blocks: 790288
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0b4
Status: RESOLVED → VERIFIED
Depends on: 847153
longsonr: just for the record, was there a particular logic for the choice of 1e-6?
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.
You need to log in before you can comment on or make changes to this bug.