Add CBDT support #74
Add CBDT support #74
Conversation
CBDT/CBLC fonts made to draft version of the spec might not contain a `glyf` table. This will make diffenator choke on CBDT fonts right away. Adding a faux table will prevent many errors like: fontTools.ttLib.TTLibError: Font contains no outlines TODO: Metrics calculated from these faux glyphs are obviously going to be wrong Co-authored-by: guidotheelen <[email protected]>
Compare CBDT glyphs and return diff ratio. Inspired by: https://android.googlesource.com/platform/external/noto-fonts/+/refs/heads/master/scripts/color_emoji_image_diff.py Co-authored-by: guidotheelen <[email protected]>
CBDT diff was slow with original code. This code is a lot faster, and produces the exact same diff values.
|
Working on tests now! |
|
Overall LGTM, @m4rc1e PTAL |
|
Thanks for the PR and for tidying up existing parts. I'm curious, does FreeType not support cbdt tables? it would be better to remove the "image" column from the html table and just generate a separate gif instead |
yes, it does. And fontdiffenator already depends on freetype-py and Pillow. Here's an example for rendering emoji fonts as PNG with freetype-py + Pillow: https://github.com/rougier/freetype-py/blob/master/examples/emoji-color.py |
| has_outlines = self.ttfont.has_key("glyf") or self.ttfont.has_key("CFF ") | ||
| if not has_outlines: | ||
| # Create faux empty glyf table with empty glyphs to make | ||
| # it a valid font, e.g. for old-style CBDT/CBLC fonts |
There was a problem hiding this comment.
@RoelN why is this needed? I think CBDT/CBLC work without glyf table. Can't find reference in OT spec about glyf being required.
There was a problem hiding this comment.
He may have added it since Diffenator assumes all fonts need a glyf/cff table (not too sure this is true but seems to make sense).
I'm going through the PR so should have some suggestions regarding this.
There was a problem hiding this comment.
This was indeed added to patch a bitmap font without glyf/cff tables, but only during runnning the script. It will not save the font, but will make the original diffenator code not choke on a glyphless font.
There was a problem hiding this comment.
Diffenator assumes all fonts need a glyf/cff table (not too sure this is true but seems to make sense)
It is not true; OpenType fonts must have a glyph table, but that can be glyf, cff, cbdt, sbix, svg
|
@m4rc1e @anthrotype I was unable to get set_char_size() to work for Noto Emoji, the CBDT font I'm testing with. Tried just about any size/strike related value. This is why we opted to both patch the font temporarily with a Any advice on how to set the pixel size properly is much appreciated! |
Funnily enough if you use 109 it works.. see @HinTak's comment in probably freetype-py's |
|
In an ideal world, we'd render fonts containing CBDT tables using FreeType, like we already do for fonts which have a cff or glyf tables. By using this approach, we won't need a separate 'cbdt' section in our reports since they'll just be treated by diffenator as normal glyphs. @anthrotype has kindly raised an issue in freetype-py, rougier/freetype-py#123 to support CBDT fonts. If enabling CBDT fonts in freetype is going to be a lot of work, I'm happy to merge this PR and we can do it properly later. |
|
Thanks! I wondered because I tried passing 109 as pixel size and I kept getting the error. |
it's 109 * 64 since it's a fixed point number |
|
If you run "ftdump " on the font, in the middle of the output is the list of sizes of embedded bitmaps. 160 for Apple and 109 for noto are just conveniently large for visual demo. In addition, fontVal traps the set_char_size error and output a suitable message too Edit: it is triggered by the apple emoji font. Here is the message, probably also gives you some hint how to copy (note some api's are written in c# style, may need to add "_" etc to go back to c/python): |
|
I should explain that we chose for one single before/after GIF per CBDT glyph is to retain as many colors as possible. Putting 2 × 50 full color emoji glyphs inside a before/after GIF with a maximum of 256 colors would make the diff useless. Even the before/after of a single CBDT glyph reduced to 256 will show color artifacts, let alone if you put 100 of 'em in a GIF. So that might be an argument to keep the current approach. |
|
Good point regarding gif limitations. We could use the apng format instead for diffs since it's 24 bit. I understand this format had patchy support a few years ago but it's apparently much better now. |
|
Let's stick with your current approach since this FT setup is going to be a pita. If we decide to support the other color formats, we'll revisit. I'll take a closer look at the details tomorrow. Thanks. |
|
Fwiw, around the time I added those comments in freetype-py and those lines in FontVal, there was a discussion with Werner about freetype doing the scaling of bitmaps to let set_char_size() work on all sizes. Werner's against that, it should be pango/cairo/pillow doing the scaling instead. I think Behdad was somewhere in the exchange too, and it is probably in the freetype-devel archive. In any case, the returned error is quite specific - "FT_Error_Invalid_pixel_size" or something liking similar, so you can distinguish fonts having only fixed size bitmaps from other scaling fonts, and other source of error from set_char_size(). So the recommendation from Werner was just to do appropriate things if "invalid_pixel_size" is returned as an error - look for a nearby valid size, or just skip over with an appropriate message (like Fontval does). Btw, I think apple's emoji contains only two sizes, 40x40 and 160x160 . Google Noto Emoji has a few, like 4-5. Think the mozilla people has a scalable SVG emoji font. |
|
@m4rc1e sounds good, please merge and file an issue explaining what needs to be done 'properly' later :) |
- "cbdt" can be removed since it isn't a diffenator table which is dumpable using the dumper tool - Simplify import
Even though this shorter and quicker for glyphs of same dimensions, Marc Foley notes: "This won't work for images which have different dimensions. Zip's iterator stops when the shortest input iterable is exhausted."
|
Remaining issues have been addressed! |
|
Anything outstanding or can we merge? |
|
Outstanding: using APNG to avoid color loss when putting PNG glyphs in an animated GIF, and getting FreeType to render CBDT. I propose making new issues for that, and merging this, as it works and allows you to diff CBDT fonts. |
|
I'm going to merge this and push a new release. We can make this better another day. |
|
New version pushed to pypi. |
This PR will add support for CBDT fonts. It will produce a
diffvalue in the same way as for vector fonts.When using HTML output, an animated before/after GIF will be added per modified glyph:
The diff routine is optimised in speed and LOC, as comparing CBDT glyphs took a lot of time. This is the only change that also touches vector font diffs, but it produces the exact same diff value.