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

Incorrect font glyphs use_system_font=false but standard fonts are available in some PDFs - possibly specific to composite font with embedded cmap #17675

Open
jkgenser opened this issue Feb 14, 2024 · 3 comments

Comments

@jkgenser
Copy link

It seems that the this PR: #14025 improves the glyph mapping but only when as part of the routine that does fallback to system font. Does it make sense to be robust to this error for the case where we are shipping the 14 standard fonts?

I have had (non-deterministic) issues with some users falling back to system font in other instances so I'm exploring making use_system_font=false by default in my application (which uses pdf.js in the browser). However, I am worried about the risk of being exposed to this issue in cases where the CMAP is incorrect.

Attach (recommended) or Link to PDF file here: This PDF: https://github.com/mozilla/pdf.js/blob/master/test/pdfs/issue11915.pdf

Configuration:

  • Web browser and its version: Chrome 121.0.6167.161 / Firefox 122.0.1
  • Operating system and its version: Windows 11
  • PDF.js version: master branch of this repo / 4.x / 3.11.x
  • Is a browser extension: No

Steps to reproduce the problem:

  1. Start the pdfjs repo using gulp
  2. Set use_system_fonts=false (but make sure the standard fonts are available
  3. Open the PDF using the test application at: http://localhost:8888/web/viewer.html

What is the expected behavior? (add screenshot)
image

What went wrong? (add screenshot)
image

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension):

@jkgenser jkgenser changed the title Unable to render fonts with incorrect CMAP if use_system_font=false but standard fonts are available. Incorrect font glyphs use_system_font=false but standard fonts are available in some PDFs (particular this with embedded CMAP) Feb 14, 2024
@jkgenser jkgenser changed the title Incorrect font glyphs use_system_font=false but standard fonts are available in some PDFs (particular this with embedded CMAP) Incorrect font glyphs use_system_font=false but standard fonts are available in some PDFs - possibly specific to composite font with embedded cmap Feb 14, 2024
@Snuffleupagus
Copy link
Collaborator

Please note that the useSystemFonts: false option is mostly intended for Node.js environments, and is generally not recommended/intended for use in browsers; see

pdf.js/src/display/api.js

Lines 150 to 154 in a83a8d7

* @property {boolean} [useSystemFonts] - When `true`, fonts that aren't
* embedded in the PDF document will fallback to a system font.
* The default value is `true` in web environments and `false` in Node.js;
* unless `disableFontFace === true` in which case this defaults to `false`
* regardless of the environment (to prevent completely broken fonts).

Hence it's not entirely clear to me if we want to try and "fix" this, for the following reasons:

  • The primary development target here is the Firefox PDF Viewer, where this shouldn't be an issue.
  • The PR you reference only applies to composite TrueType fonts, however (most of) the standard fonts that we ship are Type1 fonts where the same logic may not be correct or work well in general.
  • Changing any part of the font code always has the possibility of regressions, and this does seem like an edge-case.

@jkgenser
Copy link
Author

jkgenser commented Feb 17, 2024

Please note that the useSystemFonts: false option is mostly intended for Node.js environments, and is generally not recommended/intended for use in browsers; see

pdf.js/src/display/api.js

Lines 150 to 154 in a83a8d7

* @property {boolean} [useSystemFonts] - When `true`, fonts that aren't
* embedded in the PDF document will fallback to a system font.
* The default value is `true` in web environments and `false` in Node.js;
* unless `disableFontFace === true` in which case this defaults to `false`
* regardless of the environment (to prevent completely broken fonts).

Hence it's not entirely clear to me if we want to try and "fix" this, for the following reasons:

  • The primary development target here is the Firefox PDF Viewer, where this shouldn't be an issue.
  • The PR you reference only applies to composite TrueType fonts, however (most of) the standard fonts that we ship are Type1 fonts where the same logic may not be correct or work well in general.
  • Changing any part of the font code always has the possibility of regressions, and this does seem like an edge-case.

Got it thanks for your reply. I was having an issue where some users were not able to render PDFs due to a couple of examples:

  • user did not have a compatible font with zapfdingbats and the PDF ended up being rendered with arbitrary glyphs. Adding standard font url fixed it for this user
  • another user, non-deterministically had issues with the same PDF where in some cases the PDF renders with arbitrary or miss-mapped glyphs in and other cases it renders in a readable way. The particular font being requested by pdfjs was local("Arial-Bold-Italic"). My own machine did not have this but it was resolved to "Arial Bold Italic" but for some reason this didn't work for all users.

So as part of making sure PDFs are rendered properly for all users, I was thinking to include standard fonts as defensive compat layer.

That being said, if I were using node.js to render, then the case above would render as the second screenshot. I've gone a bit into the pdfjs code so I can 100% appreciate your comment that fixes to fonts can be very finnicky and lead to regressions. I've also tried various strategies to inspect PDF to identify whether they will have rendering issues ahead of time.

@schmitch
Copy link

btw. this looks like: #16711
I also added a comment: #16711 (comment) sometimes a windows restart helps with the issue.

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

No branches or pull requests

4 participants