foliojs / fontkit

An advanced font engine for Node and the browser
1.46k stars 219 forks source link

CFF glyph parsing doesn’t work with WOFF #199

Closed mbutterick closed 4 years ago

mbutterick commented 5 years ago

The problem is here. This sets up stream to be this._font, which is a stream representing the whole font file. In a regular OTF, this works because the offsets in cff.topDict.CharStrings are global (that is, relative to the beginning of the font file).

However this is apparently not true in WOFF — the offsets in cff.topDict.CharStrings are relative to the beginning of the CFF table.

The fix is to check if this._font is a WOFF font, and if so, use the CFF table stream as the value for stream (instead of this._font itself).

mbutterick commented 5 years ago

It may also be possible to instead fix this by changing how the CFFPointer is resolved — maybe from global to parent? — I couldn’t sort that out however.

blikblum commented 5 years ago

Do you have a sample code showing the error? With it i can try to fix it

mbutterick commented 5 years ago

I’m afraid I don’t have a full build of fonkit available. I’d suppose that if you repeat this CFF subsetting test with SourceSansPro-Regular.woff instead of SourceSansPro-Regular.otf, the problem will surface.

mbutterick commented 5 years ago

(I discovered the problem by trying to embed a WOFF font with pdfkit)

Pomax commented 5 years ago

On a spec-technical note: CFF offsets are never relative to the start of the font file, they're only ever relative either to the start of the CFF block, or to some well defined struct in it. CFF is a self contained data structure, because it's intended for arbitrary embedding, and was designed in a way that it can't know anything about whatever thing (SFNT, PDF, etc) it's in.

See Adobe Tech Note 5167 - The Compact Font Format Specification, page 8:

Data objects are often specified by byte offsets that are relative to some reference point within the CFF data. These offsets are 1 to 4 bytes in length.

It sounds like step 1 here is to create an actual test case that fontkit can be run against to verify things are indeed broken.

blikblum commented 5 years ago

(I discovered the problem by trying to embed a WOFF font with pdfkit)

With pdfkit, the supposed error can occur in other places than fontkit. A fontkit only example must be used

Anyway, here's a pdfkit example working fine with woff file https://repl.it/@blikblum/pdfkit-opensans-woff

It sounds like step 1 here is to create an actual test case that fontkit can be run against to verify things are indeed broken.

Exact. This is what i'm trying to get. Once a minimal, self contained sample code, showing the error is provided, i can work on a fix .

Pomax commented 5 years ago

Looking at the glyph tests, this should be pretty easy, as we currently have an anonymous SourceSans version in test/data/SourceSansPro.

Let's replace those with the official SourceSansPro-Regular.ttf.woff, SourceSansPro-Regular.otf.woff, SourceSansPro-Regular.ttf.woff2, and SourceSansPro-Regular.otf.woff2 sources from the SourceSans repo, and then extend the test/glyphs.js tests so that the WOFF and WOFF2 blocks cover both flavours of OpenType:

  describe('WOFF glyphs', function() {
    ['ttf', 'otf'].forEach(type => {
      let font = fontkit.openSync(__dirname + '/data/SourceSansPro/SourceSansPro-Regular.' + type + '.woff');
      let glyph = font.glyphsForString('D')[0];

      it('should get the glyph name', function() {
        return assert.equal(glyph.name, 'D');
      });

      if (type === 'ttf') {
        it('should get a TTFGlyph', function() {
          return assert.equal(glyph.constructor.name, 'TTFGlyph');
        });

        it('should get a quadratic path for the glyph', function() {
          return assert.equal(glyph.path.toSVG(), 'M90 0L90 656L254 656Q406 656 485 571.5Q564 487 564 331Q564 174 485.5 87Q407 0 258 0ZM173 68L248 68Q363 68 420.5 137.5Q478 207 478 331Q478 455 420.5 521.5Q363 588 248 588L173 588Z');
        });
      }

      if (type === 'otf') {
        it('should get a CFFGlyph', function() {
          return assert.equal(glyph.constructor.name, 'CFFGlyph');
        });

        it('should get a cubic path for the glyph', function() {
          // THIS FAILS, THERE IS NO PATH - THAT'S BAD!
          return assert.equal(!!glyph.path.toSVG(), true);
        });
      }

    });
  });

  describe('WOFF2 glyph', function() {
    ['ttf', 'otf'].forEach(type => {
      let font = fontkit.openSync(__dirname + '/data/SourceSansPro/SourceSansPro-Regular.' + type + '.woff2');

      let glyph = font.glyphsForString('D')[0];
      let expectedBox = new BBox(90, 0, 564, 656);

      it('should get the glyph name', function() {
        return assert.equal(glyph.name, 'D');
      });

      it('should get a WOFF2Glyph', function() {
        // THIS FAILS FOR OTF.WOFF2 - THAT'S BAD!
        return assert.equal(glyph.constructor.name, 'WOFF2Glyph');
      });

      if (type === 'ttf') {
        it('should get a correct quadratic path for all contours', function() {
          return assert.equal(glyph.path.toSVG(), 'M90 0L90 656L254 656Q406 656 485 571.5Q564 487 564 331Q564 174 485.5 87Q407 0 258 0ZM173 68L248 68Q363 68 420.5 137.5Q478 207 478 331Q478 455 420.5 521.5Q363 588 248 588L173 588Z');
        });

        it('should get the ttf glyph cbox', function() {
          return assert.deepEqual(glyph.cbox, expectedBox);
        });

        it('should get the ttf glyph bbox', function() {
          return assert.deepEqual(glyph.bbox, expectedBox);
        });
      }

      if (type === 'otf') {
        it('should get a correct cubic path for all contours', function() {
          return assert.equal(glyph.path.toSVG(), 'M90 0L258 0C456 0 564 122 564 331C564 539 456 656 254 656L90 656ZM173 68L173 588L248 588C401 588 478 496 478 331C478 165 401 68 248 68Z');
        });

        it('should get the otf glyph cbox', function() {
          return assert.deepEqual(glyph.cbox, expectedBox);
        });

        it('should get the otf glyph bbox', function() {
          return assert.deepEqual(glyph.bbox, expectedBox);
        });
      }
    });
  });

