984796 - SVG path getTotalLength returns large incorrect number

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jake Smith]

Attachment: path.svg

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140313141126

Steps to reproduce:

Run attached SVG, the path is a curved line.


Actual results:

getTotalLength returns a large incorrect number in version 28+.


Expected results:

In Firefox 27 reported length is 138.80595, in 28 it is reported as 379391.00000.

Firefox 27: 138.80595
Firefox 28: 379391.00000
Firefox nightly (2014-03-15): 379392.21875
Chromium 32: 138.84326
Chrome 33: 138.84326
IE 11: 138.79509

Comment 1

[Alice0775 White]

Regression window
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/595fd5dfbb5a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109063242
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/22c11c35d1a3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109080444
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=595fd5dfbb5a&tochange=22c11c35d1a3

Regressed by
22c11c35d1a3	Bas Schouten — Bug 941585: Better dealing with degenerate beziers. r=jrmuizel

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Fix some errors in path flattening code

This fixes two small issues in the path handling code, one of which was actually responsible for this bug (splitting the bezier curve at a negative value when t1 was smaller than 0), and the other which turns out to be a regression I introduced fixing an older bug. Which occurs due to inaccuracies in pow(x,1/3) when x is negative.

Comment 3

[Jonathan Watt [:jwatt]]

Hmm, there are three cube roots of a negative number, one of which is the negative of the cube root of the absolute value of the number. (For example, one of the cube roots of cbrt(-8) is -cbrt(8).) There are two other cube roots that are complex numbers though, which is presumably why pow() doesn't accept negative exponents.

So I'm not really sure what the affect of converting cbrt(-x) to -cbrt(x) and ignoring the complex roots is. Does the fact that we're getting into that situation mean that the algorithm that we're implementing results in degeneracies that we should be handling in a different way, or is using the one non-imaginary root exactly what we want? I'm really not sure. Can someone provide me with a copy of the paper that describes the algorithm?

Comment 4

[Viktor Kovacs]

There are also errors with getPointAtLength.

I have done some research and the result is here:
Firefox 27:
	path length: 95.00848388671875
	point at index 0: 180, 30
	point at index 1: 182.32949829101562, 39.207218170166016
	point at index 2: 185.4349822998047, 48.17625427246094
	point at index 3: 191.12010192871094, 55.4254150390625
	point at index 4: 197.1878662109375, 48.99620056152344
	point at index 5: 200.015869140625, 39.9317741394043
	point at index 6: 202.03709411621094, 30.65072250366211
	point at index 7: 202.22813415527344, 21.28634262084961
	point at index 8: 193.62380981445312, 18.317861557006836
	point at index 9: 190.57847595214844, 9.474796295166016
	point at index 10: 190, 0.000003157313130941475

Firefox 28:
	path length: 1189.25537109375
	point at index 0: 180, 30
	point at index 1: 177.79641723632812, 107.91682434082031
	point at index 2: 140.84165954589844, 220.95492553710938
	point at index 3: 103.88700103759766, 333.99267578125
	point at index 4: 66.93222045898438, 447.0308532714844
	point at index 5: 30.022491455078125, 559.9312133789062
	point at index 6: 66.97498321533203, 446.89227294921875
	point at index 7: 103.9274673461914, 333.85333251953125
	point at index 8: 140.8799591064453, 220.81442260742188
	point at index 9: 177.8329620361328, 107.77392578125
	point at index 10: 190, 0

This is the fiddle I created: http://jsfiddle.net/g3cKa/

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Jonathan Watt [:jwatt] from comment #3)
> Hmm, there are three cube roots of a negative number, one of which is the
> negative of the cube root of the absolute value of the number. (For example,
> one of the cube roots of cbrt(-8) is -cbrt(8).) There are two other cube
> roots that are complex numbers though, which is presumably why pow() doesn't
> accept negative exponents.
> 
> So I'm not really sure what the affect of converting cbrt(-x) to -cbrt(x)
> and ignoring the complex roots is. Does the fact that we're getting into
> that situation mean that the algorithm that we're implementing results in
> degeneracies that we should be handling in a different way, or is using the
> one non-imaginary root exactly what we want? I'm really not sure. Can
> someone provide me with a copy of the paper that describes the algorithm?

