-
Notifications
You must be signed in to change notification settings - Fork 764
Segfault fix and complete implementation of "Don't generate FFTM tables" #5509
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
Conversation
|
Looks like some Windows checks are failing due to remote configuration. What should I do? |
Please do nothing at the moment, the failure is being addressed in jtanx/fontforgebuilds#22 |
It should be ok now, please push empty commit to trigger CI run. |
How do I do that? |
|
|
Conversely, I could have accessed fftm option through at->gi.flags everywhere (exporting options are saved in glyphinfo?!?), but this seems less reliable than what's passed from the UI down to _WriteTTFFont and WriteTTC. That is, in both these functions there is an alltabs variable, but it is generated there, not passed down. And it is again re-generated in ttc_prep, I suppose from the output of all open fonts. At each of these regenerations of alltabs we'd have to make sure that the flags are copied properly. |
|
P.S.: I did some additional testing on the repository's original code. When generating a TTC file, the value of at->gi.flags in ttf_fftm_dump does not change no matter if you check FFTM or not. It does change when you generate a single font, though. So maybe FontForge gets confused as to which opened font's glyphinfo flags has to be used; I think it picks up the last one, which does not guarantee that is where the user has initiated the generate TTC command (and changed the settings). So in both cases (TTC or single font), I think it's better to use the value that is passed down from _WriteTTFFont and WriteTTC. |
skef
left a comment
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 haven't checked to see if any fftm references were missed, but what is here looks good. @iorsh thoughts?
|
Looks like everything is covered here. Looks good to me. |
The "Don't generate FFTM tables" option was not only not working, its partial implementation was causing a segfault when user aksed not to include FFTM and generated a TTC file.
See #5508 and #4417
The issue was:
There was, surprisingly, no FFTM check at all at all places where FFTM is mentionned in tottf.c, is to say in:
* ttc_perfonttables, called by ttc_dump (which is called by WriteTTC)
* buildtablestructures, called by:
** initTables (which is called by _WriteTTFFont)
** ttc_dump (two places) (which is called by WriteTTC)
* ttc_dump, called by WriteTTC
* ttc_prep, called by WriteTTC
* initTables, called by _WriteTTFFont
I fixed this issue by first propagating the flags already present in _WriteTTFFont and WriteTTC, down to initTables, ttc_dump, then to buildtablestructures, and performing the necessary check in ttc_perfonttables, buildtablestructures, ttc_dump, ttc_prep and initTables. This includes before calling ttf_fftm_dump, thus I moved the FFTM check outside of ttf_fftm_dump (we shouldn't get in there if no FFTM is to be generated) and reformed the function.
Since all of these FFTM-related stuff were entangled in _WriteTTFFont and WriteTTC, this patch should fix the issue I reported and also those related to FFTM being generated against users' will.
I tested generating TrueType, TrueType TTC, OpenType CID, all of them with FFTM checked and unchecked, Mac checked and unchecked, and everything seems to work.