[Webkit-unassigned] [Bug 183155] [FreeType] Color emojis in WebKitGTK+ for great justice
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 28 02:54:34 PST 2018
https://bugs.webkit.org/show_bug.cgi?id=183155
--- Comment #9 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #7)
> Comment on attachment 334693 [details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334693&action=review
>
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:73
> > + bool italic = fontDescription.italic();
> > + if (!FcPatternAddInteger(pattern, FC_SLANT, italic ? FC_SLANT_ITALIC : FC_SLANT_ROMAN))
>
> Since you're moving this code, might as well take the opportunity to get rid
> of the unnecessary local variable and do fontDescription.italic() ?
> FC_SLANT_ITALIC : FC_SLANT_ROMAN
Sure.
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:98
> > + if (!configurePatternForFontDescription(pattern.get(), fontDescription))
> > + return nullptr;
>
> Nit: to make this more readable, add one blank line above and below. It's
> unrelated to forcing scalable fonts and unrelated to the
> FcConfigSubstitute/cairo_ft_font_options_substitute/FcDefaultSubstitute
> dance.
Ok.
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:112
> > - return FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult);
> > + return adoptRef(FcFontSetMatch(nullptr, &fallbacks, 1, pattern, &fontConfigResult));
>
> Good catch.
>
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:126
> > + // Try first a direct match.
> > + FcResult fontConfigResult;
> > + if (RefPtr<FcPattern> resultPattern = adoptRef(FcFontMatch(nullptr, pattern.get(), &fontConfigResult))) {
> > + FontPlatformData alternateFontData(resultPattern.get(), description);
> > + return fontForPlatformData(alternateFontData);
> > + }
>
> I'm pretty sure this is wrong. I think Fontconfig will always return a
> pattern here, whatever works best. Now you'll never consider the fallback
> case. In short: I suspect this disables CSS font fallback. E.g. if you have,
> say:
>
> font: Cantarell; Liberation Sans; sans
>
> I suspect that, with this change, fontconfig will always return some
> best-effort result for Cantarell, whatever its closest match is, without
> ever considering the fallbacks Liberation Sans or sans.
>
> If I'm wrong, please explain why.
Yes, but only in part :-) It seems you are right that direct match always returns a pattern, so the fallbacks code is never used. But this has nothing to do with CSS fallbacks, those are handled by WebCore, not by fonconfig. The CSS fallbacks are used when we fail to find a font for the given family, then trying with the next fallbacks. In all those cases FontCache::createFontPlatformData() is called with the font family. When we have a match for the font family, but we fail to find a glyph, then we try the system fallbacks, ignoring the font family simply looking for a font that contains glyphs for the given characters. In this case is when we call FontCache::systemFallbackForCharacters(). So, it seems to me that in the case of system fallbacks the direct match and the fallbacks match are doing pretty much the same thing, so we can simply get rid of the fontconfig fallbacks. I've made several tests cases with a few debug output to see how this works (with this patch applied):
- test-glyph-in-css-fallbacks.html
<span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">①</span>
DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf
DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf
DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: draw glyphs with DejaVuSans
Cantarell doesn't have a glyph for the character, and libration either, so we end up using DejaVuSans. Note that systemFallbackForCharacters() is not called at all, because there's a font in the CSS fallbacks that contains the glyph.
- test-main-font-not-found-glyph-in-css-fallbacks.html
<span style="font-family: TheGreatFooFont, Cantarell, sans; font-size: 128px">≈</span>
DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: No matchedFontFamily for TheGreatFooFont - TheGreatFooFont
DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf
DBG: draw glyphs with Cantarell-Regular
The first font doesn't exist, but Cantarell has a glyph to render the character, so Cantarell-Regular is used. No systemFallbackForCharacters() in this case either.
- test-glyph-not-in-css-fallbacks-but-in-system.html
<span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">🍔</span>
DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf
DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf
DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: systemFallbackForCharacters: d83c df54
DBG: Direct fallback found for d83c df54 at /home/cgarcia/.local/share/fonts/emojione-android.ttf
DBG: draw glyphs with EmojiOne
It's an emoji that is not present in any of the CSS fallbacks, but it's present in EmojiOne, installed in my system. systemFallbackForCharacters() is called in this case and the direct match find the right font. Doing the fallbacks before the direct match in this case provides the exactly same result.
- test-glyph-not-in-css-fallbacks-nor-in-system.html
<span style="font-family: Cantarell, Liberation Sans, sans; font-size: 128px">🏱</span>
DBG: Match found for sans-serif - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: Match found for Cantarell - Cantarell at /usr/share/fonts/opentype/cantarell/Cantarell-Regular.otf
DBG: Match found for Liberation Sans - Liberation Sans at /usr/share/fonts/truetype/liberation2/LiberationSans-Regular.ttf
DBG: Match found for sans - DejaVu Sans at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: systemFallbackForCharacters: d83c dff1
DBG: Direct fallback found for d83c dff1 at /usr/share/fonts/truetype/dejavu/DejaVuSans.ttf
DBG: draw glyphs with Cantarell-Regular
There isn't any font in the system with the glyph required to render the character, so the system fallback returned is DejaVuSans, but since it doesn't contain the glyph either, the main font is used to render the square that indicates there's no glyph. In this case doing the fallbacks before the direct match provides the exactly same result.
So, I think we can get rid of the fontconfig fallbacks entirely. They were implemented in 2008 (see r36309) and only the fallbacks were used in FontCache::getFontDataForCharacters(). Then, the direct match after the fallbacks was added in 2010 (see r70688), including a layout test (platform/gtk/fonts/custom-font-missing-glyphs.html) that still passes with this patch.
> > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:310
> > - cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx,
> > - -fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);
> > +
> > + cairo_matrix_init(&fontMatrix, 1, -fontConfigMatrix.yx, -fontConfigMatrix.xy, 1, 0, 0);
>
> Is ignoring the scale really the right thing to do? This seems quite
> doubtful....
We don't expect to get a scale change from the fontconfig matrix, that's what is causing this bug. See the emoji browser test linked in description, we are actually rendering the icons using emojione, but they are tiny, because the scale is broken. See the comments in the code:
// FontConfig may return a list of transformation matrices with the pattern, for instance,
// for fonts that are oblique. We use that to initialize the cairo font matrix.
We don't expect a scale change, only in rotation for oblique fonts.
// The matrix from FontConfig does not include the scale. Scaling a font with width zero size leads
// to a failed cairo_scaled_font_t instantiations. Instead we scale the font to a very tiny
// size and just abort rendering later on.
Even clearer in this comment.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180228/7492268d/attachment-0001.html>
More information about the webkit-unassigned
mailing list