font/opentype: de-duplicate sfnt.loadGlyf calls

Prior to this commit, opentype.Face.Glyph started with:

    segments, err := f.f.LoadGlyph(&f.buf, x, etc)
    etc
    bounds, advance, err := f.f.GlyphBounds(&f.buf, x, etc)
    etc
    doStuffWith(segments)

One problem with this is that both f.f.FooBar calls ultimately lead to
sfnt.loadGlyf(etc, x, etc) calls and the second one is unnecessary
duplicate work for the same sfnt.GlyphIndex x.

A subtler problem is that the sfnt.Font.LoadGlyph doc comment says that
"the segments become invalid to use once [the buffer] is re-used" and we
were re-using &f.buf before walking the segments.

The fix to both problems is to downgrade the GlyphBounds call to a
cheaper GlyphAdvance call and to also move it above the LoadGlyph call.

This commit also defines and exports a new sfnt.Segments type (with a
Bounds method).

This commit also renames some s/segs/segments/ variables for
consistency, and tweaks some opentype.Face.Glyph comments.

Change-Id: I7d327db742dd701448dc097f30a87227b0fc61f6
Reviewed-on: https://go-review.googlesource.com/c/image/+/256957
Run-TryBot: Nigel Tao <nigeltao@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@gmail.com>
Trust: Nigel Tao <nigeltao@golang.org>
diff --git a/font/opentype/face.go b/font/opentype/face.go
index 1478582..72bdae8 100644
--- a/font/opentype/face.go
+++ b/font/opentype/face.go
@@ -94,12 +94,16 @@
 		return image.Rectangle{}, nil, image.Point{}, 0, false
 	}
 
-	segments, err := f.f.LoadGlyph(&f.buf, x, f.scale, nil)
+	// Call f.f.GlyphAdvance before f.f.LoadGlyph because the LoadGlyph docs
+	// say this about the &f.buf argument: the segments become invalid to use
+	// once [the buffer] is re-used.
+
+	advance, err = f.f.GlyphAdvance(&f.buf, x, f.scale, f.hinting)
 	if err != nil {
 		return image.Rectangle{}, nil, image.Point{}, 0, false
 	}
 
-	bounds, advance, err := f.f.GlyphBounds(&f.buf, x, f.scale, f.hinting)
+	segments, err := f.f.LoadGlyph(&f.buf, x, f.scale, nil)
 	if err != nil {
 		return image.Rectangle{}, nil, image.Point{}, 0, false
 	}
@@ -110,11 +114,12 @@
 	//  - 2.5  is a float32 number, "two and a half"
 	// Using 26.6 fixed point numbers means that there are 64 sub-pixel units
 	// in 1 integer pixel unit.
+
 	// Translate the sub-pixel bounding box from glyph space (where the glyph
 	// origin is at (0:00, 0:00)) to dst space (where the glyph origin is at
 	// the dot). dst space is the coordinate space that contains both the dot
-	// (a sub-pixel position) and dr (a pixel rectangle).
-	dBounds := bounds.Add(dot)
+	// (a sub-pixel position) and dr (an integer-pixel rectangle).
+	dBounds := segments.Bounds().Add(dot)
 
 	// Quantize the sub-pixel bounds (dBounds) to integer-pixel bounds (dr).
 	dr.Min.X = dBounds.Min.X.Floor()
diff --git a/font/sfnt/sfnt.go b/font/sfnt/sfnt.go
index d882891..501f820 100644
--- a/font/sfnt/sfnt.go
+++ b/font/sfnt/sfnt.go
@@ -1358,7 +1358,7 @@
 // It returns ErrNotFound if the glyph index is out of range. It returns
 // ErrColoredGlyph if the glyph is not a monochrome vector glyph, such as a
 // colored (bitmap or vector) emoji glyph.
-func (f *Font) LoadGlyph(b *Buffer, x GlyphIndex, ppem fixed.Int26_6, opts *LoadGlyphOptions) ([]Segment, error) {
+func (f *Font) LoadGlyph(b *Buffer, x GlyphIndex, ppem fixed.Int26_6, opts *LoadGlyphOptions) (Segments, error) {
 	if b == nil {
 		b = &Buffer{}
 	}
@@ -1529,6 +1529,10 @@
 	}
 	advance = fixed.Int26_6(u16(buf))
 	advance = scale(advance*ppem, f.cached.unitsPerEm)
+	if h == font.HintingFull {
+		// Quantize the fixed.Int26_6 value to the nearest pixel.
+		advance = (advance + 32) &^ 63
+	}
 
 	// Ignore the hmtx LSB entries and the glyf bounding boxes. Instead, always
 	// calculate bounds from the segments. OpenType does contain the bounds for
@@ -1536,51 +1540,13 @@
 	// compound glyphs. CFF/PostScript also have no explicit bounds and must be
 	// obtained from the segments.
 
-	seg, err := f.LoadGlyph(b, x, ppem, &LoadGlyphOptions{
+	segments, err := f.LoadGlyph(b, x, ppem, &LoadGlyphOptions{
 		// TODO: pass h, the font.Hinting.
 	})
 	if err != nil {
 		return fixed.Rectangle26_6{}, 0, err
 	}
-
-	if len(seg) > 0 {
-		bounds.Min.X = fixed.Int26_6(+(1 << 31) - 1)
-		bounds.Min.Y = fixed.Int26_6(+(1 << 31) - 1)
-		bounds.Max.X = fixed.Int26_6(-(1 << 31) + 0)
-		bounds.Max.Y = fixed.Int26_6(-(1 << 31) + 0)
-		for _, s := range seg {
-			n := 1
-			switch s.Op {
-			case SegmentOpQuadTo:
-				n = 2
-			case SegmentOpCubeTo:
-				n = 3
-			}
-			for i := 0; i < n; i++ {
-				if bounds.Max.X < s.Args[i].X {
-					bounds.Max.X = s.Args[i].X
-				}
-				if bounds.Min.X > s.Args[i].X {
-					bounds.Min.X = s.Args[i].X
-				}
-				if bounds.Max.Y < s.Args[i].Y {
-					bounds.Max.Y = s.Args[i].Y
-				}
-				if bounds.Min.Y > s.Args[i].Y {
-					bounds.Min.Y = s.Args[i].Y
-				}
-			}
-		}
-	}
-
-	if h == font.HintingFull {
-		// Quantize the fixed.Int26_6 value to the nearest pixel.
-		advance = (advance + 32) &^ 63
-		// TODO: hinting of bounds should be handled by LoadGlyph. See TODO
-		// above.
-	}
-
-	return bounds, advance, nil
+	return segments.Bounds(), advance, nil
 }
 
 // GlyphAdvance returns the advance width for the x'th glyph. ppem is the
@@ -1807,7 +1773,7 @@
 	// buf is a byte buffer for when a Font's source is an io.ReaderAt.
 	buf []byte
 	// segments holds glyph vector path segments.
-	segments []Segment
+	segments Segments
 	// compoundStack holds the components of a TrueType compound glyph.
 	compoundStack [maxCompoundStackSize]struct {
 		glyphIndex   GlyphIndex
@@ -1853,6 +1819,47 @@
 	SegmentOpCubeTo
 )
 
+// Segments is a slice of Segment.
+type Segments []Segment
+
+// Bounds returns s' bounding box. It returns an empty rectangle if s is empty.
+func (s Segments) Bounds() (bounds fixed.Rectangle26_6) {
+	if len(s) == 0 {
+		return fixed.Rectangle26_6{}
+	}
+
+	bounds.Min.X = fixed.Int26_6(+(1 << 31) - 1)
+	bounds.Min.Y = fixed.Int26_6(+(1 << 31) - 1)
+	bounds.Max.X = fixed.Int26_6(-(1 << 31) + 0)
+	bounds.Max.Y = fixed.Int26_6(-(1 << 31) + 0)
+
+	for _, seg := range s {
+		n := 1
+		switch seg.Op {
+		case SegmentOpQuadTo:
+			n = 2
+		case SegmentOpCubeTo:
+			n = 3
+		}
+		for i := 0; i < n; i++ {
+			if bounds.Max.X < seg.Args[i].X {
+				bounds.Max.X = seg.Args[i].X
+			}
+			if bounds.Min.X > seg.Args[i].X {
+				bounds.Min.X = seg.Args[i].X
+			}
+			if bounds.Max.Y < seg.Args[i].Y {
+				bounds.Max.Y = seg.Args[i].Y
+			}
+			if bounds.Min.Y > seg.Args[i].Y {
+				bounds.Min.Y = seg.Args[i].Y
+			}
+		}
+	}
+
+	return bounds
+}
+
 // translateArgs applies a translation to args.
 func translateArgs(args *[3]fixed.Point26_6, dx, dy fixed.Int26_6) {
 	args[0].X += dx
diff --git a/font/sfnt/truetype.go b/font/sfnt/truetype.go
index ab27f5b..ffa753f 100644
--- a/font/sfnt/truetype.go
+++ b/font/sfnt/truetype.go
@@ -304,18 +304,18 @@
 			return err
 		}
 		dx, dy := fixed.Int26_6(elem.dx), fixed.Int26_6(elem.dy)
-		segs := b.segments[base:]
+		segments := b.segments[base:]
 		if elem.hasTransform {
 			txx := elem.transformXX
 			txy := elem.transformXY
 			tyx := elem.transformYX
 			tyy := elem.transformYY
-			for j := range segs {
-				transformArgs(&segs[j].Args, txx, txy, tyx, tyy, dx, dy)
+			for j := range segments {
+				transformArgs(&segments[j].Args, txx, txy, tyx, tyy, dx, dy)
 			}
 		} else {
-			for j := range segs {
-				translateArgs(&segs[j].Args, dx, dy)
+			for j := range segments {
+				translateArgs(&segments[j].Args, dx, dy)
 			}
 		}
 	}