Skip to content

Fix saving loca index format/version when subsetting #191

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

Merged
devongovett merged 1 commit into from
Mar 11, 2019
Merged

Fix saving loca index format/version when subsetting #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/subset/TTFSubset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ export default class TTFSubset extends Subset {
this.glyf = [];
this.offset = 0;
this.loca = {
offsets: []
offsets: [],
version: this.font.loca.version
Copy link
Member

@devongovett devongovett Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@blikblum blikblum Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

this.hmtx = {
Expand All @@ -77,7 +78,6 @@ export default class TTFSubset extends Subset {
maxp.numGlyphs = this.glyf.length;

this.loca.offsets.push(this.offset);
Tables.loca.preEncode.call(this.loca);

let head = cloneDeep(this.font.head);
head.indexToLocFormat = this.loca.version;
Expand Down
5 changes: 0 additions & 5 deletions src/tables/loca.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ loca.process = function() {
};

loca.preEncode = function() {
if (this.version != null) return;

// assume this.offsets is a sorted array
this.version = this.offsets[this.offsets.length - 1] > 0xffff ? 1 : 0;

if (this.version === 0) {
for (let i = 0; i < this.offsets.length; i++) {
this.offsets[i] >>>= 1;
Expand Down
17 changes: 17 additions & 0 deletions test/subset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member

@devongovett devongovett Mar 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this font wasn't committed.

Copy link
Member Author

@blikblum blikblum Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for committing it for me

let subset = font.createSubset();
for (let glyph of font.glyphsForString('abcd')) {
subset.includeGlyph(glyph);
}

subset.encodeStream().pipe(concat(function(buf) {
let f = fontkit.create(buf);
assert.equal(f.numGlyphs, 5);
assert.equal(f.getGlyph(1).path.toSVG(), font.glyphsForString('a')[0].path.toSVG());
// must test also second glyph which has an odd loca index
assert.equal(f.getGlyph(2).path.toSVG(), font.glyphsForString('b')[0].path.toSVG());
done();
}));
});
});

describe('CFF subsetting', function() {
Expand Down