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

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tbsaunde, Unassigned)

Tracking

({arch})

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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?
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)
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?
What is the actual web-observable testcase here?
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 4

3 years ago
(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
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.
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?
Flags: needinfo?(tbsaunde+mozbugs)
(Reporter)

Comment 7

3 years ago
> 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.
(Reporter)

Comment 8

3 years ago
> (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?
Flags: needinfo?(tbsaunde+mozbugs)
(That seems fine to me, FWIW, given comment 2.)
(Reporter)

Comment 10

3 years ago
Created attachment 8717756 [details] [diff] [review]
work around x87 floating point truncations by checking a different alpha round trips
Attachment #8717756 - Flags: review?(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.
Attachment #8717756 - Flags: review?(dholbert) → review+
[/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.]
Summary: rounding issues with nsStyleUtil::{FloatToColorComponent,ColorToFloatComponent}() → Mochitest triggers rounding issues, with nsStyleUtil::{FloatToColorComponent,ColorToFloatComponent}()

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a80f023c6b7a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.