Fix for displacement mapping

Fix for displacement mapping

Fixed the skia displacement map effect to match the WebKit displacement map fix.
Committed: https://code.google.com/p/skia/source/detail?r=8225

[sugoi, 2013-03-15 17:47:06]

I fixed the displacement mapping :
- The CPU path in Skia now fits the CPU path in Webkit
- There are small differences between CPU and GPU, possibly due to interpolation
or the lack of a truncate behavior for texture coordinates. It seems like it
would require many more instructions to implement it for minimal gain, so I
didn't modify the shader.
- I changed the parameters in the gms so that their result approximately fit the
previous gm results. gms will still need to be rebased for displacement.

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
src/effects/SkDisplacementMapEffect.cpp:68: // Truncate the displacement values
I didn't see any SkScalarTruncToInt or similar function, so I implemented the
trunc here.

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
src/effects/SkDisplacementMapEffect.cpp:378: SkScalar scale =
random->nextRangeScalar(0, SkFloatToScalar(100.0f));
Changed the test range so that it's more appropriate.

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
src/effects/SkDisplacementMapEffect.cpp:496: SkScalar scaleX =
SkScalarDiv(displacementMap.scale(), SkIntToScalar(colorTex->width()));
The scale now has to be divided by the size in gpu to fit the cpu, which no
longer multiplies the scale by the size.

[bsalomon, 2013-03-15 19:09:53]

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.chromium.org/12787007/diff/1/src/effects/SkDisplacementMap...
src/effects/SkDisplacementMapEffect.cpp:68: // Truncate the displacement values
On 2013/03/15 17:47:06, sugoi wrote:
> I didn't see any SkScalarTruncToInt or similar function, so I implemented the
> trunc here.

Doesn't cast do what you want? If we care about fixed point perhaps it should be
wrapped in a macro.

[sugoi, 2013-03-15 20:11:40]

> Doesn't cast do what you want? If we care about fixed point perhaps it should
be wrapped in a macro.

Yes, static_cast<int>(...) does truncate (that's what the WebKit code does). I
can implement the SkScalarTruncToInt macro if you prefer. I didn't want to
modify SkScalar.h just for this single case unless you think it's better that
way, or did you mean that I should create a macro within this file ?

[bsalomon, 2013-03-15 20:13:37]

On 2013/03/15 20:11:40, sugoi wrote:
> > Doesn't cast do what you want? If we care about fixed point perhaps it
should
> be wrapped in a macro.
> 
> Yes, static_cast<int>(...) does truncate (that's what the WebKit code does). I
> can implement the SkScalarTruncToInt macro if you prefer. I didn't want to
> modify SkScalar.h just for this single case unless you think it's better that
> way, or did you mean that I should create a macro within this file ?

I was just suggesting either putting static_cast<int> directly here or adding
the macro to SkScalar defined to cast when scalar is float and do the if < 0
thing for fixed. I don't think it is important to make scalar=fixed work,
though.

[bsalomon, 2013-03-15 20:39:05]

On 2013/03/15 20:38:01, sugoi wrote:

lgtm

[sugoi1, 2013-03-19 15:07:50]

Committed patchset #2 manually as r8225 (presubmit successful).

[reed1, 2013-03-19 15:19:14]

Sorry to comment late.

Why are we "truncating" the float values, instead of floor/ceiling/round? How is
this behavior defined?

[sugoi, 2013-03-19 15:39:39]

Truncating is the default behavior of doing :
static_cast<int>(float_value)

Reason 1 :
I need this algorithm to be exactly the same as the WebKit algorithm so that I
don't break the WebKit tests when I integrate this change into WebKit

Reason 2 :
You need your center values to be 0 (i.e. no displacement) and since half of 255
is 127.5, but 0 can only be represented by either 127 or 128, a small rounding
error could offset the entire image by 1 pixel, so all values in the range
[-0.999..., 0.999...] are considered to be a 0 displacement value.

[reed1, 2013-03-19 15:46:23]

On 2013/03/19 15:39:39, sugoi wrote:
> Truncating is the default behavior of doing :
> static_cast<int>(float_value)
> 
> Reason 1 :
> I need this algorithm to be exactly the same as the WebKit algorithm so that I
> don't break the WebKit tests when I integrate this change into WebKit
> 
> Reason 2 :
> You need your center values to be 0 (i.e. no displacement) and since half of
255
> is 127.5, but 0 can only be represented by either 127 or 128, a small rounding
> error could offset the entire image by 1 pixel, so all values in the range
> [-0.999..., 0.999...] are considered to be a 0 displacement value.

Thanks, that would be a good comment in the code, rather than "Truncate the
values".