Allow variation fonts to record property ranges in gfxFontEntry, and update font-matching to handle ranges

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

4.09 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
76.53 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
3.23 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
131.57 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
8.34 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
A variation font can represent a range of (weight, stretch, style) values, not just a single tuple. So the gfxFontEntry for such a face needs to support ranges rather than single values, so that the font-matching algorithm can recognize the capabilities.
Just posting my current WIP here for reference. This currently breaks OpenType shaping with DejaVu Sans on Linux for some unknown reason, so that needs to be resolved before it's ready for review.
Attachment #8968664 - Flags: review?(lsalzman)
OK, I think this should basically work now. Try is currently closed, but I'll push a try job when I get the chance.
Attachment #8968926 - Flags: review?(jwatt)
Attachment #8968505 - Attachment is obsolete: true
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Minor update to fix build failure on android.
Attachment #8968941 - Flags: review?(jwatt)
Attachment #8968926 - Attachment is obsolete: true
Attachment #8968926 - Flags: review?(jwatt)
Comment on attachment 8968926 [details] [diff] [review]
part 2 - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges

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

::: gfx/src/FontPropertyTypes.h
@@ +349,5 @@
> +   */
> +  FontPropertyRange(T aMin, T aMax)
> +    : mValues(aMin, aMax)
> +  {
> +    MOZ_ASSERT(aMin <= aMax);

Is it possible that a malicious font file could end up violating this (and not being caught in Release builds of course).

::: gfx/thebes/gfxDWriteFontList.cpp
@@ +227,5 @@
>                   NS_ConvertUTF16toUTF8(fe->Name()).get(),
>                   NS_ConvertUTF16toUTF8(Name()).get(),
>                   (fe->IsItalic()) ?
>                    "italic" : (fe->IsOblique() ? "oblique" : "normal"),
> +                 fe->Weight().Min().ToFloat(), fe->Weight().Max().ToFloat(),

This sort of serialization is going to be a bit ugly in the common case of IsSingle() returning true. How about adding a ToString(nsACString& aString, const char* aSeparator) method to allow us to be smarter about this?

::: gfx/thebes/gfxFontEntry.cpp
@@ +1053,5 @@
> +                Weight().Min() <= FontWeight(axis.mMaxValue)) {
> +                mStandardFace = FontWeight(axis.mDefaultValue) == Weight().Min();
> +                mWeightRange =
> +                    WeightRange(FontWeight(std::max(1.0f, axis.mMinValue)),
> +                                FontWeight(std::min(1000.0f, axis.mMaxValue)));

Maybe we should we log/warn if the font is using out of range values that we end up clamping?

::: gfx/thebes/gfxFontEntry.h
@@ +140,5 @@
>      // The "real" name of the face, if available from the font resource;
>      // returns Name() if nothing better is available.
>      virtual nsString RealFaceName();
>  
> +    mozilla::WeightRange Weight() const { return mWeightRange; }

We shouldn't need the namespace prefix here.

@@ +149,5 @@
>      bool IsFixedPitch() const { return mFixedPitch; }
>      bool IsItalic() const { return mStyle == NS_FONT_STYLE_ITALIC; }
>      bool IsOblique() const { return mStyle == NS_FONT_STYLE_OBLIQUE; }
>      bool IsUpright() const { return mStyle == NS_FONT_STYLE_NORMAL; }
> +    bool IsBold() const { return Weight().Max().IsBold(); } // bold == weights 600 and above

It seems like we should change these Is* method names. I'm not sure what to though... CanBe* and CanInclude* don't seem quite right either.

::: gfx/thebes/gfxGDIFont.cpp
@@ +460,5 @@
>              weight = mNeedsBold ? 700 : 200;
>          }
>      } else {
> +        // GDI doesn't support variation fonts, so it's fine to just use
> +        // one of the weight-range values.

It seems to expect values that are a multiple of 100 in the range 1 - 900. It's not clear to me that it's okay to pass a value that is not one of these values to GDI.

Say the CSS specifies a weight in the range 101-250, wouldn't it be better to pass 200 to GDI rather than 101? Is so, maybe add a MultipleOf100OrClosestValue() method to WeightRange to use in places like this? And have that prefer the multiple of 100 closest to the midpoint of the range is more than one multiple is available?

If these comments are all off, it seems worth expanding the comment in tho code.
Attachment #8968926 - Attachment is obsolete: false
Attachment #8968926 - Attachment is obsolete: true
Attachment #8968664 - Flags: review?(lsalzman) → review+
(In reply to Jonathan Watt [:jwatt] from comment #5)
> > +    MOZ_ASSERT(aMin <= aMax);
> 
> Is it possible that a malicious font file could end up violating this (and
> not being caught in Release builds of course).

I don't believe so; for webfonts, the range doesn't come from the font file at all, it's provided by the descriptors in the @font-face rule. So the values should be clamped by the style system at parsing or computation time.

> ::: gfx/thebes/gfxFontEntry.h
> @@ +140,5 @@
> >      // The "real" name of the face, if available from the font resource;
> >      // returns Name() if nothing better is available.
> >      virtual nsString RealFaceName();
> >  
> > +    mozilla::WeightRange Weight() const { return mWeightRange; }
> 
> We shouldn't need the namespace prefix here.

We're not in the scope of a "using namespace mozilla;" here in the header, so we do need it.

> @@ +149,5 @@
> >      bool IsFixedPitch() const { return mFixedPitch; }
> >      bool IsItalic() const { return mStyle == NS_FONT_STYLE_ITALIC; }
> >      bool IsOblique() const { return mStyle == NS_FONT_STYLE_OBLIQUE; }
> >      bool IsUpright() const { return mStyle == NS_FONT_STYLE_NORMAL; }
> > +    bool IsBold() const { return Weight().Max().IsBold(); } // bold == weights 600 and above
> 
> It seems like we should change these Is* method names. I'm not sure what to
> though... CanBe* and CanInclude* don't seem quite right either.

Hmm, I'm not sure what I think about this. I don't have an idea that I like better offhand; can we defer this for now? (We could potentially change them all once stretch and style are also updated, if we decide on something that makes sense.)

> ::: gfx/thebes/gfxGDIFont.cpp
> @@ +460,5 @@
> >              weight = mNeedsBold ? 700 : 200;
> >          }
> >      } else {
> > +        // GDI doesn't support variation fonts, so it's fine to just use
> > +        // one of the weight-range values.
> 
> It seems to expect values that are a multiple of 100 in the range 1 - 900.
> It's not clear to me that it's okay to pass a value that is not one of these
> values to GDI.

According to the MSDN docs[1], LOGFONT.lfWeight is "in the range 0 through 1000"; the defined constants that are multiples of 100 are just provided "for convenience", but there's no suggestion that only these values are allowed. The LOGFONT struct isn't required to match any specific available face; it just provides a request, and GDI does its best to satisfy it.

Still, I guess snapping to a "standard" value here would be reasonable.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd145037(v=vs.85).aspx
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> > ::: gfx/thebes/gfxGDIFont.cpp
> > @@ +460,5 @@
> > >              weight = mNeedsBold ? 700 : 200;
> > >          }
> > >      } else {
> > > +        // GDI doesn't support variation fonts, so it's fine to just use
> > > +        // one of the weight-range values.
> > 
> > It seems to expect values that are a multiple of 100 in the range 1 - 900.
> > It's not clear to me that it's okay to pass a value that is not one of these
> > values to GDI.
> 
> According to the MSDN docs[1], LOGFONT.lfWeight is "in the range 0 through
> 1000"; the defined constants that are multiples of 100 are just provided
> "for convenience", but there's no suggestion that only these values are
> allowed. The LOGFONT struct isn't required to match any specific available
> face; it just provides a request, and GDI does its best to satisfy it.
> 
> Still, I guess snapping to a "standard" value here would be reasonable.

Oh, wait... this code path is only for system fonts (user fonts were handled above), so we don't need to worry about ranges at all. The GDI font back-end doesn't support variations, so for system fonts we can be sure that weight.min == weight.max, and it's a value that we got from GDI in the first place.
Thanks for the comments, updated where it seemed appropriate (see responses above re some of the issues).
Attachment #8969102 - Flags: review?(jwatt)
Attachment #8968941 - Attachment is obsolete: true
Attachment #8968941 - Flags: review?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #5)

> ::: gfx/thebes/gfxFontEntry.cpp
> @@ +1053,5 @@
> > +                Weight().Min() <= FontWeight(axis.mMaxValue)) {
> > +                mStandardFace = FontWeight(axis.mDefaultValue) == Weight().Min();
> > +                mWeightRange =
> > +                    WeightRange(FontWeight(std::max(1.0f, axis.mMinValue)),
> > +                                FontWeight(std::min(1000.0f, axis.mMaxValue)));
> 
> Maybe we should we log/warn if the font is using out of range values that we
> end up clamping?

Ah... actually, we don't need to clamp the max here, because if it's out of range we skip this code altogether (the font's weight axis doesn't seem to be using the same scale as CSS font-weight, so we don't attempt to map the property). And for min, the only clamping is that a value between 0.0 and 1.0 will be snapped up to 1.0; that's likely to be an imperceptible change, probably not worth warning.
Attachment #8969102 - Attachment is obsolete: true
Attachment #8969102 - Flags: review?(jwatt)
Tryserver: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f90615c63525c704e5a0592d6bf9e09a70230d9d. There's a lot of orange scattered about, but none of it looks related to this bug AFAICT.
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> (In reply to Jonathan Watt [:jwatt] from comment #5)
> > It seems like we should change these Is* method names. I'm not sure what to
> > though... CanBe* and CanInclude* don't seem quite right either.
> 
> Hmm, I'm not sure what I think about this. I don't have an idea that I like
> better offhand; can we defer this for now? (We could potentially change them
> all once stretch and style are also updated, if we decide on something that
> makes sense.)

Yeah, let's.
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> (In reply to Jonathan Watt [:jwatt] from comment #5)
> > ::: gfx/thebes/gfxFontEntry.h
> > @@ +140,5 @@
> > >      // The "real" name of the face, if available from the font resource;
> > >      // returns Name() if nothing better is available.
> > >      virtual nsString RealFaceName();
> > >  
> > > +    mozilla::WeightRange Weight() const { return mWeightRange; }
> > 
> > We shouldn't need the namespace prefix here.
> 
> We're not in the scope of a "using namespace mozilla;" here in the header,
> so we do need it.

Yeah, you're right. You'd need to add a typedef at the top of the class like we have for FontWeight. Could you do that though just to avoid making this more verbose than it needs to be? Same in the other headers where you're prefixing but we have typedefs for FontWeight.
Comment on attachment 8969109 [details] [diff] [review]
part 2 - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges

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

::: gfx/src/FontPropertyTypes.h
@@ +16,2 @@
>  #include "mozilla/Assertions.h"
> +#include "nsString.h"

(I wonder if we shouldn't create a .cpp file for some of this to keep this header lighter.)
Attachment #8969109 - Flags: review?(jwatt) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/072dc1504ce6
part 1 - Make gfxFcPlatformFontList::GetFTLibrary work before font system is fully up and running, so that the global FT_Library can be used during initialization of the font list itself.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b673e87d6134
part 2 - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges. r=jwatt
Tagging as leave-open, as there's a further patch coming here...
Keywords: leave-open
This has caused a bad regression with bug 1455494, can we back it out until fixed please?
Flags: needinfo?(jfkthame)
Backout by csabou@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/b95ae0e1e9e0
Backed out 2 changesets as requested by Mossop in developers channel. a=backout
Depends on: 1455531
Depends on: 1455526
When this landed (comment 15), we noticed these huge AWSY perf regressions.
The backout that followed canceled them.

== Change summary for alert #12802 (as of Thu, 19 Apr 2018 09:29:44 GMT) ==

Regressions:

 73%  Heap Unclassified linux64 opt stylo     56,870,163.90 -> 98,551,358.49
 72%  Heap Unclassified linux64-stylo-sequential opt stylo-sequential56,646,192.60 -> 97,532,853.00
 15%  Resident Memory linux64 opt stylo       517,567,543.28 -> 596,491,806.59
 15%  Explicit Memory linux64 opt stylo       300,355,830.78 -> 346,093,005.61
 14%  Explicit Memory linux64-stylo-sequential opt stylo-sequential298,131,469.91 -> 340,700,020.37
 13%  Heap Unclassified linux64-qr opt stylo  317,926,270.58 -> 360,014,790.29
 13%  Resident Memory linux64-stylo-sequential opt stylo-sequential520,450,101.10 -> 589,003,103.91
  8%  Explicit Memory linux64-qr opt stylo    556,856,302.79 -> 599,283,585.49
  6%  Resident Memory windows7-32 pgo stylo   480,362,707.43 -> 508,232,011.12
  6%  Resident Memory linux64-qr opt stylo    1,161,788,300.33 -> 1,225,801,228.31
  5%  Resident Memory windows7-32 opt stylo   487,927,765.79 -> 509,996,617.62

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12802
Flags: needinfo?(jfkthame)
This reduces the memory footprint of checking each face to see if it supports variations; instead of using FreeType, we can check the font pattern.
Attachment #8969894 - Flags: review?(lsalzman)
Attachment #8969894 - Flags: review?(lsalzman) → review+
Comment on attachment 8969271 [details] [diff] [review]
part 3 - Use WeightRange more extensively in place of FontWeight throughout user-font handling and font-entry creation

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

Nice.

::: gfx/thebes/gfxPlatformFontList.h
@@ +185,5 @@
>  
>      // get the system default font family
>      gfxFontFamily* GetDefaultFont(const gfxFontStyle* aStyle);
>  
> +    // Look up a font by name on the host platform.

This seems like good documentation, and worth using a Doxygen style comment for.
Attachment #8969271 - Flags: review?(jwatt) → review+
Will do, thanks.

(I'm holding off on actually landing anything here for now, so as not to bitrot the remaining patches in bug 1436048. As soon as that lands, I'll rebase here.)
This replaces the "part 3" that you previously reviewed; it now incorporates range support for all three properties, rather than going over all the same code repeatedly in separate patches. (Hope that's OK.) I'm just waiting on a new try build to see how many platforms are green, but think this is close enough to start reviewing - thanks.
Attachment #8970479 - Flags: review?(jwatt)
Attachment #8969271 - Attachment is obsolete: true
Comment on attachment 8970479 [details] [diff] [review]
part 3 - Use WeightRange more extensively in place of FontWeight throughout user-font handling and font-entry creation, and handle Stretch and SlantStyle similarly

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

r+ for this plus the changes discussed on IRC. If the outstanding reftest failure requires substantial changes please request review on an interdiff.

::: gfx/src/FontPropertyTypes.h
@@ +340,5 @@
> +      return Normal();
> +    } else if (strcmp(aString, "italic") == 0) {
> +      return Italic();
> +    } else {
> +      float angle = strtof(aString, nullptr);

Maybe worth a MOZ_ASSERT?

@@ +426,5 @@
>    {
>    }
>  
> +  explicit FontPropertyRange(const FontPropertyRange& aOther) = default;
> +  FontPropertyRange& operator= (const FontPropertyRange& aOther) = default;

operator=(

(no space)

@@ +474,5 @@
> +    return (mValues.first.ForHash() << 16) | mValues.second.ForHash();
> +  }
> +
> +  /*
> +   * FromScalar is defined in each individual subclass, because I can't

Can you add the string FIXME here?

@@ +507,3 @@
>    void ToString(nsACString& aOutString, const char* aDelim = "..") const
>    {
> +    aOutString.AppendFloat(Min().ToFloat());

It looks like this change will mean that we'll end up with 6 digits of precision, regardless of the value. I'm not sure why that's desirable. Using AppendPrintf("%g", ...) seems preferable to me on the face of it unless we have a specific reason to do otherwise.

This comment applies elsewhere too.

::: gfx/thebes/gfxDWriteFontList.h
@@ +119,5 @@
> +            (dwriteStyle == DWRITE_FONT_STYLE_ITALIC
> +                ? FontSlantStyle::Italic()
> +                : (dwriteStyle == DWRITE_FONT_STYLE_OBLIQUE
> +                   ? FontSlantStyle::Oblique()
> +                   : FontSlantStyle::Normal()));

Nit: I guess one more space of indent on this line if we're being pedantic?

::: gfx/thebes/gfxFT2FontList.cpp
@@ -655,5 @@
>              continue;
>          }
>  
> -        // We convert the weight to a float purely for transport across IPC.
> -        // Ideally we'd avoid doing that.

Nice.

::: gfx/thebes/gfxPlatformFontList.h
@@ +205,5 @@
> +     *
> +     * Note that the style attributes (weight, stretch, style) are NOT related
> +     * (necessarily) to any values within the font resource itself; these are
> +     * values to be recorded in the new font entry and used for face selection,
> +     * in place of whatever inherent style attributes the resource may have.

I wonder if the parameter names could be modified to indicate this.
Attachment #8970479 - Flags: review?(jwatt) → review+
(In reply to Jonathan Watt [:jwatt] from comment #27)
> @@ +507,3 @@
> >    void ToString(nsACString& aOutString, const char* aDelim = "..") const
> >    {
> > +    aOutString.AppendFloat(Min().ToFloat());
> 
> It looks like this change will mean that we'll end up with 6 digits of
> precision, regardless of the value. I'm not sure why that's desirable. Using
> AppendPrintf("%g", ...) seems preferable to me on the face of it unless we
> have a specific reason to do otherwise.

No, nsTSubstring::AppendFloat does the "nice" thing, as it calls a helper FormatWithoutTrailingZeros() internally:

https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/xpcom/string/nsTSubstring.cpp#1205-1210

So we normally get things like "weight: 400" in the log messages based on this, but it'll add decimals when required.
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af215d265b6
part 1 - Make gfxFcPlatformFontList::GetFTLibrary work before font system is fully up and running, so that the global FT_Library can be used during initialization of the font list itself. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/bac001216577
part 2.1 - For system-installed fonts, query FC_VARIABLE to determine if a face has variations rather than instantiating a FT_Face. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc910e36a7d7
part 2 - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/26e036b1c703
part 3 - Use WeightRange more extensively in place of FontWeight throughout user-font handling and font-entry creation, and handle Stretch and SlantStyle similarly. r=jwatt
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4185068f2e because :daleharvey reported in #fx-team that an inbound build on macOS 10.12 showed ugly bold system fonts. :\
The quick fix we looked at earlier wasn't sufficient, so here's a patch that more completely neuters variation-font support on pre-HighSierra machines. I'm a bit sad about this, but I think for now, at least, we should just disable entirely on 10.12, as it seems to be too trouble-prone. (Note that Apple also decided not to support it in Safari, so there's a good precedent for this.) :daleharvey confirmed that a try build with this patch behaves normally for him.
Attachment #8970848 - Flags: review?(jwatt)
Attachment #8970848 - Flags: review?(jwatt) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de118016272b
part 1 - Make gfxFcPlatformFontList::GetFTLibrary work before font system is fully up and running, so that the global FT_Library can be used during initialization of the font list itself. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b86f2f71a8
part 2.1 - For system-installed fonts, query FC_VARIABLE to determine if a face has variations rather than instantiating a FT_Face. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c05b11ca2b8
part 2 - Allow variation fonts to record a weight range in gfxFontEntry, and update font-matching to handle ranges. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee068c7ae8f4
part 3 - Use WeightRange more extensively in place of FontWeight throughout user-font handling and font-entry creation, and handle Stretch and SlantStyle similarly. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9eda40f477
part 4 - Disable font variations on macOS Sierra due to Core Text unreliability. r=jwatt
Keywords: leave-open
Depends on: 1458158
You need to log in before you can comment on or make changes to this bug.