fix compilation of AAT kern tables #1094

anthrotype · 2017-11-03T17:06:58Z

When compiling kern subtables for version=1.0 kern tables (AAT) the subtable header was written incorrectly: there is no version there, the length is a uint32 and there's an additional uint16 for tupleIndex (used for GX)
Use the coverage low byte to select subtable "format", instead of the version field, only present in OT kern subtable header.
KernTable_format_0 now takes an apple=False argument, used to choose the different headers and whether to read/write tupleIndex.
A new tupleIndex attribute is written out to TTX for Apple-style kern subtables. Old TTX files which lack that attribute will still be read (with a warning) and will default to 0 when recompiled or dumped with current fonttools.

- When compiling kern subtables for version=1.0 kern tables (AAT) the subtable header was written incorrectly: there is no version, the length is a uint32 and there's an additional uint16 for tupleIndex - Use the 'coverage' low byte to select subtable "format", instead of the 'version' field, only present in OT kern subtable header. The getkern method was failing with AttributeError on 'unknown' subtable formats, as their class only has 'format' instead of 'version' attribute. The 'version' attribute is renamed to 'format' also to avoid confusion, but the old one is kept for backward compatiblity. In the only implemeted subtable class, 'format' becomes a class attribute rather than instance's (it must always be 0). - KernTable_format_0 now takes an 'apple=False' argument, used to know the different headers and whether to read/write tupleIndex. - minor pep8 whitespace and indentation fixes - A new 'tupleIndex' attribute is written out to TTX for apple kern subtables. Old ttx files which lack that attribute will still be read (with a warning) and will default to tupleIndex=0 when recompiled or dumped with current fonttools. Fixes fonttools#1089

anthrotype · 2017-11-03T17:07:40Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

@@ -33,21 +33,23 @@ def decompile(self, data, ttFont):
 		else:
 			self.version = version
 			data = data[4:]
-		tablesIndex = []


this tableIndex was unused so I removed it

anthrotype · 2017-11-03T17:20:54Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

@@ -59,7 +61,7 @@ def compile(self, ttFont):
 			nTables = 0
 		if self.version == 1.0:
 			# AAT Apple's "new" format.
-			data = struct.pack(">ll", fl2fi(self.version, 16), nTables)
+			data = struct.pack(">LL", fl2fi(self.version, 16), nTables)


the Apple spec has unsigned long integers (uint32) for version and nTables, not signed

anthrotype · 2017-11-03T17:27:05Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

 			data = data[6:]
 		else:
-			version, length, coverage = struct.unpack(">LHH", data[:8])
+			length, coverage, tupleIndex = struct.unpack(">LHH", data[:8])


we were also decompiling coverage incorrectly here for apple kern subtables: the tupleIndex was being read in as "coverage" attribute, whereas the actual coverage (in second position, where length used to be) was being thrown away.

anthrotype · 2017-11-03T17:33:16Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+				kernTable[(
+					ttFont.getGlyphName(left),
+					ttFont.getGlyphName(right))] = value
+		if len(data) > 6 * nPairs + 4:  # Ignore up to 4 bytes excess


perhaps part of the responsibility for not noticing this bug before is that we are silently letting pass up to 4 extra bytes in the kern subtable. Why don't we always warn when the data length doesn't match the stated number of pairs or is not multiple of 6?

so, Calibri has 4 bytes in excess.. 5cd0a55
ok then 🤷‍♂️

Well, I still see Calibri's kern table shrink a lot after roundtripping with ttx. I mentioned it in 5cd0a55 but I don't think anyone followed up on that. Check ttx -l before and after roundtripping. But then again, I'm not entirely sure where that Calibri came from.

I can send you the version I have if you're curious.

Not particularly. I'd be more interested in having the AAT kern compilation fixed. Maybe you could also do a quick review since you wrote the original code? Thanks anyway :)

sure, you can send me that file, thanks

I see Win10 calibri.ttf has 160kb kern table. I suppose MS started ignoring the binary search header completely and use the subtable length instead. We should do the same. I'll fix HarfBuzz as well.

My bad. Ignoring subtable length, respecting bsearch header! This is now implemented in HarfBuzz:
harfbuzz/harfbuzz@82a38d1

anthrotype · 2017-11-03T17:38:22Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py


 		for left, right, value in kernTable:
 			data = data + struct.pack(">HHh", left, right, value)
-		return struct.pack(">HHH", self.version, len(data) + 6, self.coverage) + data
+		# ensure mask bits 8-15 (subtable format) are set to 0
+		self.coverage &= ~0xFF


here I follow the pattern found elsewhere in ttLib whereby any necessary fix-ups or modifications occur at compile time.

anthrotype · 2017-11-03T17:41:17Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+				# previous fontTools versions didn't export tupleIndex
+				log.warning(
+					"Apple kern subtable is missing 'tupleIndex' attribute")
+				self.tupleIndex = None


