Bug 54763 – [Freetype] Support for the -webkit-font-smoothing CSS property

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The -webkit-font-smoothing property is not supported. We should wire this through when constructing the cairo font.

[Carlos Alberto Lopez Perez]

A good test page for this is: http://files.christophzillgens.com/webkit-font-smoothing.html

[Carlos Alberto Lopez Perez]

Created attachment 385560 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 385560 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385560&action=review

> Source/WebCore/platform/graphics/FontPlatformData.h:277
> +    FontSmoothingMode m_smoothingMode { FontSmoothingMode::AutoSmoothing };

I guess we can make this freetype specific.

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:72
> +    auto platformFontData = font.platformData();

We could avoid this platform data copy here. Since what we really want is the scale font we could do something like:

RefPtr<cairo_scaled_font_t> scaledFont;
if (fontSmoothingMode == FontSmoothingMode::AutoSmoothing)
    scaledFont = font.platformData().scaledFont();
else {
    auto platformFontData = FontPlatformData::cloneWithSmothingMode(font.platformData(), fontSmoothingMode);
    scaledFont = platformFontData.scaledFont();
}

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:76
> +    if (fontSmoothingMode != FontSmoothingMode::AutoSmoothing)
> +        platformFontData = FontPlatformData::cloneWithSmothingMode(platformFontData, fontSmoothingMode);

I'm worried about this, drawGlyphs is called a lot, so this clone here is quite unfortunate. I don't know why fontSmooth is not part of the font description, but ideally we should create the font platform data with the font smooth mode. If that's not possible, then we could at least try to cache the font platform data with the smoothing mode to reuse it here somehow.

> Source/WebCore/platform/graphics/cairo/GraphicsContextImplCairo.cpp:260
> -    cairo_scaled_font_t* scaledFont = font.platformData().scaledFont();
> +    auto platformFontData = font.platformData();
>      double syntheticBoldOffset = font.syntheticBoldOffset();
>  
> +    if (fontSmoothingMode != FontSmoothingMode::AutoSmoothing)
> +        platformFontData = FontPlatformData::cloneWithSmothingMode(platformFontData, fontSmoothingMode);
> +
> +    cairo_scaled_font_t* scaledFont = platformFontData.scaledFont();

