font: have Glyph return !ok for U+FFFD substitute

The other return values may still be non-zero, but this lets callers
identify when substitution happens.

"TODO: is falling back on the U+FFFD glyph the responsibility of the
Drawer or the Face?" was resolved. The answer is "the Face". For
kerning, the previous rune is unchanged (and not set to U+FFFD).

This also fixes an inconsistency in the basicfont.Face implementation,
where GlyphAdvance and GlyphBounds would unconditionally return a
non-zero advance, but Glyph could return a zero advance when the Face
doesn't have a U+FFFD entry.

Fixes golang/go#58252

Change-Id: Ie97e68e1d5e2efd13c9e84ad12db4495d83a5ca3
Reviewed-on: https://go-review.googlesource.com/c/image/+/474376
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Reviewed-by: Nigel Tao (INACTIVE; USE @golang.org INSTEAD) <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Nigel Tao <[email protected]>

Author: nigeltao

font/basicfont/basicfont.go

@@ -89,41 +89,50 @@ func (f *Face) Metrics() font.Metrics {
 func (f *Face) Glyph(dot fixed.Point26_6, r rune) (
 	dr image.Rectangle, mask image.Image, maskp image.Point, advance fixed.Int26_6, ok bool) {
 
-loop:
-	for _, rr := range [2]rune{r, '\ufffd'} {
-		for _, rng := range f.Ranges {
-			if rr < rng.Low || rng.High <= rr {
-				continue
-			}
-			maskp.Y = (int(rr-rng.Low) + rng.Offset) * (f.Ascent + f.Descent)
-			ok = true
-			break loop
+	if found, rng := f.find(r); rng != nil {
+		maskp.Y = (int(found-rng.Low) + rng.Offset) * (f.Ascent + f.Descent)
+		x := int(dot.X+32)>>6 + f.Left
+		y := int(dot.Y+32) >> 6
+		dr = image.Rectangle{
+			Min: image.Point{
+				X: x,
+				Y: y - f.Ascent,
+			},
+			Max: image.Point{
+				X: x + f.Width,
+				Y: y + f.Descent,
+			},
 		}
-	}
-	if !ok {
-		return image.Rectangle{}, nil, image.Point{}, 0, false
-	}
 
-	x := int(dot.X+32)>>6 + f.Left
-	y := int(dot.Y+32) >> 6
-	dr = image.Rectangle{
-		Min: image.Point{
-			X: x,
-			Y: y - f.Ascent,
-		},
-		Max: image.Point{
-			X: x + f.Width,
-			Y: y + f.Descent,
-		},
+		return dr, f.Mask, maskp, fixed.I(f.Advance), r == found
 	}
-
-	return dr, f.Mask, maskp, fixed.I(f.Advance), true
+	return image.Rectangle{}, nil, image.Point{}, 0, false
 }
 
 func (f *Face) GlyphBounds(r rune) (bounds fixed.Rectangle26_6, advance fixed.Int26_6, ok bool) {
-	return fixed.R(0, -f.Ascent, f.Width, +f.Descent), fixed.I(f.Advance), true
+	if found, rng := f.find(r); rng != nil {
+		return fixed.R(0, -f.Ascent, f.Width, +f.Descent), fixed.I(f.Advance), r == found
+	}
+	return fixed.Rectangle26_6{}, 0, false
 }
 
 func (f *Face) GlyphAdvance(r rune) (advance fixed.Int26_6, ok bool) {
-	return fixed.I(f.Advance), true
+	if found, rng := f.find(r); rng != nil {
+		return fixed.I(f.Advance), r == found
+	}
+	return 0, false
+}
+
+func (f *Face) find(r rune) (rune, *Range) {
+	for {
+		for i, rng := range f.Ranges {
+			if (rng.Low <= r) && (r < rng.High) {
+				return r, &f.Ranges[i]
+			}
+		}
+		if r == '\ufffd' {
+			return 0, nil
+		}
+		r = '\ufffd'
+	}
 }

font/font.go

@@ -38,7 +38,10 @@ type Face interface {
 	// glyph at the sub-pixel destination location dot, and that glyph's
 	// advance width.
 	//
-	// It returns !ok if the face does not contain a glyph for r.
+	// It returns !ok if the face does not contain a glyph for r. This includes
+	// returning !ok for a fallback glyph (such as substituting a U+FFFD glyph
+	// or OpenType's .notdef glyph), in which case the other return values may
+	// still be non-zero.
 	//
 	// The contents of the mask image returned by one Glyph call may change
 	// after the next Glyph call. Callers that want to cache the mask must make
@@ -49,7 +52,10 @@ type Face interface {
 	// GlyphBounds returns the bounding box of r's glyph, drawn at a dot equal
 	// to the origin, and that glyph's advance width.
 	//
-	// It returns !ok if the face does not contain a glyph for r.
+	// It returns !ok if the face does not contain a glyph for r. This includes
+	// returning !ok for a fallback glyph (such as substituting a U+FFFD glyph
+	// or OpenType's .notdef glyph), in which case the other return values may
+	// still be non-zero.
 	//
 	// The glyph's ascent and descent are equal to -bounds.Min.Y and
 	// +bounds.Max.Y. The glyph's left-side and right-side bearings are equal
@@ -60,7 +66,10 @@ type Face interface {
 
 	// GlyphAdvance returns the advance width of r's glyph.
 	//
-	// It returns !ok if the face does not contain a glyph for r.
+	// It returns !ok if the face does not contain a glyph for r. This includes
+	// returning !ok for a fallback glyph (such as substituting a U+FFFD glyph
+	// or OpenType's .notdef glyph), in which case the other return values may
+	// still be non-zero.
 	GlyphAdvance(r rune) (advance fixed.Int26_6, ok bool)
 
 	// Kern returns the horizontal adjustment for the kerning pair (r0, r1). A
@@ -150,14 +159,10 @@ func (d *Drawer) DrawBytes(s []byte) {
 		if prevC >= 0 {
 			d.Dot.X += d.Face.Kern(prevC, c)
 		}
-		dr, mask, maskp, advance, ok := d.Face.Glyph(d.Dot, c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
+		dr, mask, maskp, advance, _ := d.Face.Glyph(d.Dot, c)
+		if !dr.Empty() {
+			draw.DrawMask(d.Dst, dr, d.Src, image.Point{}, mask, maskp, draw.Over)
 		}
-		draw.DrawMask(d.Dst, dr, d.Src, image.Point{}, mask, maskp, draw.Over)
 		d.Dot.X += advance
 		prevC = c
 	}
@@ -170,14 +175,10 @@ func (d *Drawer) DrawString(s string) {
 		if prevC >= 0 {
 			d.Dot.X += d.Face.Kern(prevC, c)
 		}
-		dr, mask, maskp, advance, ok := d.Face.Glyph(d.Dot, c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
+		dr, mask, maskp, advance, _ := d.Face.Glyph(d.Dot, c)
+		if !dr.Empty() {
+			draw.DrawMask(d.Dst, dr, d.Src, image.Point{}, mask, maskp, draw.Over)
 		}
-		draw.DrawMask(d.Dst, dr, d.Src, image.Point{}, mask, maskp, draw.Over)
 		d.Dot.X += advance
 		prevC = c
 	}
@@ -227,16 +228,12 @@ func BoundBytes(f Face, s []byte) (bounds fixed.Rectangle26_6, advance fixed.Int
 		if prevC >= 0 {
 			advance += f.Kern(prevC, c)
 		}
-		b, a, ok := f.GlyphBounds(c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
+		b, a, _ := f.GlyphBounds(c)
+		if !b.Empty() {
+			b.Min.X += advance
+			b.Max.X += advance
+			bounds = bounds.Union(b)
 		}
-		b.Min.X += advance
-		b.Max.X += advance
-		bounds = bounds.Union(b)
 		advance += a
 		prevC = c
 	}
@@ -251,16 +248,12 @@ func BoundString(f Face, s string) (bounds fixed.Rectangle26_6, advance fixed.In
 		if prevC >= 0 {
 			advance += f.Kern(prevC, c)
 		}
-		b, a, ok := f.GlyphBounds(c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
+		b, a, _ := f.GlyphBounds(c)
+		if !b.Empty() {
+			b.Min.X += advance
+			b.Max.X += advance
+			bounds = bounds.Union(b)
 		}
-		b.Min.X += advance
-		b.Max.X += advance
-		bounds = bounds.Union(b)
 		advance += a
 		prevC = c
 	}
@@ -278,13 +271,7 @@ func MeasureBytes(f Face, s []byte) (advance fixed.Int26_6) {
 		if prevC >= 0 {
 			advance += f.Kern(prevC, c)
 		}
-		a, ok := f.GlyphAdvance(c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
-		}
+		a, _ := f.GlyphAdvance(c)
 		advance += a
 		prevC = c
 	}
@@ -298,13 +285,7 @@ func MeasureString(f Face, s string) (advance fixed.Int26_6) {
 		if prevC >= 0 {
 			advance += f.Kern(prevC, c)
 		}
-		a, ok := f.GlyphAdvance(c)
-		if !ok {
-			// TODO: is falling back on the U+FFFD glyph the responsibility of
-			// the Drawer or the Face?
-			// TODO: set prevC = '\ufffd'?
-			continue
-		}
+		a, _ := f.GlyphAdvance(c)
 		advance += a
 		prevC = c
 	}

font/opentype/opentype.go

@@ -133,8 +133,8 @@ func (f *Face) Metrics() font.Metrics {
 
 // Kern satisfies the font.Face interface.
 func (f *Face) Kern(r0, r1 rune) fixed.Int26_6 {
-	x0 := f.index(r0)
-	x1 := f.index(r1)
+	x0, _ := f.f.GlyphIndex(&f.buf, r0)
+	x1, _ := f.f.GlyphIndex(&f.buf, r1)
 	k, err := f.f.Kern(&f.buf, x0, x1, fixed.Int26_6(f.f.UnitsPerEm()), f.hinting)
 	if err != nil {
 		return 0
@@ -251,22 +251,19 @@ func (f *Face) Glyph(dot fixed.Point26_6, r rune) (dr image.Rectangle, mask imag
 	}
 	f.rast.Draw(&f.mask, f.mask.Bounds(), image.Opaque, image.Point{})
 
-	return dr, &f.mask, f.mask.Rect.Min, advance, true
+	return dr, &f.mask, f.mask.Rect.Min, advance, x != 0
 }
 
 // GlyphBounds satisfies the font.Face interface.
 func (f *Face) GlyphBounds(r rune) (bounds fixed.Rectangle26_6, advance fixed.Int26_6, ok bool) {
-	bounds, advance, err := f.f.GlyphBounds(&f.buf, f.index(r), f.scale, f.hinting)
-	return bounds, advance, err == nil
+	x, _ := f.f.GlyphIndex(&f.buf, r)
+	bounds, advance, err := f.f.GlyphBounds(&f.buf, x, f.scale, f.hinting)
+	return bounds, advance, (err == nil) && (x != 0)
 }
 
 // GlyphAdvance satisfies the font.Face interface.
 func (f *Face) GlyphAdvance(r rune) (advance fixed.Int26_6, ok bool) {
-	advance, err := f.f.GlyphAdvance(&f.buf, f.index(r), f.scale, f.hinting)
-	return advance, err == nil
-}
-
-func (f *Face) index(r rune) sfnt.GlyphIndex {
 	x, _ := f.f.GlyphIndex(&f.buf, r)
-	return x
+	advance, err := f.f.GlyphAdvance(&f.buf, x, f.scale, f.hinting)
+	return advance, (err == nil) && (x != 0)
 }