Closed Bug 1167026 Opened 5 years ago Closed 5 years ago

http://setosa.io/bus/ buses don't render

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: BenWa, Assigned: lsalzman)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 1 obsolete file)

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.
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)
Looks like a good candidate for a reftest perhaps?
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 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-
(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.
Attachment #8609637 - Flags: review- → review?(bas)
(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.
(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: nobody → lsalzman
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)
Attachment #8614074 - Flags: review?(bas) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1a41e3cf04b3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.