Last Comment Bug 702412 - [layers] float comparisons should use an epsilon
: [layers] float comparisons should use an epsilon
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 normal (vote)
: ---
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: native_droid_panning
  Show dependency treegraph
 
Reported: 2011-11-14 13:20 PST by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-01-09 11:00 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Add epsilon to float compares (2.27 KB, patch)
2011-11-16 12:20 PST, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Add epsilon to float compares (2.28 KB, patch)
2011-11-16 12:21 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review
java code that demonstrates rounding issues (423 bytes, text/plain)
2011-11-17 06:21 PST, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details

Description Kartikaya Gupta (email:kats@mozilla.com) 2011-11-14 13:20:32 PST
Floating point comparisons in java should generally use an epsilon rather than being compared to each other directly. Doing (foo == 0.0f) may fail when even though (Math.abs(foo) < 1e-7) passes, due to floating point rounding issues.

Code that uses == for float comparisons exists in PanZoomPosition.java and a few other classes in the layers code.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-16 12:20:13 PST
Created attachment 574962 [details] [diff] [review]
Add epsilon to float compares

Turns out the other places this happened were in PanZoomPosition, which existed only briefly in the patch queue.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-16 12:21:31 PST
Created attachment 574963 [details] [diff] [review]
Add epsilon to float compares

Oops, wrong version of patch
Comment 3 Chris Lord [:cwiiis] 2011-11-17 06:07:46 PST
Comment on attachment 574963 [details] [diff] [review]
Add epsilon to float compares

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

getExcess() actually explicitly returns 0.0f, so I'd have thought the comparison would be ok (I assume that 0.0f == 0.0f holds - but perhaps it doesn't after assignment?). Does this fix the crash you were seeing?
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-17 06:20:30 PST
getExcess() explicitly returns 0.0f, but it could also return things like viewportPos or getViewportEnd() - mPageLength, and those values might be really really close to zero without actually being zero.

I've attached a test case that shows how rounding error can result in values just slightly off because of floating point representation.

I thought that this might have caused the crash in bug 700559 by sending the state into WAITING_TO_SNAP instead of STOPPED, but after looking through the logic again I don't think that's possible.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-17 06:21:11 PST
Created attachment 575161 [details]
java code that demonstrates rounding issues
Comment 6 Patrick Walton (:pcwalton) 2011-11-17 10:21:09 PST
Comment on attachment 574963 [details] [diff] [review]
Add epsilon to float compares

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

Fine by me, with the method name renamed.

::: embedding/android/ui/PanZoomController.java
@@ +333,5 @@
>      private float computeElasticity(float excess, float viewportLength) {
>          return 1.0f - excess / (viewportLength * SNAP_LIMIT);
>      }
>  
> +    private static boolean floatsEqual(float a, float b) {

I'd call this floatsNearlyEqual() or something -- floatsEqual() isn't accurate.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-17 11:44:44 PST
http://hg.mozilla.org/projects/birch/rev/e056708c00aa

Note You need to log in before you can comment on or make changes to this bug.