Closed
Bug 1167026
Opened 6 years ago
Closed 6 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: gfxnoted)
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•6 years ago

Whiteboard: gfxnoted
Assignee  
Comment 2•6 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•6 years ago


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 5•6 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 divby0. If it intentional rerequest review.
Attachment #8609637 
Flags: review?(bas) → review
Assignee  
Comment 6•6 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 divby0. If > it intentional rerequest 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•6 years ago

Attachment #8609637 
Flags: review → review?(bas)
Comment 7•6 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 divby0. If > > it intentional rerequest 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•6 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 divby0. If > > > it intentional rerequest 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 wheninRome 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•6 years ago

Assignee: nobody → lsalzman
Assignee  
Comment 9•6 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•6 years ago

Attachment #8614074 
Flags: review?(bas) → review+
Assignee  
Updated•6 years ago

Keywords: checkinneeded
Comment 10•6 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/1a41e3cf04b3
Keywords: checkinneeded
Comment 11•6 years ago


https://hg.mozilla.org/mozillacentral/rev/1a41e3cf04b3
Status: NEW → RESOLVED
Closed: 6 years ago
statusfirefox41:
 → fixed
Resolution:  → FIXED
Target Milestone:  → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•