Skip to content
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.

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

Try to skip mapping of missing TrueType and CIDFontType2 glyphs #5651

Merged
merged 3 commits into from Feb 12, 2015
Merged

Try to skip mapping of missing TrueType and CIDFontType2 glyphs #5651

merged 3 commits into from Feb 12, 2015

Conversation

Snuffleupagus
Copy link
Collaborator

This PR is based on a patch written by @brendandahl.

The first patch fixed most of the issues, and needed only a very minor adjustment to work correctly for fonts with a CIDToGIDMap.
However in some browsers (e.g. the release version of Firefox), the first patch caused a lot of notdef glyphs to appear in a number of the test files.
After quite a bit of debugging, I found that this was usually an issue with the way that the space character was mapped. In the problematic PDF files, space was usually mapped to various (low) control charCodes.

Since the space glyph (obviously) doesn't need any glyph data, and is thus empty, this causes it to be added to missingGlyphs by sanitizeGlyphLocations().
To work around this, I'm proposing that we also look at toUnicode when building the charCodeToGlyphId map. If the charCode exists in toUnicode the glyph is most likely needed, even if it's empty, hence we add it to the charCodeToGlyphId map. This lets adjustMapping(), if necessary, move the charCode to the Private Use Area and thus fixing the issues with notdef being displayed.

I hope that this approach is reasonable (it passes all tests locally)!

Fixes #5202.
Fixes #5584.
Fixes #3652.
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1122998.
Fixes #5664.
Fixes #5681.
Fixes #5704.
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=893730.

brendandahl and others added 3 commits February 7, 2015 12:19
Also don't skip mapping of glyphs which are empty, if the corresponding charCode is included in toUnicode.
Also don't skip mapping of glyphs which are empty, if the corresponding charCode is included in toUnicode.
@Snuffleupagus Snuffleupagus changed the title Try to skip mapping of missing CIDFontType2 glyphs Try to skip mapping of missing TrueType and CIDFontType2 glyphs Feb 7, 2015
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Feb 7, 2015

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/911d81633ed99c6/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Feb 7, 2015

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/caafc54b6760116/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Feb 7, 2015

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/caafc54b6760116/output.txt

Total script time: 17.18 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Feb 7, 2015

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/911d81633ed99c6/output.txt

Total script time: 22.83 mins

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

Image differences available at: http://107.21.233.14:8877/911d81633ed99c6/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

As far as I'm concerned, the test "failures" on Linux seem to be of two kinds:

  • Changes that are almost imperceivable to the naked eye.
  • Changes that make the text render better.

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Feb 7, 2015

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/1b8403e2d59c565/output.txt

@brendandahl
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/5db5a6c21ccd59b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/5030d99b13f6477/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/5db5a6c21ccd59b/output.txt

Total script time: 17.13 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Failed

Full output at http://107.21.233.14:8877/5030d99b13f6477/output.txt

Total script time: 22.78 mins

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

Image differences available at: http://107.21.233.14:8877/5030d99b13f6477/reftest-analyzer.html#web=eq.log

@brendandahl
Copy link
Contributor

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://107.22.172.223:8877/46f4236638338ee/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

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

Live output at: http://107.21.233.14:8877/8e408b14ac2a1b9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/46f4236638338ee/output.txt

Total script time: 17.17 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/8e408b14ac2a1b9/output.txt

Total script time: 22.59 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

brendandahl added a commit that referenced this pull request Feb 12, 2015
Try to skip mapping of missing TrueType and CIDFontType2 glyphs
@brendandahl brendandahl merged commit 394b38b into mozilla:master Feb 12, 2015
@brendandahl
Copy link
Contributor

Thanks for picking this up!

@Snuffleupagus Snuffleupagus deleted the missing-glyphs branch February 12, 2015 10:10
speedplane pushed a commit to speedplane/pdf.js that referenced this pull request Feb 24, 2015
Try to skip mapping of missing TrueType and CIDFontType2 glyphs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment