Skip to content

[TSI0, TSI5] Derive number of entries to decompile from data length #2477

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Apr 16, 2025
Merged

[TSI0, TSI5] Derive number of entries to decompile from data length #2477

merged 10 commits into from
Apr 16, 2025

Conversation

jenskutilek
Copy link
Collaborator

When you add glyphs to a font containing VTT source tables, the number of glyphs will not match the number of entries in the VTT source tables anymore. This causes the decompilation of VTT source tables to fail.

This change fixes this by deriving the number of entries to decompile from the length of the table data in TSI0 and TSI5.

VTT itself also accepts TSI0 and TSI5 tables that are "too short" for the current number of glyphs.

@anthrotype
Copy link
Member

the number of glyphs will not match the number of entries

but do you not also update maxp.numGlyphs when you add a glyph to the font?

@anthrotype
Copy link
Member

anthrotype commented Dec 16, 2021 •
edited
Loading

oh I see, you do update maxp.numGlyphs, hence the TSI tables become too short.
Can you add tests?

what about the other TSI tables?

anthrotype
anthrotype reviewed Dec 16, 2021
View reviewed changes
@anthrotype
Copy link
Member

to fix the lint job failing, you should rebase on origin/master (or merge origin/master into your PR branch) to include this commit 2226663

@jenskutilek
Copy link
Collaborator Author

what about the other TSI tables?

The other tables don't use the maxp.numGlyphs value. TSI2 uses the code from the TSI0 class and inherits the fix.

@khaledhosny
Copy link
Collaborator

Are there still interest in merging this PR?

@jenskutilek
Format with black
b3c8ada
@jenskutilek
Merge branch 'main' into vttsrc-decompilation-fix
cf47496 
# Conflicts:
#	Lib/fontTools/ttLib/tables/T_S_I__0.py
#	Lib/fontTools/ttLib/tables/T_S_I__5.py
@jenskutilek
Copy link
Collaborator Author

Are there still interest in merging this PR?

Sure! I'll look at the comments and add some tests.

@jenskutilek
Copy link
Collaborator Author

I added another detail: When making a test font with VTT hinting, I noticed that VTT will add zero-length fpgm, prep, and cvt tables. Such a font couldn't be roundtripped with ttx. Fixed in 07830f3.

I've added tests and log warnings when the number of TSI0/5 entries and glyphs don't match. Should be good to go now.

@jenskutilek jenskutilek requested a review from anthrotype April 16, 2025 16:46
@khaledhosny khaledhosny merged commit 7cd12d8 into main Apr 16, 2025
11 checks passed
@khaledhosny khaledhosny deleted the vttsrc-decompilation-fix branch April 16, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthrotype anthrotype Awaiting requested review from anthrotype

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jenskutilek @anthrotype @khaledhosny