-
Notifications
You must be signed in to change notification settings - Fork 219
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
base: master
Are you sure you want to change the base?
vhea
and glyf
fixes
#305
Conversation
Signed-off-by: Vsevolod Volkov <[email protected]>
Signed-off-by: Vsevolod Volkov <[email protected]>
Signed-off-by: Vsevolod Volkov <[email protected]>
src/tables/vhea.js
Outdated
majorVersion: r.uint16, // Major version number of the Vertical Header Table | ||
minorVersion: r.uint16, // Minor version number of the Vertical Header Table |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Line 5 in a5fe0a1
version: r.int32, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 <[email protected]>
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.For TrueType fonts, the values in the
loca
table were not validated when calculating cbox. Therefore, the data from theglyf
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.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).