From 8c59d4be0c7094459647c818ab8643b85b21b8c1 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Wed, 10 Mar 2021 13:01:18 -0700 Subject: [PATCH 1/2] [subset] Improve PairPosFormat2 subsetting This does two things: 1. Intersect subsetter glyphset with the table's Coverage before passing to ClassDef1 for subsetting. Anything that doesn't get past Coverage wouldn't ever get to ClassDef1, 2. Never reuse class0 of ClassDef2. There's unspoken assumption that ClassDef2's class0 is never used for actual kerning, since that's the unbounded "every other glyph" class. Previously our ClassDef subsetter was reusing class0 if "every other glyph" happened to become empty because of the subset glyphset. Don't do that for PairPos's ClassDef2. As a result of this assumption, don't keep a PairPosClass2 subtable if only ClassDef2's class0 survived subsetting. Would be good to add tests for both. Related to https://github.com/harfbuzz/harfbuzz/issues/2703 --- Lib/fontTools/subset/__init__.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Lib/fontTools/subset/__init__.py b/Lib/fontTools/subset/__init__.py index 6de8b3efd..f687b0568 100644 --- a/Lib/fontTools/subset/__init__.py +++ b/Lib/fontTools/subset/__init__.py @@ -427,13 +427,14 @@ def intersect_class(self, glyphs, klass): if v == klass and g in glyphs) @_add_method(otTables.ClassDef) -def subset(self, glyphs, remap=False): +def subset(self, glyphs, remap=False, useClass0=True): """Returns ascending list of remaining classes.""" self.classDefs = {g:v for g,v in self.classDefs.items() if g in glyphs} # Note: while class 0 has the special meaning of "not matched", # if no glyph will ever /not match/, we can optimize class 0 out too. + # Only do this if allowed. indices = _uniq_sort( - ([0] if any(g not in self.classDefs for g in glyphs) else []) + + ([0] if ((not useClass0) or any(g not in self.classDefs for g in glyphs)) else []) + list(self.classDefs.values())) if remap: self.remap(indices) @@ -569,15 +570,16 @@ def subset_glyphs(self, s): self.PairSetCount = len(self.PairSet) return bool(self.PairSetCount) elif self.Format == 2: - class1_map = [c for c in self.ClassDef1.subset(s.glyphs, remap=True) if c < self.Class1Count] - class2_map = [c for c in self.ClassDef2.subset(s.glyphs, remap=True) if c < self.Class2Count] + class1_map = [c for c in self.ClassDef1.subset(s.glyphs.intersection(self.Coverage.glyphs), remap=True) if c < self.Class1Count] + class2_map = [c for c in self.ClassDef2.subset(s.glyphs, remap=True, useClass0=False) if c < self.Class2Count] self.Class1Record = [self.Class1Record[i] for i in class1_map] for c in self.Class1Record: c.Class2Record = [c.Class2Record[i] for i in class2_map] self.Class1Count = len(class1_map) self.Class2Count = len(class2_map) + # If only Class2 0 left, no need to keep anything. return bool(self.Class1Count and - self.Class2Count and + (self.Class2Count > 1) and self.Coverage.subset(s.glyphs)) else: assert 0, "unknown format: %s" % self.Format From a4f42d3b18370a59ffcb747cca959731d6925674 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 23 Mar 2021 16:00:36 +0000 Subject: [PATCH 2/2] subset_test: add tests for PairPos2 useClass0 #2221 --- ...Pos_Format2_ClassDef1_useClass0.subset.ttx | 62 ++++ ...Pos_Format2_ClassDef2_useClass0.subset.ttx | 17 + .../data/GPOS_PairPos_Format2_PR_2221.ttx | 322 ++++++++++++++++++ Tests/subset/subset_test.py | 31 ++ 4 files changed, 432 insertions(+) create mode 100644 Tests/subset/data/GPOS_PairPos_Format2_ClassDef1_useClass0.subset.ttx create mode 100644 Tests/subset/data/GPOS_PairPos_Format2_ClassDef2_useClass0.subset.ttx create mode 100644 Tests/subset/data/GPOS_PairPos_Format2_PR_2221.ttx diff --git a/Tests/subset/data/GPOS_PairPos_Format2_ClassDef1_useClass0.subset.ttx b/Tests/subset/data/GPOS_PairPos_Format2_ClassDef1_useClass0.subset.ttx new file mode 100644 index 000000000..3df9aa8f8 --- /dev/null +++ b/Tests/subset/data/GPOS_PairPos_Format2_ClassDef1_useClass0.subset.ttx @@ -0,0 +1,62 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/subset/data/GPOS_PairPos_Format2_ClassDef2_useClass0.subset.ttx b/Tests/subset/data/GPOS_PairPos_Format2_ClassDef2_useClass0.subset.ttx new file mode 100644 index 000000000..dc599f139 --- /dev/null +++ b/Tests/subset/data/GPOS_PairPos_Format2_ClassDef2_useClass0.subset.ttx @@ -0,0 +1,17 @@ + + + + + + + + + + + + + + + + + diff --git a/Tests/subset/data/GPOS_PairPos_Format2_PR_2221.ttx b/Tests/subset/data/GPOS_PairPos_Format2_PR_2221.ttx new file mode 100644 index 000000000..d5132d159 --- /dev/null +++ b/Tests/subset/data/GPOS_PairPos_Format2_PR_2221.ttx @@ -0,0 +1,322 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + gpos2_2_font5 + + + Regular + + + gpos2_2_font5 + + + gpos2_2_font5 + + + Version1.0 + + + gpos2_2_font5 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 92 580 rmoveto + 13 6 13 7 14 4 54 16 184 1 9 -81 1 -13 -3 -13 -3 -14 -9 -45 -124 -14 -42 -8 rrcurveto + -2 -2 1 -1 hhcurveto + -2 vlineto + -30 -15 5 -40 35 -4 60 -5 62 -4 47 -43 83 -75 -108 -134 -82 -20 -75 -17 -101 91 -42 -14 -22 -8 -7 -18 10 -21 2 -2 2 -2 1 -2 10 -10 11 -3 10 2 rrcurveto + 2 2 -1 1 hhcurveto + 16 -7 15 -7 15 -7 33 -14 33 -14 35 -7 103 -18 81 94 48 78 51 83 -64 98 -77 36 -4 1 -3 2 -4 2 17 7 16 9 15 12 77 61 -32 107 -79 40 -91 47 -115 -9 -91 -40 rrcurveto + -27 -24 18 -37 36 7 rrcurveto + 408 -580 rmoveto + return + + + 41 642 rmoveto + 1 -2 1 -1 -1 vvcurveto + -7 2 -7 5 -5 vhcurveto + 15 -69 -71 -105 61 -45 71 -50 214 60 48 -116 9 -20 3 -24 -3 -22 -13 -128 -51 -35 -120 -6 -38 -1 -62 -5 -26 34 -29 22 -33 -28 16 -33 39 -51 75 0 59 2 83 5 76 21 49 69 rrcurveto + 25 36 0 48 11 42 19 72 -43 43 -42 45 -62 68 -159 -25 -76 26 -20 43 44 56 -6 66 101 14 102 -5 103 -1 37 7 0 42 -35 11 -109 1 -110 5 -108 -17 rrcurveto + -1 1 0 0 1 vvcurveto + -25 33 -45 -26 18 -38 rrcurveto + 407 -673 rmoveto + return + + + + + + endchar + + + -107 callsubr + -107 callsubr + endchar + + + -107 callsubr + -106 callsubr + endchar + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/subset/subset_test.py b/Tests/subset/subset_test.py index 3f9348b95..6fa1bf608 100644 --- a/Tests/subset/subset_test.py +++ b/Tests/subset/subset_test.py @@ -762,6 +762,37 @@ class SubsetTest(unittest.TestCase): subsetfont = TTFont(subsetpath) self.expect_ttx(subsetfont, self.getpath("CmapSubsetTest.subset.ttx"), ["cmap"]) + def test_GPOS_PairPos_Format2_useClass0(self): + # Check two things related to class 0 ('every other glyph'): + # 1) that it's reused for ClassDef1 when it becomes empty as the subset glyphset + # is intersected with the table's Coverage + # 2) that it is never reused for ClassDef2 even when it happens to become empty + # because of the subset glyphset. In this case, we don't keep a PairPosClass2 + # subtable if only ClassDef2's class0 survived subsetting. + # The test font (from Harfbuzz test suite) is constructed to trigger these two + # situations depending on the input subset --text. + # https://github.com/fonttools/fonttools/pull/2221 + _, fontpath = self.compile_font( + self.getpath("GPOS_PairPos_Format2_PR_2221.ttx"), ".ttf" + ) + subsetpath = self.temp_path(".ttf") + + for n, text in enumerate("!#", start=1): + expected_ttx = self.getpath( + f"GPOS_PairPos_Format2_ClassDef{n}_useClass0.subset.ttx" + ) + with self.subTest(text=text, expected_ttx=expected_ttx): + subset.main( + [ + fontpath, + f"--text='{text}'", + "--layout-features+=test", + "--output-file=%s" % subsetpath, + ] + ) + subsetfont = TTFont(subsetpath) + self.expect_ttx(subsetfont, expected_ttx, ["GPOS"]) + @pytest.fixture def featureVarsTestFont():