Closed
Bug 1167026
Opened 9 years ago
Closed 9 years ago
http://setosa.io/bus/ buses don't render
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: BenWa, Assigned: lsalzman)
References
Details
(Whiteboard: gfx-noted)
Attachments
(1 file, 1 obsolete file)
1.39 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
Looks to be SVG based. Not sure if it's a graphics problem, layout or just a general web compat problem.
This is fine in chrome, but also doesn't render in safari.
Updated•9 years ago
|
Whiteboard: gfx-noted
Assignee | ||
Comment 2•9 years ago
|
||
The JS code was calling getTotalLength on an SVGPathElement. Our code computes the length by flattening the path and then summing distances. Our flatten algorithm broke on certain segments where the control points happened to be equal, thus dividing by zero and inserting NaNs into the flattened path used for distance computations. This patch catches such cases and makes the buses show up again.
Attachment #8609637 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 3•9 years ago
|
||
Looks like a good candidate for a reftest perhaps?
Comment 4•9 years ago
|
||
Comment on attachment 8609637 [details] [diff] [review] Avoid division by zero when flattening a bezier curve segment with equal control points I'll let Bas take this.
Attachment #8609637 -
Flags: review?(jmuizelaar) → review?(bas)
Comment 5•9 years ago
|
||
Comment on attachment 8609637 [details] [diff] [review] Avoid division by zero when flattening a bezier curve segment with equal control points Review of attachment 8609637 [details] [diff] [review]: ----------------------------------------------------------------- This is not preventing a division by zero but a division of zero.. was that intentional? Shouldn't you be checking hypotf == 0 to avoid a div-by-0. If it intentional re-request review.
Attachment #8609637 -
Flags: review?(bas) → review-
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Bas Schouten (:bas.schouten) from comment #5) > Comment on attachment 8609637 [details] [diff] [review] > Avoid division by zero when flattening a bezier curve segment with equal > control points > > Review of attachment 8609637 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is not preventing a division by zero but a division of zero.. was that > intentional? Shouldn't you be checking hypotf == 0 to avoid a div-by-0. If > it intentional re-request review. Note that it prevents both. Consider the original code: Float s3 = (cp31.x * cp21.y - cp31.y * cp21.x) / hypotf(cp21.x, cp21.y); t = 2 * Float(sqrt(aTolerance / (3. * std::abs(s3)))); So, if s3 is 0, then you will get a div0 calculating t. If cp21.x and cp21.y are 0, then the numerator of s3 will also be 0, which means the hypotf here would end up 0 too. Thus, the sly way to rule all of these out with one comparison is to check if the numerator of s3 is 0.
Assignee | ||
Updated•9 years ago
|
Attachment #8609637 -
Flags: review- → review?(bas)
Comment 7•9 years ago
|
||
(In reply to Lee Salzman [:eihrul] from comment #6) > (In reply to Bas Schouten (:bas.schouten) from comment #5) > > Comment on attachment 8609637 [details] [diff] [review] > > Avoid division by zero when flattening a bezier curve segment with equal > > control points > > > > Review of attachment 8609637 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This is not preventing a division by zero but a division of zero.. was that > > intentional? Shouldn't you be checking hypotf == 0 to avoid a div-by-0. If > > it intentional re-request review. > > Note that it prevents both. Consider the original code: > > Float s3 = (cp31.x * cp21.y - cp31.y * cp21.x) / hypotf(cp21.x, cp21.y); > t = 2 * Float(sqrt(aTolerance / (3. * std::abs(s3)))); > > So, if s3 is 0, then you will get a div0 calculating t. If cp21.x and cp21.y > are 0, then the numerator of s3 will also be 0, which means the hypotf here > would end up 0 too. Thus, the sly way to rule all of these out with one > comparison is to check if the numerator of s3 is 0. I don't think this assumption holds in the world of limited precision floating point.
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Bas Schouten (:bas.schouten) from comment #7) > (In reply to Lee Salzman [:eihrul] from comment #6) > > (In reply to Bas Schouten (:bas.schouten) from comment #5) > > > Comment on attachment 8609637 [details] [diff] [review] > > > Avoid division by zero when flattening a bezier curve segment with equal > > > control points > > > > > > Review of attachment 8609637 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > This is not preventing a division by zero but a division of zero.. was that > > > intentional? Shouldn't you be checking hypotf == 0 to avoid a div-by-0. If > > > it intentional re-request review. > > > > Note that it prevents both. Consider the original code: > > > > Float s3 = (cp31.x * cp21.y - cp31.y * cp21.x) / hypotf(cp21.x, cp21.y); > > t = 2 * Float(sqrt(aTolerance / (3. * std::abs(s3)))); > > > > So, if s3 is 0, then you will get a div0 calculating t. If cp21.x and cp21.y > > are 0, then the numerator of s3 will also be 0, which means the hypotf here > > would end up 0 too. Thus, the sly way to rule all of these out with one > > comparison is to check if the numerator of s3 is 0. > > I don't think this assumption holds in the world of limited precision > floating point. For this code, dealing with equal values, it holds. We are dealing with 0s here, not epsilons. Now, I followed the when-in-Rome principle on this code (both on syntax and semantics), which already used comparison to 0 here to catch such cases, so that I am merely rooting out the case here of equal control points, that will produce 0, on which math is going to be predictable to a degree. So, if this is check here is inappropriate, then several checks elsewhere in the code are also inappropriate and should be using epsilons. If you want me to fix the rest of the cases to utilize epsilons along with this case, I could.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lsalzman
Assignee | ||
Comment 9•9 years ago
|
||
Various cleanups as discussed with Bas. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0b20e1bb75
Attachment #8609637 -
Attachment is obsolete: true
Attachment #8609637 -
Flags: review?(bas)
Attachment #8614074 -
Flags: review?(bas)
Updated•9 years ago
|
Attachment #8614074 -
Flags: review?(bas) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a41e3cf04b3
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a41e3cf04b3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•