736276 - Rename ScaledFontFreetype to ScaledFontCairo and use it to create an SkTypeface from a cairo_scaled_font_t

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[George Wright (:gw280) (needinfo me!)]

ScaledFontFreetype currently takes a gfxFont; we should instead be taking an FT_Face and propagating that through to Skia/Cairo/Whatever for the Freetype implementation of this class.

Comment 1

[George Wright (:gw280) (needinfo me!)]

The "native platform type" for Linux desktop and Android will be cairo_scaled_font_t instead of an FT_Face.

This is mainly because FT_Face is a really bad object to share between multiple backends, and this is something we'd like for e.g. runtime fallbacks, printing support, etc.

I'm going to create an SkFontHost that uses a cairo_scaled_font_t.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Attachment: SkFontHost for cairo_font_face_t

Here's the SkFontHost that wraps a cairo_font_face_t. It seems to work pretty well - fixes a bunch of broken canvas text reftests on try when using Skia/Canvas.

Comment 3

[George Wright (:gw280) (needinfo me!)]

Attachment: Change ScaledFontFreetype

Rename ScaledFontFreeType to ScaledFontCairo, and use Skia's API to create an SkTypeface from a cairo_scaled_font_t

Comment 4

[Karl Tomlinson (:karlt)]

I wonder why SkCreateTypefaceFromCairoFont takes style and isFixedWidth as
arguments instead of getting them from the cairo_font_face_t, particularly
given that it doesn't use them if the face is in the 
SkTypefaceCache?

Comment 5

[Karl Tomlinson (:karlt)]

Oh, you need a scaled font to get the FT_Face.  Hmmm.

Comment 6

[George Wright (:gw280) (needinfo me!)]

Yes; there's no easy way to get the FT_Face from a cairo_font_face_t. Unfortunately, we can't hack around that and have an SkTypeface wrap a scaled_font_t because if we apply a scale transformation to the SkCanvas, Skia will attempt to re-use an SkTypeface set with the wrong sized scaled_font_t object.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 664125 [details] [diff] [review]
SkFontHost for cairo_font_face_t