Ditto.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:83
> +    case FontSmoothingMode::Antialiased: {

You don't need the { } here.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:87
> +    case FontSmoothingMode::SubpixelAntialiased: {

Ditto.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:93
> +    case FontSmoothingMode::NoSmoothing: {

Ditto.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:97
> +    case FontSmoothingMode::AutoSmoothing: {

Ditto.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:98
> +

Empty line here.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:121
> +    default:
> +        ASSERT_NOT_REACHED();

Do not add a default, that way he compilar will warn about this if new values are added to the enum

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 385560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385560&action=review
> 
> > Source/WebCore/platform/graphics/FontPlatformData.h:277
> > +    FontSmoothingMode m_smoothingMode { FontSmoothingMode::AutoSmoothing };
> 
> I guess we can make this freetype specific.
> 

We can't. This patch adds two implementations of FontPlatformData::cloneWithSmothingMode()

One generic in FontPlatformData.cpp and one FreeType-specific in FontPlatformDataFreeType.cpp. Both implementations need to set this class variable.


> > Source/WebCore/platform/graphics/cairo/FontCairo.cpp:72
> > +    auto platformFontData = font.platformData();
> 
> We could avoid this platform data copy here. Since what we really want is
> the scale font we could do something like:
> 
> RefPtr<cairo_scaled_font_t> scaledFont;
> if (fontSmoothingMode == FontSmoothingMode::AutoSmoothing)
>     scaledFont = font.platformData().scaledFont();
> else {
>     auto platformFontData =
> FontPlatformData::cloneWithSmothingMode(font.platformData(),
> fontSmoothingMode);
>     scaledFont = platformFontData.scaledFont();
> }
> 

Good idea!

> > Source/WebCore/platform/graphics/cairo/FontCairo.cpp:76
> > +    if (fontSmoothingMode != FontSmoothingMode::AutoSmoothing)
> > +        platformFontData = FontPlatformData::cloneWithSmothingMode(platformFontData, fontSmoothingMode);
> 
> I'm worried about this, drawGlyphs is called a lot, so this clone here is
> quite unfortunate. I don't know why fontSmooth is not part of the font
> description, but ideally we should create the font platform data with the
> font smooth mode. If that's not possible, then we could at least try to
> cache the font platform data with the smoothing mode to reuse it here
> somehow.
> 

mmmm.. good point. I will try to do something about this.

[Carlos Alberto Lopez Perez]

Created attachment 387239 [details]
Patch

New approach: create the font platform data with the smoothing mode

[Simon Fraser (smfr)]

Comment on attachment 387239 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387239&action=review

> Source/WebCore/ChangeLog:17
> +        In order to take into account the smoothing value when the font
> +        platform data its created, this patchs moves m_fontSmoothing from
> +        FontCascadeDescription to its base class FontDescription.
> +        It also adds the smoothing value to the FontDescriptionKey so its
> +        taken into acount for the font cache.

Does Cairo require you to know the font smoothing value when a font is created? On macOS/iOS, it's something you set in the context at drawing time, so doesn't affect the font.

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:114
> +    switch (fontSmoothingMode) {
> +    case FontSmoothingMode::Antialiased:
> +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
> +        break;
> +    case FontSmoothingMode::SubpixelAntialiased:
> +        if (FcPatternGetInteger(pattern, FC_RGBA, 0, &integerResult) == FcResultMatch)
> +            cairo_font_options_set_subpixel_order(options, convertFontConfigSubpixelOrder(integerResult));
> +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_SUBPIXEL);
> +        break;
> +    case FontSmoothingMode::NoSmoothing:
> +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_NONE);
> +        break;
> +    case FontSmoothingMode::AutoSmoothing:
> +        if (FcPatternGetInteger(pattern, FC_RGBA, 0, &integerResult) == FcResultMatch) {
> +            cairo_font_options_set_subpixel_order(options, convertFontConfigSubpixelOrder(integerResult));
> +
> +            // Based on the logic in cairo-ft-font.c in the cairo source, a font with
> +            // a subpixel order implies that is uses subpixel antialiasing.
> +            if (integerResult != FC_RGBA_NONE)
> +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_SUBPIXEL);
> +        }
> +        if (FcPatternGetBool(pattern, FC_ANTIALIAS, 0, &booleanResult) == FcResultMatch) {
> +            // Only override the anti-aliasing setting if was previously turned off. Otherwise
> +            // we'll override the preference which decides between gray anti-aliasing and
> +            // subpixel anti-aliasing.
> +            if (!booleanResult)
> +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_NONE);
> +            else if (cairo_font_options_get_antialias(options) == CAIRO_ANTIALIAS_NONE)
> +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
> +        }
> +        break;
> +    default:
> +        ASSERT_NOT_REACHED();

This would be much more clearly written as a lambda that returns CAIRO_ANTIALIAS_NONE/CAIRO_ANTIALIAS_GRAY/CAIRO_ANTIALIAS_SUBPIXEL etc, and a single call to cairo_font_options_set_antialias().

[Carlos Alberto Lopez Perez]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 387239 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387239&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        In order to take into account the smoothing value when the font
> > +        platform data its created, this patchs moves m_fontSmoothing from
> > +        FontCascadeDescription to its base class FontDescription.
> > +        It also adds the smoothing value to the FontDescriptionKey so its
> > +        taken into acount for the font cache.
> 
> Does Cairo require you to know the font smoothing value when a font is
> created? On macOS/iOS, it's something you set in the context at drawing
> time, so doesn't affect the font.
> 

With Cairo you have to specify the antialising mode in the font options. Currently that its done when creating the font platform data.
However, it should be possible to change this font options before drawing without modifying the font platform data (more or less like Mac/iOS does)

However, I'm a bit worried this is less performant since you have to update the font options every time drawGlyps() is called, instead of having those options already set with the right value.


> > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:114
> > +    switch (fontSmoothingMode) {
> > +    case FontSmoothingMode::Antialiased:
> > +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
> > +        break;
> > +    case FontSmoothingMode::SubpixelAntialiased:
> > +        if (FcPatternGetInteger(pattern, FC_RGBA, 0, &integerResult) == FcResultMatch)
> > +            cairo_font_options_set_subpixel_order(options, convertFontConfigSubpixelOrder(integerResult));
> > +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_SUBPIXEL);
> > +        break;
> > +    case FontSmoothingMode::NoSmoothing:
> > +        cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_NONE);
> > +        break;
> > +    case FontSmoothingMode::AutoSmoothing:
> > +        if (FcPatternGetInteger(pattern, FC_RGBA, 0, &integerResult) == FcResultMatch) {
> > +            cairo_font_options_set_subpixel_order(options, convertFontConfigSubpixelOrder(integerResult));
> > +
> > +            // Based on the logic in cairo-ft-font.c in the cairo source, a font with
> > +            // a subpixel order implies that is uses subpixel antialiasing.
> > +            if (integerResult != FC_RGBA_NONE)
> > +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_SUBPIXEL);
> > +        }
> > +        if (FcPatternGetBool(pattern, FC_ANTIALIAS, 0, &booleanResult) == FcResultMatch) {
> > +            // Only override the anti-aliasing setting if was previously turned off. Otherwise
> > +            // we'll override the preference which decides between gray anti-aliasing and
> > +            // subpixel anti-aliasing.
> > +            if (!booleanResult)
> > +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_NONE);
> > +            else if (cairo_font_options_get_antialias(options) == CAIRO_ANTIALIAS_NONE)
> > +                cairo_font_options_set_antialias(options, CAIRO_ANTIALIAS_GRAY);
> > +        }
> > +        break;
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> This would be much more clearly written as a lambda that returns
> CAIRO_ANTIALIAS_NONE/CAIRO_ANTIALIAS_GRAY/CAIRO_ANTIALIAS_SUBPIXEL etc, and
> a single call to cairo_font_options_set_antialias().

