974575 - add color-bitmap glyph support to the freetype font backend in cairo

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

In order to support the Noto Color Emoji font that is replacing Android Emoji (monochrome) in Android 4.4.2, we need to add support for color fonts to the cairo freetype backend.

Fortunately, Behdad has already done a bunch of this work on cairo trunk, and it looks like we can trivially backport the patches from there, so this doesn't require taking a full cairo update.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: backport Behdad's patches for color bitmap glyph support from current cairo trunk.* * *

[ft] Fix resizing of bitmap fonts
From b94a519aad3d5b50aa6de47ee16ee6a099de9791 Mon Sep 17 00:00:00 2001
Say, you were asking cairo for a font at 200px.  For bitmap-only fonts,
cairo was finding the closes strike size and using it.  If the strike
was at 20px, well, that's what you were getting.  We now scale that 20px
strike by a factor of 10 to get the correct size rendering.

Note that by itself this patch doesn't change much on the Linux desktop.
The reason is that the size you are interested in (eg. 200px) is lost by
fontconfig.  When you request a font at 200px, fontconfig returns a font
pattern that says 20px, and so the next layers thing you want a font at
20px.  To address that, one also needs a piece of fontconfig config that
puts the 200 back into the pixelsize.  Something like this:

<match target="font">
  <test name="scalable" mode="eq">
    <bool>false</bool>
  </test>
  <edit name="pixelsize" mode="assign">
    <times>
      <name>size</name>
      <name>dpi</name>
      <double>0.0138888888888</double> <!--1/72.-->
    </times>
  </edit>
</match>

I'm going to try to upstream this config so it will be enabled by
default.  The config can be a bit smarter.  For example, if
metricshinting is enabled and the size difference is small, we may as
well not scale.

The nice thing about this is that the configuration of whether and when
to scale bitmaps will be done in fontconfig, not cairo / Qt / ... code.
---
* * *
[FT] Prefer downscaling bitmap glyphs to upscaling

From a8f1b456db744e33a10b2301df03528787e5b1ca Mon Sep 17 00:00:00 2001
Say, you have bitmap strikes for sizes 50ppem and 100ppem.
To render at 60ppem, it's much better to downscale the 100ppem
bitmap than upscale 50ppem one.  Prefer downscaling.
---
* * *
[ft] I meant fabs(), not abs()

From 13bd8d09b44e50649f6fc4d58d036bc32c1d5c5b Mon Sep 17 00:00:00 2001
---
* * *
[ft] Fix memory bug in copying bitmaps

From a0f556f37fb7016aa304b7cf0e811c0d38f0b969 Mon Sep 17 00:00:00 2001
---
* * *
[ft] Fix wrong assumptions

From e738079302a968b7b1fb9101cd4d92a8887bedce Mon Sep 17 00:00:00 2001
If subpixel rendering is enabled, but FT returns a 8bit gray bitmap
(perhaps because the font has 8bit embedded bitmaps) we were hitting
the assertions because the assumptions made were wrong.  Fix up.
---
* * *
Towards support loading color glyphs from FreeType

From 2cc353c3dbe01b4d8f65d6de800f2b1d6004a1c2 Mon Sep 17 00:00:00 2001
See comments.
---
* * *
Support 2bit and 4bit embedded bitmaps

From 9444ef09ccde2735258cc1bd2f1912119a32dd88 Mon Sep 17 00:00:00 2001
---
* * *
[ft] Fix math

From 7d26341072b13a78d4b3fe58779057ac020be487 Mon Sep 17 00:00:00 2001
---
* * *
[ft] Add missing include

From 0554d76402321b25cc952180e4d19436a9038d1a Mon Sep 17 00:00:00 2001
---
* * *
[ft] Fix alignment

From 34a747e7bdeba1cfe17318f80fbe6720d47bc023 Mon Sep 17 00:00:00 2001
---
* * *
[ft] Ensure alignment of bitmaps received from FreeType

From 46d9db96d460fea72f0420102e8a90c6a7231f79 Mon Sep 17 00:00:00 2001
---

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: support color glyphs in cairo_image_surface's _composite_glyphs function.

Eventually, I expect cairo will want color glyph support in various backends/compositors, but for now this appears to be the crucial one for our use. With this, the color emoji glyphs show up nicely on sites like http://emojipedia.org/ and http://en.wikipedia.org/wiki/Emoji.

Comment 3

[Jonathan Kew [:jfkthame]]

BTW, there's a tryserver build with these patches (and bug 969814) at https://tbpl.mozilla.org/?tree=Try&rev=7cf06196b79d. This build should display color emoji properly on Android 4.4.2 devices, assuming they have the Noto font.

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0d1c4456b73
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd7b1da5d36

Comment 5

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/dd9cf2bef693 for ASan bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=35580012&tree=Mozilla-Inbound&full=1#error0

Comment 6

[Jonathan Kew [:jfkthame]]

The ASan bustage here was triggered by one of Behdad's changes upstream that I believe is incorrect; see http://cgit.freedesktop.org/cairo/commit/src/cairo-ft-font.c?id=a0f556f37fb7016aa304b7cf0e811c0d38f0b969.

