Closed Bug 974575 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics: Text, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
[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
---
Attachment #8378511 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8378535 - Flags: review?(jmuizelaar)
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.
Blocks: 944872
Blocks: 939280
Attachment #8378535 - Flags: review?(jmuizelaar) → review+
Attachment #8378511 - Flags: review?(jmuizelaar) → review+
No longer blocks: 969814
Depends on: 969814
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.
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.
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.
commit f88bd92e8b3d87ec728e3fee51eb82f07db8c95c
Author: Behdad Esfahbod <behdad@behdad.org>
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.
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.
Attachment #8385967 - Flags: review?(mozilla)
Attachment #8385967 - Flags: review?(jmuizelaar)
(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.
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.
(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.
(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.
Attachment #8385967 - Flags: review?(jmuizelaar) → review+
Attachment #8385967 - Flags: review?(mozilla)
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
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
This may have caused bug 981627
Depends on: 981627
You need to log in before you can comment on or make changes to this bug.