From adc5b2997eaebbae7b9de6952351d9563ac7aa6d Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 24 Oct 2019 12:51:48 +0100 Subject: [PATCH 1/5] [otBase|otTables] enforce VarStore RegionAxisCount == fvar.AxisCount even when there are no Regions and thus we can't take the length of VarRegionAxis array. This is to appease older versions of OTS which blindly enforce this rule and reject a VF that has, e.g., an empty HVAR table with no regions if the HVAR.VarStore.VarRegionList.RegionAxisCount != fvar.AxisCount. Fixes https://github.com/fonttools/fonttools/issues/1670 Related: https://github.com/fonttools/fonttools/pull/1671 https://github.com/googlefonts/fontmake/issues/565 https://github.com/khaledhosny/ots/pull/192 --- Lib/fontTools/ttLib/tables/otBase.py | 13 +++++++++++-- Lib/fontTools/ttLib/tables/otTables.py | 18 +++++++++++++++++- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index 521a98ebd..f4a39e5f3 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -498,6 +498,11 @@ class OTTableWriter(object): return OverflowErrorRecord( (self.tableTag, LookupListIndex, SubTableIndex, itemName, itemIndex) ) +class LiteralCount(int): + """A count value that should be taken literally, rather than recomputed on compile.""" + pass + + class CountReference(object): """A reference to a Count value, not a count of references.""" def __init__(self, table, name, size=None, value=None): @@ -690,8 +695,12 @@ class BaseTable(object): # table. We will later store it here. # We add a reference: by the time the data is assembled # the Count value will be filled in. - ref = writer.writeCountReference(table, conv.name, conv.staticSize) - table[conv.name] = None + if not isinstance(value, LiteralCount): + # we ignore the current count value since it's being recomputed, + # unless this is a LiteralCount, which is assumed to be already correct. + value = None + ref = writer.writeCountReference(table, conv.name, conv.staticSize, value) + table[conv.name] = value if conv.isPropagated: writer[conv.name] = ref elif conv.isLookupType: diff --git a/Lib/fontTools/ttLib/tables/otTables.py b/Lib/fontTools/ttLib/tables/otTables.py index 252d121ca..53f9fa82f 100644 --- a/Lib/fontTools/ttLib/tables/otTables.py +++ b/Lib/fontTools/ttLib/tables/otTables.py @@ -7,7 +7,7 @@ converter objects from otConverters.py. """ from fontTools.misc.py23 import * from fontTools.misc.textTools import pad, safeEval -from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord +from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord, LiteralCount import logging import struct @@ -687,6 +687,22 @@ class VarIdxMap(BaseTable): mapping[glyph] = (outer << 16) | inner +class VarRegionList(BaseTable): + + def preWrite(self, font): + # The OT spec says VarStore.VarRegionList.RegionAxisCount should always + # be equal to the fvar.axisCount, and OTS < v8.0.0 enforces this rule + # even when the VarRegionList is empty. We can't treat RegionAxisCount + # like a normal propagated count (== len(Region[i].VarRegionAxis)), + # otherwise it would default to 0 if VarRegionList is empty. + # Thus, we force it to always be equal to fvar.axisCount. + # https://github.com/khaledhosny/ots/pull/192 + fvarTable = font.get("fvar") + if fvarTable: + self.RegionAxisCount = LiteralCount(len(fvarTable.axes)) + return self.__dict__.copy() + + class SingleSubst(FormatSwitchingBaseTable): def populateDefaults(self, propagator=None): From 4eee7c071df9e2207220db4b54b00645efaebd5a Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 24 Oct 2019 14:05:29 +0100 Subject: [PATCH 2/5] varLib: don't add empty gvar or cvar with no variations --- Lib/fontTools/varLib/__init__.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/Lib/fontTools/varLib/__init__.py b/Lib/fontTools/varLib/__init__.py index 2dfe9e12c..80bd09b4b 100644 --- a/Lib/fontTools/varLib/__init__.py +++ b/Lib/fontTools/varLib/__init__.py @@ -213,15 +213,13 @@ def _add_gvar(font, masterModel, master_ttfs, tolerance=0.5, optimize=True): log.info("Generating gvar") assert "gvar" not in font - gvar = font["gvar"] = newTable('gvar') - gvar.version = 1 - gvar.reserved = 0 - gvar.variations = {} glyf = font['glyf'] # use hhea.ascent of base master as default vertical origin when vmtx is missing baseAscent = font['hhea'].ascent + + variations = {} for glyph in font.getGlyphOrder(): isComposite = glyf[glyph].isComposite() @@ -241,7 +239,6 @@ def _add_gvar(font, masterModel, master_ttfs, tolerance=0.5, optimize=True): del allControls # Update gvar - gvar.variations[glyph] = [] deltas = model.getDeltas(allCoords) supports = model.supports assert len(deltas) == len(supports) @@ -280,7 +277,14 @@ def _add_gvar(font, masterModel, master_ttfs, tolerance=0.5, optimize=True): if optimized_len < unoptimized_len: var = var_opt - gvar.variations[glyph].append(var) + variations.setdefault(glyph, []).append(var) + + if variations: + gvar = font["gvar"] = newTable('gvar') + gvar.version = 1 + gvar.reserved = 0 + gvar.variations = {g: variations.get(g, []) for g in font.getGlyphOrder()} + def _remove_TTHinting(font): for tag in ("cvar", "cvt ", "fpgm", "prep"): @@ -350,19 +354,21 @@ def _merge_TTHinting(font, masterModel, master_ttfs, tolerance=0.5): _remove_TTHinting(font) return - # We can build the cvar table now. - - cvar = font["cvar"] = newTable('cvar') - cvar.version = 1 - cvar.variations = [] - + variations = [] deltas, supports = masterModel.getDeltasAndSupports(all_cvs) for i,(delta,support) in enumerate(zip(deltas[1:], supports[1:])): delta = [otRound(d) for d in delta] if all(abs(v) <= tolerance for v in delta): continue var = TupleVariation(support, delta) - cvar.variations.append(var) + variations.append(var) + + # We can build the cvar table now. + if variations: + cvar = font["cvar"] = newTable('cvar') + cvar.version = 1 + cvar.variations = variations + _MetricsFields = namedtuple('_MetricsFields', ['tableTag', 'metricsTag', 'sb1', 'sb2', 'advMapping', 'vOrigMapping']) From 141ff20b370e104979ad4f12993ed3c71288a971 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 24 Oct 2019 14:10:08 +0100 Subject: [PATCH 3/5] varLib_test: test building (pseudo) VF with only a single master The SingleMaster.designspace contains only one source master, so there is no real variation data. Still, and empty HVAR table is added (with RegionAxisCount == fvar.AxisCount). The gvar and cvar table are not added in this case, since they are empty and not required. --- Tests/varLib/data/SingleMaster.designspace | 13 +++ .../varLib/data/test_results/SingleMaster.ttx | 98 +++++++++++++++++++ Tests/varLib/varLib_test.py | 9 ++ 3 files changed, 120 insertions(+) create mode 100644 Tests/varLib/data/SingleMaster.designspace create mode 100644 Tests/varLib/data/test_results/SingleMaster.ttx diff --git a/Tests/varLib/data/SingleMaster.designspace b/Tests/varLib/data/SingleMaster.designspace new file mode 100644 index 000000000..53bdeabf9 --- /dev/null +++ b/Tests/varLib/data/SingleMaster.designspace @@ -0,0 +1,13 @@ + + + + + + + + + + + + + diff --git a/Tests/varLib/data/test_results/SingleMaster.ttx b/Tests/varLib/data/test_results/SingleMaster.ttx new file mode 100644 index 000000000..79cc76556 --- /dev/null +++ b/Tests/varLib/data/test_results/SingleMaster.ttx @@ -0,0 +1,98 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + wght + 0x0 + 400.0 + 400.0 + 400.0 + 256 + + + + + + Weight + + + Test Family + + + Regular + + + Version 1.001;ADBO;Test Family Regular + + + Test Family + + + Version 1.001 + + + TestFamily-Master0 + + + Frank Grießhammer + + + Master 0 + + + Weight + + + + diff --git a/Tests/varLib/varLib_test.py b/Tests/varLib/varLib_test.py index f7289e72b..545b98e0d 100644 --- a/Tests/varLib/varLib_test.py +++ b/Tests/varLib/varLib_test.py @@ -584,6 +584,15 @@ class BuildTest(unittest.TestCase): self.expect_ttx(varfont, expected_ttx_path, tables) self.check_ttx_dump(varfont, expected_ttx_path, tables, suffix) + def test_varlib_build_single_master(self): + self._run_varlib_build_test( + designspace_name='SingleMaster', + font_name='TestFamily', + tables=['GDEF', 'HVAR', 'MVAR', 'STAT', 'fvar', 'cvar', 'gvar', 'name'], + expected_ttx_name='SingleMaster', + save_before_dump=True, + ) + def test_kerning_merging(self): """Test the correct merging of class-based pair kerning. From e8f5bbcc6a68aa16cc59ebb95d75e997bcc43033 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Thu, 24 Oct 2019 15:42:09 +0100 Subject: [PATCH 4/5] renamed LiteralCount to StaticCount makes it clearer what it is --- Lib/fontTools/ttLib/tables/otBase.py | 6 +++--- Lib/fontTools/ttLib/tables/otTables.py | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index f4a39e5f3..8769162d5 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -498,7 +498,7 @@ class OTTableWriter(object): return OverflowErrorRecord( (self.tableTag, LookupListIndex, SubTableIndex, itemName, itemIndex) ) -class LiteralCount(int): +class StaticCount(int): """A count value that should be taken literally, rather than recomputed on compile.""" pass @@ -695,9 +695,9 @@ class BaseTable(object): # table. We will later store it here. # We add a reference: by the time the data is assembled # the Count value will be filled in. - if not isinstance(value, LiteralCount): + if not isinstance(value, StaticCount): # we ignore the current count value since it's being recomputed, - # unless this is a LiteralCount, which is assumed to be already correct. + # unless this is of type StaticCount, which is assumed to be correct. value = None ref = writer.writeCountReference(table, conv.name, conv.staticSize, value) table[conv.name] = value diff --git a/Lib/fontTools/ttLib/tables/otTables.py b/Lib/fontTools/ttLib/tables/otTables.py index 53f9fa82f..30ad61334 100644 --- a/Lib/fontTools/ttLib/tables/otTables.py +++ b/Lib/fontTools/ttLib/tables/otTables.py @@ -7,7 +7,7 @@ converter objects from otConverters.py. """ from fontTools.misc.py23 import * from fontTools.misc.textTools import pad, safeEval -from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord, LiteralCount +from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord, StaticCount import logging import struct @@ -699,7 +699,7 @@ class VarRegionList(BaseTable): # https://github.com/khaledhosny/ots/pull/192 fvarTable = font.get("fvar") if fvarTable: - self.RegionAxisCount = LiteralCount(len(fvarTable.axes)) + self.RegionAxisCount = StaticCount(len(fvarTable.axes)) return self.__dict__.copy() From ebadcd96e604a2611bd080922f61ed656ef91b46 Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Tue, 29 Oct 2019 12:52:42 +0000 Subject: [PATCH 5/5] set pre-initialized CountReference in VarRegionList.preWrite with fvar.axisCount so we can reuse CountReference class, as suggested in https://github.com/fonttools/fonttools/pull/1752#issuecomment-547113944 The patch is no shorter though. --- Lib/fontTools/ttLib/tables/otBase.py | 23 ++++++++++++----------- Lib/fontTools/ttLib/tables/otTables.py | 9 ++++++--- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index 8769162d5..618213473 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -498,11 +498,6 @@ class OTTableWriter(object): return OverflowErrorRecord( (self.tableTag, LookupListIndex, SubTableIndex, itemName, itemIndex) ) -class StaticCount(int): - """A count value that should be taken literally, rather than recomputed on compile.""" - pass - - class CountReference(object): """A reference to a Count value, not a count of references.""" def __init__(self, table, name, size=None, value=None): @@ -518,6 +513,8 @@ class CountReference(object): table[name] = value else: assert table[name] == value, (name, table[name], value) + def getValue(self): + return self.table[self.name] def getCountData(self): v = self.table[self.name] if v is None: v = 0 @@ -695,12 +692,16 @@ class BaseTable(object): # table. We will later store it here. # We add a reference: by the time the data is assembled # the Count value will be filled in. - if not isinstance(value, StaticCount): - # we ignore the current count value since it's being recomputed, - # unless this is of type StaticCount, which is assumed to be correct. - value = None - ref = writer.writeCountReference(table, conv.name, conv.staticSize, value) - table[conv.name] = value + # We ignore the current count value since it will be recomputed, + # unless it's a CountReference that was already initialized in a custom preWrite. + if isinstance(value, CountReference): + ref = value + ref.size = conv.staticSize + writer.writeData(ref) + table[conv.name] = ref.getValue() + else: + ref = writer.writeCountReference(table, conv.name, conv.staticSize) + table[conv.name] = None if conv.isPropagated: writer[conv.name] = ref elif conv.isLookupType: diff --git a/Lib/fontTools/ttLib/tables/otTables.py b/Lib/fontTools/ttLib/tables/otTables.py index 30ad61334..fa00ca054 100644 --- a/Lib/fontTools/ttLib/tables/otTables.py +++ b/Lib/fontTools/ttLib/tables/otTables.py @@ -7,7 +7,7 @@ converter objects from otConverters.py. """ from fontTools.misc.py23 import * from fontTools.misc.textTools import pad, safeEval -from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord, StaticCount +from .otBase import BaseTable, FormatSwitchingBaseTable, ValueRecord, CountReference import logging import struct @@ -699,8 +699,11 @@ class VarRegionList(BaseTable): # https://github.com/khaledhosny/ots/pull/192 fvarTable = font.get("fvar") if fvarTable: - self.RegionAxisCount = StaticCount(len(fvarTable.axes)) - return self.__dict__.copy() + self.RegionAxisCount = len(fvarTable.axes) + return { + **self.__dict__, + "RegionAxisCount": CountReference(self.__dict__, "RegionAxisCount") + } class SingleSubst(FormatSwitchingBaseTable):