From 2b629e51c2aaacdfcaa83c19a6a26aa784239bc9 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 3 Aug 2023 11:10:16 -0600 Subject: [PATCH 1/3] [otBase/packer] Allow sharing tables reached by different offset sizes This makes us match hb-subset now. Fixes https://github.com/fonttools/fonttools/issues/3236 --- Lib/fontTools/ttLib/tables/otBase.py | 72 +++++++++++++++------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index 9c80400e9..00a7c87ab 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -376,6 +376,20 @@ class OTTableReader(object): return self.localState and name in self.localState +class OffsetToWriter(object): + def __init__(self, subWriter, offsetSize): + self.subWriter = subWriter + self.offsetSize = offsetSize + + def __eq__(self, other): + if type(self) != type(other): + return NotImplemented + return self.subWriter == other.subWriter and self.offsetSize == other.offsetSize + + def __hash__(self): + # only works after self._doneWriting() has been called + return hash((self.subWriter, self.offsetSize)) + class OTTableWriter(object): """Helper class to gather and assemble data for OpenType tables.""" @@ -388,16 +402,6 @@ class OTTableWriter(object): self.offsetSize = offsetSize self.parent = None - # DEPRECATED: 'longOffset' is kept as a property for backward compat with old code. - # You should use 'offsetSize' instead (2, 3 or 4 bytes). - @property - def longOffset(self): - return self.offsetSize == 4 - - @longOffset.setter - def longOffset(self, value): - self.offsetSize = 4 if value else 2 - def __setitem__(self, name, value): state = self.localState.copy() if self.localState else dict() state[name] = value @@ -417,7 +421,7 @@ class OTTableWriter(object): for item in self.items: if hasattr(item, "getCountData"): l += item.size - elif hasattr(item, "getData"): + elif hasattr(item, "subWriter"): l += item.offsetSize else: l = l + len(item) @@ -431,19 +435,19 @@ class OTTableWriter(object): for i in range(numItems): item = items[i] - if hasattr(item, "getData"): + if hasattr(item, "subWriter"): if item.offsetSize == 4: - items[i] = packULong(item.pos - pos) + items[i] = packULong(item.subWriter.pos - pos) elif item.offsetSize == 2: try: - items[i] = packUShort(item.pos - pos) + items[i] = packUShort(item.subWriter.pos - pos) except struct.error: # provide data to fix overflow problem. overflowErrorRecord = self.getOverflowErrorRecord(item) raise OTLOffsetOverflowError(overflowErrorRecord) elif item.offsetSize == 3: - items[i] = packUInt24(item.pos - pos) + items[i] = packUInt24(item.subWriter.pos - pos) else: raise ValueError(item.offsetSize) @@ -454,7 +458,7 @@ class OTTableWriter(object): items = list(self.items) packFuncs = {2: packUShort, 3: packUInt24, 4: packULong} for i, item in enumerate(items): - if hasattr(item, "getData"): + if hasattr(item, "subWriter"): # Offset value is not needed in harfbuzz repacker, so setting offset to 0 to avoid overflow here if item.offsetSize in packFuncs: items[i] = packFuncs[item.offsetSize](0) @@ -474,7 +478,7 @@ class OTTableWriter(object): def __eq__(self, other): if type(self) != type(other): return NotImplemented - return self.offsetSize == other.offsetSize and self.items == other.items + return self.items == other.items def _doneWriting(self, internedTables, shareExtension=False): # Convert CountData references to data string items @@ -500,8 +504,10 @@ class OTTableWriter(object): item = items[i] if hasattr(item, "getCountData"): items[i] = item.getCountData() - elif hasattr(item, "getData"): - item._doneWriting(internedTables, shareExtension=shareExtension) + elif hasattr(item, "subWriter"): + item.subWriter._doneWriting( + internedTables, shareExtension=shareExtension + ) # At this point, all subwriters are hashable based on their items. # (See hash and comparison magic methods above.) So the ``setdefault`` # call here will return the first writer object we've seen with @@ -509,7 +515,7 @@ class OTTableWriter(object): # seen yet. We therefore replace the subwriter object with an equivalent # object, which deduplicates the tree. if not dontShare: - items[i] = item = internedTables.setdefault(item, item) + items[i].subWriter = internedTables.setdefault(item.subWriter, item.subWriter) self.items = tuple(items) def _gatherTables(self, tables, extTables, done): @@ -543,30 +549,30 @@ class OTTableWriter(object): # Find coverage table for i in range(numItems): item = self.items[i] - if getattr(item, "name", None) == "Coverage": + if hasattr(item, "subWriter") and getattr(item.subWriter, "name", None) == "Coverage": sortCoverageLast = True break - if id(item) not in done: - item._gatherTables(tables, extTables, done) + if id(item.subWriter) not in done: + item.subWriter._gatherTables(tables, extTables, done) else: # We're a new parent of item pass for i in iRange: item = self.items[i] - if not hasattr(item, "getData"): + if not hasattr(item, "subWriter"): continue if ( sortCoverageLast and (i == 1) - and getattr(item, "name", None) == "Coverage" + and getattr(item.subWriter, "name", None) == "Coverage" ): # we've already 'gathered' it above continue - if id(item) not in done: - item._gatherTables(tables, extTables, done) + if id(item.subWriter) not in done: + item.subWriter._gatherTables(tables, extTables, done) else: # Item is already written out by other parent pass @@ -601,7 +607,7 @@ class OTTableWriter(object): child_idx = 0 offset_pos = 0 for i, item in enumerate(self.items): - if hasattr(item, "getData"): + if hasattr(item, "subWriter"): pos = offset_pos elif hasattr(item, "getCountData"): offset_pos += item.size @@ -610,12 +616,12 @@ class OTTableWriter(object): offset_pos = offset_pos + len(item) continue - if id(item) not in done: - child_idx = item_idx = item._gatherGraphForHarfbuzz( + if id(item.subWriter) not in done: + child_idx = item_idx = item.subWriter._gatherGraphForHarfbuzz( tables, obj_list, done, item_idx, virtual_edges ) else: - child_idx = done[id(item)] + child_idx = done[id(item.subWriter)] real_edge = (pos, item.offsetSize, child_idx) real_links.append(real_edge) @@ -774,7 +780,8 @@ class OTTableWriter(object): self.items.append(tag) def writeSubTable(self, subWriter): - self.items.append(subWriter) + self.items.append(OffsetToWriter(subWriter, subWriter.offsetSize)) + del subWriter.offsetSize def writeCountReference(self, table, name, size=2, value=None): ref = CountReference(table, name, size=size, value=value) @@ -1376,7 +1383,6 @@ class ValueRecordFactory(object): class ValueRecord(object): - # see ValueRecordFactory def __init__(self, valueFormat=None, src=None): From 286e6466cd490205d5c963698f90de302310a6fe Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Thu, 3 Aug 2023 11:17:41 -0600 Subject: [PATCH 2/3] [otBase/packer] Remove offsetSize from OTTableWriter --- Lib/fontTools/ttLib/tables/otBase.py | 26 ++++++++++++---------- Lib/fontTools/ttLib/tables/otConverters.py | 19 +++++++--------- 2 files changed, 22 insertions(+), 23 deletions(-) diff --git a/Lib/fontTools/ttLib/tables/otBase.py b/Lib/fontTools/ttLib/tables/otBase.py index 00a7c87ab..a33d7c1ab 100644 --- a/Lib/fontTools/ttLib/tables/otBase.py +++ b/Lib/fontTools/ttLib/tables/otBase.py @@ -390,16 +390,16 @@ class OffsetToWriter(object): # only works after self._doneWriting() has been called return hash((self.subWriter, self.offsetSize)) + class OTTableWriter(object): """Helper class to gather and assemble data for OpenType tables.""" - def __init__(self, localState=None, tableTag=None, offsetSize=2): + def __init__(self, localState=None, tableTag=None): self.items = [] self.pos = None self.localState = localState self.tableTag = tableTag - self.offsetSize = offsetSize self.parent = None def __setitem__(self, name, value): @@ -515,7 +515,9 @@ class OTTableWriter(object): # seen yet. We therefore replace the subwriter object with an equivalent # object, which deduplicates the tree. if not dontShare: - items[i].subWriter = internedTables.setdefault(item.subWriter, item.subWriter) + items[i].subWriter = internedTables.setdefault( + item.subWriter, item.subWriter + ) self.items = tuple(items) def _gatherTables(self, tables, extTables, done): @@ -549,7 +551,10 @@ class OTTableWriter(object): # Find coverage table for i in range(numItems): item = self.items[i] - if hasattr(item, "subWriter") and getattr(item.subWriter, "name", None) == "Coverage": + if ( + hasattr(item, "subWriter") + and getattr(item.subWriter, "name", None) == "Coverage" + ): sortCoverageLast = True break if id(item.subWriter) not in done: @@ -704,10 +709,8 @@ class OTTableWriter(object): # interface for gathering data, as used by table.compile() - def getSubWriter(self, offsetSize=2): - subwriter = self.__class__( - self.localState, self.tableTag, offsetSize=offsetSize - ) + def getSubWriter(self): + subwriter = self.__class__(self.localState, self.tableTag) subwriter.parent = ( self # because some subtables have idential values, we discard ) @@ -779,9 +782,8 @@ class OTTableWriter(object): assert len(tag) == 4, tag self.items.append(tag) - def writeSubTable(self, subWriter): - self.items.append(OffsetToWriter(subWriter, subWriter.offsetSize)) - del subWriter.offsetSize + def writeSubTable(self, subWriter, offsetSize): + self.items.append(OffsetToWriter(subWriter, offsetSize)) def writeCountReference(self, table, name, size=2, value=None): ref = CountReference(table, name, size=size, value=value) @@ -1372,7 +1374,7 @@ class ValueRecordFactory(object): if isDevice: if value: subWriter = writer.getSubWriter() - writer.writeSubTable(subWriter) + writer.writeSubTable(subWriter, offsetSize=2) value.compile(subWriter, font) else: writer.writeUShort(0) diff --git a/Lib/fontTools/ttLib/tables/otConverters.py b/Lib/fontTools/ttLib/tables/otConverters.py index 6b2a8c396..390f1660e 100644 --- a/Lib/fontTools/ttLib/tables/otConverters.py +++ b/Lib/fontTools/ttLib/tables/otConverters.py @@ -720,7 +720,6 @@ class StructWithLength(Struct): class Table(Struct): - staticSize = 2 def readOffset(self, reader): @@ -746,16 +745,15 @@ class Table(Struct): if value is None: self.writeNullOffset(writer) else: - subWriter = writer.getSubWriter(offsetSize=self.staticSize) + subWriter = writer.getSubWriter() subWriter.name = self.name if repeatIndex is not None: subWriter.repeatIndex = repeatIndex - writer.writeSubTable(subWriter) + writer.writeSubTable(subWriter, offsetSize=self.staticSize) value.compile(subWriter, font) class LTable(Table): - staticSize = 4 def readOffset(self, reader): @@ -767,7 +765,6 @@ class LTable(Table): # Table pointed to by a 24-bit, 3-byte long offset class Table24(Table): - staticSize = 3 def readOffset(self, reader): @@ -1147,13 +1144,13 @@ class AATLookupWithDataOffset(BaseConverter): offsetByGlyph[glyph] = offset # For calculating the offsets to our AATLookup and data table, # we can use the regular OTTableWriter infrastructure. - lookupWriter = writer.getSubWriter(offsetSize=4) + lookupWriter = writer.getSubWriter() lookup = AATLookup("DataOffsets", None, None, UShort) lookup.write(lookupWriter, font, tableDict, offsetByGlyph, None) - dataWriter = writer.getSubWriter(offsetSize=4) - writer.writeSubTable(lookupWriter) - writer.writeSubTable(dataWriter) + dataWriter = writer.getSubWriter() + writer.writeSubTable(lookupWriter, offsetSize=4) + writer.writeSubTable(dataWriter, offsetSize=4) for d in compiledData: dataWriter.writeData(d) @@ -1483,9 +1480,9 @@ class STXHeader(BaseConverter): ) writer = OTTableWriter() for lookup in table.PerGlyphLookups: - lookupWriter = writer.getSubWriter(offsetSize=4) + lookupWriter = writer.getSubWriter() self.perGlyphLookup.write(lookupWriter, font, {}, lookup, None) - writer.writeSubTable(lookupWriter) + writer.writeSubTable(lookupWriter, offsetSize=4) return writer.getAllData() def _compileLigComponents(self, table, font): From d2fd4dfb240d4c1b26acfa54b497067fa46aa9be Mon Sep 17 00:00:00 2001 From: Cosimo Lupo Date: Fri, 4 Aug 2023 11:45:20 +0100 Subject: [PATCH 3/3] C_O_L_R_test: add test for paint pointed to by both 3- and 4-byte offsets --- Tests/ttLib/tables/C_O_L_R_test.py | 35 ++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Tests/ttLib/tables/C_O_L_R_test.py b/Tests/ttLib/tables/C_O_L_R_test.py index 71789a4de..43ad70496 100644 --- a/Tests/ttLib/tables/C_O_L_R_test.py +++ b/Tests/ttLib/tables/C_O_L_R_test.py @@ -116,7 +116,7 @@ COLR_V1_SAMPLE = ( (b"\x00\x03", "LayerRecordCount (3)"), (b"\x00\x00\x00\x34", "Offset to BaseGlyphList from beginning of table (52)"), (b"\x00\x00\x00\x9f", "Offset to LayerList from beginning of table (159)"), - (b"\x00\x00\x01\x62", "Offset to ClipList (354)"), + (b"\x00\x00\x01\x66", "Offset to ClipList (358)"), (b"\x00\x00\x00\x00", "Offset to DeltaSetIndexMap (NULL)"), (b"\x00\x00\x00\x00", "Offset to VarStore (NULL)"), (b"\x00\x06", "BaseGlyphRecord[0].BaseGlyph (6)"), @@ -187,22 +187,26 @@ COLR_V1_SAMPLE = ( (b"\x00\x05", "ColorLine.ColorStop[1].PaletteIndex (5)"), (b"@\x00", "ColorLine.ColorStop[1].Alpha (1.0)"), # LayerList - (b"\x00\x00\x00\x04", "LayerList.LayerCount (4)"), + (b"\x00\x00\x00\x05", "LayerList.LayerCount (5)"), ( - b"\x00\x00\x00\x14", - "First Offset to Paint table from beginning of LayerList (20)", + b"\x00\x00\x00\x18", + "First Offset to Paint table from beginning of LayerList (24)", ), ( - b"\x00\x00\x00\x23", - "Second Offset to Paint table from beginning of LayerList (35)", + b"\x00\x00\x00\x27", + "Second Offset to Paint table from beginning of LayerList (39)", ), ( - b"\x00\x00\x00\x4e", - "Third Offset to Paint table from beginning of LayerList (78)", + b"\x00\x00\x00\x52", + "Third Offset to Paint table from beginning of LayerList (82)", ), ( - b"\x00\x00\x00\x9e", - "Fourth Offset to Paint table from beginning of LayerList (158)", + b"\x00\x00\x00\xa2", + "Fourth Offset to Paint table from beginning of LayerList (162)", + ), + ( + b"\x00\x00\x00\xbc", + "Fifth Offset to Paint table from beginning of LayerList (188)", ), # BaseGlyphPaintRecord[2] (b"\x0a", "BaseGlyphPaintRecord[2].Paint.Format (10)"), @@ -296,7 +300,7 @@ COLR_V1_SAMPLE = ( ), (b"\xfc\x17", "xSkewAngle (-0.0611)"), (b"\x01\xc7", "ySkewAngle (0.0278)"), - # PaintGlyph + # PaintGlyph glyph00011 (pointed to by both PaintSkew above and by LayerList[4] offset) (b"\x0a", "LayerList.Paint[3].Paint.Paint.Paint.Format (10)"), (b"\x00\x00\x06", "Offset to Paint subtable from beginning of PaintGlyph (6)"), (b"\x00\x0b", "LayerList.Paint[2].Glyph (11)"), @@ -413,7 +417,7 @@ COLR_V1_XML = [ " ", "", "", - " ", + " ", ' ', ' ', ' ', @@ -510,6 +514,13 @@ COLR_V1_XML = [ ' ', ' ', " ", + ' ', + ' ', + ' ', + ' ', + " ", + ' ', + " ", "", '', " ",