here I decided to set tupleIndex to None when it's missing from a kern TTX file; if I had set it to 0, it wouldn't be possible to distinguish from a kern TTX file with a missing tupleIndex attribute or one with tupleIndex=0.
The value still defaults to 0 when the imported font is compiled or dumped again to XML.

behdad · 2017-11-03T17:41:50Z

Kinda relevant #314

anthrotype · 2017-11-03T17:54:23Z

#314 (comment)

we can detect [kern subtable exceeding maximum length] and possibly split into multiple subtables.

I could do that later in another PR, as this is about fixing AAT kern decompilation/compilation.

as we don't run test on big endian systems so that check is always true anyway...

had removed that fixture and squashed.. should have re-run the test before pushing :-/

anthrotype · 2017-11-04T09:57:05Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+				# Since this "version" is always 0 (and is not present in the
+				# later AAT extensions), we simply ignore it here
+				_, length, coverage = struct.unpack(">HHH", data[:6])
+			subtableFormat = coverage & 0xff


ooh.... actually also the order of format and coverage bytes is different between OT and AAT kern! In OT the first byte is format and the following one is coverage, whereas in AAT it's the reverse. I got this completely wrong, sorry.. 😬
Thank @justvanrossum that had me check that CALIBRI.TTF kern. I was surprised when I saw a microsoft font with a kern subtable format 1 (Apple-only). It turns out I was reading wrong, the 1 was the coverage bit (horizontal data), not the format.
Why did they make it so complicated 😖

Yeah it's as if someone took the existing format and said let's change everything about it! Allocate higher byte for coverage, allocate bits MSB-to-LSB, and allocate a bit for horizontal, instead of vertical :)).

In OT kern subtable header, the format is the high byte of 'coverage' bit mask (bits 8-15), and the low byte (bits 0-7) is the actual coverage bits. In AAT kern, it's the opposite: the coverage flags are the high byte, whereas the subtable format is the low byte. Also adjusted the test data, and set coverage to 1 for OT kern subtable (which means the usual horizontal kerning).

anthrotype · 2017-11-04T11:16:43Z

About that CALIBRI.TTF font mentioned by @justvanrossum, it also seems to use the trick of having a huge single kern subtable with the USHORT length truncated (like @twardoch explained in #314 (comment)).
I wonder if that kerning works at all.
But again, this PR is about fixing compilation of valid AAT kern tables, rather than rescuing broken OT kern tables.

behdad · 2017-11-04T15:16:59Z

we can detect [kern subtable exceeding maximum length] and possibly split into multiple subtables.

I could do that later in another PR, as this is about fixing AAT kern decompilation/compilation.

Note that splitting doesn't help as Windows will ignore them all. Looks like we should ignore num-items and use table length instead. But then that's not what this PR is about. Just wanted to make sure you don't go down that route.

anthrotype · 2017-11-04T15:46:18Z

The description of the kern subtable format 0 on Apple website says that:
https://developer.apple.com/fonts/TrueType-Reference-Manual/RM06/Chap6kern.html

The table needs to end with an entry for a left-hand glyph of 0xFFFF, a right-hand glyph of 0xFFFF, and a kerning value of 0.

However, Zapfino.ttf (12.0d1e6; 2016-07-18), which is the only font I was able to find on my macOS Sierra which has a kern version=1.0 and a format 0 subtable does not end with FFFF FFFF 0000, but with a normal kern pair 04BA 04BA 0019 (which is <pair l="fouroldstyle" r="fouroldstyle" v="25"/>).

Shall we enforce that requirement somehow? Not raise of course, but maybe log a warning?

behdad

Shall we enforce that requirement somehow? Not raise of course, but maybe log a warning?

No idea how to roundtrip properly or if it matters. If we see such a pair we shouldn't try looking it up as a glyph-id, but not sure what to do otherwise.

behdad · 2017-11-04T15:21:55Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

@@ -17,7 +19,7 @@ class table__k_e_r_n(DefaultTable.DefaultTable):

 	def getkern(self, format):


The name of this function is so wrong! Shall we remove it or what?

I doubt anyone ever used it because it was throwing AttributeError because of missing "version" attribute on anything other than format-0 subtables (the only one implemented). Sure, I can remove it. We can add a better one when we implement other subtable formats.

behdad · 2017-11-04T15:29:25Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+	# 'version' is kept for backward compatibility
+	version = format = 0
+
+	def __init__(self, apple=False):


The apple is really an input to decompile() and compile(), not a property of the table itself. What do you think about removing it and moving it there? (like isCFF2 if you remember that discussion.)

Also, the way I implemented this in HarfBuzz, I separated the Apple/MS subtable wrapper abstraction from the subtable types themselves. I think we can do something similar here to simplify the code if you feel like.

Or you merge this, and I do that when implementing format-2.

Here's the HB code BTW:
https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-kern-table.hh

I see Win10 calibri.ttf has 160kb kern table. I suppose MS started ignoring the binary search header completely and use the subtable length instead. We should do the same. I'll fix HarfBuzz as well.

I keep confusing myself. Even subtable length is being ignored. The whole table length is used as subtable length. Argh!

I thought about putting "apple" as an argument for decompile/compile, but then I needed that attribute inside fromXML and toXML as well to decide whether to read/write the tupleIndex...
I think I shall leave it like that until we add more formats.

behdad · 2017-11-04T15:30:11Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+			if version != 0:
+				from fontTools.ttLib import TTLibError
+				raise TTLibError(
+					"unsupported kern subtable version: %d" % version)


Someone is going to get really confused by this error some time. :)

