[api-minor] Fallback to the built-in font renderer when font loading fails #10539
Conversation
…Loader._queueLoadingCallback` when actually necessary Currently all fonts are using the `_queueLoadingCallback` method to determine when they have been loaded[1]. However in most cases this is just adding unnecessary overhead, especially with `BaseFontLoader.bind` now being asynchronous, given how fonts are loaded: - For fonts loaded using the Font Loading API, it's already possible to easily tell when a font has been loaded simply by checking the `loaded` promise on the FontFace object itself. - For browsers, e.g. Firefox, which support synchronous font loading it's already assumed that fonts are immediately available. Hence the `_queueLoadingCallback` method is moved into the `GenericFontLoader`, such that it's only utilized for fonts which are loaded using CSS. --- [1] In the "fonts loaded using CSS" case, this is already a hack anyway as outlined in the comments.
…ind` - The only existing call-site, of this method, is never passing more than *one* font at a time anyway. - As far as I can remember, this functionality has never actually been used (caveat: I didn't check the git history). - This allows simplification of the method, especially by making use of the fact that it's now asynchronous. - It should be just as easy to call `BaseFontLoader.bind` from within a loop, rather than having the loop in the method itself.
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/66d495ecfd2d66e/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/139b49487237525/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/139b49487237525/output.txt Total script time: 17.75 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/66d495ecfd2d66e/output.txt Total script time: 25.48 mins
|
Snuffleupagus
force-pushed
the
fallback-disableFontFace-v2
branch
2 times, most recently
from
5026599
to
77ebb19
Compare
After PR 9340 all glyphs are now re-mapped to a Private Use Area (PUA) which means that if a font fails to load, for whatever reason[1], all glyphs in the font will now render as Unicode glyph outlines. This obviously doesn't look good, to say the least, and might be seen as a "regression" since previously many glyphs were left in their original positions which provided a slightly better fallback[2]. Hence this patch, which implements a *general* fallback to the PDF.js built-in font renderer for fonts that fail to load (i.e. are rejected by the sanitizer). One caveat here is that this only works for the Font Loading API, since it's easy to handle errors in that case[3]. The solution implemented in this patch does *not* in any way delay the loading of valid fonts, which was the problem with my previous attempt at a solution, and will only require a bit of extra work/waiting for those fonts that actually fail to load. *Please note:* This patch doesn't fix any of the underlying PDF.js font conversion bugs that's responsible for creating corrupt font files, however it does *improve* rendering in a number of cases; refer to this possibly incomplete list: [Bug 1524888](https://bugzilla.mozilla.org/show_bug.cgi?id=1524888) Issue 10175 Issue 10232 --- [1] Usually because the PDF.js font conversion code wasn't able to parse the font file correctly. [2] Glyphs fell back to some default font, which while not accurate was more useful than the current state. [3] Furthermore I'm not sure how to implement this generally, assuming that's even possible, and don't really have time/interest to look into it either.
Snuffleupagus
force-pushed
the
fallback-disableFontFace-v2
branch
from
77ebb19
to
b6d090c
Compare
Snuffleupagus
changed the title
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/2b0970ca53316f4/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/0357aff3c435d90/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/2b0970ca53316f4/output.txt Total script time: 17.72 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/0357aff3c435d90/output.txt Total script time: 25.29 mins
|
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/6499ceb820b7f6b/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/6499ceb820b7f6b/output.txt Total script time: 1.67 mins Published |
|
This is something that we would really benefit from. We currently have a hacky workaround in our application involving getting the document with This is a much nicer solution. |
Snuffleupagus
changed the title
There was a problem hiding this comment.
Looks good! I forgot we had a cache of all the glyphs, that makes things much easier. It would be great if we could get a follow up bug to add a test case. I am also thinking, for testing, we may want a way to ensure that this path isn't allowed, unless the test specifically allows it (maybe a pref that defaults to enabled?).
Thank you; I'd also forgotten all about the
The reason that I didn't add a test-case here, is that this is nothing more than a work-around. The end goal should still, as far as I'm concerned, be to fix the font bugs that cause us to generate a font file that the OpenType Sanitizer ends up rejecting.
In an ideal world, where the underlying PDF.js bugs have been fixed, I'm not sure what you'd actually test in that case!? (Unless you somehow corrupted the font file on the main-thread, prior to loading it.) |
I agree, I'd still like to fix the underlying issue and for that specific PDF I have a fix in progress. Though it would still be nice to test this new code path and make sure it always works.
I think we'd need a font that we intentionally never fix. Or an idea I like better is: during the test, the test framework could override the font loading api and intentionally make it fail. We could add an option to test_manifest.json for this. |
Totally agree, that seems like the nicest/cleanest solution. |
After PR #9340 all glyphs are now re-mapped to a Private Use Area (PUA) which means that if a font fails to load, for whatever reason[1], all glyphs in the font will now render as Unicode glyph outlines.
This obviously doesn't look good, to say the least, and might be seen as a "regression" since previously many glyphs were left in their original positions which provided a slightly better fallback[2].
Hence this patch, which implements a general fallback to the PDF.js built-in font renderer for fonts that fail to load (i.e. are rejected by the sanitizer). One caveat here is that this only works for the Font Loading API, since it's easy to handle errors in that case[3].
The solution implemented in this patch does not in any way delay the loading of valid fonts, which was the problem with my previous attempt at a solution, and will only require a bit of extra work/waiting for those fonts that actually fail to load.
Please note: This patch doesn't fix any of the underlying PDF.js font conversion bugs that's responsible for creating corrupt font files, however it does improve rendering in a number of cases; refer to this possibly incomplete list:
[1] Usually because the PDF.js font conversion code wasn't able to parse the font file correctly.
[2] Glyphs fell back to some default font, which while not accurate was more useful than the current state.
[3] Furthermore I'm not sure how to implement this generally, assuming that's even possible, and don't really have time/interest to look into it either.