CC: Fix precision loss in scale reciprocal.

CC: Fix precision loss in scale reciprocal.

The content_scale_x/y are in single precision (and not full
representable as floats), so if we want to multiply content
scale by its reciprocal and get 1.0, we need the reciprocal
to be in double precision.  Otherwise, a floating point error
epsilon leaks into our double matrices.

BUG=259154

[epenner, 2013-08-27 01:53:38]

PTAL.

Two character change FTW!

See here for an example of what was happening:
http://ideone.com/S31WDk

There may also be an argument to make all content scales doubles?  However,
unless we actually need more accuracy in the scale itself (I don't think this is
the case), doing all math that touches scales in double precision should be
sufficient.

[shawnsingh, 2013-08-27 02:06:41]

On 2013/08/27 01:53:38, epenner wrote:
> PTAL.
> 
> Two character change FTW!
> 
> See here for an example of what was happening:
> http://ideone.com/S31WDk
> 
> There may also be an argument to make all content scales doubles?  However,
> unless we actually need more accuracy in the scale itself (I don't think this
is
> the case), doing all math that touches scales in double precision should be
> sufficient.

If we do go this direction, could we also go ahead and modify the reciprocal in
PointHitsRegion()  (and anywhere else this pattern appears...)

Do we have any sense of whether double precision for scales will really hurt us?
 Can we get some benchmarks?   On intel cpus I would predict very little
performance hit since all math is done 80 bit precision internally and the only
tradeoff would be a few extra bytes of storage that isn't likely to skew
performance.  I'm not sure about typical ARM processors though.

[epennerAtGoogle, 2013-08-27 02:24:19]

On 2013/08/27 02:06:41, shawnsingh wrote:
> On 2013/08/27 01:53:38, epenner wrote:
> > PTAL.
> > 
> > Two character change FTW!
> > 
> > See here for an example of what was happening:
> > http://ideone.com/S31WDk
> > 
> > There may also be an argument to make all content scales doubles?  However,
> > unless we actually need more accuracy in the scale itself (I don't think
this
> is
> > the case), doing all math that touches scales in double precision should be
> > sufficient.
> 
> If we do go this direction, could we also go ahead and modify the reciprocal
in
> PointHitsRegion()  (and anywhere else this pattern appears...)
> 
> Do we have any sense of whether double precision for scales will really hurt
us?
>  Can we get some benchmarks?   On intel cpus I would predict very little
> performance hit since all math is done 80 bit precision internally and the
only
> tradeoff would be a few extra bytes of storage that isn't likely to skew
> performance.  I'm not sure about typical ARM processors though.

In PointHitsRegion() the value must be a float as it is passed to
gfx::ScalePoint. I don't believe we need 1/1024th of a pixel accuracy there, so
I think floating point should be okay?

I now believe that 80bit vs 64bit wasn't ever causing any problems in CC (at
least not that we have observed yet), but this was the problem all along
(floating point precision error leaking into our double matrices).

I doubt it's a performance issue to use doubles, given how much math is already
doubles, but I'm a big fan of not using doubles "unless absolutely necessary".
It appears as though we might actually be fine using floats everywhere... This
error has shown that there is a relatively small impact when floating
point-sized errors are introduced.

[danakj, 2013-08-27 02:28:49]

https://codereview.chromium.org/23440005/diff/1/cc/trees/layer_tree_host_comm...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/23440005/diff/1/cc/trees/layer_tree_host_comm...
cc/trees/layer_tree_host_common.cc:1172: (1.0 / layer->contents_scale_x(), 1.0 /
layer->contents_scale_y());
The input to this function is a double (at the moment) so this seems fine to me.

(Also how did this function opening bracket get line wrapped?)

However, I think the right thing to do is round nearly-integer scales to
integers in the draw/screen space transforms.

[epennerAtGoogle, 2013-08-27 02:45:12]

On 2013/08/27 02:28:49, danakj wrote:
>
https://codereview.chromium.org/23440005/diff/1/cc/trees/layer_tree_host_comm...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/23440005/diff/1/cc/trees/layer_tree_host_comm...
> cc/trees/layer_tree_host_common.cc:1172: (1.0 / layer->contents_scale_x(), 1.0
/
> layer->contents_scale_y());
> The input to this function is a double (at the moment) so this seems fine to
me.
> 
> (Also how did this function opening bracket get line wrapped?)
> 
> However, I think the right thing to do is round nearly-integer scales to
> integers in the draw/screen space transforms.

