Merge pull request #3241 from fonttools/better-packer

[otBase/packer] Allow sharing tables reached by different offset sizes

Lib/fontTools/ttLib/tables/otBase.py

@@ -376,28 +376,32 @@ def __contains__(self, name):
         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."""
 
-    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
 
-    # 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 @@ def getDataLength(self):
         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 @@ def getData(self):
         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 @@ def getDataForHarfbuzz(self):
         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 @@ def __ne__(self, other):
     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,16 +504,20 @@ def _doneWriting(self, internedTables, shareExtension=False):
             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
                 # equal content, or store it in the dictionary if it's not been
                 # 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 +551,33 @@ def _gatherTables(self, tables, extTables, done):
             # 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 +612,7 @@ def _gatherGraphForHarfbuzz(self, tables, obj_list, done, objidx, virtual_edges)
         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 +621,12 @@ def _gatherGraphForHarfbuzz(self, tables, obj_list, done, objidx, virtual_edges)
                 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)
@@ -698,10 +709,8 @@ def getAllData(self, remove_duplicate=True):
 
     # 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
         )
@@ -773,8 +782,8 @@ def writeTag(self, tag):
         assert len(tag) == 4, tag
         self.items.append(tag)
 
-    def writeSubTable(self, subWriter):
-        self.items.append(subWriter)
+    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)
@@ -1365,7 +1374,7 @@ def writeValueRecord(self, writer, font, valueRecord):
             if isDevice:
                 if value:
                     subWriter = writer.getSubWriter()
-                    writer.writeSubTable(subWriter)
+                    writer.writeSubTable(subWriter, offsetSize=2)
                     value.compile(subWriter, font)
                 else:
                     writer.writeUShort(0)
@@ -1376,7 +1385,6 @@ def writeValueRecord(self, writer, font, valueRecord):
 
 
 class ValueRecord(object):
-
     # see ValueRecordFactory
 
     def __init__(self, valueFormat=None, src=None):

Lib/fontTools/ttLib/tables/otConverters.py

@@ -720,7 +720,6 @@ def write(self, writer, font, tableDict, value, repeatIndex=None):
 
 
 class Table(Struct):
-
     staticSize = 2
 
     def readOffset(self, reader):
@@ -746,16 +745,15 @@ def write(self, writer, font, tableDict, value, repeatIndex=None):
         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 @@ def writeNullOffset(self, writer):
 
 # Table pointed to by a 24-bit, 3-byte long offset
 class Table24(Table):
-
     staticSize = 3
 
     def readOffset(self, reader):
@@ -1147,13 +1144,13 @@ def write(self, writer, font, tableDict, value, repeatIndex=None):
             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 @@ def _compilePerGlyphLookups(self, table, font):
         )
         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):

Tests/ttLib/tables/C_O_L_R_test.py

@@ -116,7 +116,7 @@ def test_round_trip_xml(self, font):
     (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 @@ def test_round_trip_xml(self, font):
     (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 @@ def test_round_trip_xml(self, font):
     ),
     (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 @@ def test_round_trip_xml(self, font):
     "  </BaseGlyphPaintRecord>",
     "</BaseGlyphList>",
     "<LayerList>",
-    "  <!-- LayerCount=4 -->",
+    "  <!-- LayerCount=5 -->",
     '  <Paint index="0" Format="10"><!-- PaintGlyph -->',
     '    <Paint Format="3"><!-- PaintVarSolid -->',
     '      <PaletteIndex value="2"/>',
@@ -510,6 +514,13 @@ def test_round_trip_xml(self, font):
     '    <dx value="257"/>',
     '    <dy value="258"/>',
     "  </Paint>",
+    '  <Paint index="4" Format="10"><!-- PaintGlyph -->',
+    '    <Paint Format="2"><!-- PaintSolid -->',
+    '      <PaletteIndex value="2"/>',
+    '      <Alpha value="0.5"/>',
+    "    </Paint>",
+    '    <Glyph value="glyph00011"/>',
+    "  </Paint>",
     "</LayerList>",
     '<ClipList Format="1">',
     "  <Clip>",