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

Fix several issues with glyph id mappings (issue 8668, bug 1383504) #8681

Merged
merged 1 commit into from
Aug 3, 2017

Conversation

brendandahl
Copy link
Contributor

@brendandahl brendandahl commented Jul 21, 2017

The initial issue with #8255 was I added a missing glyphs
check to adjustMapping, but this caused us to skip re-mapping
a glyph if the fontCharCode was a missingGlyph which in turn
caused us to overwrite a valid glyph id with an invalid one. While
fixing this, I also added a warning if the private use area is full since
this also accidentally happened when I made a different mistake.

This brought to light a number of issues where we map
missing glyphs to notdef, but often the notdef is actually defined
and then ends up being drawn. Now the glyphs don't get
mapped in toFontChar and so they are not drawn by the canvas.

Fixing the above brought up another issue though in bug1050040.pdf.
In this PDF, the font fails to load by the browser and before we were still
drawing the glyphs because it looked like the font had them, but with the fixes
above the glyphs showed up as missing so we didn't attempt draw them. To
fix this, I now throw an error when the loca table is in really bad shape and
we fall back to trying to use a system font. We now also use this fall back if
there are any format errors during converting fonts.

I expect this to fix:

firefox-issue1049-page2
firefox-issue1049-page3
firefox-issue8187-page1
firefox-bug1354114-page30


Fixes #8668.
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1383504.

if (!file || file.isEmpty) {
if (file) {
// Some bad PDF generators will include empty font files,
// attempting to recover by assuming that no file exists.
warn('Font file is empty in "' + name + '" (' + this.loadedName + ')');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this is extracted into fallbackToSystemFont().

@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/4c55af5b31169a2/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/0e684409aca5854/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 16.49 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/4c55af5b31169a2/output.txt

Total script time: 29.41 mins

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

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

@Snuffleupagus
Copy link
Collaborator

It looks fine on the Windows bot, but strangely enough on Linux ligatures ("fi", "fl") are moved vertically in the tracemonkey.pdf file.

@Snuffleupagus
Copy link
Collaborator

Besides fixing issue #8668, this patch also fixes bug 1383504.

The initial issue with mozilla#8255 was I added a missing glyphs
check to adjustMapping, but this caused us to skip re-mapping
a glyph if the fontCharCode was a missingGlyph which in turn
caused us to overwrite a valid glyph id with an invalid one. While
fixing this, I also added a warning if the private use area is full since
this also accidentally happened when I made a different mistake.

This brought to light a number of issues where we map
missing glyphs to notdef, but often the notdef is actually defined
and then ends up being drawn. Now the glyphs don't get
mapped in toFontChar and so they are not drawn by the canvas.

Fixing the above brought up another issue though in bug1050040.pdf.
In this PDF, the font fails to load by the browser and before we were still
drawing the glyphs because it looked like the font had them, but with the fixes
above the glyphs showed up as missing so we didn't attempt draw them. To
fix this, I now throw an error when the loca table is in really bad shape and
we fall back to trying to use a system font. We now also use this fall back if
there are any format errors during converting fonts.
@brendandahl
Copy link
Contributor Author

For some reason linux doesn't seem to like it when we don't have cmap entries for all the type1 fonts.
/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/3a057663f4726ba/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/9f21a2c3013b0e7/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/3a057663f4726ba/output.txt

Total script time: 16.58 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 29.29 mins

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

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

@brendandahl
Copy link
Contributor Author

Hmm....well now the following fixes don't work, but the original regression, issue8187, is at least fixed.

firefox-issue1049-page2
firefox-issue1049-page3
firefox-bug1354114-page30

@Snuffleupagus
Copy link
Collaborator

Hmm....well now the following fixes don't work, but the original regression, issue8187, is at least fixed.

firefox-issue1049-page2
firefox-issue1049-page3
firefox-bug1354114-page30

Do you want to try and fix the above files in this PR, or would a follow-up PR be better?
It would be really nice to get this landed, since it fixes issue #8668 and bug 1383504 which are regressions!

@brendandahl
Copy link
Contributor Author

@Snuffleupagus let's land as is and I'll do follow ups to re-fix the above

@Snuffleupagus
Copy link
Collaborator

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2017

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/16d8b300554d7b6/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2017

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/3f96e1c97b4210c/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2017

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/16d8b300554d7b6/output.txt

Total script time: 16.60 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 2, 2017

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/3f96e1c97b4210c/output.txt

Total script time: 29.15 mins

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

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

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.

Seems fine to me, thank you!

(I'm updating the title to include the issue/bug that's fixed by this PR, since that will allow RyanVM to mark the Bugzilla dependencies when doing the next update.)

@Snuffleupagus
Copy link
Collaborator

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2017

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2017

From: Bot.io (Windows)


Received

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

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

@Snuffleupagus Snuffleupagus changed the title Fix several issues with glyph id mappings. Fix several issues with glyph id mappings (issue 8668, bug 1383504) Aug 3, 2017
@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2017

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/58bf553abb70a90/output.txt

Total script time: 15.54 mins

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

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2017

From: Bot.io (Windows)


Success

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

Total script time: 27.43 mins

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression] Missing glyph in the issue8187 reference test
4 participants