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

Map all glyphs to the private use area and duplicate the first glyph. #9340

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

brendandahl
Copy link
Contributor

This was all spurred from a discussion with Jonathan Kew about our issues glyphs when the are mapped to certain unicode values. In an ideal world we'd have a canvas API to do something like drawGlyphID(), but he agreed that for now it's best to always use the private use area.

...

There have been lots of problems with trying to map glyphs to their unicode
values. It's more reliable to just use the private use areas so the browser's
font renderer doesn't mess with the glyphs.

Using the private use area for all glyphs did highlight other issues that this
patch also had to fix:

  • small private use area - Previously, only the BMP private use area was used
    which can't map many glyphs. Now, the (much bigger) PUP 16 area can also be
    used.

  • glyph zero not shown - Browsers will not use the glyph from a font if it is
    glyph id = 0. This issue was less prevalent when we mapped to unicode values
    since the fallback font would be used. However, when using the private use
    area, the glyph would not be drawn at all. This is illustrated in one of the
    current test cases (issue Letter ä not showing in pdf #8234) where there's an "ä" glyph at position
    zero. The PDF looked like it rendered correctly, but it was actually not
    using the glyph from the font. To properly show the first glyph it is always
    duplicated and appended to the glyphs and the maps are adjusted.

  • supplementary characters - The private use area PUP 16 is 4 bytes, so
    String.fromCodePoint must be used where we previously used
    String.fromCharCode. This is actually an issue that should have been fixed
    regardless of this patch.

Fixes:

@brendandahl
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jan 4, 2018

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/2ad0196f95f80db/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 4, 2018

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 4, 2018

From: Bot.io (Linux m4)


Failed

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

Total script time: 11.72 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Jan 4, 2018

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/2ad0196f95f80db/output.txt

Total script time: 25.14 mins

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

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

@brendandahl
Copy link
Contributor Author

Looks like I have some issues on Linux (besides slight glyph movement).

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 5, 2018

Looks like I have some issues on Linux (besides slight glyph movement).

You're probably already aware of it, but even on Windows there appears to be some issues still. Note that in both issue1049 and bug1354114 the previously undefined glyphs are now replaced by actual glyphs, but unfortunately not the correct ones (so it's better, but still not quite right).

var PRIVATE_USE_OFFSET_END = 0xF8FF;
var SKIP_PRIVATE_USE_RANGE_F000_TO_F01F = false;
// Unicode Private Use Areas:
var PRIVATE_USE_AREAS = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Use const instead here.

CFF.prototype = {
duplicateFirstGlyph: function CFF_duplicateFirstGlyph() {
// Browsers will not display a glyph at position 0. Typically glyph 0 is
// notdef, but a number of fonts put a valid glyph so it must be
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jan 5, 2018

Choose a reason for hiding this comment

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

Grammar nit: Shouldn't it perhaps be a "there" after "... valid glyph", i.e
// notdef, but a number of fonts put a valid glyph there so it must be

0, // nRanges place holder
(start >> 8) & 0xFF,
start & 0xFF,
lastFD];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: For improved readability, the ]; should be moved to the next line instead.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 5, 2018

In addition to the issues/bugs listed in #9340 (comment), the following ones also appear to be fixed:

@@ -0,0 +1 @@
https://www.unicode.org/charts/PDF/U11A50.pdf
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about linking to the Internet Archive here, to avoid the file becoming unavailable, i.e. using the following link instead https://web.archive.org/web/20171116124714/https://www.unicode.org/charts/PDF/U11A50.pdf.

@@ -3327,6 +3339,12 @@
"rounds": 1,
"type": "eq"
},
{ "id": "bug1425312",
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Jan 5, 2018

Choose a reason for hiding this comment

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

This entry needs "link": true, added, to fix the following from the test logs:

WARNING: Unable to open file for reading "Error: ENOENT: no such file or directory, open 'pdfs/bug1425312.pdf'".
Unable to verify the checksum for the files that are used for testing.
Please re-download the files, or adjust the MD5 checksum in the manifest for the files listed above.

Also, it'd probably suffice to only test the second page here.

@brendandahl
Copy link
Contributor Author

The above changes don't fix everything, just the windows issues and nits. I'm still looking into the linux issues.

@brendandahl
Copy link
Contributor Author

brendandahl commented Jan 12, 2018

@brendandahl
Copy link
Contributor Author

Small update and some troubles:

Issue #2881 was failing on linux because the platform was rejecting the font (OTS was happy). After downloading and building a debug build of freetype, I used ftlint to find the font was being rejected because the number of charstrings was greater than the number of charset values. I then wrote a function to write out an updated cff charset (which fixed the issue). However, after this, endchar.pdf started failing. This is because it uses the enchchar\seac operator which requires the charset to be set to a predefined charset, whereas we were now writing out a custom format 0 charset.

So far I don't have a great plan to handle fixing both issues. Some possible ideas though:

  • Don't duplicate glyph 0, but instead use our js font renderer if we're trying to draw glyph 0. Cons: a few glyphs may look different relative to others.
  • Move glyph 0 to it's own font. Cons: lots of work to move a glyph and all it supporting stuff and we'd have to add another path to change the font in show text.
  • Merge endchar/seac into a single glyph. Not entirely sure if this is possible, but it may be the cleanest situation.

I'm open to ideas though @Snuffleupagus @yurydelendik

@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/5543dc2d2ea2b83/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/13600c876438903/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/13600c876438903/output.txt

Total script time: 20.61 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/5543dc2d2ea2b83/output.txt

Total script time: 21.36 mins

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

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

@brendandahl
Copy link
Contributor Author

Try that again with the correct changes...
/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/e2303cce49bb20b/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/464643c59b1b377/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 13.53 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/464643c59b1b377/output.txt

Total script time: 25.70 mins

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

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

@timvandermeij timvandermeij merged commit 66422eb into mozilla:master Sep 8, 2018
@timvandermeij
Copy link
Contributor

Nice work!

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.

4 participants