Fix saving loca index format/version when subsetting #191
Conversation
|
@blikblum Thanks for this PR: As far as I see, this does not change the API. However, after having updated fontkit I still get the same results as described here bpampuch/pdfmake#1659. Not sure if I did everything the right way. Does this require further changes in the downstream modules? |
|
Did you rebuild font kit? |
|
Check, I built and it worked! Great! |
| @@ -57,7 +57,8 @@ export default class TTFSubset extends Subset { | |||
| this.glyf = []; | |||
| this.offset = 0; | |||
| this.loca = { | |||
| offsets: [] | |||
| offsets: [], | |||
| version: this.font.loca.version | |||
There was a problem hiding this comment.
I think this will cause us to switch to the large format sometimes when not needed. For example, if the original font contained a lot of glyphs, but the subset only contains a few, then we shouldn't need to use the large format.
There was a problem hiding this comment.
This should not be a issue because the subset size would still be proportional to the original font. Say we have a font with 1000 glyphs with same size, a subset with 100 glyphs of such font would have the expected 10% size of the original.
Also is not doable to convert from large to short format because would require to make all offsets even numbers, needing to add some sort of data padding complicating things at write and read time. This could even work with fontkit but would not work for, e.g, pdf readers
In a font i was testing (NanumGothic) the .notdef (id 0) glyph has a size of 87 bytes = the offset of the first glyph in the subset. Since the short format requires the offset to be stored divided by 2, it saves as 43. When reading the subset, the offset is multiplied by 2 getting 86 -> the glyph data is read incorrectly.
To convert to short format it would need pad the id 0 glyph (or any odd sized glyph) to have one more byte or store a lookup somewhere the glyphs that need to have the offset corrected at read time. This should work in fontkit only and the complexity (and size increase) is not worth.
| @@ -56,6 +56,23 @@ describe('font subsetting', function() { | |||
| done(); | |||
| })); | |||
| }); | |||
|
|
|||
| it('should handle fonts with long index to location format (indexToLocFormat = 1)', function(done) { | |||
| let font = fontkit.openSync(__dirname + '/data/FiraSans/FiraSans-Regular.ttf'); | |||
There was a problem hiding this comment.
Looks like this font wasn't committed.
There was a problem hiding this comment.
Thanks for committing it for me
FWIW I work on a Racket port of |
Nice, please create a PR so we can review. |
|
I work on a Racket port of |
Fixes saving the loca index format when subsetting.
Fonts with version 1 (long format) were being saved with version 0. This was mangling the odd offsets (they were being saved divided by two and read multiplied by two creating a off by one error)
Probably fixes bpampuch/pdfmake#1659 and foliojs/pdfkit#914