[ot-font] Implement reading glyph names #568
Conversation
|
In CFF-based fonts, glyph names are stored inside the CFF table, not the post table. It seems that this PR doesn't take it into account, right?? |
|
Also: decoding CFF may a bit be expensive, so I'm not sure if it should be done in HB (especially since it will most likely be parsed again during the imaging step) |
|
Yes, HarfBuzz ot font functions do not currently parse CFF table. |
|
Thanks for this! I'll review it soon. BTW, CFF doesn't have to be slow... We'll eventually implement it. |
src/hb-ot-post-table.hh
Outdated
|
|
||
| if (version.to_int () == 0x00010000) | ||
| { | ||
| strncpy(buffer, format1_glyph_names[glyph], buffer_length); |
There was a problem hiding this comment.
You don't check glyph before accessing format1_glyph_names array.
src/hb-ot-post-table.hh
Outdated
| return false; | ||
|
|
||
| unsigned int index = v2.glyphNameIndex[glyph]; | ||
| if (index > 257) |
src/hb-ot-post-table.hh
Outdated
| if (i == index - 258) | ||
| { | ||
| name_length = MIN (name_length, buffer_length); | ||
| memcpy (buffer, data + 1, name_length); |
There was a problem hiding this comment.
This can access past the blob end...
There was a problem hiding this comment.
Hopefully fixed now.
|
Good to know that CFF performance isn't potentially a showstopper. BTW, I'm glad that with CFF2, Adobe followed my suggestion to remove glyph names from the table and use "post". I remember that initially they planned to keep names in CFF2, but having them in "post" regardless of the outline flavor is much better (especially since we now have all those color glyph flavors and when intermixing them with monochrome flavors, keeping track of where glyph names should be when would have been very annoying). |
Turns out we already have support for “post” table, it just needed to be activated and put in use.
| inline bool get_glyph_name (hb_codepoint_t glyph, | ||
| char *name, unsigned int size) const | ||
| { | ||
| if (unlikely (!name) || unlikely(!size)) |
There was a problem hiding this comment.
Humm. Returning false doesn't sound quite right. Don't know. Maybe doesn't matter.
| if (glyph >= NUM_FORMAT1_NAMES) | ||
| return false; | ||
|
|
||
| strncpy(buffer, format1_names[glyph], buffer_length); |
There was a problem hiding this comment.
From man strncpy:
strncpy() produces an unterminated string in dest. If buf has length
buflen, you can force termination using something like the following:
strncpy(buf, str, buflen - 1);
if (buflen > 0)
buf[buflen - 1]= '\0';
| return false; | ||
|
|
||
| strncpy(buffer, format1_names[glyph], buffer_length); | ||
| buffer[buffer_length] = '\0'; |
There was a problem hiding this comment.
Ok I see you have this. But it should be buffer_length - 1, no? I prefer checking for zero here as well. Maybe just here, not in the caller.
|
I merged and am fixing the rest myself. Thanks Khaled, this has been long overdue! |
Turns out we already have support for “post” table, it just needed to be activated and put in use.
Might not be the best way to handle this, as I don’t really think I totally understand how parsing tables in HarfBuzz works.