freetype/truetype: calculate advance widths correctly.

The logic is a little clunky, but as before, let's get it right first,
then get it clean once we have a full battery of tests.

R=bsiegert
CC=golang-codereviews
https://codereview.appspot.com/50910043

freetype/freetype.go

@@ -31,10 +31,11 @@ const (
 // implicitly by the quantized x and y fractional offset. It maps to a mask
 // image and an offset.
 type cacheEntry struct {
-	valid  bool
-	glyph  truetype.Index
-	mask   *image.Alpha
-	offset image.Point
+	valid        bool
+	glyph        truetype.Index
+	advanceWidth raster.Fix32
+	mask         *image.Alpha
+	offset       image.Point
 }
 
 // ParseFont just calls the Parse function from the freetype/truetype package.
@@ -124,12 +125,14 @@ func (c *Context) drawContour(ps []truetype.Point, dx, dy raster.Fix32) {
 	}
 }
 
-// rasterize returns the glyph mask and integer-pixel offset to render the
-// given glyph at the given sub-pixel offsets.
+// rasterize returns the advance width, glyph mask and integer-pixel offset
+// to render the given glyph at the given sub-pixel offsets.
 // The 24.8 fixed point arguments fx and fy must be in the range [0, 1).
-func (c *Context) rasterize(glyph truetype.Index, fx, fy raster.Fix32) (*image.Alpha, image.Point, error) {
+func (c *Context) rasterize(glyph truetype.Index, fx, fy raster.Fix32) (
+	raster.Fix32, *image.Alpha, image.Point, error) {
+
 	if err := c.glyphBuf.Load(c.font, c.scale, glyph, nil); err != nil {
-		return nil, image.ZP, err
+		return 0, nil, image.Point{}, err
 	}
 	// Calculate the integer-pixel bounds for the glyph.
 	xmin := int(fx+raster.Fix32(c.glyphBuf.B.XMin<<2)) >> 8
@@ -137,7 +140,7 @@ func (c *Context) rasterize(glyph truetype.Index, fx, fy raster.Fix32) (*image.A
 	xmax := int(fx+raster.Fix32(c.glyphBuf.B.XMax<<2)+0xff) >> 8
 	ymax := int(fy-raster.Fix32(c.glyphBuf.B.YMin<<2)+0xff) >> 8
 	if xmin > xmax || ymin > ymax {
-		return nil, image.ZP, errors.New("freetype: negative sized glyph")
+		return 0, nil, image.Point{}, errors.New("freetype: negative sized glyph")
 	}
 	// A TrueType's glyph's nodes can have negative co-ordinates, but the
 	// rasterizer clips anything left of x=0 or above y=0. xmin and ymin
@@ -155,13 +158,16 @@ func (c *Context) rasterize(glyph truetype.Index, fx, fy raster.Fix32) (*image.A
 	}
 	a := image.NewAlpha(image.Rect(0, 0, xmax-xmin, ymax-ymin))
 	c.r.Rasterize(raster.NewAlphaSrcPainter(a))
-	return a, image.Point{xmin, ymin}, nil
+	return raster.Fix32(c.glyphBuf.AdvanceWidth << 2), a, image.Point{xmin, ymin}, nil
 }
 
-// glyph returns the glyph mask and integer-pixel offset to render the given
-// glyph at the given sub-pixel point. It is a cache for the rasterize method.
-// Unlike rasterize, p's co-ordinates do not have to be in the range [0, 1).
-func (c *Context) glyph(glyph truetype.Index, p raster.Point) (*image.Alpha, image.Point, error) {
+// glyph returns the advance width, glyph mask and integer-pixel offset to
+// render the given glyph at the given sub-pixel point. It is a cache for the
+// rasterize method. Unlike rasterize, p's co-ordinates do not have to be in
+// the range [0, 1).
+func (c *Context) glyph(glyph truetype.Index, p raster.Point) (
+	raster.Fix32, *image.Alpha, image.Point, error) {
+
 	// Split p.X and p.Y into their integer and fractional parts.
 	ix, fx := int(p.X>>8), p.X&0xff
 	iy, fy := int(p.Y>>8), p.Y&0xff
@@ -171,16 +177,16 @@ func (c *Context) glyph(glyph truetype.Index, p raster.Point) (*image.Alpha, ima
 	ty := int(fy) / (256 / nYFractions)
 	t := ((tg*nXFractions)+tx)*nYFractions + ty
 	// Check for a cache hit.
-	if c.cache[t].valid && c.cache[t].glyph == glyph {
-		return c.cache[t].mask, c.cache[t].offset.Add(image.Point{ix, iy}), nil
+	if e := c.cache[t]; e.valid && e.glyph == glyph {
+		return e.advanceWidth, e.mask, e.offset.Add(image.Point{ix, iy}), nil
 	}
 	// Rasterize the glyph and put the result into the cache.
-	mask, offset, err := c.rasterize(glyph, fx, fy)
+	advanceWidth, mask, offset, err := c.rasterize(glyph, fx, fy)
 	if err != nil {
-		return nil, image.ZP, err
+		return 0, nil, image.Point{}, err
 	}
-	c.cache[t] = cacheEntry{true, glyph, mask, offset}
-	return mask, offset.Add(image.Point{ix, iy}), nil
+	c.cache[t] = cacheEntry{true, glyph, advanceWidth, mask, offset}
+	return advanceWidth, mask, offset.Add(image.Point{ix, iy}), nil
 }
 
 // DrawString draws s at p and returns p advanced by the text extent. The text
@@ -198,13 +204,14 @@ func (c *Context) DrawString(s string, p raster.Point) (raster.Point, error) {
 	for _, rune := range s {
 		index := c.font.Index(rune)
 		if hasPrev {
+			// TODO: adjust for hinting.
 			p.X += raster.Fix32(c.font.Kerning(c.scale, prev, index)) << 2
 		}
-		mask, offset, err := c.glyph(index, p)
+		advanceWidth, mask, offset, err := c.glyph(index, p)
 		if err != nil {
 			return raster.Point{}, err
 		}
-		p.X += raster.Fix32(c.font.HMetric(c.scale, index).AdvanceWidth) << 2
+		p.X += advanceWidth
 		glyphRect := mask.Bounds().Add(offset)
 		dr := c.clip.Intersect(glyphRect)
 		if !dr.Empty() {

freetype/truetype/glyph.go

@@ -17,6 +17,8 @@ type Point struct {
 // A GlyphBuf holds a glyph's contours. A GlyphBuf can be re-used to load a
 // series of glyphs from a Font.
 type GlyphBuf struct {
+	// AdvanceWidth is the glyph's advance width.
+	AdvanceWidth int32
 	// B is the glyph's bounding box.
 	B Bounds
 	// Point contains all Points from all contours of the glyph. If a
@@ -106,6 +108,22 @@ func (g *GlyphBuf) Load(f *Font, scale int32, i Index, h *Hinter) error {
 		}
 	}
 
+	advanceWidth := g.phantomPoints[1].X - g.phantomPoints[0].X
+	if h != nil {
+		if len(f.hdmx) >= 8 {
+			if n := u32(f.hdmx, 4); n > 3+uint32(i) {
+				for hdmx := f.hdmx[8:]; uint32(len(hdmx)) >= n; hdmx = hdmx[n:] {
+					if int32(hdmx[0]) == scale>>6 {
+						advanceWidth = int32(hdmx[2+i]) << 6
+						break
+					}
+				}
+			}
+		}
+		advanceWidth = (advanceWidth + 32) &^ 63
+	}
+	g.AdvanceWidth = advanceWidth
+
 	// Set g.B to the 'control box', which is the bounding box of the Bézier
 	// curves' control points. This is easier to calculate, no smaller than
 	// and often equal to the tightest possible bounding box of the curves
@@ -159,16 +177,17 @@ func (g *GlyphBuf) load(recursion int32, i Index, useMyMetrics bool) (err error)
 		g0 = u32(g.font.loca, 4*int(i))
 		g1 = u32(g.font.loca, 4*int(i)+4)
 	}
-	if g0 == g1 {
-		return nil
-	}
-	glyf := g.font.glyf[g0:g1]
 
-	// Decode the contour count and nominal bounding box.
-	// boundsYMin and boundsXMax, at offsets 4 and 6, are unused.
-	ne := int(int16(u16(glyf, 0)))
-	boundsXMin := int32(int16(u16(glyf, 2)))
-	boundsYMax := int32(int16(u16(glyf, 8)))
+	// Decode the contour count and nominal bounding box, from the first
+	// 10 bytes of the glyf data. boundsYMin and boundsXMax, at offsets 4
+	// and 6, are unused.
+	glyf, ne, boundsXMin, boundsYMax := []byte(nil), 0, int32(0), int32(0)
+	if g0+10 <= g1 {
+		glyf = g.font.glyf[g0:g1]
+		ne = int(int16(u16(glyf, 0)))
+		boundsXMin = int32(int16(u16(glyf, 2)))
+		boundsYMax = int32(int16(u16(glyf, 8)))
+	}
 
 	// Create the phantom points.
 	uhm, pp1x := g.font.unscaledHMetric(i), int32(0)
@@ -179,6 +198,12 @@ func (g *GlyphBuf) load(recursion int32, i Index, useMyMetrics bool) (err error)
 		{X: uhm.AdvanceWidth / 2, Y: boundsYMax + uvm.TopSideBearing},
 		{X: uhm.AdvanceWidth / 2, Y: boundsYMax + uvm.TopSideBearing - uvm.AdvanceHeight},
 	}
+	if len(glyf) == 0 {
+		g.addPhantomsAndScale(len(g.Point), len(g.Point), true, true)
+		copy(g.phantomPoints[:], g.Point[len(g.Point)-4:])
+		g.Point = g.Point[:len(g.Point)-4]
+		return nil
+	}
 
 	// Load and hint the contours.
 	if ne < 0 {
@@ -194,7 +219,7 @@ func (g *GlyphBuf) load(recursion int32, i Index, useMyMetrics bool) (err error)
 	} else {
 		np0, ne0 := len(g.Point), len(g.End)
 		program := g.loadSimple(glyf, ne)
-		g.addPhantomsAndScale(np0, np0, true)
+		g.addPhantomsAndScale(np0, np0, true, true)
 		pp1x = g.Point[len(g.Point)-4].X
 		if g.hinter != nil {
 			if len(program) != 0 {
@@ -213,7 +238,9 @@ func (g *GlyphBuf) load(recursion int32, i Index, useMyMetrics bool) (err error)
 			g.InFontUnits = g.InFontUnits[:len(g.InFontUnits)-4]
 			g.Unhinted = g.Unhinted[:len(g.Unhinted)-4]
 		}
-		copy(g.phantomPoints[:], g.Point[len(g.Point)-4:])
+		if useMyMetrics {
+			copy(g.phantomPoints[:], g.Point[len(g.Point)-4:])
+		}
 		g.Point = g.Point[:len(g.Point)-4]
 		if np0 != 0 {
 			// The hinting program expects the []End values to be indexed relative
@@ -397,22 +424,28 @@ func (g *GlyphBuf) loadCompound(recursion int32, uhm HMetric, i Index,
 		}
 	}
 
-	// Hint the compound glyph.
-	if g.hinter == nil || offset+2 > len(glyf) {
-		return nil
-	}
-	instrLen := int(u16(glyf, offset))
-	offset += 2
-	if instrLen == 0 {
-		return nil
+	instrLen := 0
+	if g.hinter != nil && offset+2 <= len(glyf) {
+		instrLen = int(u16(glyf, offset))
+		offset += 2
 	}
-	program := glyf[offset : offset+instrLen]
-	g.addPhantomsAndScale(np0, len(g.Point), false)
+
+	g.addPhantomsAndScale(np0, len(g.Point), false, instrLen > 0)
 	points, ends := g.Point[np0:], g.End[ne0:]
 	g.Point = g.Point[:len(g.Point)-4]
 	for j := range points {
 		points[j].Flags &^= flagTouchedX | flagTouchedY
 	}
+
+	if instrLen == 0 {
+		if !g.metricsSet {
+			copy(g.phantomPoints[:], points[len(points)-4:])
+		}
+		return nil
+	}
+
+	// Hint the compound glyph.
+	program := glyf[offset : offset+instrLen]
 	// Temporarily adjust the ends to be relative to this compound glyph.
 	if np0 != 0 {
 		for i := range ends {
@@ -430,11 +463,13 @@ func (g *GlyphBuf) loadCompound(recursion int32, uhm HMetric, i Index,
 			ends[i] += np0
 		}
 	}
-	copy(g.phantomPoints[:], points[len(points)-4:])
+	if !g.metricsSet {
+		copy(g.phantomPoints[:], points[len(points)-4:])
+	}
 	return nil
 }
 
-func (g *GlyphBuf) addPhantomsAndScale(np0, np1 int, simple bool) {
+func (g *GlyphBuf) addPhantomsAndScale(np0, np1 int, simple, adjust bool) {
 	// Add the four phantom points.
 	g.Point = append(g.Point, g.phantomPoints[:]...)
 	// Scale the points.
@@ -446,19 +481,24 @@ func (g *GlyphBuf) addPhantomsAndScale(np0, np1 int, simple bool) {
 		p.X = g.font.scale(g.scale * p.X)
 		p.Y = g.font.scale(g.scale * p.Y)
 	}
-	if g.hinter != nil {
-		// Round the 1st phantom point to the grid, shifting all other points equally.
-		// Note that "all other points" starts from np0, not np1.
-		// TODO: is the np0/np1 distinction actually a bug in C Freetype?
+	if g.hinter == nil {
+		return
+	}
+	// Round the 1st phantom point to the grid, shifting all other points equally.
+	// Note that "all other points" starts from np0, not np1.
+	// TODO: delete this adjustment and the np0/np1 distinction, when
+	// we update the compatibility tests to C Freetype 2.5.3.
+	// See http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=05c786d990390a7ca18e62962641dac740bacb06
+	if adjust {
 		pp1x := g.Point[len(g.Point)-4].X
 		if dx := ((pp1x + 32) &^ 63) - pp1x; dx != 0 {
 			for i := np0; i < len(g.Point); i++ {
 				g.Point[i].X += dx
 			}
 		}
-		if simple {
-			g.Unhinted = append(g.Unhinted, g.Point[np1:]...)
-		}
+	}
+	if simple {
+		g.Unhinted = append(g.Unhinted, g.Point[np1:]...)
 	}
 	// Round the 2nd and 4th phantom point to the grid.
 	p := &g.Point[len(g.Point)-3]

freetype/truetype/truetype.go

@@ -98,7 +98,7 @@ type cm struct {
 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, os2, prep, vmtx []byte
+	cmap, cvt, fpgm, glyf, hdmx, head, hhea, hmtx, kern, loca, maxp, os2, prep, vmtx []byte
 
 	cmapIndexes []byte
 
@@ -502,6 +502,8 @@ func parse(ttf []byte, offset int) (font *Font, err error) {
 			f.fpgm, err = readTable(ttf, ttf[x+8:x+16])
 		case "glyf":
 			f.glyf, err = readTable(ttf, ttf[x+8:x+16])
+		case "hdmx":
+			f.hdmx, err = readTable(ttf, ttf[x+8:x+16])
 		case "head":
 			f.head, err = readTable(ttf, ttf[x+8:x+16])
 		case "hhea":

freetype/truetype/truetype_test.go

@@ -254,18 +254,12 @@ func scalingTestEquals(a, b []Point) (index int, equals bool) {
 var scalingTestCases = []struct {
 	name string
 	size int32
-	// xxxHintingBrokenAt, if non-negative, is the glyph index n for which
-	// only the first n glyphs are known to be correct wrt the advance width
-	// and bounding boxes.
-	// TODO: remove these fields.
-	sansHintingBrokenAt int
-	withHintingBrokenAt int
 }{
-	{"luxisr", 12, -1, -1},
-	{"x-arial-bold", 11, -1, -1},
-	{"x-deja-vu-sans-oblique", 17, -1, -1},
-	{"x-droid-sans-japanese", 9, -1, -1},
-	{"x-times-new-roman", 13, -1, -1},
+	{"luxisr", 12},
+	{"x-arial-bold", 11},
+	{"x-deja-vu-sans-oblique", 17},
+	{"x-droid-sans-japanese", 9},
+	{"x-times-new-roman", 13},
 }
 
 func testScaling(t *testing.T, hinter *Hinter) {
@@ -320,31 +314,21 @@ func testScaling(t *testing.T, hinter *Hinter) {
 
 		glyphBuf := NewGlyphBuf()
 		for i, want := range wants {
-			if hinter == nil && i == tc.sansHintingBrokenAt {
-				break
-			}
-			if hinter != nil && i == tc.withHintingBrokenAt {
-				break
-			}
-
 			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
 			}
 			got := scalingTestData{
-				// TODO: a GlyphBuf should also provide the advance width.
-				advanceWidth: font.HMetric(tc.size*64, Index(i)).AdvanceWidth,
+				advanceWidth: glyphBuf.AdvanceWidth,
 				bounds:       glyphBuf.B,
 				points:       glyphBuf.Point,
 			}
 
-			/* TODO: check advance widths.
 			if got.advanceWidth != want.advanceWidth {
 				t.Errorf("%s: glyph #%d advance width:\ngot  %v\nwant %v",
 					tc.name, i, got.advanceWidth, want.advanceWidth)
 				continue
 			}
-			*/
 
 			if got.bounds != want.bounds {
 				t.Errorf("%s: glyph #%d bounds:\ngot  %v\nwant %v",