Closed Bug 941585 Opened 11 years ago Closed 10 years ago

Converting SVG textPath to Moz2D has broken some SVGs

Categories

(Core :: SVG, defect, P2)

28 Branch
x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 + fixed
firefox29 --- fixed

People

(Reporter: gwidion, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(Keywords: regression, reproducible, testcase)

Attachments

(5 files, 3 obsolete files)

Attached image texto_circulo_2.svg
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.32 (KHTML, like Gecko) Chrome/27.0.1422.0 Safari/537.32

Steps to reproduce:

Created an SVG document trying to apply a text along a path with """ <textPath xlink:href="..." >  """  


Actual results:

 it does not render properly - text is truncated and broken in more than one line.


Expected results:

The text shou8ld have rendered in a single line along the path referred.
Screenshot of the problem in firefox - nightly  28.0a1 (2013-11-19)
This is the same SVG file as it appears rendered in Inkscape and in <other> browser. (GIMP was used to add a white background to Inskcape's export, otherwise with a transparent bg)
It works in 25 so it's not the css text frames switchover.

João, would you be willing to help find a regression range? There's some instructions here: http://mozilla.github.io/mozregression/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/d58ab6f6ca0a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131117145336
Bad:
http://hg.mozilla.org/mozilla-central/rev/59f6274ce8f1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131118025341
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d58ab6f6ca0a&tochange=59f6274ce8f1

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/92f737230338
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131117170238
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/af0931327e49
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131117173035
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=92f737230338&tochange=af0931327e49

Suspected: Bug 930577
Blocks: 930577
OS: Linux → All
Summary: SVG text along path with "xlink" not working properly → Converting SVG textPath to Moz2D has broken some SVGs
Attached image reduced testcase
Bas, it seems like ComputePointAtLength is completely broken in some cases. In this testcase there are two almost identical paths. If I dump out the result from the ComputePointAtLength() call in nsSVGTextFrame2::DoTextPathLayout it gives me sensible results for the first, but always returns {0,200} for the second.
Priority: -- → P2
(In reply to Jonathan Watt [:jwatt] from comment #5)
> Created attachment 8336093 [details]
> reduced testcase
> 
> Bas, it seems like ComputePointAtLength is completely broken in some cases.
> In this testcase there are two almost identical paths. If I dump out the
> result from the ComputePointAtLength() call in
> nsSVGTextFrame2::DoTextPathLayout it gives me sensible results for the
> first, but always returns {0,200} for the second.

Okay, this an interesting degeneracy, you're actually giving a quadratic curve here rather than a cubic. I'll add some logic to catch this case.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Attachment #8338231 - Flags: review?(jwatt)
Comment on attachment 8338231 [details] [diff] [review]
Deal with degenerate cubic beziers

> FlattenedPath::BezierTo(const Point &aCP1,
>                         const Point &aCP2,
>                         const Point &aCP3)
> {
>   MOZ_ASSERT(!mCalculatedLength);
>-  FlattenBezier(BezierControlPoints(CurrentPoint(), aCP1, aCP2, aCP3), this, kFlatteningTolerance);
>+
>+  // Sanitize the Bezier curve.
>+  Point cp1 = aCP1;
>+  Point cp2 = aCP2;
>+  if (aCP1 == aCP2 ||
>+      aCP2 == aCP3) {
>+    cp1 = CurrentPoint() + (aCP2 - CurrentPoint()) * Float(2. / 3.);
>+    cp2 = aCP3 + (aCP2 - aCP3) * Float(2. / 3.);
>+  }
>+  FlattenBezier(BezierControlPoints(CurrentPoint(), cp1, cp2, aCP3), this, kFlatteningTolerance);
> }

As discussed on IRC, this seems wrong to me. Let's say the current point is {0,0}, aCP1 and aCP2 are both coincidental at {5,10}, and aCP3 is {10,0}. This code would move cp1 1/3 of the way towards the current point, and cp2 1/3 of the way to aCP3. But that creates a different (flatter/less pronounced) curve to the one passed in.

Even (especially) if this code is correct, it needs commenting. Cancelling review for now while you dig into this.
Attachment #8338231 - Flags: review?(jwatt)
Flags: needinfo?(bas)
We're not going to track this as of right now since we don't know impacted websites. I haven't run across this usage in my own browsing.
Attachment #8338231 - Attachment is obsolete: true
Attachment #8349620 - Flags: review?(jmuizelaar)
Flags: needinfo?(bas)
Attachment #8349620 - Flags: review?(jmuizelaar) → review+
I get a stack overflow with this patch and the test case from bug 950549.

Call stack is:

gkmedias.dll!mozilla::gfx::BasePoint<float,mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> >::operator-(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aPoint) Line 43	C++
 	gkmedias.dll!mozilla::gfx::SplitBezier(const mozilla::gfx::BezierControlPoints & aControlPoints, mozilla::gfx::BezierControlPoints * aFirstSegmentControlPoints, mozilla::gfx::BezierControlPoints * aSecondSegmentControlPoints, float t) Line 99	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 143	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
 	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
...
(In reply to Brian Birtles (:birtles, away 28 Dec~5 Jan) from comment #11)
> I get a stack overflow with this patch and the test case from bug 950549.
> 
> Call stack is:
> 
> gkmedias.dll!mozilla::gfx::BasePoint<float,mozilla::gfx::PointTyped<mozilla::
> gfx::UnknownUnits> >::operator-(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aPoint) Line 43	C++
>  	gkmedias.dll!mozilla::gfx::SplitBezier(const
> mozilla::gfx::BezierControlPoints & aControlPoints,
> mozilla::gfx::BezierControlPoints * aFirstSegmentControlPoints,
> mozilla::gfx::BezierControlPoints * aSecondSegmentControlPoints, float t)
> Line 99	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 143	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
>  	gkmedias.dll!mozilla::gfx::FlattenedPath::BezierTo(const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP1, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP2, const
> mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> & aCP3) Line 147	C++
> ...

Sadly I can't reproduce this, this patch fixes the testcase for bug 950549 for me. Can someone else test as well perhaps?
(In reply to Bas Schouten (:bas.schouten) from comment #12)
> Sadly I can't reproduce this, this patch fixes the testcase for bug 950549
> for me. Can someone else test as well perhaps?

Applying this patch again to today's nightly doesn't cause stack overflow for me. I've no idea what triggered it before.

It would be good if someone else on Win 7 could try this patch and load https://bug950549.bugzilla.mozilla.org/attachment.cgi?id=8347867
Tried on Windows 7. No stack overflow. Displays 100.50825 in big letters with a line behind it which seems correct AFAICT.
However the first testcase in bug 593601 does now cause a crash for me on Windows 7. I've not tried the other testcases in that bug but it's probably worth trying them all.
(In reply to Robert Longson from comment #16)
> However the first testcase in bug 593601 does now cause a crash for me on
> Windows 7. I've not tried the other testcases in that bug but it's probably
> worth trying them all.

Hrm, I've determined the cause of this problem, I'm not sure yet what the solution will be though.
This patch is a much improved solution to the problem. It adds a lot of robustness to the algorithm against these sort of degeneracies and deals with them directly rather than by trying to subdivide. It also solves some correctness issues it had currently.
Attachment #8349620 - Attachment is obsolete: true
Attachment #8356605 - Flags: review?(jmuizelaar)
(In reply to Bas Schouten (:bas.schouten) from comment #18)
> Created attachment 8356605 [details] [diff] [review]
> bug9505Deal with degenerate cubic beziers v3
> 
> This patch is a much improved solution to the problem. It adds a lot of
> robustness to the algorithm against these sort of degeneracies and deals
> with them directly rather than by trying to subdivide. It also solves some
> correctness issues it had currently.

Can you explain how it does this?
Blocks: 950549
(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> (In reply to Bas Schouten (:bas.schouten) from comment #18)
> > Created attachment 8356605 [details] [diff] [review]
> > bug9505Deal with degenerate cubic beziers v3
> > 
> > This patch is a much improved solution to the problem. It adds a lot of
> > robustness to the algorithm against these sort of degeneracies and deals
> > with them directly rather than by trying to subdivide. It also solves some
> > correctness issues it had currently.
> 
> Can you explain how it does this?

I've tried to add comments where it wasn't obvious, if I need to add some more let me know, a couple of things:

- Deal properly with the discriminant being 0, rather than just finding two inflection points at the same location, find one.
- Make inflection point approximations deal with s3 being 0. I.e. properly solve the equation for the s3 == 0 situation and use that.
- Remove incorrect curve adjustment for coinciding control points in the case of splitting the curve.
- Do not attempt to process inflection points -at- 1.0 (since all control points for the future of the curve will be coincident here).
- Do not require t1max to be larger than 0 when a single inflection point is found, but rather always flatten everything from t1max onward. This is mostly a readability improvement.
This uses the proper solution for the aforementioned limit.
Attachment #8356605 - Attachment is obsolete: true
Attachment #8356605 - Flags: review?(jmuizelaar)
Attachment #8357127 - Flags: review?(jmuizelaar)
Comment on attachment 8357127 [details] [diff] [review]
Deal with degenerate cubic beziers v4

Review of attachment 8357127 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Path.cpp
@@ +266,5 @@
>  
>      Point cp21 = aControlPoints.mCP2 - aControlPoints.mCP1;
>      Point cp41 = aControlPoints.mCP4 - aControlPoints.mCP1;
>  
> +    if (!cp21.x && !cp21.y) {

For float code I prefer cp21.x == 0.0 to !cp21.x because it makes it more clear that you're doing an actual numerical comparison. Also I'm not convinced that this condition is sufficient for ensuring s3 does not become 0. It would probably be good to check for IsFinite(aMin/aMax) afterwards to make sure nothing blew up if we're going to depend on those values for control flow.
Attachment #8357127 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/22c11c35d1a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Alex Keybl [:akeybl] from comment #9)
> We're not going to track this as of right now since we don't know impacted
> websites. I haven't run across this usage in my own browsing.

I think we need to track this. In my skimming of my huge post-Christmas bugmail backlog I've seen a good number of bugs that are probably fixed by this.
Wes, how come you landed this before Bas addressed Jeff's review comments?
Flags: needinfo?(kwierso)
Comment on attachment 8357127 [details] [diff] [review]
Deal with degenerate cubic beziers v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930577
User impact if declined: a fair bit of SVG breakage (textPath and motionPath)
Testing completed (on m-c, etc.): now on m-c
Risk to taking this patch (and alternatives if risky): looks low
String or IDL/UUID changes made by this patch: none
Attachment #8357127 - Flags: approval-mozilla-aurora?
(In reply to Jonathan Watt [:jwatt] from comment #25)
> Wes, how come you landed this before Bas addressed Jeff's review comments?

Bas originally landed this on mozilla-inbound yesterday in https://hg.mozilla.org/integration/mozilla-inbound/rev/22c11c35d1a3 at 8:04 AM PST. My post here with the mozilla-central link resolving the bug is an automated thing from the mc-merge tool when I merge from inbound to central.
Flags: needinfo?(kwierso)
(In reply to Wes Kocher (:KWierso) from comment #27)
> (In reply to Jonathan Watt [:jwatt] from comment #25)
> > Wes, how come you landed this before Bas addressed Jeff's review comments?
> 
> Bas originally landed this on mozilla-inbound yesterday in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/22c11c35d1a3 at 8:04
> AM PST. My post here with the mozilla-central link resolving the bug is an
> automated thing from the mc-merge tool when I merge from inbound to central.

Yes, as for how it got landed, that was my mistake (it was related to the r+ being in a separate transaction from the review comments, which made me think from my review bugmail filters that I'd gotten an r+ without comments). I've already talked about this with Jeff and I'l look into addressing it in a follow-up. Fwiw I think we agree this patch still strictly makes things better and I'm not sure yet whether the latter part of Jeff's comment is a real concern or not. I think it -might- be but I need to think of when this would happen and what the right solution would be, for the record it would be -extremely- unlikely to occur.
Attachment #8357127 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this have in-testsuite coverage?
Flags: needinfo?(bas)
Flags: in-testsuite?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #30)
> Does this have in-testsuite coverage?

No, I thought JWatt had a test for this on one of the other dupe bugs, but I might've been mistaking.
Flags: needinfo?(bas) → needinfo?(jwatt)
I don't think there was one for textPath, although the "reduced testcase" that I attached ( https://bug941585.bugzilla.mozilla.org/attachment.cgi?id=8336093 ) could probably be turned into a reftest. Any test will likely require some fuzzing though due to the fact that general path calculations necessarily involve approximation.
Flags: needinfo?(jwatt)
Depends on: 984796
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: