[fontscan] Better family substitutions

fontscan/fontmap.go

@@ -325,30 +325,27 @@ func (fm *FontMap) SetQuery(query Query) {
 }
 
 // candidates is a cache storing the indices into FontMap.database of footprints matching a Query
+// families
 type candidates struct {
-	// the two fallback slices have the same length: the number of family in the query
-	withFallback    [][]int // for each queried family
-	withoutFallback []int   // for each queried family, only one footprint is selected
+	// footprints with exact match :
+	// for each queried family, at most one footprint is selected
+	withoutFallback []int
+
+	// footprints matching the expanded query (where subsitutions have been applied)
+	withFallback []int
 
 	manual []int // manually inserted faces to be tried if the other candidates fail.
 }
 
-func (cd *candidates) resetWithSize(candidateSize int) {
-	if cap(cd.withFallback) < candidateSize { // reallocate
-		cd.withFallback = make([][]int, candidateSize)
-		cd.withoutFallback = make([]int, candidateSize)
+// reset slices, setting the capacity of withoutFallback to nbFamilies
+func (cd *candidates) resetWithSize(nbFamilies int) {
+	if cap(cd.withoutFallback) < nbFamilies { // reallocate
+		cd.withoutFallback = make([]int, nbFamilies)
 	}
 
-	// only reslice
-	cd.withFallback = cd.withFallback[0:candidateSize]
-	cd.withoutFallback = cd.withoutFallback[0:candidateSize]
-
-	// reset to "zero" values
-	for i := range cd.withoutFallback {
-		cd.withFallback[i] = nil
-		cd.withoutFallback[i] = -1
-	}
-	cd.manual = cd.manual[0:]
+	cd.withoutFallback = cd.withoutFallback[:0]
+	cd.withFallback = cd.withFallback[:0]
+	cd.manual = cd.manual[:0]
 }
 
 func (fm *FontMap) buildCandidates() {
@@ -357,39 +354,48 @@ func (fm *FontMap) buildCandidates() {
 	}
 	fm.candidates.resetWithSize(len(fm.query.Families))
 
-	selectFootprints := func(systemFallback bool) {
-		for familyIndex, family := range fm.query.Families {
-			candidates := fm.database.selectByFamily(family, systemFallback, &fm.footprintsBuffer, fm.cribleBuffer)
+	// first pass for an exact match
+	{
+		for _, family := range fm.query.Families {
+			candidates := fm.database.selectByFamilyExact(family, &fm.footprintsBuffer, fm.cribleBuffer)
 			if len(candidates) == 0 {
 				continue
 			}
 
 			// select the correct aspects
 			candidates = fm.database.retainsBestMatches(candidates, fm.query.Aspect)
 
-			if systemFallback {
-				// candidates is owned by fm.footprintsBuffer: copy its content
-				S := fm.candidates.withFallback[familyIndex]
-				if L := len(candidates); cap(S) < L {
-					S = make([]int, L)
-				} else {
-					S = S[:L]
-				}
-				copy(S, candidates)
-				fm.candidates.withFallback[familyIndex] = S
-			} else {
-				// when no systemFallback is required, the CSS spec says
-				// that only one font among the candidates must be tried
-				fm.candidates.withoutFallback[familyIndex] = candidates[0]
-			}
+			// when no systemFallback is required, the CSS spec says
+			// that only one font among the candidates must be tried
+			fm.candidates.withoutFallback = append(fm.candidates.withoutFallback, candidates[0])
+		}
+	}
+
+	// second pass with substitutions
+	{
+		candidates := fm.database.selectByFamilyWithSubs(fm.query.Families, &fm.footprintsBuffer, fm.cribleBuffer)
+
+		// select the correct aspects
+		candidates = fm.database.retainsBestMatches(candidates, fm.query.Aspect)
+
+		// candidates is owned by fm.footprintsBuffer: copy its content
+		S := fm.candidates.withFallback
+		if L := len(candidates); cap(S) < L {
+			S = make([]int, L)
+		} else {
+			S = S[:L]
 		}
+		copy(S, candidates)
+		fm.candidates.withFallback = S
 	}
 
-	selectFootprints(false)
-	selectFootprints(true)
+	// third pass with user provided fonts
+	{
+		fm.candidates.manual = fm.database.filterUserProvided(fm.candidates.manual)
+		fm.candidates.manual = fm.database.retainsBestMatches(fm.candidates.manual, fm.query.Aspect)
+
+	}
 
-	fm.candidates.manual = fm.database.filterUserProvided(fm.candidates.manual)
-	fm.candidates.manual = fm.database.retainsBestMatches(fm.candidates.manual, fm.query.Aspect)
 	fm.built = true
 }
 
@@ -451,35 +457,25 @@ func (fm *FontMap) ResolveFace(r rune) (face font.Face) {
 	defer func() {
 		fm.lru.Put(key, fm.query, face)
 	}()
+
 	// Build the candidates if we missed the cache. If they're already built this is a
 	// no-op.
 	fm.buildCandidates()
+
 	// we first look up for an exact family match, without substitutions
-	for _, footprintIndex := range fm.candidates.withoutFallback {
-		if footprintIndex == -1 {
-			continue
-		}
-		if face := fm.resolveForRune([]int{footprintIndex}, r); face != nil {
-			return face
-		}
+	if face := fm.resolveForRune(fm.candidates.withoutFallback, r); face != nil {
+		return face
 	}
 
 	// if no family has matched so far, try again with system fallback
-	for _, footprintIndexList := range fm.candidates.withFallback {
-		if face := fm.resolveForRune(footprintIndexList, r); face != nil {
-			return face
-		}
+	if face := fm.resolveForRune(fm.candidates.withFallback, r); face != nil {
+		return face
 	}
 
 	// try manually loaded faces even if the typeface doesn't match, looking for matching aspects
 	// and rune coverage.
-	for _, footprintIndex := range fm.candidates.manual {
-		if footprintIndex == -1 {
-			continue
-		}
-		if face := fm.resolveForRune([]int{footprintIndex}, r); face != nil {
-			return face
-		}
+	if face := fm.resolveForRune(fm.candidates.manual, r); face != nil {
+		return face
 	}
 
 	fm.logger.Printf("No font matched for %q and rune %U (%c) -> searching by script coverage and aspect", fm.query.Families, r, r)
@@ -544,31 +540,19 @@ func (fm *FontMap) ResolveFaceForLang(lang LangID) font.Face {
 	fm.buildCandidates()
 
 	// we first look up for an exact family match, without substitutions
-	for _, footprintIndex := range fm.candidates.withoutFallback {
-		if footprintIndex == -1 {
-			continue
-		}
-		if face := fm.resolveForLang([]int{footprintIndex}, lang); face != nil {
-			return face
-		}
+	if face := fm.resolveForLang(fm.candidates.withoutFallback, lang); face != nil {
+		return face
 	}
 
 	// if no family has matched so far, try again with system fallback
-	for _, footprintIndexList := range fm.candidates.withFallback {
-		if face := fm.resolveForLang(footprintIndexList, lang); face != nil {
-			return face
-		}
+	if face := fm.resolveForLang(fm.candidates.withFallback, lang); face != nil {
+		return face
 	}
 
 	// try manually loaded faces even if the typeface doesn't match, looking for matching aspects
 	// and rune coverage.
-	for _, footprintIndex := range fm.candidates.manual {
-		if footprintIndex == -1 {
-			continue
-		}
-		if face := fm.resolveForLang([]int{footprintIndex}, lang); face != nil {
-			return face
-		}
+	if face := fm.resolveForLang(fm.candidates.manual, lang); face != nil {
+		return face
 	}
 
 	return nil

fontscan/fontmap_test.go

@@ -260,3 +260,26 @@ func TestFontMap_AddFont_FaceLocation(t *testing.T) {
 	face := fm.ResolveFace(0x20)
 	tu.Assert(t, fm.FontLocation(face.Font).File == "Roboto2")
 }
+
+func TestQueryHelveticaLinux(t *testing.T) {
+	// This is a regression test which asserts that
+	// our behavior is similar than fontconfig
+
+	file1, err := os.Open("../font/testdata/Amiri-Regular.ttf")
+	tu.AssertNoErr(t, err)
+	defer file1.Close()
+
+	fm := NewFontMap(nil)
+	err = fm.AddFont(file1, "file1", "Nimbus Sans")
+	tu.AssertNoErr(t, err)
+
+	err = fm.AddFont(file1, "file2", "Bitstream Vera Sans")
+	tu.AssertNoErr(t, err)
+
+	fm.SetQuery(Query{Families: []string{
+		"BlinkMacSystemFont", // 'unknown' family
+		"Helvetica",
+	}})
+	family, _ := fm.FontMetadata(fm.ResolveFace('x').Font)
+	tu.Assert(t, family == meta.NormalizeFamily("Nimbus Sans")) // prefered Helvetica replacement
+}

fontscan/footprint.go

@@ -23,6 +23,8 @@ type footprint struct {
 
 	// Family is the general nature of the font, like
 	// "Arial"
+	// Note that, for performance reason, we store the
+	// normalized version of the family name.
 	Family string
 
 	// Runes is the set of runes supported by the font.

fontscan/match.go

@@ -40,7 +40,11 @@ func (fc familyCrible) reset() {
 // and applies all the substitutions coded in the package
 // to add substitutes values
 func (fc familyCrible) fillWithSubstitutions(family string) {
-	fl := newFamilyList([]string{family})
+	fc.fillWithSubstitutionsList([]string{family})
+}
+
+func (fc familyCrible) fillWithSubstitutionsList(families []string) {
+	fl := newFamilyList(families)
 	for _, subs := range familySubstitution {
 		fl.execute(subs)
 	}
@@ -104,41 +108,59 @@ func isGenericFamily(family string) bool {
 	}
 }
 
-// selectByFamily returns all the fonts in the fontmap matching
+// selectByFamilyExact returns all the fonts in the fontmap matching
 // the given `family`, with the best matches coming first.
-// `substitute` controls whether or not system substitutions are applied.
-// The generic families are always expanded to concrete families.
+//
+// The match is performed without substituting family names,
+// expect for the generic families, which are always expanded to concrete families.
 //
 // If two fonts have the same family, user provided are returned first.
 //
 // The returned slice may be empty if no font matches the given `family`.
-// buffer is used to reduce allocations
-func (fm fontSet) selectByFamily(family string, substitute bool,
+//
+// The two buffers are used to reduce allocations.
+func (fm fontSet) selectByFamilyExact(family string,
 	footprintBuffer *scoredFootprints, cribleBuffer familyCrible,
 ) []int {
 	// build the crible, handling substitutions
-	family = meta.NormalizeFamily(family)
-
-	footprintBuffer.reset(fm)
 	cribleBuffer.reset()
 
 	// always substitute generic families
-	if substitute || isGenericFamily(family) {
+	if isGenericFamily(family) {
 		cribleBuffer.fillWithSubstitutions(family)
 	} else {
-		cribleBuffer = familyCrible{family: 0}
+		cribleBuffer = familyCrible{meta.NormalizeFamily(family): 0}
 	}
 
-	// select the matching fonts:
+	return fm.selectByFamilies(cribleBuffer, footprintBuffer)
+}
+
+// selectByFamilyExact returns all the fonts in the fontmap matching
+// the given query, with the best matches coming first.
+//
+// `query` is expanded with family substitutions
+func (fm fontSet) selectByFamilyWithSubs(query []string,
+	footprintBuffer *scoredFootprints, cribleBuffer familyCrible,
+) []int {
+	cribleBuffer.reset()
+	cribleBuffer.fillWithSubstitutionsList(query)
+	return fm.selectByFamilies(cribleBuffer, footprintBuffer)
+}
+
+// select the fonts in the fontSet matching [crible], returning their indices.
+// footprintBuffer is used to reduce allocations.
+func (fm fontSet) selectByFamilies(crible familyCrible, footprintBuffer *scoredFootprints) []int {
+	footprintBuffer.reset(fm)
+
 	// loop through `footprints` and stores the matching fonts into `dst`
 	for index, footprint := range fm {
-		if score, has := cribleBuffer[footprint.Family]; has {
+		if score, has := crible[footprint.Family]; has {
 			footprintBuffer.footprints = append(footprintBuffer.footprints, index)
 			footprintBuffer.scores = append(footprintBuffer.scores, score)
 		}
 	}
 
-	// sort the matched font by score (lower is better)
+	// sort the matched fonts by score (lower is better)
 	sort.Stable(*footprintBuffer)
 
 	return footprintBuffer.footprints

fontscan/match_test.go

@@ -52,7 +52,7 @@ func fontsFromFamilies(families ...string) (out fontSet) {
 	return out
 }
 
-func TestFontMap_selectByFamily(t *testing.T) {
+func TestFontMap_selectByFamilyExact(t *testing.T) {
 	tests := []struct {
 		fontset    fontSet
 		family     string
@@ -68,22 +68,8 @@ func TestFontMap_selectByFamily(t *testing.T) {
 		{fontsFromFamilies("ar Ial", "emoji"), "Arial", false, []int{0}},
 		// substitution
 		{fontsFromFamilies("arial"), "Helvetica", false, nil},
-		{fontsFromFamilies("arial"), "Helvetica", true, []int{0}},
-		{fontsFromFamilies("caladea", "XXX"), "cambria", true, []int{0}},
-		// substitution, with order
-		{fontsFromFamilies("arial", "Helvetica"), "Helvetica", true, []int{1, 0}},
-		// substitution, with order, and no matching fonts
-		{fontsFromFamilies("arial", "Helvetica", "XXX"), "Helvetica", true, []int{1, 0}},
 		// generic families
 		{fontsFromFamilies("norasi", "XXX"), "serif", false, []int{0}},
-		// default to generic families
-		{fontsFromFamilies("DEjaVuSerif", "XXX"), "cambria", true, []int{0}},
-		// substitutions
-		{
-			fontsFromFamilies("Nimbus Roman", "Tinos", "Liberation Serif", "DejaVu Serif", "arial"),
-			"Times", true,
-			[]int{0, 1, 2, 3},
-		},
 		// user provided precedence
 		{
 			fontSet{
@@ -97,16 +83,39 @@ func TestFontMap_selectByFamily(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
-		if got := tt.fontset.selectByFamily(tt.family, tt.substitute, &scoredFootprints{}, make(familyCrible)); !reflect.DeepEqual(got, tt.want) {
+		if got := tt.fontset.selectByFamilyExact(tt.family, &scoredFootprints{}, make(familyCrible)); !reflect.DeepEqual(got, tt.want) {
 			t.Errorf("FontMap.selectByFamily() = \n%v, want \n%v", got, tt.want)
 		}
 	}
 }
 
-func BenchmarkNewFamilyCrible(b *testing.B) {
-	c := make(familyCrible)
-	for i := 0; i < b.N; i++ {
-		c.fillWithSubstitutions("Arial")
+func TestFontMap_selectByFamilyList(t *testing.T) {
+	tests := []struct {
+		fontset    fontSet
+		family     string
+		substitute bool
+		want       []int
+	}{
+		{nil, "", true, nil}, // no match on empty fontset
+		{fontsFromFamilies("arial"), "Helvetica", true, []int{0}},
+		{fontsFromFamilies("caladea", "XXX"), "cambria", true, []int{0}},
+		// substitution, with order
+		{fontsFromFamilies("arial", "Helvetica"), "Helvetica", true, []int{1, 0}},
+		// substitution, with order, and no matching fonts
+		{fontsFromFamilies("arial", "Helvetica", "XXX"), "Helvetica", true, []int{1, 0}},
+		// default to generic families
+		{fontsFromFamilies("DEjaVuSerif", "XXX"), "cambria", true, []int{0}},
+		// substitutions
+		{
+			fontsFromFamilies("Nimbus Roman", "Tinos", "Liberation Serif", "DejaVu Serif", "arial"),
+			"Times", true,
+			[]int{0, 1, 2, 3},
+		},
+	}
+	for _, tt := range tests {
+		if got := tt.fontset.selectByFamilyWithSubs([]string{tt.family}, &scoredFootprints{}, make(familyCrible)); !reflect.DeepEqual(got, tt.want) {
+			t.Errorf("FontMap.selectByFamily() = \n%v, want \n%v", got, tt.want)
+		}
 	}
 }