Bug 192151 – [FreeType] Add initial implementation of variation fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Carlos Garcia Campos]

It requires newer versions of cairo, fontconfig and freetype.

[Carlos Garcia Campos]

Created attachment 356007 [details]
WIP

This is a WIP patch. Some features are still missing and many other details (enable it only for freetype based ports when required deps are available, some ifdefs in the code, duplicated code, etc.) I've run webkit tests and also manually tested it with https://play.typedetail.com/.

$ run-webkit-tests --gtk fast/text/variations/ animations/font-variations/ 

22 tests ran as expected, 5 didn't:

Regressions: Unexpected image-only failures (5)
  fast/text/variations/font-face-clamp.html [ ImageOnlyFailure ]
  fast/text/variations/font-face-format-woff2.html [ ImageOnlyFailure ]
  fast/text/variations/font-face-format.html [ ImageOnlyFailure ]
  fast/text/variations/font-selection-properties.html [ ImageOnlyFailure ]
  fast/text/variations/skia-postscript-name.html [ ImageOnlyFailure ]

[Myles C. Maxfield]

Woohoo! Excited to see this work!!

[Myles C. Maxfield]

From a cursory look, this patch seems pretty great! We should try to share as much code as possible among all the ports. I think there’s a substantial amount of logic in FontCacheCoreText that could probably be moved out into a platform-non-specific place and shared.

[Michael Catanzaro]

Comment on attachment 356007 [details]
WIP

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

My hero

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31
> +#include "FontDescription.h"
> +#include "FontCacheFreeType.h"

Alphabetize

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:472
> +    FT_MM_Var *ftMMVar;

FT_MM_Var*

[Carlos Garcia Campos]

