Skip to content

Commit

Permalink
Merge r180563 - [GTK] Fonts loaded via @font-face look bad
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=140994

Patch by Michael Catanzaro <mcatanzaro@igalia.com> on 2015-02-24
Reviewed by Martin Robinson.

We've had several complaints that woff fonts look bad on some websites. This seems to be a
combination of multiple issues. For one, we don't look at Fontconfig settings at all when
creating a web font. This commit changes FontPlatformData::initializeWithFontFace to instead
use sane default settings from Fontconfig when loading a web font, rather than accepting the
default settings from GTK+, which are normally overridden by Fontconfig when loading system
fonts. However, we will hardcode the hinting setting for web fonts to always force use of
the light autohinter, so that we do not use a font's native hints. This avoids compatibility
issues when fonts with poor native hinting are only tested in browsers that do not use the
native hints. It also allows us to sidestep future security vulnerabilities in FreeType's
bytecode interpreter.

The net result of this is that there should be little noticable difference if you have GTK+
set to use slight hinting (which forces use of the autohinter) unless you have customized
Fontconfig configuration, but a dramatic improvement with particular fonts if you use medium
or full hinting. This should reduce complaints about our font rendering on "fancy sites."

No new tests, since the affected fonts we've found are not freely redistributable.

* platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:
(WebCore::FontCustomPlatformData::FontCustomPlatformData): Force web fonts to be autohinted.
* platform/graphics/freetype/FontPlatformDataFreeType.cpp:
(WebCore::getDefaultCairoFontOptions): Renamed to disambiguate.
(WebCore::getDefaultFontconfigOptions): Added.
(WebCore::FontPlatformData::initializeWithFontFace): Always call
FontPlatformData::setCairoOptionsFromFontConfigPattern. If the FontPlatformData was not
created with an FcPattern (e.g. because this is a web font), call
getDefaultFontconfigOptions to get a sane default FcPattern.
(WebCore::FontPlatformData::setOrientation): Renamed call to getDefaultCairoFontOptions.
(WebCore::getDefaultFontOptions): Deleted.
  • Loading branch information
@mcatanzaro @carlosgcampos
mcatanzaro authored and carlosgcampos committed Feb 28, 2015
1 parent 0e5951b commit 95ba718
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 29 deletions.
51 changes: 51 additions & 0 deletions Source/WebCore/ChangeLog
View file
@@ -1,3 +1,54 @@
2015-02-26 Michael Catanzaro <mcatanzaro@igalia.com>

[FreeType] REGRESSION(r180563): Introduced crashes
https://bugs.webkit.org/show_bug.cgi?id=142044

Reviewed by Martin Robinson.

No new tests, should be caught by any woff font test.

Use optionsPattern, not m_pattern, when m_pattern may be null.

* platform/graphics/freetype/FontPlatformDataFreeType.cpp:
(WebCore::FontPlatformData::initializeWithFontFace):

2015-02-24 Michael Catanzaro <mcatanzaro@igalia.com>

[GTK] Fonts loaded via @font-face look bad
https://bugs.webkit.org/show_bug.cgi?id=140994

Reviewed by Martin Robinson.

We've had several complaints that woff fonts look bad on some websites. This seems to be a
combination of multiple issues. For one, we don't look at Fontconfig settings at all when
creating a web font. This commit changes FontPlatformData::initializeWithFontFace to instead
use sane default settings from Fontconfig when loading a web font, rather than accepting the
default settings from GTK+, which are normally overridden by Fontconfig when loading system
fonts. However, we will hardcode the hinting setting for web fonts to always force use of
the light autohinter, so that we do not use a font's native hints. This avoids compatibility
issues when fonts with poor native hinting are only tested in browsers that do not use the
native hints. It also allows us to sidestep future security vulnerabilities in FreeType's
bytecode interpreter.

The net result of this is that there should be little noticable difference if you have GTK+
set to use slight hinting (which forces use of the autohinter) unless you have customized
Fontconfig configuration, but a dramatic improvement with particular fonts if you use medium
or full hinting. This should reduce complaints about our font rendering on "fancy sites."

No new tests, since the affected fonts we've found are not freely redistributable.

* platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:
(WebCore::FontCustomPlatformData::FontCustomPlatformData): Force web fonts to be autohinted.
* platform/graphics/freetype/FontPlatformDataFreeType.cpp:
(WebCore::getDefaultCairoFontOptions): Renamed to disambiguate.
(WebCore::getDefaultFontconfigOptions): Added.
(WebCore::FontPlatformData::initializeWithFontFace): Always call
FontPlatformData::setCairoOptionsFromFontConfigPattern. If the FontPlatformData was not
created with an FcPattern (e.g. because this is a web font), call
getDefaultFontconfigOptions to get a sane default FcPattern.
(WebCore::FontPlatformData::setOrientation): Renamed call to getDefaultCairoFontOptions.
(WebCore::getDefaultFontOptions): Deleted.

2015-02-24 Dhi Aurrahman <diorahman@rockybars.com>

Always serialize :lang()'s arguments to strings
Expand Down
16 changes: 14 additions & 2 deletions Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp
View file
Expand Up @@ -35,9 +35,21 @@ static void releaseCustomFontData(void* data)
}

FontCustomPlatformData::FontCustomPlatformData(FT_Face freeTypeFace, SharedBuffer& buffer)
: m_fontFace(cairo_ft_font_face_create_for_ft_face(freeTypeFace, 0))
: m_fontFace(cairo_ft_font_face_create_for_ft_face(freeTypeFace, FT_LOAD_FORCE_AUTOHINT))
{
// FIXME Should we be setting some hinting options here?
// FT_LOAD_FORCE_AUTOHINT prohibits use of the font's native hinting. This
// is a safe option for custom fonts because (a) some such fonts may have
// broken hinting, which site admins may not notice if other browsers do not
// use the native hints, and (b) allowing native hints exposes the FreeType
// bytecode interpreter to potentially-malicious input. Treating web fonts
// differently than system fonts is non-ideal, but the result of autohinting
// is always decent, whereas native hints sometimes look terrible, and
// unlike system fonts where Fontconfig may change the hinting settings on a
// per-font basis, the same settings are used for all web fonts. Note that
// Chrome is considering switching from autohinting to native hinting in
// https://code.google.com/p/chromium/issues/detail?id=173207 but this is
// more risk than we want to assume for now. See
// https://bugs.webkit.org/show_bug.cgi?id=140994 before changing this.

buffer.ref(); // This is balanced by the buffer->deref() in releaseCustomFontData.
static cairo_user_data_key_t bufferKey;
Expand Down
69 changes: 42 additions & 27 deletions Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp
View file
Expand Up @@ -26,6 +26,7 @@
#include "FontPlatformData.h"

#include "FontDescription.h"
#include "RefPtrCairo.h"
#include <cairo-ft.h>
#include <cairo.h>
#include <fontconfig/fcfreetype.h>
Expand Down Expand Up @@ -103,7 +104,7 @@ void setCairoFontOptionsFromFontConfigPattern(cairo_font_options_t* options, FcP
cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);
}