I developed this particular section myself. Suffice to say since the solutions to the equation here are not real numbers, but in the imaginary space, they are not solutions that concern us (i.e. they don't describe intervals of t which we care about since we never draw into the imaginary plane).

Comment 6

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e39d7d851a

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/91e63f57c896 - I'm guessing you didn't actually intend to push those printfs that ran the log size up over the maximum with thousands of lines of "INFO -  XXX - Bas - Wanting to draw client side!XXX - Bas - Starting to draw client side!XXX - Bas - Finished validation!XXX - Bas - Forwarded transaction!XXX - Bas - Finished drawing client side!XXX - Bas - Starting to draw host side!XXX - Bas - Finished compositing host side!XXX - Bas - Starting to draw host side!XXX - Bas - Finished compositing host side!" but if you did actually want to? Unwant to.

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Phil Ringnalda (:philor) from comment #7)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/91e63f57c896 - I'm
> guessing you didn't actually intend to push those printfs that ran the log
> size up over the maximum with thousands of lines of "INFO -  XXX - Bas -
> Wanting to draw client side!XXX - Bas - Starting to draw client side!XXX -
> Bas - Finished validation!XXX - Bas - Forwarded transaction!XXX - Bas -
> Finished drawing client side!XXX - Bas - Starting to draw host side!XXX -
> Bas - Finished compositing host side!XXX - Bas - Starting to draw host
> side!XXX - Bas - Finished compositing host side!" but if you did actually
> want to? Unwant to.

Erm, indeed I didn't :) I think I understand how this went wrong too, sorry about that.

Comment 9

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e6067c9514

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56e6067c9514

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8393000 [details] [diff] [review]
Fix some errors in path flattening code

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 941585
User impact if declined: SVG textPath and scrip involving path length broken
Testing completed (on m-c, etc.): been on m-c for days
Risk to taking this patch (and alternatives if risky): low - this Moz2D functionality is only used by SVG path code, currently
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a31b22a5e8a4
https://hg.mozilla.org/releases/mozilla-beta/rev/2548eb09ff87

Comment 13

[berenice]

I download the beta version 29, but still fails. Is added the patch in this beta version download?

Please revise

Comment 14

[mbostock+bugzilla]

This still seems quite broken in Firefox Beta (29.0) and Firefox Aurora (30.0a2). Here’s a simple test case using getPointAtLength:

http://bl.ocks.org/mbostock/1705868

In Firefox Beta, the point goes way too fast and flies off the line (probably because getTotalLength is way too big, as described above).

In Firefox Aurora, the total length looks fine but getPointAtLength behaves erratically / inaccurately and the point separates from the line.

Comment 15

[mbostock+bugzilla]

It does seem fixed in Nightly (31.0a1), however.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Yeh, they are not part of the beta4 [1]. It is going to be in the beta5 (available on Friday).

[1] http://sylvestre.ledru.info/blog/2014/04/02/changes-firefox-29-beta3-to-beta4

Comment 17

[Ada [:adalucinet]]

Reproduced on Nightly from 2014-02-03 with the URL from comment 14 and the attachment.
Verified as fixed with Firefox 29 beta 5 (Build ID: 20140403132807) on Windows 7 x64, Ubuntu 13.04 x64 and Mac OS X 10.9.2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0

Comment 18

[berenice]

Good morning, is fixed (the point not goes way too fast and flies off the line), but now there is a NEW PROBLEM. 

I this test case: http://bl.ocks.org/duopixel/3824661

When hover in the point is not fluid in  version Firefox 29 beta5. It make some steps caused by obtain getPointAtLength in a PATH of svg.  In firefox 27 is OK, and previews versions.

Please revise. 
Thanks.

Comment 19

[Alice0775 White]

(In reply to berenice from comment #18)
> Good morning, is fixed (the point not goes way too fast and flies off the
> line), but now there is a NEW PROBLEM. 
> 
> I this test case: http://bl.ocks.org/duopixel/3824661
> 
> When hover in the point is not fluid in  version Firefox 29 beta5. It make
> some steps caused by obtain getPointAtLength in a PATH of svg.  In firefox
> 27 is OK, and previews versions.
> 
> Please revise. 
> Thanks.

Please file a new bug.

Comment 20

[berenice]

Good morning, 
I open a new bug:

 https://bugzilla.mozilla.org/show_bug.cgi?id=992850

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed 30.0a2 (2014-04-14), 31.0a1 (2014-04-14), Win 7 x64.