search

fonttools / fontbakery

🧁 A font quality assurance tool for everyone
https://fontbakery.readthedocs.io
Apache License 2.0
554 stars 103 forks source link

All blacklisted fonts have issues with hinting tables #1021

Closed felipesanches closed 7 years ago

felipesanches commented 8 years ago

All blacklisted fonts on fontbakery-build-fontmetadata.py have issues with the OpenType hinting tables that trigger the check at https://github.com/felipesanches/fontbakery/blob/06945641497a642422a101d904420fda3eca9815/fontbakery-build-fontmetadata.py#L588-L607

See issue #704. I attempted to remove each of the blacklisted fonts in order to replicate the problem that originally caused them to be blacklisted. But instead all of those were auto-blacklisted by the all-or-nothing hinting tables criteria described at https://github.com/googlefonts/fontbakery/issues/705#issuecomment-244800828.

@davelab6 edited this issue to consolidate tasks from the discussion below.

felipesanches commented 8 years ago

And indeed, if I disable the hinting tables check I end up getting a division by zero that results from a detected text_height value of zero !

Seems likely to be caused by the bad hinting tables perhaps?

davelab6 commented 7 years ago

Next actions on this are...

felipesanches commented 7 years ago

As there are only 6 remaining blacklisted font files, I think we can track them all here. They are:

#IOError: execution context too long (issue #703)
  "Padauk",
  "KumarOne",

#ZeroDivisionError: float division by zero
  "AdobeBlank",
  "Phetsarath",

# IOError: invalid reference See also: https://github.com/google/fonts/issues/132#issuecomment-244796023
  "Corben", 

# IOError: stack overflow on text_width, text_height = font.getsize(TEXT) 
  "Rubik",
HinTak commented 7 years ago

"stack overflow" and "division by zero", if happened in the hinting instructions, are part of FontVal 2.0. "execution too long" is part of FontVal 2.1 .

felipesanches commented 7 years ago

source code patches are welcome!

HinTak commented 7 years ago

The "details" part of all Fontval's errors/warnings for hinting instructions do tell you exactly what byte offsets when the offending instruction happens, and what that instruction is. While not actually source-code patches to the fonts, it tells you exactly where to look and patch...

laerm0 commented 7 years ago

@davelab6 Is this a hotfix or should I regenerate from source?

davelab6 commented 7 years ago

These are hinting errors, so no, I think that regenerating may be too much.

Rubik should be fixed by Marc compiling the VTT data and submitting a new PR.

For AdobeBlank the error should be reported upstream. Would this be correct?

Topic: HinTak FontVal: "ZeroDivisionError: float division by zero" Body: The HinTak fork of MS FontVal says that AdobeBlank fails in hinting with

For the others, if you can still reproduce this in Padauk, KumarOne, Phetsarath and Corben, try roundtripping through TTX and that might fix it; if not, try stripping the hints (with ttfautohint -d) and then rehinting with latest release of ttfautohint and see if that fixes it.

HinTak commented 7 years ago

Most of the other errors are because of hinting being fundamentally broken, but for the stack overflow error, it could be maxp not quite right too, so you might need to change maxp instead (don't know if ttfautohint does that). Difficult to tell without see the fonts or the full output.

davelab6 commented 7 years ago

Thanks HinTak!

felipesanches commented 7 years ago

Indeed I've been thinking about it and it might be useful if ttfautohint had an option to dry-run not changing the actual hinting code, but instead suggesting an adequate value for max stack size on maxp.

felipesanches commented 7 years ago

In the more general case, this kind of computation of the ideal stacksize value for the maxp table could be inferred by loading the font with freetype with an excessive stack memory allocation and then tracing what maximum value was reached at runtime.

felipesanches commented 7 years ago

I only suggest that because it seems that it would be impractical or really challenging to calculate the max stack size needed with static analysis of the bytecode. Inference via a runtime test-run would be much easier to implement. And I believe it would be safe since hinting bytecode always runs with the same input (the outlines of the full glyph-set of a font) so it is not prone to having a different stack-size requirement depending on user input - there's no varying user input at all as far as I can tell, so the proposed dynamic analysis would be very deterministic.

HinTak commented 7 years ago

Yes and no. Freetype over-allocates maxstack by 32 and does not trust maxp as correct. So Freetype's behavior does not match microsoft's, and is of no use if you want your fonts to work on windows.

FontVal 2.1's behavior is tuned to match Microsoft's, rather than Freetype's, while still preserving Freetype's general fault-tolerance. Needless to say, FontVal 2.1 's behavior matches the legacy Microsoft 1.0 backend in this. FontVal 2.0 exhibits the Freetype over-allocate behavior and was reported as a bug a while ago.

felipesanches commented 7 years ago

Exactly what I meant ! :-)

HinTak commented 7 years ago

The stack requirement can change with requested glyph size . Hence you have to run with a range of glyph sizes to see whether it is exceeded. I seem to be trying to explain why the microsoft people wrote what they wrote, in the font validator that way :-).

felipesanches commented 7 years ago

yep, with point sizes from 4 to 72...

HinTak commented 7 years ago

not just 4-72 ; up to 126. Here is the list:

 vp.sizes = List[int]([4,5,6,7,8,9,10,
                              11,12,13,14,15,16,17,18,19,20,
                              21,22,23,24,25,26,27,28,29,30,
                              31,32,33,34,35,36,37,38,39,40,
                              41,42,43,44,45,46,47,48,49,50,
                              51,52,53,54,55,56,57,58,59,60,
                              61,62,63,64,65,66,67,68,69,70,
                              71,72,
                              80,88,96,102,110,118,126])
HinTak commented 7 years ago

Btw, removing the 32-over-allocation to match Microsoft's strict behavior, is the kind of change that FontVal does , which could never be upstreamed.

Freetype always goes on the generous side , assuming the value is always wrong, but FontVal needs to know exactly when the assumed-correct value is exceeded.

felipesanches commented 7 years ago

Sure. But the boundary check could compensate for that overallocation testing and logging whenever the stack reaches "allocated minus overallocation". And that would be perfectly upstreamable as it is just a diagnostics log call as all the other ones.

HinTak commented 7 years ago

I don't think this is a healthy discussion - code that is of no use for upstream is just random crap, that one throws at others to ask them to maintain afterwards. Rather irresponsible.