search

googlefonts / fontdiff

tool for finding visual differences between two font versions
Other
135 stars 29 forks source link

Bad shaping with Noto Sans Cherokee #4

Closed brawer closed 8 years ago

brawer commented 8 years ago

fontdiff uses Harfbuzz, but the shaping is currently broken sometimes.

To reproduce, use fontdiff on NotoSansCherokee-Regular.otf with the following command:

fontdiff --before NotoSansCherokee-Regular.otf --after NotoSansCherokee-Regular.otf
    --specimen sample.html --out output.pdf

with the following sample.html file:

<html lang="chr">
  <div>Ꭰ̋ Ꭱ̋ Ꭲ̋ Ꭳ̋ Ꭴ̋ Ꭵ̋ Ꭶ̋ Ꭷ̋ Ꭸ̋ Ꭹ̋ Ꭺ̋ Ꭻ̋ Ꭼ̋ Ꭽ̋ Ꭾ̋ Ꭿ̋</div>
</html>

Expected to see the same shapes as rendered by hb-view:

expected-hb-view

Observed:

observed-fontdiff
brawer commented 8 years ago

When rendering this text, fontdiff calls the following HarfBuzz functions on the buffer:

hb_buffer_add_utf16(); text="\u13a0\u030b\u0020\u13a1\u030b\u0020\u13a2\u030b\u0020\u13a3\u030b\u0020\u13a4\u030b\u0020\u13a5\u030b\u0020\u13a6\u030b\u0020\u13a7\u030b\u0020\u13a8\u030b\u0020\u13a9\u030b\u0020\u13aa\u030b\u0020\u13ab\u030b\u0020\u13ac\u030b\u0020\u13ad\u030b\u0020\u13ae\u030b\u0020\u13af\u030b"
hb_shape(); direction=ltr script=Cher language=chr length=47

After the call to hb_shape(), the result is:

glyph #0: info.codepoint=15 info.cluster=0 pos.x_advance=617 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #1: info.codepoint=9 info.cluster=1 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-350 pos.y_offset=137
glyph #2: info.codepoint=13 info.cluster=2 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #3: info.codepoint=17 info.cluster=3 pos.x_advance=562 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #4: info.codepoint=9 info.cluster=4 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-322 pos.y_offset=137
glyph #5: info.codepoint=13 info.cluster=5 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #6: info.codepoint=19 info.cluster=6 pos.x_advance=536 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #7: info.codepoint=9 info.cluster=7 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-309 pos.y_offset=137
glyph #8: info.codepoint=13 info.cluster=8 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #9: info.codepoint=21 info.cluster=9 pos.x_advance=674 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #10: info.codepoint=9 info.cluster=10 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-363 pos.y_offset=137
glyph #11: info.codepoint=13 info.cluster=11 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #12: info.codepoint=23 info.cluster=12 pos.x_advance=847 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #13: info.codepoint=9 info.cluster=13 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-600 pos.y_offset=137
glyph #14: info.codepoint=13 info.cluster=14 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #15: info.codepoint=25 info.cluster=15 pos.x_advance=260 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #16: info.codepoint=9 info.cluster=16 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-171 pos.y_offset=137
glyph #17: info.codepoint=13 info.cluster=17 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #18: info.codepoint=27 info.cluster=18 pos.x_advance=542 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #19: info.codepoint=9 info.cluster=19 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-312 pos.y_offset=137
glyph #20: info.codepoint=13 info.cluster=20 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #21: info.codepoint=29 info.cluster=21 pos.x_advance=630 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #22: info.codepoint=9 info.cluster=22 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-357 pos.y_offset=137
glyph #23: info.codepoint=13 info.cluster=23 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #24: info.codepoint=31 info.cluster=24 pos.x_advance=550 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #25: info.codepoint=9 info.cluster=25 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-442 pos.y_offset=137
glyph #26: info.codepoint=13 info.cluster=26 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #27: info.codepoint=33 info.cluster=27 pos.x_advance=578 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #28: info.codepoint=9 info.cluster=28 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-331 pos.y_offset=137
glyph #29: info.codepoint=13 info.cluster=29 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #30: info.codepoint=35 info.cluster=30 pos.x_advance=574 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #31: info.codepoint=9 info.cluster=31 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-328 pos.y_offset=137
glyph #32: info.codepoint=13 info.cluster=32 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #33: info.codepoint=37 info.cluster=33 pos.x_advance=455 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #34: info.codepoint=9 info.cluster=34 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-182 pos.y_offset=137
glyph #35: info.codepoint=13 info.cluster=35 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #36: info.codepoint=39 info.cluster=36 pos.x_advance=541 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #37: info.codepoint=9 info.cluster=37 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-288 pos.y_offset=137
glyph #38: info.codepoint=13 info.cluster=38 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #39: info.codepoint=41 info.cluster=39 pos.x_advance=768 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #40: info.codepoint=9 info.cluster=40 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-375 pos.y_offset=137
glyph #41: info.codepoint=13 info.cluster=41 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #42: info.codepoint=43 info.cluster=42 pos.x_advance=482 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #43: info.codepoint=9 info.cluster=43 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-292 pos.y_offset=178
glyph #44: info.codepoint=13 info.cluster=44 pos.x_advance=200 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #45: info.codepoint=45 info.cluster=45 pos.x_advance=591 pos.y_advance=0 pos.x_offset=0 pos.y_offset=0
glyph #46: info.codepoint=9 info.cluster=46 pos.x_advance=0 pos.y_advance=0 pos.x_offset=-278 pos.y_offset=137

