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

Set ExpansionFactor to 0.06 when it's equals to 0 in the private dict of CFF fonts #15900

Merged
merged 1 commit into from
Jan 7, 2023
Merged

Set ExpansionFactor to 0.06 when it's equals to 0 in the private dict of CFF fonts #15900

merged 1 commit into from
Jan 7, 2023

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman calixteman linked an issue Jan 7, 2023 that may be closed by this pull request
Showing squares with hex values instead of text in some PDFs #15289
Closed
Snuffleupagus
Snuffleupagus approved these changes Jan 7, 2023
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.

That's a fascinating find, how did you figure it out?

r=me, with the comment addressed and passing tests; thank you!

test/test_manifest.json Outdated Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

That's a fascinating find, how did you figure it out?

I compared the original font I extracted from the pdf and the one we generate.
I ran tx on both fonts and noticed some differences in the charsets, so I manually fix the charset to have exactly the same as in the original font but nothing.
I ran fontlint on the generated font and noticed few errors about BlueValues, I fixed them but nothing. I was pretty sure that something was wrong in the top dict.
Finally I had the wonderful idea to just remove the private dict and magically it worked, then I just tried to remove the properties one by one and finally the last one (as usual) was the culprit.

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/64f2133afab6d67/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/30e862bdb952fee/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/64f2133afab6d67/output.txt

Total script time: 26.10 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 10
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/64f2133afab6d67/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/30e862bdb952fee/output.txt

Total script time: 33.51 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 16

Image differences available at: http://54.193.163.58:8877/30e862bdb952fee/reftest-analyzer.html#web=eq.log

@calixteman calixteman changed the title Remove ExpansionFactor equals to 0 from the private dict of CFF fonts Set ExpansionFactor to 0.06 when it's equals to 0 in the private dict of CFF fonts Jan 7, 2023
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/f3c7c518d8c2d65/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/2393a951c01ade6/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/f3c7c518d8c2d65/output.txt

Total script time: 26.09 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 9
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/2393a951c01ade6/output.txt

Total script time: 33.89 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 14

Image differences available at: http://54.193.163.58:8877/2393a951c01ade6/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

@Snuffleupagus instead of removing the property I set it to 0.06 to follow Jonathan's advice: #15289 (comment)
Are you ok with this change ?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 7, 2023 •
edited
Loading

Are you ok with this change ?

Yes, since I do agree that it sounds like an even safer approach here.

Given the amount of duplicates, we might want to uplift this to previous Firefox versions?

As a follow-up to this PR, to prevent future bugs, I do wonder if we want to add similar code when parsing "regular" Type1 fonts as well or if we should perhaps wait until an actual bug is reported?

pdf.js/src/core/type1_parser.js

Line 657 in d6984f9

case "ExpansionFactor":

@calixteman calixteman merged commit e1f7355 into mozilla:master Jan 7, 2023
@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/20f19d378bd4926/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Received

Command cmd_makeref from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/93d0a02e449a61f/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/20f19d378bd4926/output.txt

Total script time: 21.90 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/93d0a02e449a61f/output.txt

Total script time: 25.09 mins

  • Lint: Passed
  • Make references: FAILED

@calixteman
Copy link
Contributor Author

/botio-windows makeref

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Received

Command cmd_makeref from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/77fb7dc33853a43/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 7, 2023

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/77fb7dc33853a43/output.txt

Total script time: 25.58 mins

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

@calixteman calixteman deleted the 15289 branch January 7, 2023 16:45
@calixteman calixteman mentioned this pull request Jan 7, 2023
Closed
@Snuffleupagus Snuffleupagus mentioned this pull request Jan 7, 2023
Closed
This was linked to issues Jan 7, 2023
Blank PDF but the text layer isn't empty #15902
Closed
Chinese font not displayed #14759
Closed
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 font-type1 font-wont-sanitize
Projects
None yet
Milestone
No milestone
4 participants
@calixteman @pdfjsbot @Snuffleupagus @timvandermeij