Fix how we detect and handle missing glyph data. #8580

brendandahl · 2017-06-28T19:07:32Z

I had started looking at #8255 awhile back, but after seeing @Snuffleupagus pr #8572 I realized what was going on.

This fixes a couple of things:

If dupFirstEntry was used in sanitizeGlyphLocations then the glyph count was too high and we ended up reading beyond loca table and disabling glyph that could actually be there.
We didn't always mark missinglyphs as missing, now we always check the glyph length.
We tried to adjust the mapping of missing glyphs. Often there are thousand of missing glyphs in regions that would be re-mapped which caused us to run out of private use locations.

Fixes #8255.
Fixes #8570.
Fixes #7696.

brendandahl · 2017-06-28T19:07:39Z

/botio test

pdfjsbot · 2017-06-28T19:07:40Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/b3573d0a7ceda07/output.txt

pdfjsbot · 2017-06-28T19:07:40Z

From: Bot.io (Windows)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.215.176.217:8877/9bad1d590e6e492/output.txt

pdfjsbot · 2017-06-28T19:37:11Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/9bad1d590e6e492/output.txt

Total script time: 29.51 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/9bad1d590e6e492/reftest-analyzer.html#web=eq.log

pdfjsbot · 2017-06-28T20:07:41Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/b3573d0a7ceda07/output.txt

Total script time: 60.00 mins

brendandahl · 2017-06-28T20:11:36Z

/botio-linux test

pdfjsbot · 2017-06-28T20:11:37Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/0ed859f6e25b17d/output.txt

brendandahl · 2017-06-28T20:12:48Z

I didn't see any failure locally (on macOS), but it looks like things have changed (for the worse) on windows. I'm guessing more glyphs are in the private use area not using the CJK shaper, but I'll have to look into it.

pdfjsbot · 2017-06-28T20:28:10Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/0ed859f6e25b17d/output.txt

Total script time: 16.54 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/0ed859f6e25b17d/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2017-06-28T20:49:47Z

I didn't see any failure locally (on macOS), but it looks like things have changed (for the worse) on windows. I'm guessing more glyphs are in the private use area not using the CJK shaper, but I'll have to look into it.

One additional data-point, in case it's helpful: I ran the full test-suite locally on Windows 7 (in Firefox Nightly 56), and I didn't notice any failures locally either.
However, I've got HWA (hardware acceleration) enabled locally, and I'm assuming that the bot runs with HWA disabled, which may help explain/pinpoint the issue!?

Snuffleupagus · 2017-06-28T22:09:08Z

src/core/fonts.js

