Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Produces invalid CFF OTF files under Python 3 (repro available) #306

Closed
rsms opened this issue Jan 3, 2019 · 18 comments
Closed

Produces invalid CFF OTF files under Python 3 (repro available) #306

rsms opened this issue Jan 3, 2019 · 18 comments
Labels

Comments

@rsms
Copy link

rsms commented Jan 3, 2019

What I did: Compiled a simple UFO to OTF (CFF) using ufo2ft 2.5.0 (compileOTF(ufo, optimizeCFF=CFFOptimization.NONE))

What I expected: Resulting OTF file to be valid as per OTS, FontVal and macOS Font Book.

What actually happened: OTF file is unreadable by macOS Font Book and fails validation by OTS.

If I compile the same source with the same settings using the same version of ufo2ft using Python 2.7 instead of Python 3.7 the resulting OTF file is valid — this seems to indicate a bug related to Python 3.

Full repro available here: https://github.com/rsms/ufo2ft-py3-bug

  • Platform: macOS (Darwin 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct 5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64)
  • Python 2: 2.7.15
  • Python 3: 3.7.2
  • ufo2ft: 2.5.0
@anthrotype
Copy link
Member

anthrotype commented Jan 3, 2019

thanks for filing this issue and providing an exact reproducer!

Comparing the ttx dumps of the two fonts generated respectively from python2 and python3 I only get this difference in the FontMatrix operator of the CFF table:

$ diff -Naur output{2,3}.ttx
--- output2.ttx	2019-01-03 21:40:37.000000000 +0000
+++ output3.ttx	2019-01-03 21:40:37.000000000 +0000
@@ -14,7 +14,7 @@
     <!-- Most of this table will be recalculated by the compiler -->
     <tableVersion value="1.0"/>
     <fontRevision value="1.0"/>
-    <checkSumAdjustment value="0x7def7972"/>
+    <checkSumAdjustment value="0x90ebc0da"/>
     <magicNumber value="0x5f0f3cf5"/>
     <flags value="00000000 00000011"/>
     <unitsPerEm value="2816"/>
@@ -197,7 +197,7 @@
       <UnderlineThickness value="220"/>
       <PaintType value="0"/>
       <CharstringType value="2"/>
-      <FontMatrix value="0.000355113636364 0 0 0.000355113636364 0 0"/>
+      <FontMatrix value="0.0003551136363636364 0 0 0.0003551136363636364 0 0"/>
       <FontBBox value="72 -660 1896 2708"/>
       <StrokeWidth value="0"/>
       <!-- charset is dumped separately as the 'GlyphOrder' element -->

the extra precision of the python3 float division seems to trigger this invalid font file somehow. I'll investigate more tomorrow, but we probably need to make sure we round these floats to some reasonable number of digits.

https://github.com/googlei18n/ufo2ft/blob/39ab30c54ab71b24d96bb68e0965c41937d39b35/Lib/ufo2ft/outlineCompiler.py#L986

@anthrotype anthrotype added the bug label Jan 3, 2019
@anthrotype
Copy link
Member

anthrotype commented Jan 3, 2019

