Fix problems with flakes in SVG LayoutTests.

Fix problems with flakes in SVG LayoutTests.

A bug would make 4.99999952 be printed as "5.00" instead of "5" because
4.99999952 would be compared to 4 instead of 5.

Also, 4.99999952 would cause some positions and rects
to be shifted by a pixel. Fix here is to snap numbers very close
to integers to the closest integer before printing it.

That 4.99999952 appeared only sometimes, and only on Windows, is
not fully known. A suspect is that it's related to the 32-bit ABI which
requires use of x87 for passing floats. That combined with various
inlining decisions and context switches could possibly affect the final
value.

This operation gives 4.99999952: float(25 + 200 * double(1.f*(-10.0f/100.f)))
This operation gives 5.00000000: float(25 + 200 * float(1.f*(-10.0f/100.f)))


BUG=438282

[Daniel Bratell, 2015-01-16 13:21:28]

bratell@opera.com changed reviewers:
+ fs@opera.com, pdr@chromium.org

[Daniel Bratell, 2015-01-16 13:21:29]

This patch fixes the issue with the trailing zeros. I thought it would also fix
the flakiness with one-pixel y offset, but I've actually seen it happen once in
one of the flaky tests after changing the code so it might just be a partial
fix.

[pdr., 2015-01-20 19:51:38]

pdr@chromium.org changed reviewers:
+ leviw@chromium.org

[pdr., 2015-01-20 19:53:45]

In general I support this kind of change!

I don't understand why the hasFractions check isn't all that's required here.
Shouldn't isFractions return true which will be all that's needed here? Why do
we need snapToCloseInteger?

[Daniel Bratell, 2015-01-20 22:42:22]

On 2015/01/20 19:53:45, pdr wrote:
> In general I support this kind of change!
> 
> I don't understand why the hasFractions check isn't all that's required here.
> Shouldn't isFractions return true which will be all that's needed here? Why do
> we need snapToCloseInteger?

I might move that out to another CL because it's fixing a different problem. The
TextStream change fixes that we got decimals (5.00) for 4.99999..., while
snapToCloseInteger tries to make numbers be exactly 5 when they were supposed to
be 5. I had a discussion with fs and ed about this approach earlier today and
it's clear that none of us really likes trying to "fix" numbers that have small
floating point errors introduced after the fact but nobody had a better plan.
The best would be if the test system would accept tiny differences, but all it
sees is a "4" when it expects a "5" which isn't even a tiny difference.

Avoiding floating point errors completely would mean moving to completely
different arithmetics and it's not really an option.

There are some places where we could reorder the operations (another patch I've
been trying out) to avoid some problems (save the error-introducing division for
last when it might be safe to do), but not all.

So what happens exactly in the most unreliable test is that some operation ends
up at 4.99999952 (i.e. 5 + tiny error) unless a certain function is inlined
(that is the flake part).
4.99999952 is then rounded to "enclosing LayoutPoint" which is ~4.98 
4.98 is then rounded to enclosing integer 4.

And so the "tiny error" turned into something much larger.

[pdr., 2015-01-21 00:33:56]

On 2015/01/20 at 22:42:22, bratell wrote:
> On 2015/01/20 19:53:45, pdr wrote:
> > In general I support this kind of change!
> > 
> > I don't understand why the hasFractions check isn't all that's required
here.
> > Shouldn't isFractions return true which will be all that's needed here? Why
do
> > we need snapToCloseInteger?
> 
> I might move that out to another CL because it's fixing a different problem.
The TextStream change fixes that we got decimals (5.00) for 4.99999...,

Could we split this one out into a separate CL (or split out the
snapToCloseInteger into a separate CL)? The TextStream change seems like a clear
win.

> while snapToCloseInteger tries to make numbers be exactly 5 when they were
supposed to be 5. I had a discussion with fs and ed about this approach earlier
today and it's clear that none of us really likes trying to "fix" numbers that
have small floating point errors introduced after the fact but nobody had a
better plan. The best would be if the test system would accept tiny differences,
but all it sees is a "4" when it expects a "5" which isn't even a tiny
difference.
> 
> Avoiding floating point errors completely would mean moving to completely
different arithmetics and it's not really an option.
> 
> There are some places where we could reorder the operations (another patch
I've been trying out) to avoid some problems (save the error-introducing
division for last when it might be safe to do), but not all.
> 
> So what happens exactly in the most unreliable test is that some operation
ends up at 4.99999952 (i.e. 5 + tiny error) unless a certain function is inlined
(that is the flake part).
> 4.99999952 is then rounded to "enclosing LayoutPoint" which is ~4.98 
> 4.98 is then rounded to enclosing integer 4.
> 
> And so the "tiny error" turned into something much larger.

For the most part, SVG doesn't use LayoutUnits internally. I wish we could
change that but there's a common pattern in SVG where the whole SVG file will be
contained in (0,0) to (1,1), and then everything gets scaled up using a viewBox
on the svg root, or a transform on the SVG image itself.

What if we changed the logic here a little and didn't call (LayoutUnit)
absoluteClippedOverflowRect in SVGRenderTreeAsText but instead used FloatRects
all the way, and then snapped at the last possible moment? This would require
making a FloatRect version of mapRectToSVGRootForPaintInvalidation (or similar),
and calling that instead of absoluteClippedOverflowRect.

[fs, 2015-01-21 09:30:43]

On 2015/01/21 00:33:56, pdr wrote:
> On 2015/01/20 at 22:42:22, bratell wrote:
> > On 2015/01/20 19:53:45, pdr wrote:
...
> > So what happens exactly in the most unreliable test is that some operation
> ends up at 4.99999952 (i.e. 5 + tiny error) unless a certain function is
inlined
> (that is the flake part).
> > 4.99999952 is then rounded to "enclosing LayoutPoint" which is ~4.98 
> > 4.98 is then rounded to enclosing integer 4.
> > 
> > And so the "tiny error" turned into something much larger.
> 
> For the most part, SVG doesn't use LayoutUnits internally. I wish we could
> change that but there's a common pattern in SVG where the whole SVG file will
be
> contained in (0,0) to (1,1), and then everything gets scaled up using a
viewBox
> on the svg root, or a transform on the SVG image itself.
> 
> What if we changed the logic here a little and didn't call (LayoutUnit)
> absoluteClippedOverflowRect in SVGRenderTreeAsText but instead used FloatRects
> all the way, and then snapped at the last possible moment? This would require
> making a FloatRect version of mapRectToSVGRootForPaintInvalidation (or
similar),
> and calling that instead of absoluteClippedOverflowRect.

I'm not sure that will help - i.e. since we'd still need to add in the position
of the <svg> (which should have a decent chance of being representable exactly
as float), that would at best leave us at the mercy of cancellation type errors
(which ought to be less than or equal to the error in a LayoutUnit.) But I'm
writing this before coffee...

When walking home after talking to Daniel yesterday evening, I managed to mostly
convince myself that rounding to the nearest LayoutUnit when producing the
relevant LayoutRect could work out (based on a more extensive CL Daniel showed
me.)

[Daniel Bratell, 2015-01-22 09:21:40]

https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):

https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGRenderTreeAsText.cpp:357: FloatRect rect =
snapToCloseInteger(object.absoluteClippedOverflowRect());
This is too late. It's in the conversion from float to LayoutRect the large
error is introduced.

[pdr., 2015-01-30 22:21:22]

On 2015/01/22 at 09:21:40, bratell wrote:
>
https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
> File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):
> 
>
https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
> Source/core/rendering/svg/SVGRenderTreeAsText.cpp:357: FloatRect rect =
snapToCloseInteger(object.absoluteClippedOverflowRect());
> This is too late. It's in the conversion from float to LayoutRect the large
error is introduced.

Sorry--I'm catching up on reviews after being out this week.

This seems too good to drop :) What should we do here?

@fs, do you still think this is a good idea?

[fs, 2015-02-02 09:33:42]

On 2015/01/30 22:21:22, pdr wrote:
> On 2015/01/22 at 09:21:40, bratell wrote:
> >
>
https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/SVGRenderTreeAsText.cpp (right):
> > 
> >
>
https://codereview.chromium.org/856533002/diff/80001/Source/core/rendering/sv...
> > Source/core/rendering/svg/SVGRenderTreeAsText.cpp:357: FloatRect rect =
> snapToCloseInteger(object.absoluteClippedOverflowRect());
> > This is too late. It's in the conversion from float to LayoutRect the large
> error is introduced.
> 
> Sorry--I'm catching up on reviews after being out this week.
> 
> This seems too good to drop :) What should we do here?
> 
> @fs, do you still think this is a good idea?

I think the "alternative" way turned out to be good enough?