Is the calculation of maxComponentDepth really correct?

[be5invis]

Since FV is always complaining about maxComponentDepth being too small for Iosevka I walked into its source to see how it analyzes the component depth of a particular glyph. And the logic in GetCompositeGlyphStats (https://github.com/Microsoft/Font-Validator/blob/master/OTFontFileVal/val_maxp.cs#L371) seems that, every time you recurse DOWN you increase a counter, which may perform incorrect number in a complex glyph like this:

2785 [
  2665 [        <- trigger nComponentDepth++
    2601 [      <- trigger nComponentDepth++
      2597 [    <- trigger nComponentDepth++
        2595    <- trigger nComponentDepth++
        2595
      ]
      2595
    ]
    2595
  ]
  2601 [
    2597 [       <- trigger nComponentDepth++
      2595       <- trigger nComponentDepth++ 
      2595
    ]
    2595
  ]
]

The recursion depth of glyph 2785 is 4 (2785 → 2665 → 2601 → 2597 → 2595) but FV increased the counter by two more times when looking inside its second reference.

[be5invis]

cc. @HinTak

[HinTak]

@be5invis : I think you are making assumptions about the rendering engine(s) behaving in a certain idealized way ( namely, de-allocating nested component glyphs when they go out of scope) which may not necessarily be true in general, or historically. You cannot make such assumptions about all engines current and past. Therefore, it is prudent to err on the generous side - i.e. the current behavior. I don't think the curent code should be changed, if that's what you are suggesting.

[be5invis]

I think as an validator it should follow the Spec, rather than details of rasterizers. 发自我的 iPhone 在 2017年10月22日，02:52，HinTak <notifications@github.com<mailto:notifications@github.com>> 写道： @be5invis<https://github.com/be5invis> : I think you are making assumptions about the rendering engine(s) behaving in a certain idealized way ( namely, de-allocating nested component glyphs when they go out of scope) which may not necessarily be true in general, or historically. You cannot make such assumptions about all engines current and past. Therefore, it is prudent to err on the generous side - i.e. the current behavior. I don't think the curent code should be changed, if that's what you are suggesting. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAOp28pARr7-XMAeGBeN7pijTzY4k39Qks5suj13gaJpZM4QBcV->.

[HinTak]

I guess, the question is, did the spec specify how it should be calculated? If not, then it shall be calculated as the maximum of all common implementations of all the different engines past and present, to be compatible with all common engines past and present. (i.e. the current code).

[be5invis]

The Spec said that it is the level of recursion, so for my example, 4 is the correct value. I also asked some guys in MSFT and they confirmed this, and rasterizers in Windows can handle the recursion properly with right depth. 发自我的 iPhone 在 2017年10月22日，03:42，HinTak <notifications@github.com<mailto:notifications@github.com>> 写道： I guess, the question is, did the spec specify how it should be calculated? If not, then it shall be calculated as the maximum of all common implementations of all the different engines past and present, to be compatible with all common engines past and present. (i.e. the current code). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAOp2zttxENmWsGjWAHZW4x5j19hXlVIks5sukkIgaJpZM4QBcV->.

[HinTak]

i.e. if the spec says the value should be such and such, then engine implmentations should be corrected to behave that way. If the spec did not say, then engines are allowed to behave differently. In that case, validator should use the maximum value from all such engines.

[HinTak]

You really mean the current windows rasterizer can handle a tight value. Anyway, if there is any argument on such max* values, the larger value should be used, which is again, the current code.

[be5invis]

I also asked the former maintainer of FV and he said that it might be a bug. The Spec said that it should be level of recursion, not “times we recourse”. 发自我的 iPhone 在 2017年10月22日，03:47，HinTak <notifications@github.com<mailto:notifications@github.com>> 写道： You really mean the current windows rasterizer can handle a tight value. Anyway, if there is any argument on such max* values, the larger value should be used, which is again, the current code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#62 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAOp28axTqNIy03Qp-8f-5lgYB25WdAtks5sukpEgaJpZM4QBcV->.

[HinTak]

@be5invis well, if you can find the former maintainer of FV, then you should just get him to issue a pull and explain his reasoning himself, and get his changes merged...

[be5invis]

@HinTak Sorry for late reply.
Per Greg Hitchcock:

It should be the deepest leaf. That is what the TrueType rasterizer assumes.
If the Validator doesn't represent the deepest leaf, then yes, it is wrong.

[HinTak]

@be5invis : Can you prepare a patch?

[santhoshtr]

In case if it is relevant here, fonttools recently fixed a 21 year old bug about the calculation of maxComponentDepth. fonttools/fonttools#2044

It would be helpful to cross check the calculations so that fonts using fontools using build chain and later validating the fonts using MS FontValidator not affected by this mismatch.