Bug 1440938 - Fall back to cairo's glyph metrics API if FreeType fails in some way, or if we're not using a variation font. r=lsalzman
authorJonathan Kew <jkew@mozilla.com>
Wed, 28 Feb 2018 22:02:25 +0000
changeset 461081 e3aa74bcd4955d8ed40e184c7daf75e305f56452
parent 461080 d02fd1a8e0eab4dc13945d55581d4215d3bb9677
child 461082 fdbf87c9cfa7a61c9068632ff1a33babd4a9899e
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslsalzman
bugs1440938
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1440938 - Fall back to cairo's glyph metrics API if FreeType fails in some way, or if we're not using a variation font. r=lsalzman
gfx/thebes/gfxFT2FontBase.cpp
gfx/thebes/gfxFT2FontBase.h
--- a/gfx/thebes/gfxFT2FontBase.cpp
+++ b/gfx/thebes/gfxFT2FontBase.cpp
@@ -169,17 +169,23 @@ gfxFT2FontBase::GetCharExtents(char aCha
  * exists.  aWidth is only set when this returns a non-zero glyph id.
  * This is just for use during initialization, and doesn't use the width cache.
  */
 uint32_t
 gfxFT2FontBase::GetCharWidth(char aChar, gfxFloat* aWidth)
 {
     FT_UInt gid = GetGlyph(aChar);
     if (gid) {
-        *aWidth = FLOAT_FROM_16_16(GetFTGlyphAdvance(gid));
+        int32_t width;
+        if (!GetFTGlyphAdvance(gid, &width)) {
+            cairo_text_extents_t extents;
+            GetGlyphExtents(gid, &extents);
+            width = NS_lround(0x10000 * extents.x_advance);
+        }
+        *aWidth = FLOAT_FROM_16_16(width);
     }
     return gid;
 }
 
 void
 gfxFT2FontBase::InitMetrics()
 {
     mFUnitsConvFactor = 0.0;
@@ -521,78 +527,89 @@ gfxFT2FontBase::GetGlyph(uint32_t unicod
             return GetGlyph(unicode);
         }
         return 0;
     }
 
     return GetGlyph(unicode);
 }
 
-FT_Fixed
-gfxFT2FontBase::GetFTGlyphAdvance(uint16_t aGID)
+bool
+gfxFT2FontBase::GetFTGlyphAdvance(uint16_t aGID, int32_t* aAdvance)
 {
     gfxFT2LockedFace face(this);
     MOZ_ASSERT(face.get());
     if (!face.get()) {
         // Failed to get the FT_Face? Give up already.
-        return 0;
+        NS_WARNING("failed to get FT_Face!");
+        return false;
     }
+
+    // Due to bugs like 1435234 and 1440938, we currently prefer to fall back
+    // to reading the advance from cairo extents, unless we're dealing with
+    // a variation font (for which cairo metrics may be wrong, due to FreeType
+    // bug 52683).
+    if (!(face.get()->face_flags & FT_FACE_FLAG_SCALABLE) ||
+        !(face.get()->face_flags & FT_FACE_FLAG_MULTIPLE_MASTERS)) {
+        return false;
+    }
+
     bool hinting = gfxPlatform::GetPlatform()->FontHintingEnabled();
     int32_t flags =
         hinting ? FT_LOAD_ADVANCE_ONLY
                 : FT_LOAD_ADVANCE_ONLY | FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_HINTING;
     FT_Error ftError = FT_Load_Glyph(face.get(), aGID, flags);
-    MOZ_ASSERT(!ftError);
     if (ftError != FT_Err_Ok) {
         // FT_Face was somehow broken/invalid? Don't try to access glyph slot.
-        return 0;
+        // This probably shouldn't happen, but does: see bug 1440938.
+        NS_WARNING("failed to load glyph!");
+        return false;
     }
-    FT_Fixed advance = 0;
+
     // Due to freetype bug 52683 we MUST use the linearHoriAdvance field when
-    // dealing with a variation font; also use it for scalable fonts when not
-    // applying hinting. Otherwise, prefer hinted width from glyph->advance.x.
-    if ((face.get()->face_flags & FT_FACE_FLAG_SCALABLE) &&
-        (!hinting || (face.get()->face_flags & FT_FACE_FLAG_MULTIPLE_MASTERS))) {
-        advance = face.get()->glyph->linearHoriAdvance;
-    } else {
-        advance = face.get()->glyph->advance.x << 10; // convert 26.6 to 16.16
-    }
+    // dealing with a variation font. (And other fonts would have returned
+    // earlier, so only variation fonts currently reach here.)
+    FT_Fixed advance = face.get()->glyph->linearHoriAdvance;
 
     // If freetype emboldening is being used, and it's not a zero-width glyph,
     // adjust the advance to account for the increased width.
     if (mEmbolden && advance > 0) {
         // This is the embolden "strength" used by FT_GlyphSlot_Embolden,
         // converted from 26.6 to 16.16
         FT_Fixed strength = 1024 *
             FT_MulFix(face.get()->units_per_EM,
                       face.get()->size->metrics.y_scale) / 24;
         advance += strength;
     }
 
     // Round the 16.16 fixed-point value to whole pixels for better consistency
     // with how cairo renders the glyphs.
-    advance = (advance + 0x8000) & 0xffff0000u;
+    *aAdvance = (advance + 0x8000) & 0xffff0000u;
 
-    return advance;
+    return true;
 }
 
 int32_t
 gfxFT2FontBase::GetGlyphWidth(DrawTarget& aDrawTarget, uint16_t aGID)
 {
     if (!mGlyphWidths) {
         mGlyphWidths =
             mozilla::MakeUnique<nsDataHashtable<nsUint32HashKey,int32_t>>(128);
     }
 
     int32_t width;
     if (mGlyphWidths->Get(aGID, &width)) {
         return width;
     }
 
-    width = GetFTGlyphAdvance(aGID);
+    if (!GetFTGlyphAdvance(aGID, &width)) {
+        cairo_text_extents_t extents;
+        GetGlyphExtents(aGID, &extents);
+        width = NS_lround(0x10000 * extents.x_advance);
+    }
     mGlyphWidths->Put(aGID, width);
 
     return width;
 }
 
 bool
 gfxFT2FontBase::SetupCairoFont(DrawTarget* aDrawTarget)
 {
--- a/gfx/thebes/gfxFT2FontBase.h
+++ b/gfx/thebes/gfxFT2FontBase.h
@@ -40,17 +40,22 @@ public:
 
     static void SetupVarCoords(FT_Face aFace,
                                const nsTArray<gfxFontVariation>& aVariations,
                                nsTArray<FT_Fixed>* aCoords);
 
 private:
     uint32_t GetCharExtents(char aChar, cairo_text_extents_t* aExtents);
     uint32_t GetCharWidth(char aChar, gfxFloat* aWidth);
-    FT_Fixed GetFTGlyphAdvance(uint16_t aGID);
+
+    // Get advance of a single glyph from FreeType, and return true;
+    // or return false if we should fall back to getting the glyph
+    // extents from cairo instead.
+    bool GetFTGlyphAdvance(uint16_t aGID, int32_t* aWidth);
+
     void InitMetrics();
 
 protected:
     virtual const Metrics& GetHorizontalMetrics() override;
 
     uint32_t mSpaceGlyph;
     Metrics mMetrics;
     bool    mEmbolden;