Add FreeType-based Pen for rasterisation #2494
Conversation
|
Suggestion to use freetype-py, which already offers a ctypes-based FT wrapper, and it's on PyPI. https://pypi.org/project/freetype-py/ |
I didn't expect that the PyPI package contains the pre-built binary for each platform. Skip the test when freetype-py is not avaiable.
|
Oh, I didn't realise the freetype-py distribution comes with the pre-built binary, thanks for the advice! |
|
It would be great if this didn't rely on numpy. I understand this may be a pita though. Same could also be said for PIL. What about just returning a list of ints if it's greyscale or a list of tuples if rgba? the client can then use PIL. I'm not a reviewer for this, I'm just thinking out loud here. |
Lib/fontTools/pens/ftPen.py
Outdated
|
|
||
| def array(self, width=1000, ascender=880, descender=-120, even_odd=False, scale=None): | ||
| # Return a numpy array. Each element takes values in the range of [0.0, 1.0]. | ||
| if not self.np: |
There was a problem hiding this comment.
I would not cache PIL, numpy or matplotlib: imports are cached by Python already (via sys.modules), so importing again doesn't add a huge cost.
There was a problem hiding this comment.
Absolutely right. Thank you for pointing this out!
|
@m4rc1e The only mandatory dependency is freetype-py. PIL, numpy and matplotlib are optional. from fontTools.ttLib import TTFont
from fontTools.pens.ftPen import FTPen
font = TTFont('SourceSans3-Regular.otf')
glyph_set = font.getGlyphSet()
pen = FTPen(glyph_set)
glyph = glyph_set['A']
glyph.draw(pen)
width, ascender, descender = glyph.width, font['OS/2'].usWinAscent, -font['OS/2'].usWinDescent
# bytes
buf, size = pen.buffer(width, ascender, descender)
print(tuple(buf))
# PIL
print(pen.image(width, ascender, descender))
# numpy.ndarray
a = pen.array(width, ascender, descender)
print(type(a), a.shape)
# matplotlib
pen.show(width, ascender, descender)The code above might give some clues. Currently |
After experimenting with uharfbuzz for a while, I found out it was hard to handle top-to-bottom texts, so I gave up an idea to put an ascender or a descender value in the arguments. Instead, I simply expose 'offset', 'width' and 'height', which is way more straightforward than the previous design. In addition, 'contain' option is added to easily compensate and render glyphs such as combining accents or excessively tall letters.
Note that the example covers a ltr/rtl/ttb typesetting with uharfbuzz.
|
After playing around with uharfbuzz, I slightly modified the method arguments to make top-to-bottom layout less painful. Also wrote a docstring. See the uharfbuzz example in the docstring for more practical application. |
Lib/fontTools/pens/ftPen.py
Outdated
|
|
||
| """Pen to rasterize paths with FreeType.""" | ||
|
|
||
| __all__ = ['FTPen'] |
There was a problem hiding this comment.
Let's call it freetypePen/FreeTypePen to avoid the unnecessary confusion.
Lib/fontTools/pens/ftPen.py
Outdated
| >>> pen.save('glyph.png' width=500, height=1000) | ||
| """ | ||
| img = self.image(offset=offset, width=width, height=height, even_odd=even_odd, scale=scale, contain=contain) | ||
| img.save(fp, format=format, **kwargs) |
There was a problem hiding this comment.
I think users can call image then save it themselves, wrapping it does not seem to provide much value.
Lib/fontTools/pens/ftPen.py
Outdated
| :Example: | ||
| >>> pen = FTPen(None) | ||
| >>> glyph.draw(pen) | ||
| >>> pen.save('glyph.png' width=500, height=1000) |
There was a problem hiding this comment.
| >>> pen.save('glyph.png' width=500, height=1000) | |
| >>> pen.save('glyph.png', width=500, height=1000) |
Tests/pens/ftPen_test.py
Outdated
| offset, width, height = (0, 0), 500, 500 | ||
| buf1, _ = pen.buffer(offset=offset, width=width, height=height) | ||
| buf2 = zlib.decompress(base64.b64decode(ZLIB_B64_BIN)) | ||
| self.assertEqual(buf1, buf2) |
There was a problem hiding this comment.
asserting that the rendered buffer is exactly equal to something may be too precise for our tests, as it could fail unless a specific freetype version is used. Maybe we could relax this a bit, just assert that it is not empty or use Pillow to diff images within a threshold
|
I think you need to add |
As Khaled suggests, 'pen.image().save()' will do.
There are possibilities that the rendering results may change among FreeType versions. I've already used the PSNR comparison for cubic vs quadratic testing, so I applied the same technique and threshold to all rendering tests to relax assertions. Also handles the case that MSE becomes zero. Optional dependencies are not needed for the tests.
PointInsidePen already uses camelCase convention for the argument.
Tests/pens/freetypePen_test.py
Outdated
| @@ -31,84 +30,92 @@ def star(pen): | |||
| pen.lineTo((800, -200)) | |||
| pen.closePath() | |||
|
|
|||
| # Assume the buffers are equal when PSNR > 38dB. | |||
There was a problem hiding this comment.
pardon my ignorance, I had to google what "PSNR" means. Maybe a little explanation in a comment or a link to the wikipedia page for https://en.wikipedia.org/wiki/Peak_signal-to-noise_ratio could help clarify
There was a problem hiding this comment.
also i'm curious about the 38db magic number, does it have any special meaning or is just some abitrary cut-off value?
There was a problem hiding this comment.
Thanks for the comment. It is a arbitrary cut-off value – at first I randomly took the draw() function from another test, and then I realised that I needed a test for quadratic curves as well, so draw_quadratic() contains a quadratic contour converted from the cubic one. That's where I chose 38db as a threshold just to make the test pass when comparing the rendering results. I think the value is neither strict nor loose, but I don't have a good reasoning for the threshold. PSNR was one of the the easiest metrics for me to implement for comparison, but maybe there is better metrics for such a redering test.
There was a problem hiding this comment.
I see, thanks. Honestly I'm not so sure about embedding those base64-encoded zlib-compressed binary blobs for the expected renderings in the freetypePen_test.py.. There's no way a reader could make any sense of them without looking at them as an image with pixels on screen.
Maybe it's not fonttools's job to validate what the exact freetype rendering should be, I believe we could simply accept whatever comes out of freetype as good (provided it doesn't crash python).. What do you think?
There was a problem hiding this comment.
I agree that the tests are not for tracking the changes of FreeType. Still, I think they are valuable to verify the pen is emitting something meaningful. The reason I embedded test data in the source was because I was just hesitant to add test random images to the repo...and I've just come up with a simpler way to store them without size bloats in the latest commits. Please let me know if you have any concerns.
|
looks like the CI tests are failing with NameErrors because pytest is attempting to run your example docstrings as doctests. If those are not meant to be executed, I'd suggest you replace the special string prefix fonttools/Lib/fontTools/ttLib/ttFont.py Lines 21 to 24 in e778f2c |
|
Thank you so much for taking the time for the review and the feedback! They are so helpful. The examples are not meant to be run as part of doctest, thanks for the suggestion. Another thing I'm not still 100% confident is the API design for positioning, dimension and scaling. I'd appreciate any feedback on it. |
maybe instead of separate offset and scale parameters, you could have a single transform that takes a 6-tuple (2x3 affine transformation)? When you have separate offset and scale it's not obvious what comes first, e.g. if one should first translate and then scale or viceversa. I believe you currently have the offset unscaled (whereas I'd have expected the offset to already be scaled). Also, for the width/height parameters, I'd have thought they would not be affected by the scale parameter but would instead determine the final exact size of the buffer. |
Should put dimension-related arguments into a group.
Eliminates the assumption of any specific metrics from the pen. It still gives some image without giving any parameters, thus it should be a good starting point for new users.
Surprisingly I found Preview.app can still display PGM images. While somewhat legacy, it's a super straightforward format to (de)serialize. The images are scaled to 50x50 px and only consume 5KB in total. Makes more sense to human being than the previous base64-encoded zlib compressed data, plus we don't have to add pillow as a dependency.
|
Thank you for the feedback! Now changed to pass a single transformation matrix instead of the cluttered offset/scale parameters. I also removed the |
|
as first time contributor, you may want to add your name to the Acknowledgements section of the README |
|
correction, github reminds me you are not a first time contributor, nevertheless feel free to add your name if it's not already there :) |
|
Thanks for the review! Modified the requirements/acknowledgements sections in README. |
Add a new pen that rasterises the given outlines with FreeType installed on the system. Closes #2492.
Some implemation notes:
FTPen/ftPen: the ft prefix conflicts with fontTools. More verbose naming should be less confusing.Currently it doesn't use freetype-py. Part of the reason is we don't need a access toFT_FaceorFT_Glyphand all I was interested in was included inftoutln.c. Will fix the ctypes portion if freetype-py is preferred.load_freetype_lib()looks quirky. My homebrew installation is at~/homebrew, and the only way I came up to locate the shared library was to usesubprocess.width=1000, ascender=880, descender=-120) are questionable. Possibly there is a better way to specify the size of the resulted bitmap.