Not sure, this function does more than simply calling cairo_font_options_set_antialias(). It also calls cairo_font_options_set_subpixel_order().

[Simon Fraser (smfr)]

(In reply to Carlos Alberto Lopez Perez from comment #7)
> With Cairo you have to specify the antialising mode in the font options.
> Currently that its done when creating the font platform data.
> However, it should be possible to change this font options before drawing
> without modifying the font platform data (more or less like Mac/iOS does)
> 
> However, I'm a bit worried this is less performant since you have to update
> the font options every time drawGlyps() is called, instead of having those
> options already set with the right value.

The value shouldn't change all that often though, so this solution sounds like a better match to the rest of the code.

[Carlos Alberto Lopez Perez]

> (In reply to Carlos Alberto Lopez Perez from comment #7)
> > With Cairo you have to specify the antialising mode in the font options.
> > Currently that its done when creating the font platform data.
> > However, it should be possible to change this font options before drawing
> > without modifying the font platform data (more or less like Mac/iOS does)
> > 
> > However, I'm a bit worried this is less performant since you have to update
> > the font options every time drawGlyps() is called, instead of having those
> > options already set with the right value.
> 
> The value shouldn't change all that often though, so this solution sounds
> like a better match to the rest of the code.

I have created some benchmarks, here: https://people.igalia.com/clopez/wkbug/font_smooth_bench

And I can't see any meaningful performance difference between one patch or the other.
I even get the same performance with the current webkitgtk (that ignores -webkit-font-smoothing).

For the record, this is what I get
      
current webkitgtk (no support for passing -webkit-font-smoothing property down to cairo)
100k words test:    2.5 runs/sec
10k words test:   15.47 runs/sec
1k words test:    80.54 runs/sec

previous patch (doing a clone of the font platform data with different antialias mode in FontCascade::drawGlyphs)
100k words test:   2.48 runs/sec
10k words test:   15.65 runs/sec
1k words test:    82.81 runs/sec

current patch (set antialiasing mode when creating the font platform data)
100k words test:    2.5 runs/sec
10k words test:   15.35 runs/sec
1k words test:    80.54 runs/sec

alternative patch (override antialiasing mode when drawing the glyph)
100k words test:   2.49 runs/sec
10k words test:   15.28 runs/sec
1k words test:    81.54 runs/sec


so... ¯\_(ツ)_/¯

I guess the bigger cost here its just drawing the text (which seems to happen any time any of its CSS properties its changed).
Doing the antialising in one way or the other doesn't seem to impact performance.
According to a perf trace <http://sprunge.us/yAcoUK>, it seems the bigger cost come from the complext text controller and text shapping (harfbuzz).
drawGlyphs() only seems to account for around 3% of the cost.

[Carlos Alberto Lopez Perez]

Created attachment 387546 [details]
Patch

alternative patch: override antialiasing mode when drawing the glyph

[Carlos Alberto Lopez Perez]

Created attachment 387648 [details]
Patch

try to fix wincairo build. diff with previous patch: http://sprunge.us/NgxJ0b?diff

[Carlos Alberto Lopez Perez]

Committed r254506: <https://trac.webkit.org/changeset/254506>

[Fujii Hironori]

This change causes a trouble for me.

  Bug 206573 – [WinCairo] -webkit-font-smoothing:antialiased makes fonts blurry