static cairo_font_options_t* getDefaultFontOptions()
static cairo_font_options_t* getDefaultCairoFontOptions()
{
#if PLATFORM(GTK)
if (GdkScreen* screen = gdk_screen_get_default()) {
Expand All @@ -115,6 +116,24 @@ static cairo_font_options_t* getDefaultFontOptions()
return cairo_font_options_create();
}

static FcPattern* getDefaultFontconfigOptions()
{
// Get some generic default settings from fontconfig for web fonts. Strategy
// from Behdad Esfahbod in https://code.google.com/p/chromium/issues/detail?id=173207#c35
// For web fonts, the hint style is overridden in FontCustomPlatformData::FontCustomPlatformData
// so Fontconfig will not affect the hint style, but it may disable hinting completely.
static FcPattern* pattern = nullptr;
static std::once_flag flag;
std::call_once(flag, [](FcPattern*) {
pattern = FcPatternCreate();
FcConfigSubstitute(nullptr, pattern, FcMatchPattern);
FcDefaultSubstitute(pattern);
FcPatternDel(pattern, FC_FAMILY);
FcConfigSubstitute(nullptr, pattern, FcMatchFont);
}, pattern);
return pattern;
}

static void rotateCairoMatrixForVerticalOrientation(cairo_matrix_t* matrix)
{
// The resulting transformation matrix for vertical glyphs (V) is a
Expand Down Expand Up @@ -283,7 +302,9 @@ String FontPlatformData::description() const

void FontPlatformData::initializeWithFontFace(cairo_font_face_t* fontFace, const FontDescription& fontDescription)
{
cairo_font_options_t* options = getDefaultFontOptions();
cairo_font_options_t* options = getDefaultCairoFontOptions();
FcPattern* optionsPattern = m_pattern ? m_pattern.get() : getDefaultFontconfigOptions();
setCairoFontOptionsFromFontConfigPattern(options, optionsPattern);

cairo_matrix_t ctm;
cairo_matrix_init_identity(&ctm);
Expand All @@ -292,31 +313,25 @@ void FontPlatformData::initializeWithFontFace(cairo_font_face_t* fontFace, const
// Instead we scale we scale the font to a very tiny size and just abort rendering later on.
float realSize = m_size ? m_size : 1;

// 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.
cairo_matrix_t fontMatrix;
if (!m_pattern)
cairo_matrix_init_scale(&fontMatrix, realSize, realSize);
else {
setCairoFontOptionsFromFontConfigPattern(options, m_pattern.get());

// 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.
FcMatrix fontConfigMatrix, *tempFontConfigMatrix;
FcMatrixInit(&fontConfigMatrix);

// These matrices may be stacked in the pattern, so it's our job to get them all and multiply them.
for (int i = 0; FcPatternGetMatrix(m_pattern.get(), FC_MATRIX, i, &tempFontConfigMatrix) == FcResultMatch; i++)
FcMatrixMultiply(&fontConfigMatrix, &fontConfigMatrix, tempFontConfigMatrix);
cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx,
-fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);

// We requested an italic font, but Fontconfig gave us one that was neither oblique nor italic.
int actualFontSlant;
if (fontDescription.italic() && FcPatternGetInteger(m_pattern.get(), FC_SLANT, 0, &actualFontSlant) == FcResultMatch)
m_syntheticOblique = actualFontSlant == FC_SLANT_ROMAN;

// The matrix from FontConfig does not include the scale.
cairo_matrix_scale(&fontMatrix, realSize, realSize);
}
FcMatrix fontConfigMatrix, *tempFontConfigMatrix;
FcMatrixInit(&fontConfigMatrix);

// These matrices may be stacked in the pattern, so it's our job to get them all and multiply them.
for (int i = 0; FcPatternGetMatrix(optionsPattern, FC_MATRIX, i, &tempFontConfigMatrix) == FcResultMatch; i++)
FcMatrixMultiply(&fontConfigMatrix, &fontConfigMatrix, tempFontConfigMatrix);
cairo_matrix_init(&fontMatrix, fontConfigMatrix.xx, -fontConfigMatrix.yx,
-fontConfigMatrix.xy, fontConfigMatrix.yy, 0, 0);

// We requested an italic font, but Fontconfig gave us one that was neither oblique nor italic.
int actualFontSlant;
if (fontDescription.italic() && FcPatternGetInteger(optionsPattern, FC_SLANT, 0, &actualFontSlant) == FcResultMatch)
m_syntheticOblique = actualFontSlant == FC_SLANT_ROMAN;

// The matrix from FontConfig does not include the scale.
cairo_matrix_scale(&fontMatrix, realSize, realSize);

if (syntheticOblique()) {
static const float syntheticObliqueSkew = -tanf(14 * acosf(0) / 90);
Expand Down Expand Up @@ -388,7 +403,7 @@ void FontPlatformData::setOrientation(FontOrientation orientation)
cairo_matrix_t fontMatrix;
cairo_scaled_font_get_font_matrix(m_scaledFont, &fontMatrix);

cairo_font_options_t* options = getDefaultFontOptions();
cairo_font_options_t* options = getDefaultCairoFontOptions();

// In case of vertical orientation, rotate the transformation matrix.
// Otherwise restore the horizontal orientation matrix.
Expand Down

0 comments on commit 95ba718

Please sign in to comment.