Fix FreeType glyph cache upload for embedded bitmap glyphs#155
Fix FreeType glyph cache upload for embedded bitmap glyphs#155arsnyder16 wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #155 +/- ##
==========================================
- Coverage 97.61% 97.60% -0.01%
==========================================
Files 159 159
Lines 17282 17302 +20
==========================================
+ Hits 16870 16888 +18
- Misses 412 414 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mosra
left a comment
There was a problem hiding this comment.
Thank you and sorry for the delay, too many things happening this week.
The code is top-notch (and your previous PRs were as well), I have just a few questions / minor suggestions. If you don't have time to do the followup changes, let me know, I can handle those myself.
By the way, may I ask what are you working on / what's your use case, if it can be shared publicly?
| .sliceSize({std::size_t(glyphs[i].offset.y()), | ||
| std::size_t(glyphs[i].offset.x())}, glyphSize); | ||
| CORRADE_INTERNAL_ASSERT(bitmap->pixel_mode == FT_PIXEL_MODE_GRAY); | ||
| CORRADE_INTERNAL_ASSERT(bitmap->num_grays > 1); |
There was a problem hiding this comment.
Thanks for changing the error into assertions, that would have been my suggestion as well (as it'd be impossible-to-test code, which is as good as not handling that case at all).
| const unsigned char* const src = bitmap->buffer + y*bitmap->pitch; | ||
| char* const dst = &glyphDst[bitmap->rows - y - 1][0]; | ||
| for(std::size_t x = 0; x != bitmap->width; ++x) { | ||
| dst[x] = char((UnsignedInt(src[x])*255)/(bitmap->num_grays - 1)); |
There was a problem hiding this comment.
Hmm, I was desperately trying to find some other way to do this, but as I don't have any convenient pixel format conversion APIs in Magnum, what you have here is probably the best possible right now. (FreeType seems to set num_grays to just 2, 4, 16 or 256, but branching on that and then having to test each such case would be excessive, such logic doesn't belong to this plugin.)
Once I have the required APIs in Magnum I'll turn this part into something similar to the copy() above.
| const FT_Bitmap* bitmap = &glyph->bitmap; | ||
| if(bitmap->pixel_mode != FT_PIXEL_MODE_GRAY) { | ||
| FT_Bitmap_Init(&convertedBitmap); | ||
| CORRADE_INTERNAL_ASSERT_OUTPUT(FT_Bitmap_Convert(freeType.library, bitmap, &convertedBitmap, 1) == 0); | ||
| bitmap = &convertedBitmap; | ||
| } |
There was a problem hiding this comment.
| const FT_Bitmap* bitmap = &glyph->bitmap; | |
| if(bitmap->pixel_mode != FT_PIXEL_MODE_GRAY) { | |
| FT_Bitmap_Init(&convertedBitmap); | |
| CORRADE_INTERNAL_ASSERT_OUTPUT(FT_Bitmap_Convert(freeType.library, bitmap, &convertedBitmap, 1) == 0); | |
| bitmap = &convertedBitmap; | |
| } | |
| const FT_Bitmap* bitmap; | |
| if(bitmap->pixel_mode == FT_PIXEL_MODE_GRAY) { | |
| bitmap = &glyph->bitmap; | |
| } else { | |
| FT_Bitmap_Init(&convertedBitmap); | |
| bitmap = &convertedBitmap; | |
| CORRADE_INTERNAL_ASSERT_OUTPUT(FT_Bitmap_Convert(freeType.library, &glyph->bitmap, &convertedBitmap, 1) == 0); | |
| } |
would be perhaps slightly more readable? I hope I didn't misunderstand what's going on here.
| CORRADE_VERIFY(glyphId); | ||
| font->fillGlyphCache(cache, "X"); | ||
| CORRADE_VERIFY(cache.called); | ||
|
|
There was a problem hiding this comment.
This is verifying that the image matches exactly the expectation, but maybe it'd be enough to make the test similar to the fillGlyphCache() test, and compare against a PNG image, possibly together with checking that min of all pixel values is 0 and max is 255?
And the Gray4 would be just a second instance of this test case, with different input TTF and different output PNG.
(Just in case, you can save the actual PNG image in current working directory by running the test with -XS$(pwd). I can also do this change myself if you don't want to deal with it.)
| "--name-IDs=*", | ||
| "--name-legacy", | ||
| "--name-languages=*" | ||
| ], check=True) |
There was a problem hiding this comment.
Out of curiosity I tried to rasterize the X glyph from MonochromeBitmap.ttf with StbTrueTypeFont to see how / if it fails, and there it produced the same antialiased glyph as it'd do with Oxygen.ttf.
So if I understand correctly, in the script you're keeping the original vector glyph and adding a bitmap one as well, and FreeType prefers the bitmap glyph while stb_truetype the vector one? IOW, if FreeType would get FT_LOAD_NO_BITMAP, it'd behave the same as stb_truetype?
Is it possible for the script to produce a font with just a bitmap outline for X, so I could use the generated files to verify also if / how stb_truetype handles those? Thank you.
|
|
||
| const UnsignedInt glyphId = font->glyphId('X'); | ||
| CORRADE_VERIFY(glyphId); | ||
| font->fillGlyphCache(cache, "X"); |
There was a problem hiding this comment.
| font->fillGlyphCache(cache, "X"); | |
| font->fillGlyphCache(cache, {glyphId}); |
will render just the X glyph alone into the output, without the implicitly-added glyph 0, and thus should be less annoying for the comparison after.
This fixes glyph cache corruption when FreeType returns embedded bitmap glyphs in a packed bitmap format instead of an 8-bit grayscale bitmap.
Previously
FreeTypeFont::fillGlyphCache()assumed thatFT_Render_Glyph()always produced bitmap data that could be copied directly into theR8Unormglyph cache as one byte per pixel. That is not true for embedded bitmap strikes. For example, FreeType can returnFT_PIXEL_MODE_MONOorFT_PIXEL_MODE_GRAY4, where the bitmap data is packed andbitmap.pitchdoes not matchbitmap.width. Copying that data as if it were tightly packed 8-bit grayscale causes corrupted glyph cache contents.The fix uses
FT_Bitmap_Convert()if the pixel mode is notFT_PIXEL_MODE_GRAYbefore uploading the glyph bitmap. This converts the bitmap storage toFT_PIXEL_MODE_GRAY, giving us one byte per pixel and a usable pitch for row-by-row copying.One subtlety is that
FT_Bitmap_Convert()changes the storage format, but it does not always expand the coverage values to the full0..255range. The converted bitmap still preserves num_grays, so a monochrome bitmap becomes byte values in0..1, and a 4bpp grayscale bitmap becomes byte values in0..15. The upload path now checksbitmap.num_grays:num_grays == 256: copy the converted bytes directly0..num_grays - 1into0..255before writing to the glyph cacheTest coverage was added with generated font fixtures containing embedded bitmap strikes:
MonochromeBitmap.ttf: exercisesFT_PIXEL_MODE_MONO,num_grays = 2Gray4Bitmap.ttf: exercisesFT_PIXEL_MODE_GRAY4,num_grays = 16The fixtures are generated from the existing
Oxygen.ttf testfont usinggenerate-embedded-bitmaps.py, so the test data can be reproduced.