Closed Bug 1276594 Opened 4 years ago Closed 4 years ago

Couldn't find characters in bundled font via gfxFontconfigFontEntry::TestCharacterMap

Categories

(Core :: Graphics: Text, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: timdream, Assigned: jfkthame)

References

Details

Attachments

(1 file, 1 obsolete file)

This is strange because it only happens on EmojiOne Mozilla font, not Firefox Emoji, but here is what I found with gdb: At this function, it returns false and thinks the font file has no desired emoji characters.

https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/gfx/thebes/gfxFcPlatformFontList.cpp#378-385

If I try some bad thing like this

     // for system fonts, use the charmap in the pattern
-    if (!mIsDataUserFont) {
+    if (!mIsDataUserFont && aCh != 0x1f4a9) {

The poo character will show up -- meaning for our instance, `mIsDataUserFont` was incorrectly set to `false`.

The font file was indeed picked up in other places, including gfxFcPlatformFontList::InitFontList(). I was trying to come up with a patch that would set the mIsDataUserFont all the way from there to

gfxFcPlatformFontList::AddFontSetFamilies()
gfxFontconfigFontFamily::gfxFontconfigFontFamily()
...

but I don't know if it's the right approach (or, if the approach would be too fragile and be confused when a user drops a font file in the app font dir that belongs to an existing font family).
> This is strange because it only happens on EmojiOne Mozilla font, not Firefox Emoji

It could be happening to Firefox Emoji as well on *the latest m-c*; I didn't try it.
Summary: Couldn't find characters for bundled fonts from gfxFontconfigFontEntry::TestCharacterMap → Couldn't find characters in bundled font via gfxFontconfigFontEntry::TestCharacterMap
Argh, I think I know what's happening here. Fontconfig doesn't recognize the presence of the color glyphs, so as far as it's concerned, all those emoji codepoints are blank in the E1-Moz font; and it considers blanks to be characters that are unsupported by the font [except for things like <space>, obviously) and so falls back to another font.

Setting mIsDataUserFont to true would work around this because it claims the font wasn't loaded through fontconfig at all, but through @font-face; and in that case we don't depend on fontconfig's idea of the supported character map. But we can't simply do such a hack, because that will have other side-effects.

A possible workaround would be to modify the condition in gfxFontconfigFontEntry::TestCharacterMap such that it checks for the presence of COLR or SVG tables (the same issue would apply to SVG-in-OT fonts with blank b/w fallback glyphs, I believe).

Another solution would be to ensure that the converted emoji font contains b/w default glyphs. I didn't attempt to do this in the conversion script because our target usage is narrowly controlled, and we know color rendering will be supported, so b/w glyphs would simply add extra size for no benefit. But in general they would make the font more widely usable, and they would solve this issue.

(FWIW, the SVG-in-OT conversion of EmojiOne does have b/w fallback glyphs, which are generated quite cleverly by running an edge-detection tool on the color renderings, and then turning the resulting edges into b/w outlines. We could borrow that idea, but the resulting glyphs tend to be quite big in many cases, and the readability of the results is rather variable -- some are amazingly good, others a bit confusing/odd-looking.)

I'll try to look into the options a bit....
This should fix the EmojiOne Mozilla failure on Linux, I think; try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11108b11360e. We'll also need to check talos results to see if there's any measurable perf cost from the extra conditions.
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8757876 - Flags: review?(karlt)
Comment on attachment 8757876 [details] [diff] [review]
Don't rely on the fontconfig charmap for COLR or SVG fonts in case the default (fallback) glyphs are blank

The disadvantage with doing this is that TestCharacterMap() will need to open
each file tested once to read the cmaps.  I expect that will only have a significant impact on performance the first time font fallback is required for one or more characters.

Recent versions of fontconfig and freetype have support for color glyphs, so I
wonder whether that will resolve this problem anyway?

Is it important to add support to Gecko for such system fonts that are not supported by the system?

Do you think people are likely to have SVG fonts installed in the
system?  Are other apps likely to support these, given FreeType does not?
Flags: needinfo?(jfkthame)
(In reply to Karl Tomlinson (ni?:karlt) from comment #4)
> Comment on attachment 8757876 [details] [diff] [review]
> Don't rely on the fontconfig charmap for COLR or SVG fonts in case the
> default (fallback) glyphs are blank
> 
> The disadvantage with doing this is that TestCharacterMap() will need to open
> each file tested once to read the cmaps.  I expect that will only have a
> significant impact on performance the first time font fallback is required
> for one or more characters.

Right, there's a cost here. With the patch as written, we'll open each font file once (the first time we encounter it when doing fallback) to check if it has 'COLR'/'CPAL' or 'SVG ' tables (and if so, those tables will be cached by the font entry). If they're not found, though (which will be the 99% case), we don't manually load & interpret the cmap; we'll continue to use fontconfig's (likely pre-cached) charmap, as we do now.

(For the tiny minority of fonts that *do* have color glyphs, this patch will mean we load the color or svg table at font-fallback time, rather than waiting until the first glyph is actually rendered, but I don't think that's a significant issue given how rare such fonts will be.)

If we want to avoid the cost of opening all the font files to check for color tables here, perhaps we could add a flag to the font entry to distinguish application-bundled fonts from system-installed ones, and do this only for bundled ones. That would solve the current use case (bug 1231701), but would mean such font wouldn't work if installed locally as system fonts -- perhaps that's OK, as they wouldn't work anywhere else either.

> Recent versions of fontconfig and freetype have support for color glyphs, so
> I
> wonder whether that will resolve this problem anyway?

No, AFAICS it won't (or at least not yet). I checked the implementation of FcFreeTypeCheckGlyph() at https://cgit.freedesktop.org/fontconfig/tree/src/fcfreetype.c#n2206, and it looks only for the presence of an outline in the default glyph mapped from the character code; it doesn't consider whether that glyph will actually be replaced at rendering time by a set of color layers.

> Is it important to add support to Gecko for such system fonts that are not
> supported by the system?

The motivation here is to support a color-emoji font bundled with Firefox (bug 1231701), so that we can provide emoji support across all platforms regardless of whether the OS provides color font support or not.

> Do you think people are likely to have SVG fonts installed in the
> system?  Are other apps likely to support these, given FreeType does not?

Unlikely, I'd guess; we could drop the TryGetSVGData test here and most likely it wouldn't matter. I included that just for completeness/consistency -- but also, note that it should be a pretty lightweight check, given that we'd have already opened the font file to check for color tables.

Would you prefer to see a patch that limits this to app-bundled fonts, to eliminate the overhead of looking into system font files (at the cost of adding flags to track whether font families and entries come from the system or application font set)?
Flags: needinfo?(jfkthame) → needinfo?(karlt)
Comment on attachment 8757876 [details] [diff] [review]
Don't rely on the fontconfig charmap for COLR or SVG fonts in case the default (fallback) glyphs are blank

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #4)
> > The disadvantage with doing this is that TestCharacterMap() will need to open
> > each file tested once to read the cmaps.  I expect that will only have a
> > significant impact on performance the first time font fallback is required
> > for one or more characters.
> 
> Right, there's a cost here. With the patch as written, we'll open each font
> file once (the first time we encounter it when doing fallback) to check if
> it has 'COLR'/'CPAL' or 'SVG ' tables

Yes.  This is the only part I'm concerned about.

I guess font fallback would not usually happen at start-up, unless wikipedia
is the home page.  Still wikipedia may take some time to display the first
time on a system with many fonts.  I measured to see.

Times in seconds to load https://www.wikipedia.org/ from network cache:

            first time   cached files
with patch     8.0          2.2
without        3.4          2.0

The initial timing can be repeated by running as root
echo 1 >| /proc/sys/vm/drop_caches

> If we want to avoid the cost of opening all the font files to check for
> color tables here, perhaps we could add a flag to the font entry to
> distinguish application-bundled fonts from system-installed ones, and do
> this only for bundled ones. That would solve the current use case (bug
> 1231701), but would mean such font wouldn't work if installed locally as
> system fonts -- perhaps that's OK, as they wouldn't work anywhere else
> either.

I think the cost is significant enough that it is worth trying to avoid it, by
this method or another.

I wonder whether an alternative to consider would be to skip loading the files
during fallback, but still check for color/svg if the family name is specified
explicitly?

I guess the idea is that the this emoji font is used for fallback, so that
would only be a partial solution, but adding it to x-unicode prefs may be
sufficient, if you think that is reasonable.

> > Recent versions of fontconfig and freetype have support for color glyphs, so
> > I
> > wonder whether that will resolve this problem anyway?
> 
> No, AFAICS it won't (or at least not yet). I checked the implementation of
> FcFreeTypeCheckGlyph() at
> https://cgit.freedesktop.org/fontconfig/tree/src/fcfreetype.c#n2206, and it
> looks only for the presence of an outline in the default glyph mapped from
> the character code; it doesn't consider whether that glyph will actually be
> replaced at rendering time by a set of color layers.

Thanks for looking.

I was assuming from https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#FT_LOAD_COLOR
that slot->format would be ft_glyph_format_bitmap.
Are the layers bitmaps?

https://bugzilla.mozilla.org/show_bug.cgi?id=1231701#c73
indicates some kind of native support in Ubuntu 16.04.

> The motivation here is to support a color-emoji font bundled with Firefox
> (bug 1231701), so that we can provide emoji support across all platforms
> regardless of whether the OS provides color font support or not.

Thanks.  I didn't read the summary carefully enough.

> we could drop the TryGetSVGData test here and most
> likely it wouldn't matter. I included that just for completeness/consistency
> -- but also, note that it should be a pretty lightweight check, given that
> we'd have already opened the font file to check for color tables.

I agree that this is lightweight if the file is open anyway.

> Would you prefer to see a patch that limits this to app-bundled fonts, to
> eliminate the overhead of looking into system font files (at the cost of
> adding flags to track whether font families and entries come from the system
> or application font set)?

I'm not too worried about the cost of another flag.  The extra flag wouldn't
be needed on the font entries if mSVGInitialized were set directly, but that
wouldn't work well with the idea of checking when the family name is
specified.

InitFontList() already calls AddFontSetFamilies() in two passes, so something
like this seems workable.  I guess a flag would be needed on the family until
FindStyleVariations() is called.  Looking at fontconfig source, it looks like even app font patterns are cached.  If so, they can't be modified after read from FcConfigGetFonts().
Flags: needinfo?(karlt)
Attachment #8757876 - Flags: review?(karlt)
Here's a version that should avoid any perf impact for system fonts. The trickiest part of this, I think, turned out to be fonts loaded using @font-face with src:local(), because when that looks up a font name, it can't tell whether the font comes from the system or application font-set. Hence the extra gyrations in gfxFontconfigFontEntry::TestCharacterMap (without that, it failed a couple of src:local reftests on try, I think because it may end up loading legacy Type 1 faces). Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fdc5dda3e2e.
Attachment #8758691 - Flags: review?(karlt)
Attachment #8757876 - Attachment is obsolete: true
Comment on attachment 8758691 [details] [diff] [review]
Don't rely on the fontconfig charmap for user fonts or app-bundled fonts; check the cmap directly instead

Review of attachment 8758691 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +1198,5 @@
>  
> +    gfxFontconfigFontEntry* fe =
> +        new gfxFontconfigFontEntry(aFontName, fontPattern,
> +                                   aWeight, aStretch, aStyle);
> +    return fe;

Oops, this change (and the similar one below, in MakePlatformFont) can be reverted; it's just residue from a previous iteration of the patch.
Priority: -- → P2
Comment on attachment 8758691 [details] [diff] [review]
Don't rely on the fontconfig charmap for user fonts or app-bundled fonts; check the cmap directly instead

>-    if (!mIsDataUserFont) {
>-        return HasChar(mFontPattern, aCh);

>+        if (mIsLocalUserFont && !HasFontTable(TRUETYPE_TAG('c','m','a','p'))) {
>+            mIgnoreFcCharmap = false;
>+            // ...and continue with HasChar() below.

I think its best to do this even for entries in families where some may be app
fonts.  The families that have app fonts may also have system fonts.
i.e. s/mIsLocalUserFont/!mIsDataUserFont/.

>         gfxFontconfigFontEntry *fontEntry =
>             new gfxFontconfigFontEntry(faceName, face);
>+        fontEntry->SetIgnoreFcCharmap(mIsAppFontFamily);

Looks like a parameter to the constructor would be simpler, and would make it
clearer when this value might change.

>+                static_cast<gfxFontconfigFontFamily*>(fontFamily)->
>+                    SetFamilyContainsAppFonts(true);

There is another static_cast on this below.
Seems it would be simpler to declare gfxFontconfigFontFamily* fontFamily, and
cast the rv of mFontFamilies.GetWeak().

>+    AddFontSetFamilies(systemFonts, false);

>+    AddFontSetFamilies(appFonts, true);

Please either use an enum or add an inline comment to indicate what the
boolean arguments are indicating. i.e. /* aAppFonts = */

>-    if (!mIsDataUserFont) {
>-        return HasChar(mFontPattern, aCh);
>+    // Whether TestCharacterMap should check the actual cmap rather than asking
>+    // fontconfig about character coverage.
>+    // We do this for app-bundled (rather than system) fonts, as they may
>+    // include color glyphs that fontconfig would overlook, and for fonts
>+    // loaded via @font-face (which FC doesn't know about).

Remove "(which FC doesn't know about)" because fontconfig does have a charmap
even for data fonts.  I guess the remaining reasons are SVG and color fonts,
and being consistent with other platforms.

>-    return new gfxFontconfigFontEntry(aFontName,
>-                                      fontPattern,
>-                                      aWeight, aStretch, aStyle);
>+    gfxFontconfigFontEntry* fe =
>+        new gfxFontconfigFontEntry(aFontName, fontPattern,
>+                                   aWeight, aStretch, aStyle);
>+    return fe;

>-    return new gfxFontconfigFontEntry(aFontName, aWeight, aStretch,
>-                                      aStyle, aFontData, face);
>+    gfxFontconfigFontEntry* fe =
>+        new gfxFontconfigFontEntry(aFontName, aWeight, aStretch,
>+                                   aStyle, aFontData, face);
>+    return fe;

Leave as is.

>+    bool      mIsAppFontFamily;

Please rename to clarify that not all entries are necessarily app fonts.
e.g. m(Contains|Has|Includes)AppFonts
Attachment #8758691 - Flags: review?(karlt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/43175cc63c6ec5349678f93112a78fde1fd29b06
Bug 1276594 - Don't rely on the fontconfig charmap for user fonts or app-bundled fonts; check the cmap directly instead. r=karlt
https://hg.mozilla.org/mozilla-central/rev/43175cc63c6e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.