If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 19

Status

()

Core
SVG
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Martin Egge, Assigned: Robert Longson)

Tracking

({regression, testcase})

13 Branch
mozilla20
x86_64
Windows 7
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 affected, firefox17 affected, firefox18 affected, firefox19 verified, firefox-esr17 wontfix)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 661771 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 661772 [details]
File is rendered correctly
(Assignee)

Updated

5 years ago
Component: Untriaged → SVG
Product: Firefox → Core
(Reporter)

Comment 2

5 years ago
Both files are rendered correctly with FF 12.0. Things are broken since FF 13.0
(Reporter)

Comment 3

5 years ago
Created attachment 661778 [details]
ok.svg
Attachment #661772 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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
(Reporter)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
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

Updated

5 years ago
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?
(Assignee)

Comment 8

5 years ago
No, but I guess I can compile them again.
Keywords: testcase
(Assignee)

Comment 9

5 years ago
bug 614732 is the winner.

a439c947dc99	broken
6d5192687c91	OK.
Blocks: 614732
Assignee: nobody → jwatt
(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.
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
> 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.
(Assignee)

Comment 17

5 years ago
Created attachment 666313 [details] [diff] [review]
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?
Attachment #666313 - Flags: feedback?(jwatt)
(Assignee)

Updated

5 years ago
Attachment #666313 - Attachment is patch: true
(Assignee)

Comment 18

5 years ago
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.
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox-esr17: --- → affected
(Reporter)

Comment 19

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

Comment 20

5 years ago
+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)

Comment 23

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=3af14d5aa409
(Assignee)

Comment 24

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=80ab807f9a09
(Assignee)

Updated

5 years ago
Assignee: jwatt → longsonr
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/022244d0ca81
https://hg.mozilla.org/mozilla-central/rev/022244d0ca81
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 27

5 years ago
Created attachment 694752 [details] [diff] [review]
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
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).
(Assignee)

Comment 29

5 years ago
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?
(Assignee)

Comment 30

5 years ago
Created attachment 695178 [details] [diff] [review]
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
Attachment #695178 - Flags: approval-mozilla-beta?
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 32

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee3ce57fb2f9
status-firefox19: affected → fixed

Updated

5 years ago
Blocks: 790288
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0b4
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified

Updated

5 years ago
status-firefox-esr17: affected → wontfix

Updated

5 years ago
Depends on: 847153
longsonr: just for the record, was there a particular logic for the choice of 1e-6?
(Assignee)

Comment 35

4 years ago
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.