code review 34700043: freetype/truetype: load vertical metrics from the OS/2 ...

freetype/truetype: load vertical metrics from the OS/2 table.

[nigeltao, 2013-11-28 09:27:23]

Hello bsiegert@gmail.com (cc: golang-dev@googlegroups.com,
remyoudompheng@gmail.com),

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

[bsiegert, 2013-11-28 09:45:41]

LGTM

Maybe mention the yMax parameter in the docstring for unscaledVMetric.
On jeu., nov. 28, 2013 at 10:27 AM, <nigeltao@golang.org> wrote:

> Reviewers: bsiegert,
>
> Message:
> Hello bsiegert@gmail.com (cc: golang-dev@googlegroups.com,
> remyoudompheng@gmail.com),
>
> I'd like you to review this change to
> https://code.google.com/p/freetype-go
>
>
> Description:
> freetype/truetype: load vertical metrics from the OS/2 table.
>
> Please review this at https://codereview.appspot.com/34700043/
>
> Affected files (+21, -27 lines):
>    freetype/truetype/glyph.go
>    freetype/truetype/truetype.go
>    freetype/truetype/truetype_test.go
>
>
> Index: freetype/truetype/glyph.go
> ===================================================================
> --- a/freetype/truetype/glyph.go
> +++ b/freetype/truetype/glyph.go
> @@ -377,12 +377,12 @@
>
>   func (g *GlyphBuf) addPhantomsAndScale(b Bounds, uhm HMetric, i Index,
> np0
> int, simple bool) {
>         // Add the four phantom points.
> -       uvm := g.font.unscaledVMetric(i)
> +       uvm := g.font.unscaledVMetric(i, b.YMax)
>         g.Point = append(g.Point,
>                 Point{X: b.XMin - uhm.LeftSideBearing},
>                 Point{X: b.XMin - uhm.LeftSideBearing + uhm.AdvanceWidth},
> -               Point{Y: b.YMax + uvm.TopSideBearing},
> -               Point{Y: b.YMax + uvm.TopSideBearing - uvm.AdvanceHeight},
> +               Point{X: uhm.AdvanceWidth / 2, Y: b.YMax +
> uvm.TopSideBearing},
> +               Point{X: uhm.AdvanceWidth / 2, Y: b.YMax +
> uvm.TopSideBearing -
> uvm.AdvanceHeight},
>         )
>         // Scale the points.
>         if simple && g.hinter != nil {
> Index: freetype/truetype/truetype.go
> ===================================================================
> --- a/freetype/truetype/truetype.go
> +++ b/freetype/truetype/truetype.go
> @@ -98,7 +98,7 @@
>   type Font struct {
>         // Tables sliced from the TTF data. The different tables are
> documented
>         // at http://developer.apple.com/fonts/TTRefMan/RM06/Chap6.html
> -       cmap, cvt, fpgm, glyf, head, hhea, hmtx, kern, loca, maxp, prep,
> vmtx
> []byte
> +       cmap, cvt, fpgm, glyf, head, hhea, hmtx, kern, loca, maxp, os2,
> prep,
> vmtx []byte
>
>         cmapIndexes []byte
>
> @@ -374,7 +374,7 @@
>
>   // unscaledVMetric returns the unscaled vertical metrics for the glyph
> with
>   // the given index.
> -func (f *Font) unscaledVMetric(i Index) (v VMetric) {
> +func (f *Font) unscaledVMetric(i Index, yMax int32) (v VMetric) {
>         j := int(i)
>         if j < 0 || f.nGlyph <= j {
>                 return VMetric{}
> @@ -385,6 +385,19 @@
>                         TopSideBearing: int32(int16(u16(f.vmtx, 4*j+2))),
>                 }
>         }
> +       // The OS/2 table has grown over time.
> +       // https://developer.apple.com/fonts/TTRefMan/RM06/Chap6OS2.html
> +       // says that it was originally 68 bytes. Optional fields, including
> +       // the ascender and descender, are described at
> +       // http://www.microsoft.com/typography/otspec/os2.htm
> +       if len(f.os2) >= 72 {
> +               sTypoAscender := int32(int16(u16(f.os2, 68)))
> +               sTypoDescender := int32(int16(u16(f.os2, 70)))
> +               return VMetric{
> +                       AdvanceHeight:  sTypoAscender - sTypoDescender,
> +                       TopSideBearing: sTypoAscender - yMax,
> +               }
> +       }
>         return VMetric{
>                 AdvanceHeight:  f.fUnitsPerEm,
>                 TopSideBearing: 0,
> @@ -393,7 +406,7 @@
>
>   // VMetric returns the vertical metrics for the glyph with the given
> index.
>   func (f *Font) VMetric(scale int32, i Index) VMetric {
> -       v := f.unscaledVMetric(i)
> +       v := f.unscaledVMetric(i, 0)
>         v.AdvanceHeight = f.scale(scale * v.AdvanceHeight)
>         v.TopSideBearing = f.scale(scale * v.TopSideBearing)
>         return v
> @@ -500,6 +513,8 @@
>                         f.loca, err = readTable(ttf, ttf[x+8:x+16])
>                 case "maxp":
>                         f.maxp, err = readTable(ttf, ttf[x+8:x+16])
> +               case "OS/2":
> +                       f.os2, err = readTable(ttf, ttf[x+8:x+16])
>                 case "prep":
>                         f.prep, err = readTable(ttf, ttf[x+8:x+16])
>                 case "vmtx":
> Index: freetype/truetype/truetype_test.go
> ===================================================================
> --- a/freetype/truetype/truetype_test.go
> +++ b/freetype/truetype/truetype_test.go
> @@ -261,22 +261,6 @@
>         {"x-times-new-roman", 13, 0},
>   }
>
> -var scalingExceptions = map[string]map[int]bool{
> -       // TODO: fix these exceptions now that C Freetype version 2.5.1 is
> released:
> -       // see http://lists.nongnu.org/archive/html/freetype/2013-11/
> msg00004.html
> -       "x-deja-vu-sans-oblique": map[int]bool{
> -               269:  true,
> -               733:  true,
> -               734:  true,
> -               2071: true,
> -               2072: true,
> -               2077: true,
> -               2078: true,
> -               2171: true,
> -               2172: true,
> -       },
> -}
> -
>   // TODO: also test bounding boxes, not just points.
>
>   func testScaling(t *testing.T, hinter *Hinter) {
> @@ -329,7 +313,6 @@
>                         continue
>                 }
>
> -               exceptions := scalingExceptions[tc.name]
>                 glyphBuf := NewGlyphBuf()
>                 for i, want := range wants {
>                         // TODO: completely implement hinting. For now,
> only the first
> @@ -338,10 +321,6 @@
>                                 break
>                         }
>
> -                       if exceptions != nil && exceptions[i] {
> -                               continue
> -                       }
> -
>                         if err = glyphBuf.Load(font, tc.size*64, Index(i),
> hinter); err != nil {
>                                 t.Errorf("%s: glyph #%d: Load: %v",
> tc.name, i, err)
>                                 continue
>
>
>

[nigeltao, 2013-11-28 23:33:26]

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

freetype/truetype: load vertical metrics from the OS/2 table.

R=bsiegert
CC=golang-dev, remyoudompheng
https://codereview.appspot.com/34700043