Bug 56829 – REGRESSION (r81625): fast/css/percentage-non-integer.html fails on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Roben (:aroben)]

fast/css/percentage-non-integer.html has been failing on Windows since r81625. See the URL for the failure diff.

It looks like the rounding on Windows is worse than on Mac. On Mac, the table cells increase in width by 2px each time. On Windows, they sometimes increase by other amounts.

[Adam Roben (:aroben)]

Rik, could you please look into this?

[Adam Roben (:aroben)]

Added expected failure results in r81666.

[Adam Roben (:aroben)]

<rdar://problem/9168471>

[Rik Cabanier]

(In reply to comment #1)
> Rik, could you please look into this?

I will sync the code down on windows and take a look.

[Rik Cabanier]

(In reply to comment #0)
> fast/css/percentage-non-integer.html has been failing on Windows since r81625. See the URL for the failure diff.
> 
> It looks like the rounding on Windows is worse than on Mac. On Mac, the table cells increase in width by 2px each time. On Windows, they sometimes increase by other amounts.

This probably has to do with how floats and doubles interact with different compilers.

[Adam Roben (:aroben)]

Is there some way we can coerce MSVC into giving us better rounding behavior?

[Rik Cabanier]

(In reply to comment #6)
> Is there some way we can coerce MSVC into giving us better rounding behavior?

This is a 32bit intel rounding issue.
The FPU will always convert a 32 bit float of 80bit. This means that a value of 1.4 becomes 1.3999999997.
In this case the logic becomes:
1000*1.4f/100.0f -> 10000.0*1.3999997/100.0 -> 13.999997 -> (cast to int) 13

64 bit or MMX doesn't suffer this problem.
Setting/resetting the fpu precision should work around this issue but would result in slow code.
Adding a fudge factor would fix this issue as well as enforcing MMX...

[Adam Roben (:aroben)]

Why are we truncating in the last step instead of rounding?

[Rik Cabanier]

(In reply to comment #8)
> Why are we truncating in the last step instead of rounding?

Because that is what the old class did as well.

[Rik Cabanier]

(In reply to comment #1)
> Rik, could you please look into this?

I have a fix for the issue and will submit a review shortly.

[Rik Cabanier]

Created attachment 86532 [details]
Fix rounding issue

[Rik Cabanier]

Created attachment 86537 [details]
Fixed style

[Rik Cabanier]

Created attachment 86539 [details]
Fixed style

[Adam Roben (:aroben)]

Comment on attachment 86539 [details]
Fixed style

View in context: https://bugs.webkit.org/attachment.cgi?id=86539&action=review

> Source/WebCore/platform/Length.h:143
> +                    // do not try to optimize this. It will cause incorrect rounding on 32-bit intel machines that use the FPU
> +                    float retVal = maxValue * percent() / 100.0f;
> +                    return static_cast<int>(retVal);

I don't think this comment is clear enough. "optimize" could have many different interpretations. Explicitly saying "Don't remove the local float variable" would be more helpful.

The first word of the comment should be capitalized, as should "Intel".

It is very surprising that a float local fixes the problem. I assume appending "f" to 100.0 wasn't enough?

[Rik Cabanier]

(In reply to comment #14)
> (From update of attachment 86539 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86539&action=review
> 
> > Source/WebCore/platform/Length.h:143
> > +                    // do not try to optimize this. It will cause incorrect rounding on 32-bit intel machines that use the FPU
> > +                    float retVal = maxValue * percent() / 100.0f;
> > +                    return static_cast<int>(retVal);
> 
> I don't think this comment is clear enough. "optimize" could have many different interpretations. Explicitly saying "Don't remove the local float variable" would be more helpful.
> 
> The first word of the comment should be capitalized, as should "Intel".
> 
> It is very surprising that a float local fixes the problem. I assume appending "f" to 100.0 wasn't enough?

Adding 'f' to the number didn't make a difference on Intel 32 bit since all calculations are done with 85bit precision.
Because the floats don't have that much precision and the cast happened on the 85bit register, we got imprecise results. Going through a float causes correct rounding.

On other platform that don't have this problem, the 'f' behind the number is good practise so the processor does all calculations in float precision.

I will fix the comment and update the patch.

[Rik Cabanier]

Created attachment 86637 [details]
updated comment

[Adam Roben (:aroben)]

I'm hoping one of our layout folks can review this.

Rik, don't we need to do something about the expected failure results I checked in in r81666?

[Rik Cabanier]

(In reply to comment #17)
> I'm hoping one of our layout folks can review this.
> 
> Rik, don't we need to do something about the expected failure results I checked in in r81666?

Not sure what r81666 is. Is it different from what is checked in?
I know that the pixel test would need to be updated for some platforms.

[Adam Roben (:aroben)]

As explained in comment 1, r81625 made this test fail on Windows. In r81666 I checked in new results for Windows that matched the current (incorrect) behavior of Windows on that test.

Since you're now giving Windows the correct behavior, presumably those results I added in r81666 should be removed, which will cause Windows to fall back to the Mac results. (You'll need to test that Windows actually matches the Mac results.)

[Rik Cabanier]

(In reply to comment #19)
> As explained in comment 1, r81625 made this test fail on Windows. In r81666 I checked in new results for Windows that matched the current (incorrect) behavior of Windows on that test.
> 
> Since you're now giving Windows the correct behavior, presumably those results I added in r81666 should be removed, which will cause Windows to fall back to the Mac results. (You'll need to test that Windows actually matches the Mac results.)

yes, that code should be removed.
The test has to render the same on all platforms.

Do I need to do this? (I have never done this before) Or, can we stage it?

[Adam Roben (:aroben)]

It is standard practice to update test results in the same checkin as the code that changes the test's behavior. If you don't do that then we will end up with red bots (even if only temporarily).

[Rik Cabanier]

(In reply to comment #21)
> It is standard practice to update test results in the same checkin as the code that changes the test's behavior. If you don't do that then we will end up with red bots (even if only temporarily).

How do I remove the layout tests you added?
Do I just delete the files from my drive and create a patch?

[Adam Roben (:aroben)]

Use "svn rm" to delete the files.

[Rik Cabanier]

Created attachment 86679 [details]
removed obsolete layout results

[Rik Cabanier]

Created attachment 86680 [details]
removed obsolete layout results

[Rik Cabanier]

Created attachment 86681 [details]
removed obsolete layout results

[Rik Cabanier]

(In reply to comment #23)
> Use "svn rm" to delete the files.

can you check if I did it right?

[Adam Roben (:aroben)]

The file removal looks right, though I don't think you've verified that Windows actually matches the Mac results now (have you?).

[Rik Cabanier]

(In reply to comment #28)
> The file removal looks right, though I don't think you've verified that Windows actually matches the Mac results now (have you?).

true :-)
Where is it pulling the expected results from? I can't tell what it is using as the expected output

[Adam Roben (:aroben)]

Windows looks in these locations (in this order):

platform/win-xp (only if you're running Windows XP)
platform/win
platform/mac-snowleopard
platform/mac
top level

[Rik Cabanier]

Created attachment 86692 [details]
updated layout tests

[Rik Cabanier]

Created attachment 86695 [details]
updated layout tests

[Rik Cabanier]

(In reply to comment #30)
> Windows looks in these locations (in this order):
> 
> platform/win-xp (only if you're running Windows XP)
> platform/win
> platform/mac-snowleopard
> platform/mac
> top level

OK.
Now it should be OK. I just updated the windows layout tests.

[Rik Cabanier]

Created attachment 86831 [details]
changed to simple cast

[Rik Cabanier]

Created attachment 86832 [details]
changed to simple cast

[Rik Cabanier]

Created attachment 86836 [details]
Fixed patch

[Dave Hyatt]

Comment on attachment 86836 [details]
Fixed patch

r=me

[WebKit Commit Bot]

Comment on attachment 86836 [details]
Fixed patch

Clearing flags on attachment: 86836

Committed r81925: <http://trac.webkit.org/changeset/81925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81925 might have broken Windows XP Debug (Tests)

[Adam Roben (:aroben)]

The new Windows results were generated using the wrong fonts. Since we match Mac now, we don't need Windows-specific results at all. I removed them in r81958.

[Rik Cabanier]

(In reply to comment #41)
> The new Windows results were generated using the wrong fonts. Since we match Mac now, we don't need Windows-specific results at all. I removed them in r81958.

I was wondering about that.
Can you tell me where I can find the windows fonts?

I tried to follow the instructions, but couldn't get it to work...