Skip to content

Conversation

@Yukinoroh
Copy link
Contributor

@Yukinoroh Yukinoroh commented Dec 10, 2024

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:

  1. WriteTTC (tottf.c) called ttc_prep (tottf.c), which called ttf_fftm_dump (ttfspecial.c), and there the check "if ( at->gi.flags & ttf_flag_noFFTMtable )" succeeded, function returned false and no FFTM table was generated.
  2. The line "tab->offset = ftell(ttc);" after "tab = findtabindir(&all[cnt].tabdir,CHR('F','F','T','M'));" in ttc_dump (tottf.c) caused a segfault, due to the pointer pointing at a place outside of the file.

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.

This was referenced Dec 10, 2024
@Yukinoroh
Copy link
Contributor Author

Looks like some Windows checks are failing due to remote configuration. What should I do?

@iorsh
Copy link
Contributor

iorsh commented Dec 10, 2024

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

@iorsh
Copy link
Contributor

iorsh commented Dec 10, 2024

Looks like some Windows checks are failing due to remote configuration. What should I do?

It should be ok now, please push empty commit to trigger CI run.

@Yukinoroh
Copy link
Contributor Author

Looks like some Windows checks are failing due to remote configuration. What should I do?

It should be ok now, please push empty commit to trigger CI run.

How do I do that?

@iorsh
Copy link
Contributor

iorsh commented Dec 10, 2024

git commit --allow-empty -m "trigger CI"
git push

@Yukinoroh
Copy link
Contributor Author

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.

@Yukinoroh
Copy link
Contributor Author

Yukinoroh commented Dec 15, 2024

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.

Copy link
Contributor

@skef skef left a 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?

@iorsh
Copy link
Contributor

iorsh commented Jan 1, 2025

Looks like everything is covered here. Looks good to me.

@skef skef merged commit ce1c89c into fontforge:master Jan 1, 2025
7 checks passed
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.

3 participants