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

vhea and glyf fixes #305

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

StLyn4
Copy link

@StLyn4 StLyn4 commented Apr 7, 2023

  1. This branch fixes a bug with the processing of the vhea table. The version should consist of two 16-bit numbers instead of one. Because of this, errors have previously occurred in the processing of some fonts that use a vertical matrix.

  2. For TrueType fonts, the values in the loca table were not validated when calculating cbox. Therefore, the data from the glyf table was loaded even if it simply does not exist (for example, the space glyph). Added a basic check for the presence of data, and if there is none, it tries to calculate cbox using the "classic" method.

  3. If the glyph does not contain drawing commands (and, accordingly, coordinate points), then cbox / bbox contained Infinity as minX, minY, maxX, maxY values. Again, say hello to space. Therefore, a check has been added for such a case, with setting all previously named parameters to zero, as is done in FreeType (https://gitlab.freedesktop.org/freetype/freetype/-/blob/4d8db130ea4342317581bab65fc96365ce806b77/src/truetype/ttgload.c#L1648).

Signed-off-by: Vsevolod Volkov <st.lyn4@gmail.com>
Signed-off-by: Vsevolod Volkov <st.lyn4@gmail.com>
Signed-off-by: Vsevolod Volkov <st.lyn4@gmail.com>
Comment on lines 5 to 6
majorVersion: r.uint16, // Major version number of the Vertical Header Table
minorVersion: r.uint16, // Minor version number of the Vertical Header Table
Copy link
Contributor

@Pomax Pomax Apr 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the vhea version field is a 16dot16 number, which is not the same as two 16 bit ints.

The first number is a uint16, but the second number uses a very different format, in which only values 0x0000, 0x1000, ..., 0x9000 are valid, and represent minor versions 0 through 9. When parsed as uint16, the result needs to be bitshifted right by 12 to get the actual value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good day! 👋 Thank you for the clarification. As I see it, the maxp table uses the same approach (16dot16) to versioning. In this library, maxp describes the version as a simple 32-bit number with no additional major/minor divisions, because version, as I understand it, is not used anywhere else.

version: r.int32,
Should I apply the same approach here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, there are three tables (maxp, post, vhea) that use 16dot16 for historical reasons, and thankfully no future table will ever be allowed to use it again. If the maxp table uses a 32 int, that may need fixing too, as there are technically two versions for the maxp table (0.5 and 1.0).

Copy link
Contributor

@Pomax Pomax Apr 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at https://github.com/foliojs/fontkit/tree/master/src/tables/post.js, it seems like that table, too, is not doing the right thing: as per the OpenType docs, "In earlier versions, these fields were documented as using a Fixed value, but had minor version numbers that did not follow the definition of the Fixed type" so if you want to make your code do "the same" as the post table, using the r.fixed32 as versioning field is probably the right way to go, unless restructure adds a 16dot16 type.

Hopefully @devongovett can have a quick look here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that's fun. You might have to implement a custom type for that. Here is how Fixed is implemented. You'd need to do something similar to handle this type.

I would also not switch this to two separate fields as that might be considered a breaking change (if someone was using the version field before).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a small encoder/decoder that should handle the given type and applied it to all 3 tables. A small note regarding the Base class of the restructure library: it is not exported, so we had to abandon inheritance and add the fromBuffer and toBuffer methods directly to Version16Dot16. The test results did not change in any way (at least no degradation was noticed).

Signed-off-by: Vsevolod Volkov <st.lyn4@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants