Bug 192151 - [FreeType] Add initial implementation of variation fonts
Summary: [FreeType] Add initial implementation of variation fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
Depends on:
Blocks: 192589
  Show dependency treegraph
 
Reported: 2018-11-29 08:00 PST by Carlos Garcia Campos
Modified: 2019-01-20 14:48 PST (History)
8 users (show)

See Also:


Attachments
WIP (31.86 KB, patch)
2018-11-29 08:05 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (37.69 KB, patch)
2018-12-05 05:38 PST, Carlos Garcia Campos
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.47 MB, application/zip)
2018-12-05 06:47 PST, Build Bot
no flags Details
Patch (40.26 KB, patch)
2018-12-10 01:56 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (40.86 KB, patch)
2018-12-13 00:39 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-11-29 08:00:31 PST
It requires newer versions of cairo, fontconfig and freetype.
Comment 1 Carlos Garcia Campos 2018-11-29 08:05:06 PST
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 ]
Comment 2 Myles C. Maxfield 2018-11-29 08:21:49 PST
Woohoo! Excited to see this work!!
Comment 3 Myles C. Maxfield 2018-11-29 08:27:14 PST
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.
Comment 4 Michael Catanzaro 2018-11-29 11:55:11 PST
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*
Comment 5 Carlos Garcia Campos 2018-11-29 23:23:42 PST
(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.
Comment 6 Carlos Garcia Campos 2018-12-05 05:38:51 PST
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.
Comment 7 Build Bot 2018-12-05 05:41:54 PST
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.
Comment 8 Carlos Garcia Campos 2018-12-05 05:50:38 PST
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.
Comment 9 Build Bot 2018-12-05 06:47:52 PST
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
Comment 10 Build Bot 2018-12-05 06:47:54 PST
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
Comment 11 Michael Catanzaro 2018-12-05 08:54:53 PST
Myles, do you want to do the review?
Comment 12 Myles C. Maxfield 2018-12-06 10:41:46 PST
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.
Comment 13 Myles C. Maxfield 2018-12-06 10:42:22 PST
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.
Comment 14 Michael Catanzaro 2018-12-06 11:35:29 PST
Comment on attachment 356597 [details]
Patch

The FreeType parts are r=me.
Comment 15 Adrian Perez 2018-12-06 12:30:00 PST
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.
Comment 16 Carlos Garcia Campos 2018-12-10 01:19:50 PST
(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.
Comment 17 Carlos Garcia Campos 2018-12-10 01:56:22 PST
Created attachment 356952 [details]
Patch
Comment 18 Build Bot 2018-12-10 01:58:35 PST
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.
Comment 19 Michael Catanzaro 2018-12-11 15:00:25 PST
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. :)
Comment 20 Michael Catanzaro 2018-12-11 15:03:04 PST
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.
Comment 21 Carlos Garcia Campos 2018-12-12 00:34:59 PST
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.
Comment 22 Carlos Garcia Campos 2018-12-12 01:07:27 PST
Committed r239100: <https://trac.webkit.org/changeset/239100>
Comment 23 Radar WebKit Bug Importer 2018-12-12 01:08:34 PST
<rdar://problem/46655586>
Comment 25 Michael Catanzaro 2018-12-12 10:46:17 PST
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.
Comment 26 Chris Dumez 2018-12-12 10:49:32 PST
(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>
Comment 27 Michael Catanzaro 2018-12-12 10:51:47 PST
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.
Comment 28 Chris Dumez 2018-12-12 10:52:23 PST
(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.
Comment 29 Michael Catanzaro 2018-12-12 11:02:58 PST
Committed r239122: <https://trac.webkit.org/changeset/239122>
Comment 30 Michael Catanzaro 2018-12-12 11:04:30 PST
Reopening due to rollout in r239122.
Comment 31 Michael Catanzaro 2018-12-12 11:08:47 PST
(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.
Comment 32 Carlos Garcia Campos 2018-12-12 23:20:22 PST
I'm sorry I didn't pay attention to EWS :-(
Comment 33 Carlos Garcia Campos 2018-12-13 00:39:07 PST
Created attachment 357217 [details]
Patch for landing
Comment 34 Build Bot 2018-12-13 00:42:42 PST
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.
Comment 35 Carlos Garcia Campos 2018-12-13 01:17:39 PST
Committed r239156: <https://trac.webkit.org/changeset/239156>