Next step: Check whether pos.y_offset gets correctly passed to Cairo.

brawer commented 8 years ago

When computing coordinates for Cairo, it looks like x_offset wants to be added, but y_offset wants to be substracted. No idea why, but it seems to fix the problem, both in the left-to-right and right-to-left version.

       cairo_glyph_t cairoGlyph;
       cairoGlyph.index = glyphs[gi].codepoint;
       cairoGlyph.x = (x + pos[gi].x_offset) / 64.0;
-      cairoGlyph.y = (y + pos[gi].y_offset) / 64.0;
+      cairoGlyph.y = (y - pos[gi].y_offset) / 64.0;
       x += pos[gi].x_advance;
       y += pos[gi].y_advance;
       cairoGlyphs.push_back(cairoGlyph);

Before the change:

before

After the change:

after
behdad commented 8 years ago

No idea why

Because fonts are in y-grows-up coordinate system while cairo is y-grows-down. You can negate the y-scale value you pass to hb_font_set_scale(), or just do what you did now.

marekjez86 commented 8 years ago

Very nice debugging! This also fixes our Arabic problem. I like a single fix that solves many issues :-)

brawer commented 8 years ago

@marekjez86, glad you like it! Actually, I didn’t even know we had a problem in Arabic. If you run into such problems, feel free to file bugs any time; it really helps for keeping track.

@behdad, thanks so much for the explanation! In that case, shouldn’t y_advance also be negated?

       cairo_glyph_t cairoGlyph;
       cairoGlyph.index = glyphs[gi].codepoint;
       cairoGlyph.x = (x + pos[gi].x_offset) / 64.0;
       cairoGlyph.y = (y - pos[gi].y_offset) / 64.0;
       x += pos[gi].x_advance;
-      y += pos[gi].y_advance;
+      y -= pos[gi].y_advance;

I thought that without this change, Nastaliq rendering might be broken. But actually it works just fine, with or without negating y_advance. Turns out that HarfBuzz always returns a zero y_advance, also with Noto Nastaliq. Here’s a verse rendered by current fontdiff:

fontdiff-nastaliq
khaledhosny commented 8 years ago

Y advance is used for vertical text, so if text is set horizontally then you will always get zero. Moving glyph vertically in horizontal text (e.g. in Nastaliq) will change the Y offset.