Note that two tests that were assumed to pass actually fail with otf thrown into the mix: the otf.woff is failing to get glyph data, and the otf.woff2 yields a glyph that "works" correctly but isn't a WOFF2Glyph object:

528 passing (2s)
  2 failing

  1) glyphs WOFF glyphs should get a cubic path for the glyph:

      AssertionError [ERR_ASSERTION]: false == true
      + expected - actual

      -false
      +true

      at Context.<anonymous> (J:/Junctions/Users/Pomax/Documents/Git/temp/fontkit/test/glyphs.js:185:25)
      at callFn (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runnable.js:326:21)
      at Test.Runnable.run (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:422:10)
      at J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:528:12
      at next (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:342:14)
      at J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:352:7
      at next (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:284:14)
      at Immediate._onImmediate (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:320:5)

  2) glyphs WOFF2 glyph should get a WOFF2Glyph:

      AssertionError [ERR_ASSERTION]: 'CFFGlyph' == 'WOFF2Glyph'
      + expected - actual

      -CFFGlyph
      +WOFF2Glyph

      at Context.<anonymous> (J:/Junctions/Users/Pomax/Documents/Git/temp/fontkit/test/glyphs.js:205:23)
      at callFn (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runnable.js:326:21)
      at Test.Runnable.run (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runnable.js:319:7)
      at Runner.runTest (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:422:10)
      at J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:528:12
      at next (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:342:14)
      at J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:352:7
      at next (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:284:14)
      at Immediate._onImmediate (J:\Junctions\Users\Pomax\Documents\Git\temp\fontkit\node_modules\mocha\lib\runner.js:320:5)

npm ERR! Test failed.  See above for more details.
mbutterick commented 5 years ago

Anyway, here's a pdfkit example working fine with woff file https://repl.it/@blikblum/pdfkit-opensans-woff

OpenSans is a WOFF with TTF tables inside. To reveal a bug with CFF parsing, you would need to test a WOFF with a CFF table.

PS: Thanks for showing me repl.it. Here’s a forked version of your test that fails with a CFF-flavored WOFF.

Let's replace those with the official SourceSansPro-Regular.ttf.woff, SourceSansPro-Regular.otf.woff, SourceSansPro-Regular.ttf.woff2, and SourceSansPro-Regular.otf.woff2 … Note that two tests that were assumed to pass actually fail with otf thrown into the mix: the otf.woff is failing to get glyph data

Right — consistent with my prediction.

You show me a test that’s “assumed to pass” and I’ll show you a failing test :grin:

mbutterick commented 5 years ago

CFF is a self contained data structure, because it's intended for arbitrary embedding, and was designed in a way that it can't know anything about whatever thing (SFNT, PDF, etc) it's in.

In that case, I suspect the deeper problem has to do with the CFFPointer being defined relative to a global byte position. I think this works “accidentally” in certain cases but is not consistent with the spec. It seems like CFFPointer should be relative to the start of the CFF table, and then CFFGlyph should parse out of this data stream, not a stream representing the whole font.

blikblum commented 5 years ago

@Pomax thanks. Reproduced here.

Why did you get the glyph for 'D' instead 'T' like other tests?

blikblum commented 5 years ago

and the otf.woff2 yields a glyph that "works" correctly but isn't a WOFF2Glyph object:

This behavior seems to correct, since WOFF2Glyph is specific for TTF (descends from TTFGlyph)

Pomax commented 5 years ago

because the T doesn't have a cutout, and could even be pure lines, so it's a bit of a silly choice for a testing glyph. As for the WOFF2Glyph vs. CFFGlyph: that should be fine, then.

blikblum commented 5 years ago

I fixed getting the path. May be there are other places that need to be fixed accordingly