I doubt anyone will ever see that error. All the version=0 kern tables out there almost certainly have a subtable version=0, whatever that means. We'll see..

behdad · 2017-11-04T15:30:53Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+				raise TTLibError(
+					"unsupported kern subtable version: %d" % version)
+			tupleIndex = None
+			# Should we also assert length == len(data)?


That should always pass, right? If that's the case, sure. If it would fail on bogus fonts, no.

It should, because the wrapper class used the same length to trim the data and pass it down. The assert would simply check that this assumption holds.

behdad · 2017-11-04T15:33:34Z

Lib/fontTools/ttLib/tables/_k_e_r_n.py

+				kernTable[(
+					ttFont.getGlyphName(left),
+					ttFont.getGlyphName(right))] = value
+		if len(data) > 6 * nPairs + 4:  # Ignore up to 4 bytes excess


I see Win10 calibri.ttf has 160kb kern table. I suppose MS started ignoring the binary search header completely and use the subtable length instead. We should do the same. I'll fix HarfBuzz as well.

behdad · 2017-11-04T16:34:15Z

Let's merge and move on. We'll fix more later. I've asked for clarification re calibri.ttf.

Apparently calibri.ttf does this: fonttools/fonttools#1094 (comment)

anthrotype added 3 commits November 2, 2017 10:37

[_k_e_r_n_test] rewrite as pytest-style; fix indentation

240ef76

[_k_e_r_n_test] Add tests for OT and AAT kern format 0 subtable

5f2177a

anthrotype requested a review from behdad November 3, 2017 17:06

anthrotype commented Nov 3, 2017

View reviewed changes

anthrotype added 3 commits November 3, 2017 19:59

[_k_e_r_n] add nocover for sys.byteorder check

982c956

as we don't run test on big endian systems so that check is always true anyway...

[_k_e_r_n_test] decompile/compile tests for unknown formats

a2a4f45

fixup

5d16828

had removed that fixture and squashed.. should have re-run the test before pushing :-/

anthrotype commented Nov 4, 2017

View reviewed changes

anthrotype added 2 commits November 4, 2017 10:56

minor: fix comment and line length

0f16a79

[_k_e_r_n_test] test toXML/fromXML for unknown formats

a12bfa1

behdad reviewed Nov 4, 2017

View reviewed changes

anthrotype merged commit 34f06f4 into fonttools:master Nov 4, 2017

anthrotype deleted the apple-kern branch November 4, 2017 16:41

behdad added a commit to harfbuzz/harfbuzz that referenced this pull request Nov 6, 2017

[kern] Allow subtables longer than 64kb

82a38d1

Apparently calibri.ttf does this: fonttools/fonttools#1094 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix compilation of AAT kern tables #1094

fix compilation of AAT kern tables #1094

anthrotype commented Nov 3, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

justvanrossum Nov 3, 2017

justvanrossum Nov 3, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

behdad Nov 4, 2017

behdad Nov 6, 2017

anthrotype Nov 3, 2017

anthrotype Nov 3, 2017

behdad commented Nov 3, 2017

anthrotype commented Nov 3, 2017

anthrotype Nov 4, 2017 •

edited

Loading

behdad Nov 4, 2017

anthrotype commented Nov 4, 2017

behdad commented Nov 4, 2017

anthrotype commented Nov 4, 2017

behdad left a comment

behdad Nov 4, 2017

anthrotype Nov 4, 2017

behdad Nov 4, 2017

behdad Nov 4, 2017

anthrotype Nov 4, 2017

behdad Nov 4, 2017

anthrotype Nov 4, 2017

behdad Nov 4, 2017

anthrotype Nov 4, 2017

behdad Nov 4, 2017

behdad commented Nov 4, 2017

		@@ -17,7 +19,7 @@ class table__k_e_r_n(DefaultTable.DefaultTable):

		def getkern(self, format):

fix compilation of AAT kern tables #1094

fix compilation of AAT kern tables #1094

Conversation

anthrotype commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad commented Nov 3, 2017

anthrotype commented Nov 3, 2017

anthrotype Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthrotype commented Nov 4, 2017

behdad commented Nov 4, 2017

anthrotype commented Nov 4, 2017

behdad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad commented Nov 4, 2017

anthrotype Nov 4, 2017 •

edited

Loading