This is wrong because in the case where stride > bitmap->pitch, it will memcpy bytes from the following row into the "padding" at the end of each destination row - which is mostly harmless, but on the last row of the bitmap, it means we read from beyond the bitmap buffer's allocated space.

cc'ing Behdad, to look into fixing this upstream; I'll attach a proposed patch here so that we can move forward with landing this bug.

Comment 7

[Behdad Esfahbod]

Huh...  I'll check the memory bug out.

Jonathan, should we upstream your image compositor change?  I'm still undecided how to go about implementing color-font support in all compositors.

Comment 8

[Behdad Esfahbod]

The problem, of course, is that I don't remember, and have not documented, what memory problem I was trying to fix.  And my fix is clearly wrong. :(  Will keep thinking about it.

Comment 9

[Behdad Esfahbod]

commit f88bd92e8b3d87ec728e3fee51eb82f07db8c95c
Author: Behdad Esfahbod <[e-mail]>
Date:   Wed Mar 5 01:13:59 2014 -0800

    Revert "[ft] Fix memory bug in copying bitmaps"
    
    This reverts commit a0f556f37fb7016aa304b7cf0e811c0d38f0b969.
    
    The change was clearly wrong now that I read.  I was probably
    tricked by what was fixed in the follow-up commit
    e738079302a968b7b1fb9101cd4d92a8887bedce.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: correction for bad commit (a0f556f37fb7016aa304b7cf0e811c0d38f0b969) ported from upstream.

AFAICS, this fixes the bitmap-buffer read overrun here. It's not clear to me that we actually need to zero-fill any extra stride length in the destination, as presumably that's merely padding that will never actually be used; but the original code did this, so I've left it in place. Try run (including ASan): https://tbpl.mozilla.org/?tree=Try&rev=d7c46cb7417e.

Comment 11

[Behdad Esfahbod]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Created attachment 8385967 [details] [diff] [review]
> correction for bad commit (a0f556f37fb7016aa304b7cf0e811c0d38f0b969) ported
> from upstream.
> 
> AFAICS, this fixes the bitmap-buffer read overrun here. It's not clear to me
> that we actually need to zero-fill any extra stride length in the
> destination, as presumably that's merely padding that will never actually be
> used; but the original code did this, so I've left it in place. Try run
> (including ASan): https://tbpl.mozilla.org/?tree=Try&rev=d7c46cb7417e.

Your fix is definitely not wrong.  But I'm guessing that the original code, before my broken commit, was probably just fine and that I was tricked by sideeffects of some other bug.  I'll think more about it, but currently that's my best guess at why I made that commit.

Comment 12

[Jonathan Kew [:jfkthame]]

My guess was that you might have run into a case where bitmap->pitch is greater than the destination's stride. In that case, the expression used as the third argument to memset would be negative - except that memset takes a size_t, so it would actually be a huge value, and much of memory would get wiped out. (Ouch.) My suggested fix aimed to handle bitmap->pitch vs stride discrepancies in either direction safely.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Behdad Esfahbod from comment #7)
> Huh...  I'll check the memory bug out.
> 
> Jonathan, should we upstream your image compositor change?  I'm still
> undecided how to go about implementing color-font support in all compositors.

If you want it upstream, that's fine with me. I didn't pursue that for now as I assume you'd want to think more about how to handle this across all the various compositors and font backends, whereas all I did was try to solve the one specific case we currently need. If it looks acceptable for upstream, though, feel free to go ahead.

Comment 14

[Behdad Esfahbod]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> My guess was that you might have run into a case where bitmap->pitch is
> greater than the destination's stride. In that case, the expression used as
> the third argument to memset would be negative - except that memset takes a
> size_t, so it would actually be a huge value, and much of memory would get
> wiped out. (Ouch.) My suggested fix aimed to handle bitmap->pitch vs stride
> discrepancies in either direction safely.

Yeah.  Understood.  Let me check the code again to see if we can simplify that.

Comment 15

[Jonathan Kew [:jfkthame]]

Re-landed the patches here, including the fix for the memory error. We may eventually end up with a slightly different version when we take a cairo update, depending what gets committed upstream, but this should work for the time being.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b656ca5ec50
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f6a0106f43
https://hg.mozilla.org/integration/mozilla-inbound/rev/23ab61f4bba1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashtest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc5d321b2b6

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

Comment 17

[Jonathan Kew [:jfkthame]]

The assert was triggered because in cherry-picking the cairo-ft-font patches, I inadvertently lost the MAX_FONT_SIZE limit that we impose in _cairo_ft_unscaled_font_set_scale.

Restored that, and tryserver says it's all good now:
https://tbpl.mozilla.org/?tree=Try&rev=046c97b907b4

Relanded (again)...
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb213ed89b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/062968884256
https://hg.mozilla.org/integration/mozilla-inbound/rev/be5f4eeb0ec2

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eeb213ed89b1
https://hg.mozilla.org/mozilla-central/rev/062968884256
https://hg.mozilla.org/mozilla-central/rev/be5f4eeb0ec2

Comment 19

[Chris AtLee [:catlee]]

This may have caused bug 981627