455138 - Bug 435293 landed failing tests

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/mozilla-central/rev/db31a92daecb
[
[e-mail]
Sat Sep 13 04:28:36 2008 -0700
db31a92daecb	Robert O'Callahan — Marking failing tests random
]

to workaround
[
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-macosx/build/mozilla/layout/reftests/transform/translatex-1b.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-macosx/build/mozilla/layout/reftests/transform/translatey-1b.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-macosx/build/mozilla/layout/reftests/transform/translate-1b.html | 
]

Comment 1

[Serge Gautherie (:sgautherie)]

I landed additional
http://hg.mozilla.org/mozilla-central/rev/c833b8069d4d

to workaround
[
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-linux/build/mozilla/layout/reftests/transform/percent-1d.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-linux/build/mozilla/layout/reftests/transform/percent-1e.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/comm-central-linux/build/mozilla/layout/reftests/transform/percent-1f.html | 
]

Comment 2

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/mozilla-central/rev/7819481fc8d1
[
[e-mail]
Sat Sep 13 06:53:04 2008 -0700
7819481fc8d1	Robert O'Callahan — Mark failing tests with bug#
]

Comment 3

[Keith Schwarz [:kschwarz]]

What Linux configuration are the tests running on?  I'd like to fix whatever's causing these bugs, but on my Linux system the tests are passing.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know, I can't reproduce those failures on my Linux box either.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The translate failures should be easier to track down since they happened on both Windows and Mac. Unfortunately my Mac/Windows box is down at the moment.

Comment 6

[Keith Schwarz [:kschwarz]]

All of these problems stem from rounding errors accumulating using floats, and unfortunately I don't think there's that much I can do to fix it.  I've found a few spots where I can curb error by constraining angular values to specific ranges internally, but overall I think that these sorts of errors are tricky to fix - some of the errors I've found are caused by discrepancies of less than 0.00001.  Long chains of transform computations are bound to lead to problems even when using doubles.  Any suggestions on what to do?  Should I scrap the reftests that are getting clobbered by rounding errors?

Comment 7

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #1

Fixes all of the failing reftests from the original patch by correcting rounding errors as much as possible.  Also patched the rotation tests, since the current version was rotating the elements out of the viewport and effectively rendering the test meaningless.  However, fixing the rotate tests ended up exposing another class of reftest failures stemming from subpixel (<0.00001 gfx units) rounding errors in the rotate tests, so I've marked them as random.  I'm not sure how to go about fixing those sorts of rounding errors, or if it's even possible to do so.  Suggestions appreciated.

Comment 8

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #1 (correction)

Whoops, left in some debugging printfs.  They've been removed in this patch.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 339626 [details] [diff] [review]
Potential Patch #1 (correction)

>+/* Helper function to constrain an angle in the range [0, 2pi) to a value
>+ * in the range [-pi, pi), which reduces accumulated floating point errors
>+ * from trigonometric functions by keeping the error terms small.
>+ */
>+static inline float ConstrainFloatValue(float aValue)
>+{
>+  return aValue >= kPi ? aValue - 2.0f * kPi : aValue;
>+}

Given that every caller of this calls fmod, maybe it would be good to have the fmod call actually be in this function?

>-    /* 360deg = 2pi rad, so deg = pi / 180 rad */
>+    /* 360deg = 2pi rad, so deg = pi / 180 rad. */

Seems nicer to have the deg and grad comments formatted consistently...

>+    /* Constrain to [0, 2*pi) */

This comment doesn't match the rest (or tho code).


r+sr=dbaron, although it would probably be good to have roc take a look as well.

Comment 10

[Keith Schwarz [:kschwarz]]

Attachment: Potential Patch #2 [Checkin: Comment 12]

Addresses r/sr comments from dbaron.  Only functional change is to push fmod into ConstrainFloatValue.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 339679 [details] [diff] [review]
Potential Patch #2
[Checkin: Comment 12]

Clever!

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 339679 [details] [diff] [review]
Potential Patch #2
[Checkin: Comment 12]

http://hg.mozilla.org/mozilla-central/rev/6b03a4f27f8f