googlefonts / fontdiffenator

Font comparison tool
Apache License 2.0
79 stars 13 forks source link

Add CBDT support #74

Closed RoelN closed 4 years ago

RoelN commented 4 years ago

This PR will add support for CBDT fonts. It will produce a diff value in the same way as for vector fonts.

When using HTML output, an animated before/after GIF will be added per modified glyph:

image

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.

RoelN commented 4 years ago

Working on tests now!

davelab6 commented 4 years ago

Overall LGTM, @m4rc1e PTAL

guidotheelen commented 4 years ago

@davelab6 @m4rc1e @rsheeter We added some CBDT tests.

m4rc1e commented 4 years ago

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

anthrotype commented 4 years ago

does FreeType not support cbdt tables?

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

RoelN commented 4 years ago

@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 glyf table with empty glyphs, and skip some checks when the font is deemed not resizable, which sets apart glyph fonts from CBDT fonts.

Any advice on how to set the pixel size properly is much appreciated!

anthrotype commented 4 years ago

I was unable to get set_char_size() to work for Noto Emoji.

Funnily enough if you use 109 it works.. see @HinTak's comment in https://github.com/rougier/freetype-py/blob/c578643b5736130c4341584655cb9813c32b5482/examples/emoji-color-cairo.py#L21-L23

probably freetype-py's set_char_size should be amended to lookup the closest bitmap strike like FTDemo_Set_Current_Charsize in freetype2-demos/src/ftcommon.c is doing.

m4rc1e commented 4 years ago

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, https://github.com/rougier/freetype-py/issues/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.

RoelN commented 4 years ago

Thanks! I wondered because I tried passing 109 as pixel size and I kept getting the error.

anthrotype commented 4 years ago

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

HinTak commented 4 years ago

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):

https://github.com/HinTak/Font-Validator/blob/149195e72d10a20d88d1a77c52e9b5e5a6240298/Compat/Compat.cs#L451

RoelN commented 4 years ago

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.

m4rc1e commented 4 years ago

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.

m4rc1e commented 4 years ago

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.

HinTak commented 4 years ago

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.

davelab6 commented 4 years ago

@m4rc1e sounds good, please merge and file an issue explaining what needs to be done 'properly' later :)

RoelN commented 4 years ago

Remaining issues have been addressed!

rsheeter commented 4 years ago

Anything outstanding or can we merge?

RoelN commented 4 years ago

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.

m4rc1e commented 4 years ago

I'm going to merge this and push a new release. We can make this better another day.

m4rc1e commented 4 years ago

New version pushed to pypi.