Make NotoColorEmoji actually work and implement IndexSubtableFormat{0,2} on CBLC

[ebraminio]

Steps to repro:

1. Download NotoColorEmoji.ttf
2. make -Cbuild/src dump-emoji
3. rmdir out || true; mkdir out
4. build/src/dumo-emoji NotoColorEmoji.ttf

Actual:
Not all emojis can be dumped on out/ folder.

Expected:
All the glyphs be dumped correctly.

[ebraminio]

There is no IndexSubTable0 on https://docs.microsoft.com/en-us/typography/opentype/spec/eblc#indexsubtable1-variable-metrics-glyphs-with-4-byte-offsets how must of them are 0 on Noto? (put printf ("%d\n", (int) u.header.indexFormat); on top of IndexSubtable's sanitization or get_image_data to see yourself)

[ebraminio]

I wonder why this is implemented at the first place (probably for CLBC on Android) and how now that is broken no body cares

[khaledhosny]

Looks like his was introduced to get glyph extents since fonts with CBDT/CBLC tables must not have glyph outlines tables, see 8318525.

[ebraminio]

I know, and I feel it is implemented somewhere else so now nobody cares about harfbuzz implementation, but am thinking about why it is introduced first here and moved afterward.

[behdad]

I'll check this today.

[behdad]

@nona-google

[behdad]

I can confirm this doesn't work:

$ ./hb-shape  NotoColorEmoji.ttf -u 1f98a --font-funcs ot --show-extents 
[gid1120=0+2550<0,0,0,0>]

[behdad]

That said, we have a color-fonts.tests that seem to work.

[behdad]

Test font has:

    <strikedata index="0"> 
      <cbdt_bitmap_format_17 name="u1F42F"> 
        <SmallGlyphMetrics> 
          <height value="128"/> 
          <width value="136"/> 
          <BearingX value="0"/> 
          <BearingY value="100"/> 
          <Advance value="136"/> 
        </SmallGlyphMetrics> 
        <rawimagedata> 

NotoColorEmoji has:

    <strikedata index="0"> 
      <cbdt_bitmap_format_17 name="glyph02253"> 
        <SmallGlyphMetrics> 
          <height value="128"/> 
          <width value="136"/> 
          <BearingX value="0"/> 
          <BearingY value="101"/> 
          <Advance value="136"/> 
        </SmallGlyphMetrics> 
        <rawimagedata> 

so not sure why it doesn't work.

[behdad]

That code is too complicated for me to wrap my head around of. :(

[behdad]

Or rather, that table...

[behdad]

IIRC @nona-google implemented this and I cleaned up and merged it, but he also sent a patch to FreeType to allow loading metrics without loading bitmaps, and that got merged and he ended up using that. That would explain why this was broken and we never noticed.

[nona-google]

I just debugged and somehow the image format is invalid number and leached here.
https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-ot-color-cbdt-table.hh?rcl=54d332dd9b0263821376161cdffb60ffb3c7847f&l=450

[nona-google]

Android still uses this code but not sure why I wasn't noticed the breakage.
I'm going to take some time tomorrow for further debugging and explain why we missed this.

[behdad]

Okay. Believe I fixed this. dump-emoji still crashes though. @ebraminio can you debug?

[behdad]

Thanks @nona-google. I believe I fixed the core issue here.

[khaledhosny]

dump-emoji is trying to write files to a directory named out without checking for its existence or creating it, so it gets NULL from fopen() and crashes writing to it.

[ebraminio]

dump-emoji is trying to write files to a directory named out without checking for its existence or creating it, so it gets NULL from fopen() and crashes writing to it.

Correct. As described on the first comment and as I thought finding cross platform API creating and purging folder is not possible so left that to users.

This is not fixed however I am still getting only these from the font,

AFAICR there was subtables not implemented on harfbuzz itself. Lets open it again and investigate

[khaledhosny]

I get lots of more glyphs here. The code should at least check if fopen() returned NULL and abort or warn, not just crash.

[khaledhosny]

$ ./src/dump-emoji /usr/share/fonts/noto/NotoColorEmoji.ttf
$ ls out/ | wc -l
2581

[ebraminio]

Oh thanks. Hmm, just to confirm, are you using a new version of NotoColorEmoji.ttf? Maybe we just lack the support on new one.

[khaledhosny]

Font version is 2.017, which I think is the latest.

[ebraminio]

It works now with the fixes, also applying your comments on #1251, have a look, thanks :)