1246772 - Mochitest triggers rounding issues, with nsStyleUtil::{FloatToColorComponent,ColorToFloatComponent}()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Trevor Saunders (:tbsaunde)]

loating point values don't always round trip exactly through nsScanner::ScanNumber to FloatToColorComponent() back through ColorComponentToFloat() on x86 when x87  floating point is used.

the first failing test is that "0.9" round trips.  When nsCSSScanner::ScanNumber() is done it assigns 0.899999976
 to aToken.mNumber.  then ParseColorOpacity() uses FloatToColorComponent() on that, since we rounded the long double for 0.9 to something slightly less than 0.9
0.899999976 * 255.0 + 0.5 < 230
so it returns 229 as the color component (with sse the precision of the multiply is less and you get 230).  

However in ColorComponentToFloat() we first calculate rounded to be 0.90000000000000002220446049250313081
but we don't round that to fit a 32 bit float before calculating what the FloatToColorComponent() for that value is, so this time it returns 230.  Then aAlpha != 230 so we return the 3 decimal version of 0.898.

Comment 1

[Trevor Saunders (:tbsaunde)]

I can "fix" this by making rounded in ColorComponentToFloat() volatile and so forcing rounding equivelent to what happened when parsing, but I'm not sure how to actually fix this.  Boris / dbaron thoughts?

Comment 2

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

Testing opacities of 0.9 (and 0.1, 0.3, 0.5, 0.7) is a bit dangerous since they're right on the threshold between two 8-bit values.  One possibility is that we shouldn't care -- although I suspect other browsers may do reliable things.  Do they?

Comment 3

[Boris Zbarsky [:bzbarsky]]

What is the actual web-observable testcase here?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> What is the actual web-observable testcase here?

err sorry forgot that part :(

the parse+serialize test  in layout/style/test/test_value_storage.html around line 180  fails for http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#668

Comment 5

[Daniel Holbert [:dholbert]]

Given comment 2, perhaps we should just update that line of property_database.js to use 0.8 instead of 0.9?  I added it, without realizing that 0.9 had any special trickiness w.r.t. representability.  We could just as easily use some other arbitrary value there.

I don't have an x86 machine handy, but I do see similar rounding (truncation?) error on my 64-bit machine from doing e.g.:
  document.documentElement.style.backgroundColor = "rgba(5, 10, 20, 0.912);"

If I then read back document.documentElement.style.backgroundColor, I see that it's got 0.914 as the opacity value, which is .02 larger than what I asked for. So, not a perfect representation.

Comment 6

[Daniel Holbert [:dholbert]]

tbsaunde, does the problem go away if we change that test to use 0.8 instead of 0.9?  (and perhaps on the next line, use 0.4 instead of 0.5)

Or are there more test failures on this hardware from the same issue?

Comment 7

[Trevor Saunders (:tbsaunde)]

> I don't have an x86 machine handy, but I do see similar rounding
> (truncation?) error on my 64-bit machine from doing e.g.:
>   document.documentElement.style.backgroundColor = "rgba(5, 10, 20, 0.912);"
> 
> If I then read back document.documentElement.style.backgroundColor, I see
> that it's got 0.914 as the opacity value, which is .02 larger than what I
> asked for. So, not a perfect representation.

I'm testing on a x86_64 machine, but with a 32 bit build of firefox.  I'd be ind of suprised if there was similar issues with a64 bit build, *shrug*.

(In reply to Daniel Holbert [:dholbert] from comment #6)
> tbsaunde, does the problem go away if we change that test to use 0.8 instead
> of 0.9?  (and perhaps on the next line, use 0.4 instead of 0.5)
> 
> Or are there more test failures on this hardware from the same issue?

there are a couple more test failures in test_value_storage.html from the same issue, leaving NI to post the full list tomorrow when I can more easily get at them.

Comment 8

[Trevor Saunders (:tbsaunde)]

> (In reply to Daniel Holbert [:dholbert] from comment #6)
> > tbsaunde, does the problem go away if we change that test to use 0.8 instead
> > of 0.9?  (and perhaps on the next line, use 0.4 instead of 0.5)
> > 
> > Or are there more test failures on this hardware from the same issue?
> 
> there are a couple more test failures in test_value_storage.html from the
> same issue, leaving NI to post the full list tomorrow when I can more easily
> get at them.

ok, so it looks like all of the test failures are due to that one 0.9, and changing it to 0.8 makes the tests pass.  Are we all ok with changing it to 0.8 and not really worrying aboutthis right now?

Comment 9

[Daniel Holbert [:dholbert]]

(That seems fine to me, FWIW, given comment 2.)

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: work around x87 floating point truncations by checking a different alpha round trips

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8717756 [details] [diff] [review]
work around x87 floating point truncations by checking a different alpha round trips

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

Somewhere in the commit message, please mention "in a test" or "in mochitest", so that it's clear that this isn't an actual functional change.

r=me with that.

Comment 12

[Daniel Holbert [:dholbert]]

[/me adjusts bug summary to be about the test failure, rather than about the gecko rounding error, so that we can meaningfully close the bug as FIXED when the patch lands.]

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a80f023c6b7a

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a80f023c6b7a