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

GSUB behaviour differences between harfbuzz and MS Word #3545

Closed
seehuhn opened this issue Apr 22, 2022 · 11 comments
Closed

GSUB behaviour differences between harfbuzz and MS Word #3545

seehuhn opened this issue Apr 22, 2022 · 11 comments

Comments

@seehuhn
Copy link

seehuhn commented Apr 22, 2022

I constructed a test font which has two GSUB lookups:

  • lookup 0 (type 5, format 1): this only applies if the input is one of AB, AAB or AAAB. For these inputs it recursively applies lookups 1 (adding another A) and then lookup 0 again.

  • lookup 1 (type 2, format 1): this replaces A -> AA.

Using this font, the current version of Word from Office 365 (on Mac) and the Mac "Font Book" typeset the input "AB" as "AAAAB", while harfbuzz just gives "AAB". From my reading of the OpenType spec I believe that "AAAAB" is correct.

The font file (both as .otf and decoded via ttx) is here:

I believe that the problem is caused by the following test:

/* Don't recurse to ourself at same position.
* Note that this test is too naive, it doesn't catch longer loops. */
if (unlikely (idx == 0 && lookupRecord[i].lookupListIndex == c->lookup_index))
continue;
Recursing to the same lookup at the same position does not necessarily indicate an infinite loop, since the input may have changed in the mean time.

@behdad
Copy link
Member

behdad commented Apr 22, 2022

Thanks. Indeed, removing that condition fixes it. Can I use your test font in our test suite?

@behdad behdad closed this as completed in 6695bf0 Apr 22, 2022
@seehuhn
Copy link
Author

seehuhn commented Apr 23, 2022

Sure, feel free to use the font. The glyph outlines come from the "Go Regular" TrueType font, which can be re-distributed under a BSD 3-clause license https://cs.opensource.google/go/x/image/+/master:font/gofont/ttfs/README . You probably need to put a copy of this licence next to the font. As far as my changes to the font are concerned (subsetting, conversion to Opentype etc.), I'm happy for you to use these in any way you see fit.

@behdad
Copy link
Member

behdad commented Apr 23, 2022

Sure, feel free to use the font. The glyph outlines come from the "Go Regular" TrueType font,

Somehow hb-view doesn't render the outlines for me.

@seehuhn
Copy link
Author

seehuhn commented Apr 25, 2022

This is strange! Outlines work fine for me on Mac (see screenshot). What system are you on?

Screenshot 2022-04-25 at 09 32 36

@seehuhn
Copy link
Author

seehuhn commented Apr 25, 2022

Sure, feel free to use the font. The glyph outlines come from the "Go Regular" TrueType font,

Somehow hb-view doesn't render the outlines for me.

One idea: there are very few glyphs in this font, to keep the file size small. Maybe you tried glyphs which are simply not there? It's just the capital letters A-Z, space, and three extra symbols which I plan to use for GPOS alignment tests.

@behdad
Copy link
Member

behdad commented Apr 25, 2022

Interesting. HB_DRAW=0 works, but HB_DRAW=1 fails. And this is a CFF font. Who wants to debug this, ugh. cc @khaledhosny @ebraminio

@khaledhosny
Copy link
Collaborator

khaledhosny commented Apr 27, 2022

The sanitize() call here returns false:

priv->localSubrs = &StructAtOffsetOrNull<CFF1Subrs> (&privDictStr, priv->subrsOffset);
if (priv->localSubrs != &Null (CFF1Subrs) &&
unlikely (!priv->localSubrs->sanitize (&sc)))
{ fini (); return; }

The localSubrs is empty, but the check here is failing:

bool sanitize (hb_sanitize_context_t *c) const
{
TRACE_SANITIZE (this);
return_trace (likely ((c->check_struct (this) && count == 0) || /* empty INDEX */
(c->check_struct (this) && offSize >= 1 && offSize <= 4 &&
c->check_array (offsets, offSize, count + 1) &&
c->check_array ((const HBUINT8*) data_base (), 1, max_offset () - 1))));
}

Without knowing much about the code, I assume the first check should be true, but c->check_struct (this) is returning false.

@khaledhosny
Copy link
Collaborator

khaledhosny commented Apr 27, 2022

If I remove the empty Subrs from the Private dict and rerun ttx the font renders fine, so this empty index our code is tripping over.

@khaledhosny
Copy link
Collaborator

khaledhosny commented Apr 27, 2022

It seems that FontTools does not write offSize if the count is zero, but our code does not allow this.

@behdad
Copy link
Member

behdad commented Apr 27, 2022

return_trace (likely ((c->check_struct (this) && count == 0) || /* empty INDEX */

This check is definitely wrong. If count is zero, nothing else is encoded. Thanks for debugging this. I know the fix.

@behdad
Copy link
Member

behdad commented Apr 27, 2022

Fixed it.

behdad added a commit that referenced this issue Apr 28, 2022
From #3545

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

No branches or pull requests

3 participants