freetype/truetype: fix IP rounding for negative numbers.

freetype/truetype: fix IP rounding for negative numbers.

Found in Adobe Source Sans Pro.

[remyoudompheng, 2013-10-20 09:08:46]

Hello bsiegert@gmail.com, golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/freetype-go

[bsiegert, 2013-10-20 13:32:30]

Forgive me if I am confused, but isn't this change a no-op? The way I understand
this, the sign change above cancels out the one below.

[remyoudompheng, 2013-10-20 15:10:14]

On 2013/10/20 13:32:30, bsiegert wrote:
> Forgive me if I am confused, but isn't this change a no-op? The way I
understand
> this, the sign change above cancels out the one below.

For example, if oldRange is 1000 and oldDist*curRange is -9600, we want to round
to -10000/1000 = -10
but the current code will add 500, giving -9100/1000 = -9

[bsiegert, 2013-10-20 17:04:22]

LGTM then

But wait for Nigel.


-- 
The first essential in chemistry is that you should perform practical work
and conduct experiments, for he who performs not practical work nor makes
experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[nigeltao, 2013-10-20 23:51:06]

https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go
File freetype/truetype/hint.go (right):

https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go#...
freetype/truetype/hint.go:476: oldRange, curRange = -oldRange, -curRange
I don't see this in the C Freetype code. Am I missing something?

The +/- oldRange/2 below I can understand, coming from the FT_MulDiv
implemention.

BTW, there is a bug in the Go code. Hinter.move says

if h.gs.fv[0] == 0 {
  p.Y += int32(distance)
  etc
}

but it doesn't check the sign of h.gs.fv[1], so the += should sometimes be a -=.
I have a CL brewing that fixes this, as part of a larger change, but I haven't
sent it out yet. Does that fix Adobe Source Sans Pro for you?

Also, do you have the exact font (which flavor of Adobe Source Sans Pro), size
and glyph for which Go differs from C? I'd like to reproduce this myself.

[remyoudompheng, 2013-10-21 06:36:07]

2013/10/21  <nigeltao@golang.org>:
>
> https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go
> File freetype/truetype/hint.go (right):
>
>
https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go#...
> freetype/truetype/hint.go:476: oldRange, curRange = -oldRange, -curRange
> I don't see this in the C Freetype code. Am I missing something?
>
> The +/- oldRange/2 below I can understand, coming from the FT_MulDiv
> implemention.

It is merely to make the rounding code easier to read. In my clone of
C Freetype I see a similar thing:

  FT_EXPORT_DEF( FT_Long )
  FT_MulDiv( FT_Long  a,
             FT_Long  b,
             FT_Long  c )
  {
    FT_Int   s;
    FT_Long  d;


    s = 1;
    if ( a < 0 ) { a = -a; s = -1; }
    if ( b < 0 ) { b = -b; s = -s; }
    if ( c < 0 ) { c = -c; s = -s; }

    d = (FT_Long)( c > 0 ? ( (FT_Int64)a * b + ( c >> 1 ) ) / c
                         : 0x7FFFFFFFL );

    return ( s > 0 ) ? d : -d;
  }


>
> BTW, there is a bug in the Go code. Hinter.move says
>
> if h.gs.fv[0] == 0 {
>   p.Y += int32(distance)
>   etc
> }
>
> but it doesn't check the sign of h.gs.fv[1], so the += should sometimes
> be a -=. I have a CL brewing that fixes this, as part of a larger
> change, but I haven't sent it out yet. Does that fix Adobe Source Sans
> Pro for you?
>
> Also, do you have the exact font (which flavor of Adobe Source Sans
> Pro), size and glyph for which Go differs from C? I'd like to reproduce
> this myself.
>
> https://codereview.appspot.com/15360043/

The font is the light flavour :
https://github.com/adobe/source-sans-pro/releases/download/1.055/SourceSansPr...

The problem arises on glyph 0 at size 14pt (also at 12pt but I think
there is a separate rounding issue at 12pt).

After this patch and implementing GC, MD and MSIRP (and making
INSTCTRL a no-op) I got it to pass completely for 14pt size.

[nigeltao, 2013-10-22 04:47:24]

LGTM.

I'll make the trivial edit and submit.

https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go
File freetype/truetype/hint.go (right):

https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go#...
freetype/truetype/hint.go:476: oldRange, curRange = -oldRange, -curRange
Ah, I thought that this was a no-op but I was wrong.
http://play.golang.org/p/Mj6GhjgM9f

https://codereview.appspot.com/15360043/diff/30001/freetype/truetype/hint.go#...
freetype/truetype/hint.go:488: // Compute and round oldDist * curRange/oldRange
Add a full stop to complete the sentence.

[nigeltao, 2013-10-22 06:14:04]

*** Submitted as
https://code.google.com/p/freetype-go/source/detail?r=0bd12cbef314 ***

freetype/truetype: fix IP rounding for negative numbers.

Found in Adobe Source Sans Pro.

R=golang-dev, bsiegert, nigeltao
CC=golang-dev
https://codereview.appspot.com/15360043

Committer: Nigel Tao <nigeltao@golang.org>