>    SkTypeface* typeface = SkTypefaceCache::FindByProcAndRef(FindByCairoFont, fontFace);
>    if (typeface) {
>        typeface->ref();

Ref'ing twice?

Does FindByProcAndRef need to iterate over each typeface in the cache to find,
or not find, the match?

Looks like cairo_font_face_set_user_data would be very useful.

What is the lifetime of the SkTypeface in the cache?


Getting the FT_Face from a cairo_scaled_font can be slow if the FT_Face is not
one of the few that cairo has kept open.  Opening an FT_Face requires
in-thread disk I/O.

The alternative is for SkTypeface to create a(n arbitrary size)
cairo_scaled_font from the cairo_font_face, when it needs to get the style and
spacing of the font.  There is potential cost in that because FreeType will
need to set a scale on the font, but I hope that doesn't require disk I/O.  If
the cairo_font_face is at all likely to be in the SkTypefaceCache, then I'd
suspect this would be better.  It also has the advantage of simplifying the
public API.

I wonder what SkTypeface uses these for.
It might be ideal if SkTypeface could ask for these when it needs them rather
than require them on construction, because I doubt it will need them; but I
don't know whether that is practical.

Perhaps this is not so much of an issue if the cairo_font_face was created
from an FT_Face because then cairo would never close it.  gfxFT2Fonts does
this; gfxPangoFonts does not.  I don't know how much it is costing us with
gfxFT2Fonts to keep all these faces open.  I wonder whether one day we'll want
a solution there to close faces not in use.

>    glyph->fAdvanceX = SkDoubleToFixed(SkScalarCeil(extents.x_advance));
>    glyph->fAdvanceY = SkDoubleToFixed(SkScalarCeil(extents.y_advance));

I suspect the SkScalarCeil should not be here, as fAdvanceX/Y are fixed point
rationals.

SkScalarCeil makes more sense for fWidth/Height/Left/Top, which are integers.

SkScalerContext_FreeType also returns/sets fRsbDelta and fLsbDelta.
Should they be set?

Haven't reviewed SkScalerContext_CairoFT, generateFontMetrics yet.

The metrics are coming from cairo, but the glyph images/paths are coming directly from FreeType.  This could be a recipe for metrics not matching the glyphs.

Comment 8

[George Wright (:gw280) (needinfo me!)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> Comment on attachment 664125 [details] [diff] [review]
> SkFontHost for cairo_font_face_t
> 
> >    SkTypeface* typeface = SkTypefaceCache::FindByProcAndRef(FindByCairoFont, fontFace);
> >    if (typeface) {
> >        typeface->ref();
> 
> Ref'ing twice?


My bad; I should remove that.

> Does FindByProcAndRef need to iterate over each typeface in the cache to
> find,
> or not find, the match?

I believe so.

> Looks like cairo_font_face_set_user_data would be very useful.

Yes; this is a very good idea!

> What is the lifetime of the SkTypeface in the cache?

Seems to be that there's a typeface cache size of 128 typefaces; if we go over that, it purges the 32 LRU typefaces with a refcount of 1 from the cache.

> Getting the FT_Face from a cairo_scaled_font can be slow if the FT_Face is
> not
> one of the few that cairo has kept open.  Opening an FT_Face requires
> in-thread disk I/O.
> 
> The alternative is for SkTypeface to create a(n arbitrary size)
> cairo_scaled_font from the cairo_font_face, when it needs to get the style
> and
> spacing of the font.  There is potential cost in that because FreeType will
> need to set a scale on the font, but I hope that doesn't require disk I/O. 
> If
> the cairo_font_face is at all likely to be in the SkTypefaceCache, then I'd
> suspect this would be better.  It also has the advantage of simplifying the
> public API.
>
> I wonder what SkTypeface uses these for.
> It might be ideal if SkTypeface could ask for these when it needs them rather
> than require them on construction, because I doubt it will need them; but I
> don't know whether that is practical.

This sounds like something that needs to be discussed with upstream; I'd rather avoid invasive refactoring in this patch and file a bug with upstream about that instead.

> Perhaps this is not so much of an issue if the cairo_font_face was created
> from an FT_Face because then cairo would never close it.  gfxFT2Fonts does
> this; gfxPangoFonts does not.  I don't know how much it is costing us with
> gfxFT2Fonts to keep all these faces open.  I wonder whether one day we'll
> want
> a solution there to close faces not in use.
> 
> >    glyph->fAdvanceX = SkDoubleToFixed(SkScalarCeil(extents.x_advance));
> >    glyph->fAdvanceY = SkDoubleToFixed(SkScalarCeil(extents.y_advance));
> 
> I suspect the SkScalarCeil should not be here, as fAdvanceX/Y are fixed point
> rationals.
> 
> SkScalarCeil makes more sense for fWidth/Height/Left/Top, which are integers.
> 
> SkScalerContext_FreeType also returns/sets fRsbDelta and fLsbDelta.
> Should they be set?

I don't think so, but I will double check.

> Haven't reviewed SkScalerContext_CairoFT, generateFontMetrics yet.
> 
> The metrics are coming from cairo, but the glyph images/paths are coming
> directly from FreeType.  This could be a recipe for metrics not matching the
> glyphs.

I would like to avoid pulling the metrics in from FreeType directly because we're pulling in the metrics in layout, and cairo should already have these cached for us. So far, this code passes all our reftests so I think they are matching fairly consistently.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to George Wright (:gw280) from comment #8)
> > I wonder what SkTypeface uses these for.
> > It might be ideal if SkTypeface could ask for these when it needs them rather
> > than require them on construction, because I doubt it will need them; but I
> > don't know whether that is practical.
> 
> This sounds like something that needs to be discussed with upstream; I'd
> rather avoid invasive refactoring in this patch and file a bug with upstream
> about that instead.

Yes.  I expect that could be quite a change.  The comment was more planning ahead: if the API doesn't require the font data now, then it will be easier to make internal changes later if optimization is necessary.

Given the 128 face cache, I suspect we'll usually be finding the face in the cache.

> > Haven't reviewed SkScalerContext_CairoFT, generateFontMetrics yet.
> > 
> > The metrics are coming from cairo, but the glyph images/paths are coming
> > directly from FreeType.  This could be a recipe for metrics not matching the
> > glyphs.
> 
> I would like to avoid pulling the metrics in from FreeType directly because
> we're pulling in the metrics in layout, and cairo should already have these
> cached for us.

Makes sense.  I'll look over the relevant code to see what potential issues there may be.

Comment 10

[George Wright (:gw280) (needinfo me!)]

Attachment: updated skfonthost

Updated patch incorporating the first batch of Karl's review comments.

I spoke with Mike and fLsbDelta and fRsbDelta can be safely ignored here. Cairo seems to ignore them anyway.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 667066 [details] [diff] [review]
updated skfonthost

Some quick comments, to let you know I haven't forgotten about this.

FindByCairoFont is unused now.

SkCairoFTTypeface needs to hold a reference to fFontFace AFAIK.

>+void SkScalerContext_CairoFT::generateAdvance(SkGlyph* glyph)
>+{
>+    cairo_text_extents_t extents;
>+    cairo_glyph_t cairoGlyph = { glyph->getGlyphID(fBaseGlyphCount), 0.0, 0.0 };
>+    cairo_scaled_font_glyph_extents(fScaledFont, &cairoGlyph, 1, &extents);
>+    glyph->fAdvanceX = SkDoubleToFixed(SkScalarCeil(extents.x_advance));
>+    glyph->fAdvanceY = SkDoubleToFixed(SkScalarCeil(extents.y_advance));

Remove SkScalarCeil here too, or better, just call generateMetrics.

>+    cairo_matrix_init(&fontMatrix, matrix.getScaleX(), matrix.getSkewX(), matrix.getSkewY(), matrix.getScaleY(), 0.0, 0.0);

This looks like the transpose of what SkScalerContext_FreeType uses.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 667066 [details] [diff] [review]
updated skfonthost

>+    bool linearMetrics = SkToBool(fRec.fFlags & SkScalerContext::kSubpixelPositioning_Flag);

unused.

To ensure glyph rendering is consistent with the metrics, the load flags
passed to FT_Load_Glyph need to be consistent.

gfxFT2Fonts uses FT_LOAD_DEFAULT or (FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_HINTING)
depending on the platform.  FT_LOAD_NO_AUTOHINT should have no effect when
FT_LOAD_NO_HINTING is specified, but somehow Skia needs to be told whether
FT_LOAD_DEFAULT or FT_LOAD_NO_HINTING should be used.
Is that happening?

How are fRec.fMaskFormat and fRec.getHinting() controlled from outside Skia?
I wonder whether a load_flags parameter to SkCreateTypefaceFromCairoFont would be appropriate?

>+        case SkPaint::kNormal_Hinting:
>+            if (fRec.fFlags & SkScalerContext::kAutohinting_Flag)
>+                loadFlags = FT_LOAD_FORCE_AUTOHINT;
>+            else
>+                loadFlags = FT_LOAD_NO_AUTOHINT;

cairo only uses FT_LOAD_NO_AUTOHINT if passed to cairo_ft_font_face_create_for_ft_face, so this may need to be part of the parameters passed to SkCreateTypefaceFromCairoFont.

>+        case SkPaint::kFull_Hinting:
>+            if (fRec.fFlags & SkScalerContext::kAutohinting_Flag) {
>+                loadFlags = FT_LOAD_FORCE_AUTOHINT;
>+                break;
>+            }
>+            loadFlags = FT_LOAD_TARGET_NORMAL;
>+            if (isLCD(fRec)) {
>+                if (SkToBool(fRec.fFlags & SkScalerContext::kLCD_Vertical_Flag)) {
>+                    loadFlags = FT_LOAD_TARGET_LCD_V;
>+                } else {
>+                    loadFlags = FT_LOAD_TARGET_LCD;
>+                }
>+            }

When cairo uses autohinting, it still considers subpixel_order in determining
the load_target.  So remove the early "break", and "loadFlags =
FT_LOAD_TARGET_NORMAL;".  FT_LOAD_TARGET_NORMAL is just 0 == FT_LOAD_DEFAULT.

There are some more parameters affecting load flags when cairo fonts are used
with fontconfig.  Let's consider these beyond the scope of this bug, but bear
in mind that we may wish to have API to handle this in the future:

  FC_AUTOHINT -> FT_LOAD_FORCE_AUTOHINT 
  FC_EMBEDDED_BITMAP/FC_HINTING/FC_ANTIALIAS -> FT_LOAD_NO_BITMAP
  FC_EMBOLDEN -> FT_GlyphSlotEmbolden()

>+SK_API extern SkTypeface* SkCreateTypefaceFromCairoFont(cairo_font_face_t* fontFace, SkTypeface::Style style, bool isFixedWidth);

I would have simplified the interface here and let Skia find style and
isFixedWidth if it needs them.  It is probably acceptable passing these for gfxFT2Fonts but it will need to change if this is used with backends that don't keep all the FT_Faces open.

Comment 13

[George Wright (:gw280) (needinfo me!)]

Attachment: ScaledFontCairo attempt 2

New attempt at changing ScaledFontFreeType to allow for Cairo fonts to be used by backends such as Skia. This time we should address the hinting concerns that Karl had.

Comment 14

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated SkFontHost_cairo (attempt 2)

SkFontHost update that applies to rebased Skia (although in all likelihood I'll have to change it to apply back against the current in-tree Skia, as we probably don't want the risk of the rebase quite yet). It currently ends up waiting forever for a mutex and hanging the browser, but the main substance of the patch can probably be reviewed already.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 744296 [details] [diff] [review]
ScaledFontCairo attempt 2

Using GlyphRenderingOptions sounds sensible to me,

>+  if (aRenderingOptions) {
>+    switch (static_cast<const GlyphRenderingOptionsCairo*>(aRenderingOptions)->GetHinting()) {

but I'm not sure it is safe to assume this is a GlyphRenderingOptionsCairo here.

Comment 16

[Karl Tomlinson (:karlt)]

In reply to:
<gw280> so looking through the cairo source, there is no way FT_LOAD_NO_AUTOHINT can end up in the load flags, it seems
<gw280> so I figure we can probably remove the FT_LOAD_NO_AUTOHINT loadflag from the kNormal_Hinting case in the switch statement

I think you are probably right that it would make sense to drop the
FT_LOAD_NO_AUTOHINT.  For one thing, it would be more consistent with the other hinting styles.

FreeType has three states: FT_LOAD_NO_AUTOHINT, FT_LOAD_FORCE_AUTOHINT, and
neither, which means use the autohinter if the native hinter is not available.
The SkScalerContext has just two states, so we should pick the two that most
closely correspond to cairo.  Cairo would only use FT_LOAD_NO_AUTOHINT if
passed to cairo_ft_font_face_create_for_ft_face(), and we can choose not to
support that.  gfxFT2Fonts may set (FT_LOAD_NO_AUTOHINT | FT_LOAD_NO_HINTING),
but the FT_LOAD_NO_AUTOHINT is not doing anything there as hinting is disabled
anyway (as you said on irc).

Comment 17

[George Wright (:gw280) (needinfo me!)]

Attachment: Updated SkFontHost_cairo (attempt 3)

SkFontHost_cairo diffed against mozilla-central (not my local skia rebase).

I've incorporated (I hope!) all changes Karl has suggested with the exception of removing FT_LOAD_NO_AUTOHINT. Upon further thinking, I don't think it does any harm to be in there, and it'll mean parity in terms of behaviour when compared to SkFontHost_FreeType.

I've run a full try build with Skia enabled for Azure/Canvas on Linux, and the results are extremely promising. There are a few (very few!) reftest failures, but none of them are font related. It looks like this FontHost now fixes every single one of our Skia font reftest failures. Here is the try run: https://tbpl.mozilla.org/?tree=Try&rev=b970c0be80ff

The mutex infinite wait was caused by a mismatch in the way we were building Skia. We were specifying through a compiler define that we were using pthreads, and so SkThread_platform.h was expecting SkThread_pthreads.cpp to be built, but we weren't. This has been rectified in this iteration of the patch, in Makefile.in.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 745473 [details] [diff] [review]
Updated SkFontHost_cairo (attempt 3)

>@@ -316,25 +317,34 @@ CPPSRCS += \
> 
> OS_CXXFLAGS += $(CAIRO_FT_CFLAGS)
> DEFINES += -DSK_USE_POSIX_THREADS=1
>+EXPORTS_skia += \
>+	include/ports/SkTypeface_cairo.h \
>+	$(NULL)
>+
>+OS_CXXFLAGS += $(MOZ_CAIRO_CFLAGS) $(CAIRO_FT_CFLAGS)
>+OS_CFLAGS += $(MOZ_CAIRO_CFLAGS) $(CAIRO_FT_CFLAGS)
> else
> CPPSRCS += \
> 	SkDebug_stdio.cpp \
>-	SkThread_none.cpp \
> 	$(NULL)
> endif

CAIRO_FT_CFLAGS is added twice.

I think OS_CFLAGS changes can and so should be dropped here.

>+        case SkPaint::kFull_Hinting:
>+            cairo_font_options_set_hint_style(fontOptions, CAIRO_HINT_STYLE_FULL);
>+            if (isLCD(fRec)) {

This version has dropped the FT_LOAD_FORCE_AUTOHINT when kAutohinting_Flag is
set.  That was consistent with cairo.  It was just the |break| that wasn't
consistent because it meant that the isLCD() check was skipped.

(In reply to George Wright (:gw280) from comment #17)
> I've incorporated (I hope!) all changes Karl has suggested with the
> exception of removing FT_LOAD_NO_AUTOHINT. Upon further thinking, I don't
> think it does any harm to be in there, and it'll mean parity in terms of
> behaviour when compared to SkFontHost_FreeType.

I don't expect this to make any difference with existing usage of gfxFT2Fonts.
If/when this gets used for desktop/fontconfig cairo fonts, the main difference
will be on (perhaps older) systems where the bytecode interpreter is disabled.
There the NO_AUTOHINT used when rendering will differ from the autohinting used
in the cairo_scaled_font for metrics.

Please fix up the Makefile variables.  The other things I've commented on are
not going to affect gfxFT2Fonts usage.  They should be addressed before using
this for fontconfig fonts, but I'll let you decide whether to do that now or
later.

Comment 19

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/mozilla-central/rev/84bdd96375ba
https://hg.mozilla.org/mozilla-central/rev/c6904473984e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest crashes. Thanks for watching your push after landing on m-c, btw.
https://hg.mozilla.org/mozilla-central/rev/b25afb305360

https://tbpl.mozilla.org/php/getParsedLog.php?id=22711192&tree=Mozilla-Central

Comment 21

[Ryan VanderMeulen [:RyanVM]]

And a mochitest failure.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22711902&tree=Mozilla-Central

And a reftest unexpected pass.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22711772&tree=Mozilla-Central

Comment 22

[Karl Tomlinson (:karlt)]

SkScalerContext_CairoFT::fScaledFont is not released.

I'd like to know why Ubuntu reftests are using SkScalerContext_CairoFT.

Comment 23

[George Wright (:gw280) (needinfo me!)]

Yes, this one's got me puzzled as well, especially as we've passed try many times.

Comment 24

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/120285554c44
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bd02a8da04f

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out for reftest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9dcad2319f

Comment 26

[George Wright (:gw280) (needinfo me!)]

Investigating whether this is a clobber issue, as this crash doesn't make any sense from where I'm sitting. The last try run was https://tbpl.mozilla.org/?tree=Try&rev=660255a73983.

Got https://tbpl.mozilla.org/?tree=Try&rev=e7fdeeef7fd3 running through try at the moment, and will respin R a few times to see if it's a clobber issue or an intermittent crash.

Comment 27

[Karl Tomlinson (:karlt)]

Might be worth separating the SkThread changes to exclude them from the investigation.

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Bug 874768 comment 2 shows our remaining Valgrind errors on linux 64bit and that seems to point to FontHost and other font-related things.

Also, I plan to work on this bug this week. I hope.

Comment 29

[George Wright (:gw280) (needinfo me!)]

Attachment: Only create the SkTypeface when needed

Comment 30

[George Wright (:gw280) (needinfo me!)]

Attachment: Avoid the possibility of double addref in scaledfontbase

Comment 31

[George Wright (:gw280) (needinfo me!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f43d4e45db
https://hg.mozilla.org/integration/mozilla-inbound/rev/e373f689e646
https://hg.mozilla.org/integration/mozilla-inbound/rev/894b1aa4abd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a995eb241d7d

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0f43d4e45db
https://hg.mozilla.org/mozilla-central/rev/e373f689e646
https://hg.mozilla.org/mozilla-central/rev/894b1aa4abd8
https://hg.mozilla.org/mozilla-central/rev/a995eb241d7d