@@ -834,6 +834,7 @@ var Font = (function FontClosure() {
      if ((usedFontCharCodes[fontCharCode] !== undefined ||
           isProblematicUnicodeLocation(fontCharCode) ||
           (isSymbolic && !hasUnicodeValue)) &&
+          !missingGlyphs[glyphId] &&


Just asking, and it might be a totally pointless micro-optimization, but would it make sense (at all) to check !missingGlyphs[glyphId] first in this condition? I.e. instead do:

if (!missingGlyphs[glyphId] && (usedFontCharCodes[fontCharCode] !== undefined || isProblematicUnicodeLocation(fontCharCode) || (isSymbolic && !hasUnicodeValue)) && nextAvailableFontCharCode <= PRIVATE_USE_OFFSET_END) { // Room left.

The reason that I'm asking is that we could then avoid pointless isProblematicUnicodeLocation function calls when we know that a glyph is missing, since I'm assuming that it's cheaper to lookup missingGlyphs[glyphId] than it's to call a function?

Again, this might be a completely unnecessary change, but I figured it can't hurt to ask!
Edit: If nothing else, it will help improve my understanding of these things :-)

I think it depends on a number of things and would really need to be benchmarked, but I don't thing this code is particularly hot, so it's not really worth worrying about it. I've updated it to be first though.

brendandahl · 2017-06-29T17:20:15Z

Looks like the windows failures may be a firefox update since the refs failed over in #8585 (comment)

brendandahl · 2017-07-03T20:12:09Z

/botio test

pdfjsbot · 2017-07-03T20:12:10Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.67.70.0:8877/6d0b994bd5d9a33/output.txt

pdfjsbot · 2017-07-03T20:12:10Z

From: Bot.io (Windows)

Received

Command cmd_test from @brendandahl received. Current queue size: 0

Live output at: http://54.215.176.217:8877/f18d8ce6030aa74/output.txt

pdfjsbot · 2017-07-03T20:28:41Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/6d0b994bd5d9a33/output.txt

Total script time: 16.52 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/6d0b994bd5d9a33/reftest-analyzer.html#web=eq.log

pdfjsbot · 2017-07-03T20:41:21Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/f18d8ce6030aa74/output.txt

Total script time: 29.18 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/f18d8ce6030aa74/reftest-analyzer.html#web=eq.log

Snuffleupagus

This is a really nice improvement, not to mention simplification, of the way we detect missing glyphs.
This makes a lot more sense than the old hacks (that I added); thanks for doing this!

Snuffleupagus · 2017-07-04T09:40:35Z

/botio makeref

pdfjsbot · 2017-07-04T09:40:36Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.215.176.217:8877/fe3d0ee9524daaa/output.txt

pdfjsbot · 2017-07-04T09:40:37Z

From: Bot.io (Linux m4)

Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/bcbdfcc542acc37/output.txt

pdfjsbot · 2017-07-04T09:56:16Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/bcbdfcc542acc37/output.txt

Total script time: 15.64 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2017-07-04T10:07:44Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/fe3d0ee9524daaa/output.txt

Total script time: 27.11 mins

Lint: Passed
Make references: Passed
Check references: Passed

Fix how we detect and handle missing glyph data.

brendandahl requested a review from Snuffleupagus June 28, 2017 19:07

timvandermeij added the font-conversion label Jun 28, 2017

Snuffleupagus reviewed Jun 28, 2017

View reviewed changes

Fix how we detect and handle missing glyph data.

6d4f748

brendandahl force-pushed the missing-glyf branch from 3575a09 to 6d4f748 Compare July 3, 2017 20:06

Snuffleupagus approved these changes Jul 4, 2017

View reviewed changes

Snuffleupagus merged commit eff257b into mozilla:master Jul 4, 2017

This was referenced Jul 19, 2017

[Regression] Missing glyph in the issue8187 reference test #8668

Closed

PDF file containing hyeroglyphical characters: cmap Adobe-Japan1-UCS2 (8.001) does not work but 5.001 do. #7696

Closed

This was referenced Jul 23, 2017

PDF text missing #2884

Closed

Add a reduced test-case for issue 7696 #8692

Merged

Spaces are render as dots #8707

Closed

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8580 from brendandahl/missing-glyf

880ec14

Fix how we detect and handle missing glyph data.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix how we detect and handle missing glyph data. #8580

Fix how we detect and handle missing glyph data. #8580

brendandahl commented Jun 28, 2017 •

edited by Snuffleupagus

Loading

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

Snuffleupagus commented Jun 28, 2017

Snuffleupagus Jun 28, 2017 •

edited

Loading

brendandahl Jul 3, 2017

brendandahl commented Jun 29, 2017

brendandahl commented Jul 3, 2017

pdfjsbot commented Jul 3, 2017

pdfjsbot commented Jul 3, 2017

pdfjsbot commented Jul 3, 2017

pdfjsbot commented Jul 3, 2017

Snuffleupagus left a comment

Snuffleupagus commented Jul 4, 2017

pdfjsbot commented Jul 4, 2017

pdfjsbot commented Jul 4, 2017

pdfjsbot commented Jul 4, 2017

pdfjsbot commented Jul 4, 2017

Fix how we detect and handle missing glyph data. #8580

Fix how we detect and handle missing glyph data. #8580

Conversation

brendandahl commented Jun 28, 2017 • edited by Snuffleupagus Loading

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 28, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jun 28, 2017

From: Bot.io (Windows)

Failed

pdfjsbot commented Jun 28, 2017

From: Bot.io (Linux m4)

Failed

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

From: Bot.io (Linux m4)

Received

brendandahl commented Jun 28, 2017

pdfjsbot commented Jun 28, 2017

From: Bot.io (Linux m4)

Failed

Snuffleupagus commented Jun 28, 2017

Snuffleupagus Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

brendandahl Jul 3, 2017

Choose a reason for hiding this comment

brendandahl commented Jun 29, 2017

brendandahl commented Jul 3, 2017

pdfjsbot commented Jul 3, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 3, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 3, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Jul 3, 2017

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus commented Jul 4, 2017

pdfjsbot commented Jul 4, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 4, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 4, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 4, 2017

From: Bot.io (Windows)

Success

brendandahl commented Jun 28, 2017 •

edited by Snuffleupagus

Loading

Snuffleupagus Jun 28, 2017 •

edited

Loading