1004489 - reftest failures with skia content: font-features/font-variant-*

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: reftest log for font-variant-caps.html

These look like a real bug; reftest-analyzer suggests that it's an off-by-half-a-pixel bug, in the vertical direction.

Comment 1

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

I originally thought this was a problem related to hinting settings, like bug 915938, but after investigating it looks like our ft loadflags match in both Cairo and Skia, so it's not that.

Going to try and reduce the testcases down to narrow down further what's going on here.

Weirdly enough - this only reproduces on Ubuntu. I couldn't reproduce in Fedora and had to install a 12.04 chroot on my Fedora box to reproduce this.

Comment 2

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

Attachment: Testcase

Reduced testcase

Comment 3

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

Attachment: font-bug-rendering.png

Screenshot of the testcase showing the incorrect rendering.

Comment 4

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

I've concluded this bug isn't to do with Skia content. It appears to be an issue outside of the scope of Skia/Azure. Two reasons for thinking this:

a) I've gone through pretty much all the draw state objects I can think of when we call FillGlyphs() in DrawTargetSkia between the two cases, and there don't appear to be any discrepancies.
b) I can actually reproduce this using Cairo.

This may be a regression that we introduced with some of the other patches on the graphics branch. Looking further afield.

If it is reproducible on m-c (I'll confirm soon) then we should find out why it's passing at the moment and not when we enable Skia content.

Comment 5

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

Confirmed that this behaviour is the same on m-c.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: propagate Cairo font AA settings to Skia font

It seems this reproduces only on Ubuntu 12.04 is because they use the fontconfig "slight" hinting setting by default, and this hinting algorithm has these deficiencies.

Meanwhile, the more superficial bug here that is causing our test to fail on Skia but not on Cairo is that in cairo-ft, AA settings override hinting settings when AA is turned off. The Skia CairoFT typeface code actually has a check for this (checks for kBW format), but we never properly activated it because the AA being turned off for the font was never indicated to the SkPaint instance which makes the format decision.

This patch propagates the AA settings from the font (which comes all the way from fontconfig) through to the SkPaint, so that AA=none can properly disable hinting. With this, the hinting status finally matches the Cairo content backend and we pass these font tests. This is probably the least invasive way to accomplish this, as there is no way to pull the font options from the unscaled font out of a scaled font or to get the final resulting load flags without adding yet more of our own extensions to Cairo. So GlyphRenderingOptionsCairo had to be extended with the AA mode, unfortunately.

As well, there is an incidental finding with FT_Load_Glyph flags, that both Skia's FreeType font host and cairo-ft use the FT_LOAD_COLOR flag always when available, and we weren't in our font host, so I included a small fix for that as well.

Comment 7

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

Comment on attachment 8656016 [details] [diff] [review]
propagate Cairo font AA settings to Skia font

Review of attachment 8656016 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/skia/src/ports/SkFontHost_cairo.cpp
@@ +277,5 @@
>      loadFlags |= FT_LOAD_IGNORE_GLOBAL_ADVANCE_WIDTH;
>  
> +#ifdef FT_LOAD_COLOR
> +    loadFlags |= FT_LOAD_COLOR;
> +#endif

Why is this necessary?

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +936,5 @@
> +    case CAIRO_ANTIALIAS_DEFAULT:
> +    default:
> +      aaMode = mozilla::gfx::AntialiasMode::DEFAULT;
> +      break;
> +  }

Can we put all this in HelpersCairo.h?

@@ +942,1 @@
>    // We don't want to force the use of the autohinter over the font's built in hints

I'd like to see an expansion of this comment stating why we're plumbing the AA mode through.

::: gfx/thebes/gfxFontconfigFonts.cpp
@@ +2177,5 @@
> +    case CAIRO_ANTIALIAS_DEFAULT:
> +    default:
> +      aaMode = mozilla::gfx::AntialiasMode::DEFAULT;
> +      break;
> +  }

HelpersCairo

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: propagate Cairo font AA settings to Skia font

Commented on how the fontconfig AA mode may override the hinting style, and factored out various enum conversions into HelpersCairo.h and HelpersSkia.h, as requested by George.

Regarding FT_LOAD_COLOR, currently it is somewhat innocuous as we don't use font embedded bitmaps in DrawTargetSkia, but the actual Cairo font host allows for the possibility. So if we did, it prevents them from being grayscalized, as that is the behavior of both Skia's FreeType font host and also cairo-ft, and makes us match them in that case. Prevents future gotchas and makes load flags look more consistent. Just something I noticed while verifying that cairo-ft and our Cairo font host were loading glyphs the same way when trying to force embedded bitmaps on.

Comment 9

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=667b2394a3bd

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f784cd4caa2

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f784cd4caa2