1350783 - Support bitmap fonts in gfxFcPlatformFontList

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P1)

Description

[Lee Salzman [:lsalzman]]

Attachment: support bitmap fonts in gfxFcPlatformFontList

As part of bug 1285533 we removed the old pango/gfxFontconfigFonts code in favor of just always using gfxFcPlatformFontList. This exposed the problem that gfxFcPlatformFontList does not support bitmap fonts, whereas several users were enabling gfxFontconfigFonts via a pref to support that use-case.

During testing, this also turned up the surprise regression that due to the switchover to Skia content on Linux (bug 1278957, so 51+), that synthetic italic/oblique slants no longer actually work with bitmap fonts, even when the pref workaround is enabled.

So I've made a patch (lumping together to ensure ease of uplift) that attempts to address these issues in two ways:

1) Fix the issue with Skia not supporting italics with bitmap fonts. This was basically just due to the fact that we were not passing in our Cairo shape matrix to generateGlyphImage due to a misunderstanding/oversight on my part. So I modified the code to remember the SkMatrix version of the Cairo shape matrix, and pass it in to generateGlyphImage as is proper.

This also required a slight change with the glyph format to be kA8_Format to remember intermediate filtering if the glyph is transformed after rendering, instead of kBW_Format, which would have stored things as just a bitvector and horribly aliased the results. This is necessary to match the Cairo content output which also stores the transformed result in A8 format.

At least these changes are fairly innocuous, and I am certain are correct.

2) Keep non-scalable fonts around rather than filter them out, but leave the sorting preference for scalable fonts in place.

So this presents the challenge that we have filter out non-scalable fonts that are outside a 20% size distance from the requested size. I did this by overriding gfxFontconfigFontFamily::FindAllFontsForStyle to do the filtering there.

However, we're still left with many possible size variants of the bitmap font within the family we must choose from. Because sorting the entire list is unnecessary/expensive given the actual users of FindAllFontsForStyle, I opted to override gfxFontconfigFontFamily::FindFontForStyle to do the actual size selection there. This allows us to just quickly pick the closest size match rather than doing the slower full sort.

Having looked inside Fontconfig's code, this will match the results of the size sorting that FcFontSort was effectively giving us in the old gfxFontconfigFonts code.

...

A depressing incidental finding I noticed is that on an initial version of this patch, the spacing of the letters was off, as in the glyphs looked too small given their spacing relative to what Cairo content gave us, and the glyphs were also thinner. So it turns out that the shape matrix that Cairo calculates for bitmap fonts will actually scale the font to the requested size, which is what it uses to calculate the metrics that are determining the spacing as well.

Since in SkFontHost_cairo, we MUST make sure we match both the metrics and shape matrix transformations Cairo is doing, which we unfortunately can't change, I had to ensure this behavior is preserved as well.

So in the end, with this patch, we now look pretty much the same as what Cairo has always been giving us. but this was not actually what we would ideally have expected it to do - not scale the bitmap font that we laboriously selected to closely match the size. There's no easy way to change that behavior without modifying Cairo itself, and since our users have, pretty much since forever, been habituated to the way such bitmap fonts look, I opted to just match that behavior in SkFontHost_cairo such as it was. If someone actually finds that behavior undesirable, addressing that in a further bug down the road would probably be best.

...

For now though, I would like to just get this patch in ideally to 52 ESR to get rid of the necessity for our users to do pref hackery and also to fix the regression that snuck in with bitmap font italics in 51.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Screenshot showing rendering of font-family:terminus at various sizes, using the old fontconfig backend

The result I'm seeing with this patch doesn't fully match what I get with a current release Firefox and the old font-list code activated via prefs. Here's a sample showing the rendering I get with the older code. Note how the bitmaps are never scaled; instead the used size snaps to the nearest available bitmap size. (The smallest sizes here don't use the bitmap font at all because they're too small for the smallest available strike, so they fall back to a different face.)

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Comparison using the new fontlist code with the patch above

Here's what the patched new code gives me. The bitmaps are clearly being scaled to intermediate sizes (which ends up rather ugly, and AIUI is contrary to what CSS Fonts expects).

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Created attachment 8851752 [details]
> Comparison using the new fontlist code with the patch above
> 
> Here's what the patched new code gives me. The bitmaps are clearly being
> scaled to intermediate sizes (which ends up rather ugly, and AIUI is
> contrary to what CSS Fonts expects).

Actually, BOTH of your screenshots show scaling (the old one and the new one), just to different degrees - this I am rather certain of given how Cairo behaves and my investigation thereof. I would need to have the testcase you are using to investigate why the Skia case is getting slightly different scaling/offsetting though. Can you please attach your testcase for me to look at?

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: testcase using Terminus font

Here's the sample I used.

Comment 5

[Jonathan Kew [:jfkthame]]

(Note that the size(s) for which the bitmap font is unavailable and fallback to the browser's default kicks in will depend on the devicePixelRatio you're using. My screenshots were using devPixRatio=1 for simplicity; at that size, the 9px and 10px samples fall back, but if devPixRatio is 1.5, for example, then all the sizes will render using bitmap Terminus faces. At least, that's what I see on my Ubuntu system; results could depend on what specific sizes your distro packaged.)

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: support bitmaps fonts in gfxFcPlatformFontList

Okay, after investigating the testcase, I figured out that the old gfxFontconfigFonts code was forcing FC_PIXEL_SIZE on the font's pattern to the bitmap font size if applicable, which prevents Cairo's internal scaling code from doing any major damage. I just needed to modify my version to do similarly.

While analyzing the testcase, I also discovered that overriding FindFontForStyle to do the size selection doesn't work, since some users of FindAllFontsForStyle needed a sorted result.

I implemented a compaction scheme that will just remove all but the best available unscalable font given multiple options so that the list is effectively sorted and the testcase is happy again in terms of picking font sizes that match the old code.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8851862 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

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

This is looking good, I tried it briefly and it seemed to work exactly as expected. Thanks for digging in to this!

One thought before we go ahead and land this: it might be worth having a flag `mHasNonScalableFaces` in gfxFontconfigFontFamily, which can be set by gfxFcPlatformFontList::AddFontSetFamilies only for those (few) families that actually have such faces. Then the FindAllFontsForStyle override will be able to skip the extra work for the common case of scalable-font families. WDYT?

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Comment on attachment 8851862 [details] [diff] [review]
> support bitmaps fonts in gfxFcPlatformFontList
> 
> Review of attachment 8851862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking good, I tried it briefly and it seemed to work exactly as
> expected. Thanks for digging in to this!
> 
> One thought before we go ahead and land this: it might be worth having a
> flag `mHasNonScalableFaces` in gfxFontconfigFontFamily, which can be set by
> gfxFcPlatformFontList::AddFontSetFamilies only for those (few) families that
> actually have such faces. Then the FindAllFontsForStyle override will be
> able to skip the extra work for the common case of scalable-font families.
> WDYT?

I guess I could do this. It would require adding back in those checks for FC_SCALABLE inside AddFontSetFamilies, just setting the boolean instead of skipping as a result. I'll give it a try.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: support bitmaps fonts in gfxFcPlatformFontList

Once more, with feeling...

I now check in gfxFontconfigFontFamily::AddFontPattern if the added font pattern is not scalable, and if so mark the boolean as such. This then controls whether or not FindAllFontsForStyle does the compaction or not.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8852148 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

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

Awesome, let's go for it! :)

Comment 11

[Ian Moody [:Kwan] (UTC+1)]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d83d5d9e324
mark failing Stylo reftests. r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02a2c5c7004
support bitmaps fonts in gfxFcPlatformFontList. r=jfkthame

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/a02a2c5c7004
https://hg.mozilla.org/mozilla-central/rev/0d83d5d9e324

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8852148 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

Approval Request Comment
[Feature/Bug causing the regression]: Broken since 51 when we switched Linux to Skia content.
[User impact if declined]: Bitmap (non-scalable) fonts fail to render properly on Linux, especially slanted variants. Linux users currently are relying on older and now untested code that is currently only guarded by a pref to access this functionality, and we would like to obviate the need to use this older deprecated code and to avoid pref hackery. 
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not very risky
[Why is the change risky/not risky?]: Only affects Linux. Fixes regressions with scaled and slanted bitmap fonts. Allows users to select bitmap font families where available, while still preferring to use scalable fonts as normal/previously when they are also available. 
[String changes made/needed]: None

Comment 17

[[:mirabilos]]

I can confirm that the rendering of today’s Nightly (55.0) with the new fontlist code enabled is now correct, and even slightly better than with it disabled.

Thank you!

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8852148 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

Fix a regression that bitmap (non-scalable) fonts fail to render and was verified. Aurora54+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a069a170c51

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

We are heading into beta 10 tomorrow, RC build on Monday. I'm somewhat reluctant to take this uplift at the very last minute in beta 53 even though I know it would make people happy to fix this faster in release. Could we live with this for one more cycle and let it ride with 54?

Comment 21

[Lee Salzman [:lsalzman]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> We are heading into beta 10 tomorrow, RC build on Monday. I'm somewhat
> reluctant to take this uplift at the very last minute in beta 53 even though
> I know it would make people happy to fix this faster in release. Could we
> live with this for one more cycle and let it ride with 54?

I suppose, so long as we can still get this into 52 ESR. Otherwise, users then have to utilize a risky/untested combination of prefs to get the previous behavior that allowed bitmap fonts on Linux, which would likely cause us more headaches if 52 ESR gets out the door essentially broken.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8852148 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

For the happiness of Linux and ESR users everywhere then, let's do it...

Comment 23

[[:mirabilos]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)

> For the happiness of Linux and ESR users everywhere then, let's do it...

❦

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8664b8d321f7

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8852148 [details] [diff] [review]
support bitmaps fonts in gfxFcPlatformFontList

support bitmap fonts like it's 1990. esr52+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/26cd34db3c14

Comment 27

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Lee's assessment on manual testing needs (see Comment 16) and the fact that this fix has automated coverage.