From 32874cb3723b86ba2bdb67ddcd474ddb52943a89 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 2 Dec 2021 18:01:24 +0000 Subject: [PATCH 1/4] subset/COLR: add reproducer for issue #2461 this currently fails with struct.error. Fix will ensue shortly --- Tests/subset/data/BungeeColor-Regular.ttx | 438 ++++++++++++++++++++++ Tests/subset/subset_test.py | 21 ++ 2 files changed, 459 insertions(+) create mode 100644 Tests/subset/data/BungeeColor-Regular.ttx diff --git a/Tests/subset/data/BungeeColor-Regular.ttx b/Tests/subset/data/BungeeColor-Regular.ttx new file mode 100644 index 000000000..d14c89224 --- /dev/null +++ b/Tests/subset/data/BungeeColor-Regular.ttx @@ -0,0 +1,438 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Copyright 2008 The Bungee Project Authors (david@djr.com) + + + Bungee Color Regular + + + Regular + + + 1.000;djr ;BungeeColor-Regular + + + Bungee Color Regular Regular + + + Version 1.000;PS 1.0;hotconv 1.0.72;makeotf.lib2.5.5900 + + + BungeeColor-Regular + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Tests/subset/subset_test.py b/Tests/subset/subset_test.py index d8f4dac0c..d1afb296f 100644 --- a/Tests/subset/subset_test.py +++ b/Tests/subset/subset_test.py @@ -1352,5 +1352,26 @@ def test_subset_svg_missing_lxml(ttf_path): subset.main([str(ttf_path), "--gids=0,1"]) +def test_subset_COLR_glyph_closure(tmp_path): + font = TTFont() + ttx = pathlib.Path(__file__).parent / "data" / "BungeeColor-Regular.ttx" + font.importXML(ttx) + font_path = tmp_path / "BungeeColor-Regular.ttf" + subset_path = font_path.with_suffix(".subset.ttf)") + font.save(font_path) + + subset.main( + [ + str(font_path), + "--glyph-names", + f"--output-file={subset_path}", + "--glyphs=Agrave", + ] + ) + subset_font = TTFont(subset_path) + # TODO + + + if __name__ == "__main__": sys.exit(unittest.main()) From 780f2428b60e33715bc6384d329c98bdffaa4fb1 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 2 Dec 2021 18:12:04 +0000 Subject: [PATCH 2/4] subset: when subsetting COLR only include glyphs after COLR closure, excluding glyf closure Fixes #2461 --- Lib/fontTools/subset/__init__.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/fontTools/subset/__init__.py b/Lib/fontTools/subset/__init__.py index 8fb732f34..94fdc7b7d 100644 --- a/Lib/fontTools/subset/__init__.py +++ b/Lib/fontTools/subset/__init__.py @@ -2064,6 +2064,20 @@ def subset_glyphs(self, s): from fontTools.colorLib.unbuilder import unbuildColrV1 from fontTools.colorLib.builder import buildColrV1, populateCOLRv0 + # only include glyphs after COLR closure, which in turn comes after cmap and GSUB + # closure, but importantly before glyf/CFF closures. COLR layers can refer to + # composite glyphs, and that's ok, since glyf/CFF closures happen after COLR closure + # and take care of those. If we also included glyphs resulting from glyf/CFF closures + # when deciding which COLR base glyphs to retain, then we may end up with a situation + # whereby a COLR base glyph is kept, not because directly requested (cmap) + # or substituted (GSUB) or referenced by another COLRv1 PaintColrGlyph, but because + # it corresponds to (has same GID as) a non-COLR glyph that happens to be used as a + # component in glyf or CFF table. Best case scenario we retain more glyphs than + # required; worst case we retain incomplete COLR records that try to reference + # glyphs that are no longer in the final subset font. + # https://github.com/fonttools/fonttools/issues/2461 + s.glyphs = s.glyphs_colred + self.ColorLayers = {g: self.ColorLayers[g] for g in s.glyphs if g in self.ColorLayers} if self.version == 0: return bool(self.ColorLayers) @@ -2776,6 +2790,7 @@ class Subsetter(object): log.info("Closed glyph list over '%s': %d glyphs after", table, len(self.glyphs)) log.glyphs(self.glyphs, font=font) + setattr(self, f"glyphs_{table.lower()}ed", frozenset(self.glyphs)) if 'glyf' in font: with timer("close glyph list over 'glyf'"): From d89f90f46cffca0ffd760e8bc432d2dc4dbeba0d Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 2 Dec 2021 18:18:09 +0000 Subject: [PATCH 3/4] subset_test: test COLR subset excludes glyphs resulting from glyf closure --- Tests/subset/subset_test.py | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/Tests/subset/subset_test.py b/Tests/subset/subset_test.py index d1afb296f..9011885e8 100644 --- a/Tests/subset/subset_test.py +++ b/Tests/subset/subset_test.py @@ -1353,9 +1353,17 @@ def test_subset_svg_missing_lxml(ttf_path): def test_subset_COLR_glyph_closure(tmp_path): + # https://github.com/fonttools/fonttools/issues/2461 font = TTFont() ttx = pathlib.Path(__file__).parent / "data" / "BungeeColor-Regular.ttx" font.importXML(ttx) + + + color_layers = font["COLR"].ColorLayers + assert ".notdef" in color_layers + assert "Agrave" in color_layers + assert "grave" in color_layers + font_path = tmp_path / "BungeeColor-Regular.ttf" subset_path = font_path.with_suffix(".subset.ttf)") font.save(font_path) @@ -1369,7 +1377,28 @@ def test_subset_COLR_glyph_closure(tmp_path): ] ) subset_font = TTFont(subset_path) - # TODO + + glyph_order = subset_font.getGlyphOrder() + + assert glyph_order == [ + ".notdef", # '.notdef' is always included automatically + "A", + "grave", # included because glyf component of Agrave + "Agrave", + ".notdef.alt001", + ".notdef.alt002", + "A.alt002", + "Agrave.alt001", + "Agrave.alt002", + "grave.alt002", + ] + + color_layers = subset_font["COLR"].ColorLayers + assert ".notdef" in color_layers + assert "Agrave" in color_layers + # 'grave' not included in COLR because absent after COLR glyph closure; + # only added in subset font after subsequent glyf closure. + assert "grave" not in color_layers From de3830ba2b1fa4283343e9b7a73badb9d1afd596 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 3 Dec 2021 11:16:59 +0000 Subject: [PATCH 4/4] clarify comment a bit as per review --- Tests/subset/subset_test.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Tests/subset/subset_test.py b/Tests/subset/subset_test.py index 9011885e8..d195af0fe 100644 --- a/Tests/subset/subset_test.py +++ b/Tests/subset/subset_test.py @@ -1358,7 +1358,6 @@ def test_subset_COLR_glyph_closure(tmp_path): ttx = pathlib.Path(__file__).parent / "data" / "BungeeColor-Regular.ttx" font.importXML(ttx) - color_layers = font["COLR"].ColorLayers assert ".notdef" in color_layers assert "Agrave" in color_layers @@ -1383,7 +1382,7 @@ def test_subset_COLR_glyph_closure(tmp_path): assert glyph_order == [ ".notdef", # '.notdef' is always included automatically "A", - "grave", # included because glyf component of Agrave + "grave", "Agrave", ".notdef.alt001", ".notdef.alt002", @@ -1396,8 +1395,8 @@ def test_subset_COLR_glyph_closure(tmp_path): color_layers = subset_font["COLR"].ColorLayers assert ".notdef" in color_layers assert "Agrave" in color_layers - # 'grave' not included in COLR because absent after COLR glyph closure; - # only added in subset font after subsequent glyf closure. + # Agrave 'glyf' uses grave. It should be retained in 'glyf' but NOT in + # COLR when we subset down to Agrave. assert "grave" not in color_layers