the culprit seems to be in the encodeFloat function of fontTools.misc.psCharStrings module (despite according to git blame it hasn't changed in 17 years..)

https://github.com/fonttools/fonttools/blob/3ba285e2504c73cac4970ae846f56db2b41fa4a0/Lib/fontTools/misc/psCharStrings.py#L228

it's calling str on the float before encoding it to binary, and on python3 the float repr seems to be more precise than on python2.

Your font UPEM is 2816 (which is not as common value as say 1000 or 2048, which perhaps explain why we haven't caught this issue before). The FontMatrix value 1.0/2816, when it's converted to str, becomes '0.000355113636364' in python 2.7 and '0.0003551136363636364' in python 3.7.

If I apply this patch to round the FontMatrix values to 15 decimal digits, the output from python2 and python3 is identical, and both your test fonts pass validation:

diff --git a/Lib/ufo2ft/outlineCompiler.py b/Lib/ufo2ft/outlineCompiler.py
index 60a4a97..965b575 100644
--- a/Lib/ufo2ft/outlineCompiler.py
+++ b/Lib/ufo2ft/outlineCompiler.py
@@ -983,7 +983,14 @@ class OutlineOTFCompiler(BaseOutlineCompiler):
         topDict.UnderlineThickness = otRound(underlineThickness)
         # populate font matrix
         unitsPerEm = otRound(getAttrWithFallback(info, "unitsPerEm"))
-        topDict.FontMatrix = [1.0 / unitsPerEm, 0, 0, 1.0 / unitsPerEm, 0, 0]
+        topDict.FontMatrix = [
+            round(1.0 / unitsPerEm, 15),
+            0,
+            0,
+            round(1.0 / unitsPerEm, 15),
+            0,
+            0,
+        ]
         # populate the width values
         if not any(hasattr(info, attr) and getattr(info, attr) is not None
                    for attr in ("postscriptDefaultWidthX",

I'm thinking of rather changing the encodeFloat function in upstream fonttools, to prevent this and similar issues with real numbers in CFF dicts.

In makeotf, they seem to round to 8 decimal digits
adobe-type-tools/afdko#174

I'll sleep over it, and push a fix for it tomorrow.

@rsms
Copy link
Author

rsms commented Jan 3, 2019

Thank you!

@rsms
Copy link
Author

rsms commented Jan 4, 2019

Patching outlineCompiler.py as shown above solves the issue with most UFOs I'm building except one. Even really small values for round() (like 4) yields incorrect OTF files for this other UFO source.

I've updated the repro https://github.com/rsms/ufo2ft-py3-bug with:

  • the above patch (run.sh builds OTFs with and without the patch to compare results)
  • the second/alternate UFO source which still fails with the patch

@anthrotype
Copy link
Member

the reason the other font also fails to render is related. If you compare the ttx dumps of output2-B.otf with output3-B.otf, this time it's the value of ItalicAngle operator in CFF table that ends up being represented as a too long float number -9.399999999999999.

--- /Users/clupo/Github/ufo2ft-py3-bug/build/output2-B.ttx	2019-01-04 11:27:18.000000000 +0000
+++ /Users/clupo/Github/ufo2ft-py3-bug/build/output3-B.ttx	2019-01-04 11:27:18.000000000 +0000
@@ -14,7 +14,7 @@
     <!-- Most of this table will be recalculated by the compiler -->
     <tableVersion value="1.0"/>
     <fontRevision value="1.0"/>
-    <checkSumAdjustment value="0x67a819be"/>
+    <checkSumAdjustment value="0x15b40627"/>
     <magicNumber value="0x5f0f3cf5"/>
     <flags value="00000000 00011011"/>
     <unitsPerEm value="2816"/>
@@ -199,12 +199,12 @@
       <FamilyName value="Inter UI"/>
       <Weight value="Semi-bold"/>
       <isFixedPitch value="0"/>
-      <ItalicAngle value="-9.4"/>
+      <ItalicAngle value="-9.399999999999999"/>
       <UnderlinePosition value="-422"/>
       <UnderlineThickness value="286"/>
       <PaintType value="0"/>
       <CharstringType value="2"/>
-      <FontMatrix value="0.000355113636364 0 0 0.000355113636364 0 0"/>
+      <FontMatrix value="0.0003551136363636364 0 0 0.0003551136363636364 0 0"/>
       <FontBBox value="-59 -660 2051 2708"/>
       <StrokeWidth value="0"/>
       <!-- charset is dumped separately as the 'GlyphOrder' element -->

i'll push a fix soon

@anthrotype
Copy link
Member

@rsms although, the latter issue with italicAngle you should be able to fix it yourself, but modifying that value in the fontinfo.plist. Currently it is set to -9.399999999999999. Changing that to -9.4 should work around that particular problem

@behdad
Copy link
Collaborator

behdad commented Jan 4, 2019

What's causing the font to be declared invalid though?

@anthrotype
Copy link
Member

anthrotype commented Jan 4, 2019

the real number as encoded in the CFF table by encodeFloat function seems to be too long (i.e. has too many digits of decimal precision) when run under python3, and trips up macOS font rendering.

this is the output from python2.7

>>> str(1.0/2816)
'0.000355113636364'
>>> from fontTools.misc.psCharStrings import *
>>> encodeFloat(1.0/2816)
'\x1e\xa0\x005Q\x13ccd\xff'
>>> read_realNumber(None, None, b'\x1e\xa0\x005Q\x13ccd\xff', 1)
(0.000355113636364, 10)

and this is the output from python3.7

>>> str(1.0/2816)
'0.0003551136363636364'
>>> from fontTools.misc.psCharStrings import *
>>> encodeFloat(1.0/2816)
b'\x1e\xa0\x005Q\x13ccccd\xff'
>>> read_realNumber(None, None, b'\x1e\xa0\x005Q\x13ccccd\xff', 1)
(0.0003551136363636364, 12)

@anthrotype
Copy link
Member

Interestingly, if I run the ots-validator-checker tool (which is part of the opentype sanitizer, and renders the font using FreeType library) on my Linux box, the font built with python3 with the super-long real number passes with message "OK: FreeType2 didn't crash".
Only when I run the same ots-validator-checker tool on macOS 10.14, I get the error "font renderer couldn't open the transcoded font"

@anthrotype
Copy link
Member

these are the two test fonts, one made with python2.7, the other with python3.7:
ufo2ft-cff-float-bug.zip

If I run them through macOS Font Book.app's "Validate File...", the one made with python2.7 ("output2-B.otf") passes with all green, the other one "output3-B.otf" get

image

the only difference between the two is the one I pasted in my previous comment: #306 (comment)

@anthrotype
Copy link
Member

the ots-validator-checker tool when running on macOS uses CoreText API to load the font, otherwise if the latter isn't available (e.g. on Linux) it uses FreeType:
https://github.com/khaledhosny/ots/blob/dcbdbe08b207964c39ea53b0998448df6a93d1fd/util/ots-validator-checker.cc#L35-L69

If we round to 15 decimal digits (which is plenty) in encodeFloat function in psCharStrings module, before converting the float to str, then python2 and 3 produce the same output and CoreText is happy.

@behdad Shall I just do that?

@anthrotype
Copy link
Member

actually, I need to set it to maximum 14 decimal digits of precision.
If I use 15 digits, then the ItalicAngle="-9.399999999999999" in the test input-B.ufo stays like that (is not rounded to 9.4) and it makes CoreText complain.

@anthrotype
Copy link
Member

something like this patch works:

diff --git a/Lib/fontTools/misc/psCharStrings.py b/Lib/fontTools/misc/psCharStrings.py
index 555d9d50..5e854549 100644
--- a/Lib/fontTools/misc/psCharStrings.py
+++ b/Lib/fontTools/misc/psCharStrings.py
@@ -225,7 +225,7 @@ def encodeFixed(f, pack=struct.pack):

 def encodeFloat(f):
        # For CFF only, used in cffLib
-       s = str(f).upper()
+       s = str(round(f, 14)).upper()
        if s[:2] == "0.":
                s = s[1:]
        elif s[:3] == "-0.":

@anthrotype
Copy link
Member

by trial and error, it appears that what triggers this issue is the length (in bytes) of the encoded real number. When it is up to 10 bytes, it is accepted; if it is > 10, it is rejected as invalid.
For example, the number -9.399999999999999, when run through encodeFloat (on py3) yields b'\x1e\xe9\xa3\x99\x99\x99\x99\x99\x99\x99\xff' which is 11 bytes long, and thus rejected.
If I remove one digit or I remove the leading sign (I make the number positive with the same number of decimal digits), it is encoded with 10 bytes and it passes...

maybe the encodeFloat should be capped to max 10 bytes?

/cc @readroberts

@rsms
Copy link
Author

rsms commented Jan 4, 2019

I've put together a better version of encodeFloat

  • does not have the "E+" bug of encodeFloat
  • limits precision to 14 decimals
  • about twice as fast

https://gist.github.com/rsms/efd5663749a9b66bc53be8f85becc9d2#file-encode_float-py-L78

Comes with tests and benchmarks, using the number decoder from OTFCC for test reference.

Benchmark results from a run on my machine:

  • encodeFloat1 (old with E+ patch): 4.601s total, 46.0 us per call
  • encodeFloat2 (new): 2.521s total, 25.2 us per call

(I do realize this is in the fontTools code and not the ufo2ft code)

@rsms
Copy link
Author

rsms commented Jan 4, 2019

Patch submitted to the fonttools project: fonttools/fonttools#1430

@anthrotype
Copy link
Member

patch was merged upstream, a new fonttools release should be out later today, after which we shall bump the requirement in ufo2ft and release the latter as well.

@anthrotype
Copy link
Member

released ufo2ft 2.6.0

rsms added a commit to rsms/inter that referenced this issue Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants