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.

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

Jump to bottom

Add unique glyph names for CFF fonts. #10591

Merged
merged 1 commit into from
Mar 1, 2019
Merged

Add unique glyph names for CFF fonts. #10591

merged 1 commit into from
Mar 1, 2019

Conversation

brendandahl
Copy link
Contributor

@brendandahl
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/20248ab9d595e55/output.txt

@pdfjsbot
Copy link

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/1ba0856f1392631/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/1ba0856f1392631/output.txt

Total script time: 18.01 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/20248ab9d595e55/output.txt

Total script time: 25.62 mins

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

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

@brendandahl
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

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/a0f05cd31dfd317/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/125fff818aecc56/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

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

Total script time: 17.96 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/125fff818aecc56/output.txt

Total script time: 25.41 mins

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

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

Snuffleupagus
Snuffleupagus approved these changes Feb 27, 2019
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

I'm assuming that testing this would be difficult in general, given that it's MacOS specific, but could e.g. a unit-test be added to avoid future regressions?

src/core/cff_parser.js Outdated
@@ -1295,6 +1298,17 @@ var CFFCompiler = (function CFFCompilerClosure() {
output.add(compiled.output);
var topDictTracker = compiled.trackers[0];

// Create names for all of the glyphs so they can be put in the charset
// below.
let firstGlyphSid = cff.strings.strings.length + NUM_STANDARD_CFF_STRINGS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Assuming that cff.strings is an instance of CFFStrings, you should be able to use

pdf.js/src/core/cff_parser.js

Lines 945 to 947 in df7756d

get count() {
return this.strings.length;
},
to simply write this as let firstGlyphSid = cff.strings.count + NUM_STANDARD_CFF_STRINGS;

@brendandahl
Add unique glyph names for CFF fonts.
8a596ef 
Printing on MacOS was broken with the previous approach of just mapping
all the glyphs to notdef.
@brendandahl
Copy link
Contributor Author

I'm assuming that testing this would be difficult in general, given that it's MacOS specific, but could e.g. a unit-test be added to avoid future regressions?

Yeah, even more difficult since this issue only seems to happen with printing on MacOS. Added a unit test.

@brendandahl
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

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/e0cd5d81ed87e61/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/79a98c1e8a5addd/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

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

Total script time: 18.02 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/79a98c1e8a5addd/output.txt

Total script time: 25.57 mins

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

@timvandermeij
Copy link
Contributor

@Snuffleupagus Could you perhaps re-check this now that changes have been made?

@brendandahl brendandahl merged commit 34022d2 into mozilla:master Mar 1, 2019
Snuffleupagus
Snuffleupagus reviewed Mar 1, 2019
View reviewed changes
src/core/cff_parser.js
sid = strings.getSID(name);
if (sid === -1) {
sid = 0;
warn(`Couldn't find ${name} in CFF strings`);
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@brendandahl This seem to have caused a lot of warning "spam" in the console even for something as simple as e.g. the default tracemonkey.pdf file.
From a cursory look it appears that charset.charset only contains numbers, and not strings, for many PDF files.

@Snuffleupagus Snuffleupagus mentioned this pull request Mar 1, 2019
Closed
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 7, 2019
Bug 1530881 - Uplift CFF font fixes for MacOS. r=yury a=lizzard
ef7b80e 
Summary:
Includes pdf.js pull requests:
mozilla/pdf.js#10591
mozilla/pdf.js#10604

Reviewers: yury

Reviewed By: yury

Bug #: 1530881

Differential Revision: https://phabricator.services.mozilla.com/D22131

--HG--
extra : rebase_source : a325a575ce4511bfa37c899411ece4fac17d6900
extra : histedit_source : d3026260dd6701c4def4c0e74ad84516c138b292
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Mar 7, 2019
Bug 1530881 - Uplift CFF font fixes for MacOS. r=yury a=lizzard
a51a6c0 
Summary:
Includes pdf.js pull requests:
mozilla/pdf.js#10591
mozilla/pdf.js#10604

Reviewers: yury

Reviewed By: yury

Bug #: 1530881

Differential Revision: https://phabricator.services.mozilla.com/D22131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
font-conversion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brendandahl @pdfjsbot @timvandermeij @Snuffleupagus