Oh.. Good call on the odd style. I'll fix it. 

The nearly-integer scales thing can be tricky I think. And this bug shows we
didn't actually hit rounding issues but rather just matrix corruption.

Also, content-scale can map any integer to any other integer right?. If a page
is 768 pixels wide, and you zoom it up to 1280 pixels, the scale is 1280/768
which can't be represented as a float. Rounding that by itself to a float is
fine I think, as long as exact float that becomes the 'correct' value in all
other calculations.

[enne (OOO), 2013-08-27 18:37:14]

On 2013/08/27 01:53:38, epenner wrote:
 
> Two character change FTW!

How do you fit a unit test in two characters? ;)

More seriously, I want to make SkMScalar (the type used in
gfx::Transform/SkMatrix44) a float instead of a double because it looks like a
perf win.  I want to make sure I don't regress this issue at the same time.

[shawnsingh, 2013-08-27 18:52:28]

On 2013/08/27 18:37:14, enne wrote:
> On 2013/08/27 01:53:38, epenner wrote:
>  
> > Two character change FTW!
> 
> How do you fit a unit test in two characters? ;)
> 
> More seriously, I want to make SkMScalar (the type used in
> gfx::Transform/SkMatrix44) a float instead of a double because it looks like a
> perf win.  I want to make sure I don't regress this issue at the same time.

I'm not skeptical about this, but I'm just wondering where the perf win has been
observed?  I looked it up last night - ARM does have a perf difference in single
vs double arithmetic.  x87 does not.  Or, do you see the perf win due to the
reduced footprint / other?

[enne (OOO), 2013-08-27 18:55:57]

On 2013/08/27 18:52:28, shawnsingh wrote:

> I'm not skeptical about this, but I'm just wondering where the perf win has
been
> observed?  I looked it up last night - ARM does have a perf difference in
single
> vs double arithmetic.  x87 does not.  Or, do you see the perf win due to the
> reduced footprint / other?

This was on ARM, yes.  It was about a 5-6% win on LTH-related cc_perftests.

[epennerAtGoogle, 2013-08-27 19:03:51]

On 2013/08/27 18:37:14, enne wrote:
> On 2013/08/27 01:53:38, epenner wrote:
>  
> > Two character change FTW!
> 
> How do you fit a unit test in two characters? ;)
> 

Ha! Yeah that will be slightly more difficult.

> More seriously, I want to make SkMScalar (the type used in
> gfx::Transform/SkMatrix44) a float instead of a double because it looks like a
> perf win.  I want to make sure I don't regress this issue at the same time.

This sounds like an awesome goal! I predict a 15-25% gain followed by a 300-400%
on all math if we can use SSE/Neon.

For the test though, I can make one for doubles, but it's a bit tricky for
floats, as float rounding can actually mask this bug as well. For example,
here's an example where float rounding 'fixes' the issue:
http://ideone.com/VVCDfl

Single precision seems inherently not good enough in this case, but I don't know
any particular scenariou that is guaranteed to trigger it.

[enne (OOO), 2013-08-27 19:07:47]

On 2013/08/27 19:03:51, epennerAtGoogle wrote:

> For the test though, I can make one for doubles, but it's a bit tricky for
> floats, as float rounding can actually mask this bug as well. For example,
> here's an example where float rounding 'fixes' the issue:
> http://ideone.com/VVCDfl

I think I was looking for a higher level test than that.  Like, adding a layer
with a contents scale at some +20k translation offset, CalcDrawProps, generate
quads, make sure those quads don't use AA.

[epennerAtGoogle, 2013-08-27 19:10:30]

On 2013/08/27 19:07:47, enne wrote:
> On 2013/08/27 19:03:51, epennerAtGoogle wrote:
> 
> > For the test though, I can make one for doubles, but it's a bit tricky for
> > floats, as float rounding can actually mask this bug as well. For example,
> > here's an example where float rounding 'fixes' the issue:
> > http://ideone.com/VVCDfl
> 
> I think I was looking for a higher level test than that.  Like, adding a layer
> with a contents scale at some +20k translation offset, CalcDrawProps, generate
> quads, make sure those quads don't use AA.

I'm on the same page there, but I think float rounding might still 'fix' it
randomly. But maybe I'm wrong and there is enough math operations to guarantee a
float error (especially with a large enough scroll offset).  Can't hurt so I'll
put together that test. It also protects against 'double corruption' if nothing
else.