1160506 - implement style matching for fontconfig platform fontlist for "super families"

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[John Daggett (:jtd)]

As described in bug 1056479, comment 41, Debian distros use a fontconfig config file to wrap all of the various language-specific versions of the Droid Sans family into a Droid Sans "super family". So instead of matching "Droid Sans Japanese" and doing style selection with a single face in a family, Debian uses fontconfig language handling to pick the Droid Sans Japanese face within the Droid Sans super family.

Normally a face is picked within a family based on width/weight/slant. If that face doesn't have a glyph for a given codepoint fallback occurs. However, with one of these "super families", there's now a need to fallback *within* the family. In some cases where the language is not specified, it's hard to prioritize between the different faces that support a given codepoint (e.g. Droid Sans Japanese vs. Droid Sans Fallback).

This only affects Droid Sans on Debian so I think we should simply figure out a general solution that gets invoked for this situation. I don't think this complexity should be exposed on other platforms, creating "super families" like this is usually a mistake (e.g. mixing Type 1 Helvetica Neue with OpenType Helvetica Neue).

Comment 1

[Karl Tomlinson (:karlt back Feb 10)]

https://bugzilla.mozilla.org/show_bug.cgi?id=1056479#c42 describes the bigger issue.  I don't think it is likely that any change to the family name aliasing referenced in c41 will be required to address problems.

(In reply to John Daggett (:jtd) from comment #0)
> This only affects Droid Sans on Debian

and Fedora.  e.g. 
http://koji.fedoraproject.org/koji/buildinfo?buildID=591071

> (e.g. mixing Type 1 Helvetica Neue with OpenType Helvetica Neue).

I assume those fonts are different generations rather than intended to be used together as the Droid fonts are.

Comment 2

[Karl Tomlinson (:karlt back Feb 10)]

Not as important, but these faces also share fullnames.

Comment 3

[John Daggett (:jtd)]

Attachment: testcase, simple style matching for droid sans

Simple style matching for Droid Sans. Use of a serif font reflects fallback.

Comment 4

[John Daggett (:jtd)]

Attachment: testcase, simple style matching for droid sans, with bold/italic and lang tagging

So the big problem with these "super family" arrangements is that they basically require support for multiple faces per weight/width/slant value *and* fallback between styles.  For example, with all the Droid faces smushed into the Debian family, you have:

Droid Sans (regular)
Droid Sans Bold (bold)
Droid Sans Japanese (regular)
Droid Sans Fallback (regular)

When style matching for a Japanese string, if 'font-weight' is 'normal', the "Droid Sans Japanese" font should be used. Unless, there's an obscure character that's only found in "Droid Sans Fallback". If the weight is set to bold, the style matching will find Droid Sans Bold but they algorithm needs to find Droid Sans Japanese and use a synthetically bolded version of that.

Comment 5

[John Daggett (:jtd)]

Attachment: patch, allow intra-family font fallback

For families with a large number of faces that share style characteristics and cmap coverage, check all faces within a family to find whether a given face supports a character. Need to do some more testing before asking for review.

Comment 6

[John Daggett (:jtd)]

Attachment: droid-sans-unify-fontconfig.conf

The latest versions of Debian and Fedora no longer map all the various Droid Sans xxx font families into a single, monolithic version of Droid Sans. To test, need to use a specifically tailored fontconfig config file.

Steps to reproduce:
1. Copy the attached file to ~/.fonts.conf
2. sudo fc-cache -rf

Result: fc-list | grep Droid will now group all flavors of Droid Sans font families into "Droid Sans"

Comment 7

[John Daggett (:jtd)]

Attachment: patch, allow intra-family font fallback

Implements intra-family font fallback for families that have been explicitly tagged as requiring this. For fontconfig font families, if there's more than one regular face, mark it as requiring this sort of fallback. For the default set of fonts under Ubuntu, Debian and Fedora, this is typically not needed (as of Debian 8).

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8685822 [details] [diff] [review]
patch, allow intra-family font fallback

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

r=me with comments addressed.

Nit: s/fedora/Fedora/ in the commit message.

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +879,5 @@
>                   NS_ConvertUTF16toUTF8(fullname).get()));
>          }
>      }
> +
> +    // somewhat arbitrary, but define a family with more that two regular