(In reply to Myles C. Maxfield from comment #3)
> From a cursory look, this patch seems pretty great! We should try to share
> as much code as possible among all the ports. I think there’s a substantial
> amount of logic in FontCacheCoreText that could probably be moved out into a
> platform-non-specific place and shared.

Indeed, my idea is to refactor the code to share as much as possible once we have landed the first implementation.

[Carlos Garcia Campos]

Created attachment 356597 [details]
Patch

This is ready for review now. Only 3 tests are still failing. I decided to leave those for a follow up patch because I don't understand how that clamping works yet.

[EWS Watchlist]

Attachment 356597 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:148:  Declaration has space between type name and * in scaleFactor * fontDescription  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.h:22:  FT_Face is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

I've realized that we need to add a cairo patch to avoid crashes (see https://gitlab.freedesktop.org/cairo/cairo/merge_requests/5). I'll add that to our jhbuild before landing this.

[EWS Watchlist]

Comment on attachment 356597 [details]
Patch

Attachment 356597 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10277719

New failing tests:
http/tests/misc/resource-timing-resolution.html

[EWS Watchlist]

Created attachment 356600 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Myles, do you want to do the review?

[Myles C. Maxfield]

Comment on attachment 356597 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:46
> +#include FT_MULTIPLE_MASTERS_H

Is this a header file? I'm confused.

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:523
> +String buildVariationSettings(FT_Face face, const FontDescription& fontDescription)
> +{
> +    auto defaultValues = defaultVariationValues(face);
> +    const auto& variations = fontDescription.variationSettings();
> +
> +    VariationsMap variationsToBeApplied;
> +    auto applyVariation = [&](const FontTag& tag, float value) {
> +        auto iterator = defaultValues.find(tag);
> +        if (iterator == defaultValues.end())
> +            return;
> +        float valueToApply = clampTo(value, iterator->value.minimumValue, iterator->value.maximumValue);
> +        variationsToBeApplied.set(tag, valueToApply);
> +    };
> +
> +    for (auto& variation : variations)
> +        applyVariation(variation.tag(), variation.value());
> +
> +    StringBuilder builder;
> +    for (auto& variation : variationsToBeApplied) {
> +        if (!builder.isEmpty())
> +            builder.append(',');
> +        builder.append(variation.key[0]);
> +        builder.append(variation.key[1]);
> +        builder.append(variation.key[2]);
> +        builder.append(variation.key[3]);
> +        builder.append('=');
> +        builder.appendNumber(variation.value);
> +    }
> +    return builder.toString();

Can't we share this code? We could build up a HashMap and then the cocoa ports could re-encode it as a CFDictionary at the last minute.

[Myles C. Maxfield]

I can't review the Freetype parts of this since I don't know that API very well. I think we can probably share more code than this, though.

[Michael Catanzaro]

Comment on attachment 356597 [details]
Patch

The FreeType parts are r=me.

[Adrian Perez]

Comment on attachment 356597 [details]
Patch

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

Without being an expert myself when it comes to FreeType, this
is looking very good!

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:46
>> +#include FT_MULTIPLE_MASTERS_H
> 
> Is this a header file? I'm confused.

Yes, this is correct. FreeType is one of the few projects which makes
use of include expansions from macros. Look:

  % grep -r 'define FT_MULTIPLE' /usr/include/freetype2/
  /usr/include/freetype2/freetype/config/ftheader.h:#define FT_MULTIPLE_MASTERS_H  <freetype/ftmm.h>

(:

> Source/WebCore/platform/graphics/freetype/FontCustomPlatformDataFreeType.cpp:60
>  }

Given that the destructor is empty, this one here could be removed to
let the C++ compiler generate a default destructor automatically. If you
want to be explicit, you can write this in the struct/class definition:

  struct FontPlatformData {
     // ...
     ~FontPlatformData() = default;
  };

> Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:212
> +    if (m_pattern != other.m_pattern && !FcPatternEqual(m_pattern.get(), other.m_pattern.get()))

This can be a bit puzzling: The comment above says that FcPatternEqual() does NOT
accept NULL pointers, but now that the checks before calling it are removed, there
is no explanation telling why is is known that the m_pattern.get() calls would
always return a non-NULL pointer.

Maybe it would be a good thing to add a note in the comment telling about this,
or even a couple of ASSERTs as you have done in other parts of the code.

[Carlos Garcia Campos]

(In reply to Myles C. Maxfield from comment #12)
> Comment on attachment 356597 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356597&action=review
> 
> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:46
> > +#include FT_MULTIPLE_MASTERS_H
> 
> Is this a header file? I'm confused.

Yes, freetype is weird.

> > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:523
> > +String buildVariationSettings(FT_Face face, const FontDescription& fontDescription)
> > +{
> > +    auto defaultValues = defaultVariationValues(face);
> > +    const auto& variations = fontDescription.variationSettings();
> > +
> > +    VariationsMap variationsToBeApplied;
> > +    auto applyVariation = [&](const FontTag& tag, float value) {
> > +        auto iterator = defaultValues.find(tag);
> > +        if (iterator == defaultValues.end())
> > +            return;
> > +        float valueToApply = clampTo(value, iterator->value.minimumValue, iterator->value.maximumValue);
> > +        variationsToBeApplied.set(tag, valueToApply);
> > +    };
> > +
> > +    for (auto& variation : variations)
> > +        applyVariation(variation.tag(), variation.value());
> > +
> > +    StringBuilder builder;
> > +    for (auto& variation : variationsToBeApplied) {
> > +        if (!builder.isEmpty())
> > +            builder.append(',');
> > +        builder.append(variation.key[0]);
> > +        builder.append(variation.key[1]);
> > +        builder.append(variation.key[2]);
> > +        builder.append(variation.key[3]);
> > +        builder.append('=');
> > +        builder.appendNumber(variation.value);
> > +    }
> > +    return builder.toString();
> 
> Can't we share this code? We could build up a HashMap and then the cocoa
> ports could re-encode it as a CFDictionary at the last minute.

Yes, I plan to refactor it once I add the clamp and normalization part, but I still don't understand how that works.

[Carlos Garcia Campos]

Created attachment 356952 [details]
Patch

[EWS Watchlist]

Attachment 356952 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:148:  Declaration has space between type name and * in scaleFactor * fontDescription  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.h:22:  FT_Face is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 356952 [details]
Patch

If Myles is OK with the cross-platform bits, then let's proceed.

Of course, sharing as much code as possible would be ideal. As time goes on, we know Myles will fix bugs in the shared code, but not in ours. :)

[Michael Catanzaro]

Comment on attachment 356952 [details]
Patch

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

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31
> +#include "FontCacheFreeType.h"

Style checker is right, it should be included after Config.h.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:148
> +        scaleFactor * fontDescription.computedSize(),

The style checker failure here merits a Tools bug report.

[Carlos Garcia Campos]

Comment on attachment 356952 [details]
Patch

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

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31
>> +#include "FontCacheFreeType.h"
> 
> Style checker is right, it should be included after Config.h.

Not really, this is the FreeType implementation of FontCache.h, the specific header simply adds a global function.

[Carlos Garcia Campos]

Committed r239100: <https://trac.webkit.org/changeset/239100>

[Radar WebKit Bug Importer]

<rdar://problem/46655586>

[Chris Dumez]

Seems to have broken wincairo build:
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Build%29/builds/5442/steps/compile-webkit/logs/stdio

[Michael Catanzaro]

Oops, EWS was red and we missed it.

This also broke WPE (runtime crash, can't be caught by EWS) so I will do a rollout of this commit series and we can try again soon.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #25)
> Oops, EWS was red and we missed it.
> 
> This also broke WPE (runtime crash, can't be caught by EWS) so I will do a
> rollout of this commit series and we can try again soon.

Tried to fix win build in <https://trac.webkit.org/changeset/239116>

[Michael Catanzaro]

Thanks for trying to fix the WinCairo build, Chris, but I think we really need to roll this out anyway until Carlos Garcia has time to look at it.

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> (In reply to Michael Catanzaro from comment #25)
> > Oops, EWS was red and we missed it.
> > 
> > This also broke WPE (runtime crash, can't be caught by EWS) so I will do a
> > rollout of this commit series and we can try again soon.
> 
> Tried to fix win build in <https://trac.webkit.org/changeset/239116>

If you do roll out, please roll out r239116 with the rest to avoid breaking WinCairo again.

[Michael Catanzaro]

Committed r239122: <https://trac.webkit.org/changeset/239122>

[Michael Catanzaro]

Reopening due to rollout in r239122.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #28)
> If you do roll out, please roll out r239116 with the rest to avoid breaking
> WinCairo again.

Um, I notice my rollout commit message says I rolled out r239114. I actually rolled out r239116, not r239114. So I did the right thing, except the commit message is wrong.

[Carlos Garcia Campos]

I'm sorry I didn't pay attention to EWS :-(

[Carlos Garcia Campos]

Created attachment 357217 [details]
Patch for landing

[EWS Watchlist]

Attachment 357217 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:148:  Declaration has space between type name and * in scaleFactor * fontDescription  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.h:22:  FT_Face is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r239156: <https://trac.webkit.org/changeset/239156>