s/that/than/

::: gfx/thebes/gfxTextRun.cpp
@@ +1650,5 @@
>      }
> +    // for a family marked as "check fallback faces", only mark the last
> +    // entry so that fallbacks for a family are only checked once
> +    if (aFamily->CheckForFallbackFaces() &&
> +        fontEntryList.Length() > 0 && mFonts.Length() > 0) {

Better to use IsEmpty where you can:

  !fontEntryList.IsEmpty() && !mFonts.IsEmpty()

@@ +1651,5 @@
> +    // for a family marked as "check fallback faces", only mark the last
> +    // entry so that fallbacks for a family are only checked once
> +    if (aFamily->CheckForFallbackFaces() &&
> +        fontEntryList.Length() > 0 && mFonts.Length() > 0) {
> +        mFonts[mFonts.Length() - 1].SetCheckForFallbackFaces();

You can use:

  mFonts.LastElement()

@@ +2566,5 @@
> +                                      int32_t aRunScript)
> +{
> +    GlobalFontMatch data(aCh, aRunScript, &mStyle);
> +    aFamily->SearchAllFontsForChar(&data);
> +    gfxFontEntry *fe = data.mBestMatch;

"*" next to type.

@@ +2753,5 @@
>          }
>  
> +        // check other family faces if needed
> +        if (ff.CheckForFallbackFaces()) {
> +            font = FindFallbackFaceForChar(ff.Family(), aCh, aRunScript);

Is it worth asserting or warning in here that the previous FamilyFace, mFonts[i - 1] (if i > 0), should either have a different family or not have CheckForFallbackFaces() set?

::: gfx/thebes/gfxTextRun.h
@@ +916,5 @@
>                mNeedsBold(aOtherFamilyFace.mNeedsBold),
>                mFontCreated(aOtherFamilyFace.mFontCreated),
>                mLoading(aOtherFamilyFace.mLoading),
> +              mInvalid(aOtherFamilyFace.mInvalid),
> +              mCheckForFallbackFaces(false)

Any reason not to copy mCheckForFallbackFaces from aotherFamilyFace?

@@ +1106,5 @@
>      // Return null if regular face doesn't support aCh
>      already_AddRefed<gfxFont>
>      FindNonItalicFaceForChar(gfxFontFamily* aFamily, uint32_t aCh);
>  
> +    // search all faces in a family for a fallback

I gather that since we don't know which face is the fallback face, we need to check all faces.  Maybe add a comment to that effect?

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cdabbfe799a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1cdabbfe799a

Comment 11

[John Daggett (:jtd)]

Comment on attachment 8685822 [details] [diff] [review]
patch, allow intra-family font fallback

Approval Request Comment
[Feature/regressing bug #]: This is one of the bugs that need to be fixed to enable the new fontconfig back end for beta/release builds. Enabling the new fontconfig back end will allow us to release unicode-range support across all platforms (bugs 1180560, 1119062).
[User impact if declined]: unicode-range support can't be enabled until FF45
[Describe test coverage new/current, TreeHerder]: landed on trunk last week, no issues reported
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8685822 [details] [diff] [review]
patch, allow intra-family font fallback

This has been on Nightly for a week, seems safe to uplift to Aurora44.

Comment 13

[John Daggett (:jtd)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8c58b5949a9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f8c58b5949a9

Comment 15

[John Daggett (:jtd)]

(In reply to Ritu Kothari (:ritu) from bug 1167284, comment #27)
> John, would it be possible to add automated tests for this "font super
> families" related changes? It would help catch regressions. I noticed that
> we also uplifted bug 1160506 this week.

Automated tests for this bug are hard, as they require specific font families and/or fontconfig setttings for the functionality of this bug to apply. This bug is mainly to support handling font families like "Droid Sans". Some Linux distros have taken to unifying all the subfamilies of Droid Sans (e.g. Droid Sans Japanese) into a *single* family via fontconfig wizardry.

I'll create a new follow-on bug for this but I'm skeptical that it will be easy to figure out a way to test this functionality other than through manual testing (i.e. set up a special configuration and test).