Last Comment Bug 889401 - implement support for Windows 8.1 colored emoji font
: implement support for Windows 8.1 colored emoji font
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Text (show other bugs)
: unspecified
: x86 Windows 8.1
-- normal with 3 votes (vote)
: mozilla32
Assigned To: Makoto Kato [:m_kato]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 1066933
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-02 09:15 PDT by Jonathan Kew (:jfkthame)
Modified: 2014-09-15 19:06 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.60 KB, patch)
2014-03-09 20:27 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 1. Add Segoe UI Emoji to fallback font list (2.41 KB, patch)
2014-04-04 04:39 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 2. Render color glyph using COLR/CPAL (WIP) (27.39 KB, patch)
2014-04-04 04:39 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 3. reftest (WIP) (6.76 KB, patch)
2014-04-06 03:16 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 1. Add Segoe UI Emoji to fallback font list (2.52 KB, patch)
2014-04-23 20:51 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 2. Render color glyph using COLR/CPAL (30.21 KB, patch)
2014-04-23 20:54 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 3. reftest (6.79 KB, patch)
2014-04-23 20:55 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Part 1. Add Segoe UI Emoji to fallback font list (2.52 KB, patch)
2014-05-18 23:02 PDT, Makoto Kato [:m_kato]
jfkthame: review+
Details | Diff | Splinter Review
Part 2. Render color glyph using COLR/CPAL (29.74 KB, patch)
2014-05-18 23:02 PDT, Makoto Kato [:m_kato]
jfkthame: review+
Details | Diff | Splinter Review
Part 3. Add Preference for color font (6.08 KB, patch)
2014-05-18 23:03 PDT, Makoto Kato [:m_kato]
jfkthame: review+
Details | Diff | Splinter Review
Part 4. reftest (5.44 KB, patch)
2014-05-18 23:04 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
Reftest (5.30 KB, patch)
2014-05-26 01:14 PDT, Makoto Kato [:m_kato]
jfkthame: review+
Details | Diff | Splinter Review

Description User image Jonathan Kew (:jfkthame) 2013-07-02 09:15:52 PDT
Microsoft will be shipping a color emoji font with Windows 8.1, based on their new COLR and CPAL tables.[1]

We should probably consider supporting this in Gecko. (I haven't looked at Win8.1 Preview yet, but I'm assuming it won't "just work" for us, due to the low level at which we interact with the Windows font/text systems.)

[1] http://typophile.com/node/104174
Comment 1 User image Masatoshi Kimura [:emk] 2013-07-03 02:52:04 PDT
When Google will create yet another color emoji spec for Android? :(
Comment 2 User image Jonathan Kew (:jfkthame) 2013-07-03 03:04:34 PDT
(In reply to Masatoshi Kimura [:emk] from comment #1)
> When Google will create yet another color emoji spec for Android? :(

http://google-opensource.blogspot.de/2013/05/open-standard-color-font-fun-for.html
https://code.google.com/p/color-emoji/
Comment 3 User image Masatoshi Kimura [:emk] 2013-10-21 17:51:41 PDT
For D2D, the new D2D1_DRAW_TEXT_OPTIONS_ENABLE_COLOR_FONT flag will enable drawing color emoji on Windows 8.1.
Comment 4 User image Jonathan Kew (:jfkthame) 2013-10-21 23:23:34 PDT
(In reply to Masatoshi Kimura [:emk] from comment #3)
> For D2D, the new D2D1_DRAW_TEXT_OPTIONS_ENABLE_COLOR_FONT flag will enable
> drawing color emoji on Windows 8.1.

I doubt that will help us, as we don't draw text via the high-level DrawText API where we could pass that flag. We'll need to read the COLR/CPAL tables ourselves and paint the glyphs for each color layer in turn, instead of the default monochrome glyphs.

An advantage we'll gain from implementing it this way is that it should work across all platforms - and I think it's likely people will start deploying Win8.1-style color fonts as webfonts, so supporting them cross-platform will be an attractive feature.
Comment 5 User image Makoto Kato [:m_kato] 2014-03-09 20:27:57 PDT
Created attachment 8388310 [details] [diff] [review]
WIP
Comment 6 User image Makoto Kato [:m_kato] 2014-03-09 23:09:07 PDT
TODO
- add more fallback to GetCommonFallbackFonts.
- cairo implementation
- D2D1.1 implementation
Comment 7 User image Jonathan Kew (:jfkthame) 2014-03-10 04:23:08 PDT
Note that MS has submitted this color font format for standardization through the MPEG process, and font designers are interested in using it to implement various types of color fonts (e.g. icons/dingbats, layered titling fonts, etc in addition to emoji).

As such, I think we should try to implement support for this in a platform-independent way at the text-run drawing level, rather than directly in the Windows backends using Windows APIs.

It should be possible to read the COLR/CPAL tables and paint the component glyphs in the appropriate colors e.g. at the level of gfxFont::Draw, or something like that. This will allow these color fonts to work uniformly across all platforms.
Comment 8 User image Makoto Kato [:m_kato] 2014-03-10 18:34:41 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Note that MS has submitted this color font format for standardization
> through the MPEG process, and font designers are interested in using it to
> implement various types of color fonts (e.g. icons/dingbats, layered titling
> fonts, etc in addition to emoji).

Where draft document they submit?  They say "We plan to submit documentation on this approach for standardization through the ISO MPEG process (for inclusion in the Open Font Format) within the next few weeks."..., but I cannot find this draft.  Is it member only of ISO MPEG?

> As such, I think we should try to implement support for this in a
> platform-independent way at the text-run drawing level, rather than directly
> in the Windows backends using Windows APIs.
> 
> It should be possible to read the COLR/CPAL tables and paint the component
> glyphs in the appropriate colors e.g. at the level of gfxFont::Draw, or
> something like that. This will allow these color fonts to work uniformly
> across all platforms.

Actually, except to Windows and OSX, all platforms use FreeType to render glyph.  So if is is standardized and non-patent issue, I think FreeType project will support this format like Android's emoji.  Then we can support it like bug 969814.

Also now, we support Apple format using OSX API.  If Apple supports Microsoft's format, they will also support it by same API.

If you want to say that all platform includes Windows GDI, I agree.  Color font API is for Direct2D/DirectWrite only.  IDWriteBitmapRenderTarget doesn't support alpha, so it cannot render color font correctly.  And no API for GDI font rendering.
Comment 9 User image Makoto Kato [:m_kato] 2014-03-10 19:21:23 PDT
(In reply to Makoto Kato (:m_kato) from comment #8)
> Where draft document they submit?  They say "We plan to submit documentation
> on this approach for standardization through the ISO MPEG process (for
> inclusion in the Open Font Format) within the next few weeks."..., but I
> cannot find this draft.  Is it member only of ISO MPEG?

I got it from Brian.
Comment 10 User image Makoto Kato [:m_kato] 2014-03-19 22:09:11 PDT
for OTS.  https://code.google.com/p/chromium/issues/detail?id=354315
Comment 11 User image Jonathan Kew (:jfkthame) 2014-03-20 05:54:08 PDT
(In reply to Makoto Kato (:m_kato) from comment #8)

> Actually, except to Windows and OSX, all platforms use FreeType to render
> glyph.  So if is is standardized and non-patent issue, I think FreeType
> project will support this format like Android's emoji.  Then we can support
> it like bug 969814.
> 

Yes, it seems likely FT will add support for this format, which will give us a solution on Android and B2G.

> Also now, we support Apple format using OSX API.  If Apple supports
> Microsoft's format, they will also support it by same API.

I'm less confident that Apple will support this via the Core Text APIs in the near future. They have their own color font format (as used for the Apple Color Emoji font), so they may not regard this as a priority. In any case, this would not give us a solution on existing OS X versions, which we will continue to support for some time yet.

> 
> If you want to say that all platform includes Windows GDI, I agree.  Color
> font API is for Direct2D/DirectWrite only.  IDWriteBitmapRenderTarget
> doesn't support alpha, so it cannot render color font correctly.  And no API
> for GDI font rendering.

Right; and I believe we still have a substantial number of users on GDI systems (i.e. not using Direct2D/DW, either because they're still on WinXP or because of hardware limitations, driver issues, etc.).

So I'd still prefer to try for a solution at the platform-independent gfxFont::Draw level.
Comment 12 User image Makoto Kato [:m_kato] 2014-04-04 04:39:02 PDT
Created attachment 8401808 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Comment 13 User image Makoto Kato [:m_kato] 2014-04-04 04:39:39 PDT
Created attachment 8401809 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL (WIP)

This is WIP
Comment 14 User image Makoto Kato [:m_kato] 2014-04-04 04:43:38 PDT
I tests on Windows 8.1/7 and Linux.

Also,
- I don't test on non-azure yet.
- Also, I don't test on OSX yet.
- we should not set italic style if color glyph.
Comment 15 User image Jonathan Kew (:jfkthame) 2014-04-04 06:39:58 PDT
(In reply to Makoto Kato (:m_kato) from comment #14)
> I tests on Windows 8.1/7 and Linux.

Thanks for working on this; looks very promising.

> 
> Also,
> - I don't test on non-azure yet.
> - Also, I don't test on OSX yet.

I'll try this on OS X over the weekend, if you like.

> - we should not set italic style if color glyph.

I think we probably *should* respect the italic style for color glyphs just the same as standard glyphs (using a true italic face if available in the family, and applying a synthetic oblique effect otherwise). Color fonts will be used for other purposes than just emoji (where it's true that slanting might look a bit odd); people are already using this technology to create "layered" color text fonts for headlines, etc., and if italic styling is applied to these, the expectation would be for it to work just like any other font.
Comment 16 User image Makoto Kato [:m_kato] 2014-04-06 03:16:03 PDT
Created attachment 8402289 [details] [diff] [review]
Part 3. reftest (WIP)

https://tbpl.mozilla.org/?tree=Try&rev=c7729f785741

passed on all Windows and OSX, but Linux does't pass due to clipping?/alignment even if color layer is rendered.  I'll investigate this.
Comment 17 User image Jonathan Kew (:jfkthame) 2014-04-23 09:10:09 PDT
(In reply to Makoto Kato (:m_kato) from comment #16)
> Created attachment 8402289 [details] [diff] [review]
> Part 3. reftest (WIP)
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c7729f785741
> 
> passed on all Windows and OSX, but Linux does't pass due to
> clipping?/alignment even if color layer is rendered.  I'll investigate this.

I wonder if this might be related to odd behavior in the freetype auto-hinting engine, which doesn't know about these glyphs.

I'd suggest modifying the test to (a) use a larger font size, and (b) replace yellow with a stronger color such as green or blue, just to make it easier to see the colored pixels when inspecting the results by eye.

If the discrepancy remains pretty minor (a small number of edge pixels) at larger sizes, I'd be in favor of landing the patch, given that it seems to render the real Segoe UI Emoji font nicely, marking the test as fuzzy, and filing a followup for any further investigation.
Comment 18 User image Makoto Kato [:m_kato] 2014-04-23 20:51:00 PDT
Created attachment 8411538 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Comment 19 User image Makoto Kato [:m_kato] 2014-04-23 20:53:49 PDT
Comment on attachment 8401808 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list

># HG changeset patch
># Parent 01ca94b4729ffa8ba577cb55e0176f66b836fe48
>Bug 889401 - Part 1. Add Segoe UI Emoji to fallback font list
>
>diff --git a/gfx/thebes/gfxWindowsPlatform.cpp b/gfx/thebes/gfxWindowsPlatform.cpp
>--- a/gfx/thebes/gfxWindowsPlatform.cpp
>+++ b/gfx/thebes/gfxWindowsPlatform.cpp
>@@ -752,31 +752,33 @@ static const char kFontMicrosoftTaiLe[] 
> static const char kFontMicrosoftUighur[] = "Microsoft Uighur";
> static const char kFontMicrosoftYaHei[] = "Microsoft YaHei";
> static const char kFontMicrosoftYiBaiti[] = "Microsoft Yi Baiti";
> static const char kFontMeiryo[] = "Meiryo";
> static const char kFontMongolianBaiti[] = "Mongolian Baiti";
> static const char kFontNyala[] = "Nyala";
> static const char kFontPlantagenetCherokee[] = "Plantagenet Cherokee";
> static const char kFontSegoeUI[] = "Segoe UI";
>+static const char kFontSegoeUIEmoji[] = "Segoe UI Emoji";
> static const char kFontSegoeUISymbol[] = "Segoe UI Symbol";
> static const char kFontSylfaen[] = "Sylfaen";
> static const char kFontTraditionalArabic[] = "Traditional Arabic";
> 
> void
> gfxWindowsPlatform::GetCommonFallbackFonts(const uint32_t aCh,
>                                            int32_t aRunScript,
>                                            nsTArray<const char*>& aFontList)
> {
>     // Arial is used as the default fallback for system fallback
>     aFontList.AppendElement(kFontArial);
> 
>     if (!IS_IN_BMP(aCh)) {
>         uint32_t p = aCh >> 16;
>         if (p == 1) { // SMP plane
>+            aFontList.AppendElement(kFontSegoeUIEmoji);
>             aFontList.AppendElement(kFontSegoeUISymbol);
>             aFontList.AppendElement(kFontEbrima);
>             aFontList.AppendElement(kFontCambriaMath);
>         }
>     } else {
>         uint32_t b = (aCh >> 8) & 0xff;
> 
>         switch (b) {
>@@ -826,16 +828,17 @@ gfxWindowsPlatform::GetCommonFallbackFon
>         case 0x25:
>         case 0x26:
>         case 0x27:
>         case 0x29:
>         case 0x2a:
>         case 0x2b:
>         case 0x2c:
>             aFontList.AppendElement(kFontSegoeUI);
>+            aFontList.AppendElement(kFontSegoeUIEmoji);
>             aFontList.AppendElement(kFontSegoeUISymbol);
>             aFontList.AppendElement(kFontCambria);
>             aFontList.AppendElement(kFontMeiryo);
>             aFontList.AppendElement(kFontArial);
>             aFontList.AppendElement(kFontLucidaSansUnicode);
>             aFontList.AppendElement(kFontEbrima);
>             break;
>         case 0x2d:
Comment 20 User image Makoto Kato [:m_kato] 2014-04-23 20:54:18 PDT
Created attachment 8411539 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL
Comment 21 User image Makoto Kato [:m_kato] 2014-04-23 20:55:37 PDT
Created attachment 8411540 [details] [diff] [review]
Part 3. reftest
Comment 22 User image Jonathan Kew (:jfkthame) 2014-04-24 04:14:17 PDT
Comment on attachment 8411539 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL

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

A couple of drive-by comments:

::: gfx/thebes/gfxFont.cpp
@@ +479,5 @@
>      return mMathTable->GetMathVariantsParts(aGlyphID, aVertical, aGlyphs);
>  }
>  
> +bool
> +gfxFontEntry::TryGetColorGlyph(gfxFont* aFont)

Maybe name this TryGetColorGlyphs (plural) or ...GlyphTables? It isn't getting a specific color glyph, but the tables for color glyphs in general.

@@ +495,5 @@
> +
> +    mCPAL = GetFontTable(TRUETYPE_TAG('C', 'P', 'A', 'L'));
> +    if (!mCPAL) {
> +        hb_blob_destroy(mCPAL);
> +        mCPAL = nullptr;

I think you meant to destroy and null out mCOLR here, not mCPAL.
Comment 23 User image Makoto Kato [:m_kato] 2014-04-27 19:19:20 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> (In reply to Makoto Kato (:m_kato) from comment #16)
> > Created attachment 8402289 [details] [diff] [review]
> > Part 3. reftest (WIP)
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=c7729f785741
> > 
> > passed on all Windows and OSX, but Linux does't pass due to
> > clipping?/alignment even if color layer is rendered.  I'll investigate this.
> 
> I wonder if this might be related to odd behavior in the freetype
> auto-hinting engine, which doesn't know about these glyphs.
> 
> I'd suggest modifying the test to (a) use a larger font size, and (b)
> replace yellow with a stronger color such as green or blue, just to make it
> easier to see the colored pixels when inspecting the results by eye.
> 
> If the discrepancy remains pretty minor (a small number of edge pixels) at
> larger sizes, I'd be in favor of landing the patch, given that it seems to
> render the real Segoe UI Emoji font nicely, marking the test as fuzzy, and
> filing a followup for any further investigation.

even if blue or big size, it is still failure.  So I will modify glyph design for passing test if found.
Comment 24 User image Makoto Kato [:m_kato] 2014-04-27 19:20:42 PDT
more investigating reftest.  after that, I will review it with comment #22
Comment 25 User image Makoto Kato [:m_kato] 2014-05-18 23:02:06 PDT
Created attachment 8424616 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Comment 26 User image Makoto Kato [:m_kato] 2014-05-18 23:02:54 PDT
Created attachment 8424617 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL
Comment 27 User image Makoto Kato [:m_kato] 2014-05-18 23:03:46 PDT
Created attachment 8424618 [details] [diff] [review]
Part 3. Add Preference for color font
Comment 28 User image Makoto Kato [:m_kato] 2014-05-18 23:04:13 PDT
Created attachment 8424619 [details] [diff] [review]
Part 4. reftest
Comment 29 User image Makoto Kato [:m_kato] 2014-05-18 23:06:17 PDT
Comment on attachment 8424616 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list

add Segoe UI Emoji for Windows Platform.  This font is on Windows 8+
Comment 30 User image Makoto Kato [:m_kato] 2014-05-18 23:08:24 PDT
Comment on attachment 8424617 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL

Render color glyph using COLR and CPAL.

Microsoft uses CPAL version 0 header only.  Current draft includes version 1, but no font that it uses.
Comment 31 User image Makoto Kato [:m_kato] 2014-05-18 23:09:44 PDT
Comment on attachment 8424618 [details] [diff] [review]
Part 3. Add Preference for color font

Add pref (gfx.font_rendering.opentype_colr.enabled) for this support.  If this is unnecessary, pleace cancel this.
Comment 32 User image Jonathan Kew (:jfkthame) 2014-05-20 08:21:01 PDT
Comment on attachment 8424617 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL

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

This looks awesome - some minor nits and suggestions below, but I hope to see this landing soon! :)

r=me with the fixes mentioned; or if they don't seem to make sense, let me know and we'll reconsider how to address them.

::: gfx/thebes/gfxFont.cpp
@@ +173,5 @@
>  
>  gfxFontEntry::~gfxFontEntry()
>  {
> +    if (mCOLR) {
> +      hb_blob_destroy(mCOLR);

Local style here is 4-space indent.

@@ +177,5 @@
> +      hb_blob_destroy(mCOLR);
> +    }
> +
> +    if (mCPAL) {
> +      hb_blob_destroy(mCPAL);

ditto

@@ +496,5 @@
>      return mMathTable->GetMathVariantsParts(aGlyphID, aVertical, aGlyphs);
>  }
>  
> +bool
> +gfxFontEntry::TryGetColorGlyphs(gfxFont* aFont)

The parameter is unused, just get rid of it.

@@ +517,5 @@
> +        return false;
> +    }
> +
> +    // validation COLR and CPAL table
> +    if (gfxFontUtils::ReadColorGlyphs(mCOLR, mCPAL)) {

s/Read/Validate/

@@ +3438,5 @@
>  
> +bool
> +gfxFont::GetColorLayersInfo(uint32_t aGlyphId,
> +                            nsTArray<uint16_t>& layerGlyphs,
> +                            nsTArray<mozilla::gfx::Color>& layerColors)

Argument names should have an "a" prefix

@@ +3446,5 @@
> +        reinterpret_cast<const uint8_t*>(hb_blob_get_data(GetFontEntry()->mCOLR,
> +                                                          &blobLength));
> +    if (!colrTable || !blobLength) {
> +        return false;
> +    }

It shouldn't be necessary to do these checks here (or to get the blob length at all, AFAICS), because we shouldn't ever call this unless TryGetColorGlyphs returned true - which guarantees that the tables exist (and passed the validation checks).

@@ +3451,5 @@
> +
> +    void* baseGlyph = gfxFontUtils::LookForBaseGlyphRecord(colrTable, aGlyphId);
> +    if (!baseGlyph) {
> +        return false;
> +    }

Rather than this void*, just pass the glyph ID to GetColorGlyphLayers and let it find the record.

@@ +3458,5 @@
> +        reinterpret_cast<const uint8_t*>(hb_blob_get_data(GetFontEntry()->mCPAL,
> +                                                          &blobLength));
> +    if (!cpalTable || !blobLength) {
> +        return false;
> +    }

Same here.

Make these assertions instead?

@@ +3464,5 @@
> +    if (!gfxFontUtils::GetColorGlyphLayers(colrTable,
> +                                           cpalTable,
> +                                           baseGlyph,
> +                                           layerGlyphs,
> +                                           layerColors)) {

Actually, why not just pass GetFontEntry()->mCOLR and mCPAL here, as blobs, and let GetColorGlyphLayers get the data pointers from them?

And no need for the "if" statement here, either.

Then I think this method can be reduced to simply

bool
gfxFont::GetColorLayersInfo(uint32_t aGlyphId,
                            nsTArray<uint16_t>& aGlyphs,
                            nsTArray<mozilla::gfx::Color>& aColors)
{
    gfxFontEntry* fe = GetFontEntry();
    return gfxFontUtils::GetColorGlyphLayers(fe->mCOLR, fe->mCPAL,
                                             aGlyphID,
                                             aGlyphs, aColors);
}

Wait, better still: we can make this a method of gfxFontEntry, rather than gfxFont. And RenderColorGlyph below will then just call it as GetFontEntry()->GetColorLayersInfo(...).

@@ +3475,5 @@
> +gfxFont::RenderColorGlyph(gfxContext* aContext, gfxPoint& point,
> +                          uint32_t aGlyphId)
> +{
> +    nsTArray<uint16_t> layerGlyphs;
> +    nsTArray<mozilla::gfx::Color> layerColors;

Use nsAutoTArray for these, so that in the usual case of a few layers, we won't need to do additional allocation. An auto-buffer of about 8 elements is likely to be enough for the vast majority of uses.

@@ +3513,5 @@
> +                          const mozilla::gfx::Point& aPoint,
> +                          uint32_t aGlyphId)
> +{
> +    nsTArray<uint16_t> layerGlyphs;
> +    nsTArray<mozilla::gfx::Color> layerColors;

nsAutoTArray again

::: gfx/thebes/gfxFontUtils.cpp
@@ +1465,5 @@
> +bool
> +gfxFontUtils::ReadColorGlyphs(hb_blob_t* aCOLR, hb_blob_t* aCPAL)
> +{
> +    // DirectWrite (Analzye method) on Windows 8.1 has the validation code too.
> +    // Also, if OTS supports both tables, we can remove this.

Just a note: I think we should keep the validation here (regardless of what OTS does). Then the rest of the color-font code doesn't need to worry about malformed tables with bad offsets - even for locally-installed fonts, which don't go through OTS.

(So maybe omit this comment. Or revise it to note that we validate the tables here so that later, while rendering each glyph, we can assume the offsets are safe to use.)

@@ +1477,5 @@
> +
> +    if (!colr || !cpal || !colrLength || !cpalLength) {
> +        return false;
> +    }
> +  

stray whitespace

@@ +1483,5 @@
> +        // We only support version 0 heaers.
> +        return false;
> +    }
> +
> +    const uint32_t offsetBaseGlyphRecord = colr->offsetBaseGlyphRecord;

Check that offsetBaseGlyphRecord < colrLength

@@ +1485,5 @@
> +    }
> +
> +    const uint32_t offsetBaseGlyphRecord = colr->offsetBaseGlyphRecord;
> +    const uint16_t numBaseGlyphRecord = colr->numBaseGlyphRecord;
> +    const uint32_t offsetLayerRecord = colr->offsetLayerRecord;

Check < length

@@ +1488,5 @@
> +    const uint16_t numBaseGlyphRecord = colr->numBaseGlyphRecord;
> +    const uint32_t offsetLayerRecord = colr->offsetLayerRecord;
> +    const uint16_t numLayerRecords = colr->numLayerRecords;
> +
> +    const uint32_t offsetFirstColorRecord = cpal->offsetFirstColorRecord;

Check < length

@@ +1490,5 @@
> +    const uint16_t numLayerRecords = colr->numLayerRecords;
> +
> +    const uint32_t offsetFirstColorRecord = cpal->offsetFirstColorRecord;
> +    const uint16_t numColorRecords = cpal->numColorRecords;
> +    const uint32_t numPaletteEntries = cpal->numPaletteEntries;

Also need to check that cpal->numPalettes > 0, as the code assumes palette 0 can be used safely, if I'm reading it correctly.

@@ +1541,5 @@
> +            return false;
> +        }
> +    }
> +
> +    const COLRLayerRecord* layer = 

trailing space

@@ +1589,5 @@
> +gfxFontUtils::GetColorGlyphLayers(const uint8_t* aCOLR,
> +                                  const uint8_t* aCPAL,
> +                                  void* aBaseGlyph,
> +                                  nsTArray<uint16_t> &aGlyphs,
> +                                  nsTArray<mozilla::gfx::Color> &aColors)

Use & as a suffix on the type, please, instead of a prefix on the parameter name.

::: gfx/thebes/gfxFontUtils.h
@@ +942,5 @@
>      // generate a unique font name
>      static nsresult MakeUniqueUserFontName(nsAString& aName);
>  
> +    // for color layer from glyph using COLR and CPAL tables
> +    static bool ReadColorGlyphs(hb_blob_t* aCOLR, hb_blob_t* aCPAL);

Rename this as ValidateColorGlyphTables, to make it clearer what it is doing.

@@ +948,5 @@
> +    static bool GetColorGlyphLayers(const uint8_t* aCOLR,
> +                                    const uint8_t* aCPAL,
> +                                    void* aBaseGlyph,
> +                                    nsTArray<uint16_t> &aGlyphs,
> +                                    nsTArray<mozilla::gfx::Color> &aColors);

I think it'd be cleaner from an API point of view if GetColorGlyphLayers takes the glyph ID, not a baseGlyph pointer, and does the base lookup internally.

Then LookForBaseGlyphRecord would be purely internal to gfxFontUtils (take it out of the public API).

::: gfx/thebes/gfxUserFontSet.cpp
@@ +334,3 @@
>  OTSTableAction(uint32_t aTag, void *aUserData)
>  {
>      // preserve Graphite and SVG tables

s/SVG/color-glyph/
Comment 33 User image Jonathan Kew (:jfkthame) 2014-05-20 08:24:19 PDT
Comment on attachment 8424618 [details] [diff] [review]
Part 3. Add Preference for color font

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

I'm not entirely sure we need a pref for this, but let's do it, I guess - maybe after a few cycles with no problems, we can remove it again.

(I suppose it's possible there may be users who really *dislike* the color glyphs, in which case the existence of the pref gives them a way to disable them even though the font is now standard on Windows.)
Comment 34 User image Jonathan Kew (:jfkthame) 2014-05-20 08:27:07 PDT
Comment on attachment 8424619 [details] [diff] [review]
Part 4. reftest

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

As we've got a pref, we should also have a test with gfx.font_rendering.opentype_colr.enabled = false to make sure it actually works.
Comment 35 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2014-05-25 15:27:22 PDT
I don't think we need a pref for this.
Comment 36 User image Makoto Kato [:m_kato] 2014-05-26 01:09:31 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> I don't think we need a pref for this.

OK, I don't land part 3. and modify part 4.
Comment 37 User image Makoto Kato [:m_kato] 2014-05-26 01:14:55 PDT
Created attachment 8428570 [details] [diff] [review]
Reftest

Roc says, pref is unnecessary.  I remove prefs.
Comment 38 User image Jonathan Kew (:jfkthame) 2014-05-26 02:11:06 PDT
Comment on attachment 8428570 [details] [diff] [review]
Reftest

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

OK, omitting the pref is fine with me.
Comment 41 User image Terrell Kelley 2014-08-03 02:20:42 PDT
Sorry to bug you guys, but it's unclear from the description whether this fix is Windows 8.1 only. It is listed as being a Windows 8.1 bug, but comments seem to indicate a cross-platform solution. Do I understand correctly that, if a user has a COLR/CPAL compatible font, it will render on all OSes?
Comment 42 User image Jonathan Kew (:jfkthame) 2014-08-03 03:30:41 PDT
Yes, it will.
Comment 43 User image ShawnDion 2014-09-15 19:06:26 PDT
@Everyone on this project.

I'd like to say thank you for such a wonderful add-on to firefox.

If you search for "Social Fonts" on Kickstarter (No intent in promoting it will relaunch it differently now that Firefox can work with it so many more possibilities now) I had pushed the limits on what the COLR/CPAL could do unfortunately until recently I was stuck with Windows 8.1 and Internet Explorer 11 or some 3rd party uncommon software to try and make this crazy art project.

So seriously here's the font link "dist.be/files/SocialFont.zip" this thing has 256 colored layers per glyph for the letters ABC if it can help you in optimizing the implementation of the COLR/CPAL by all means use it to your hearts content.

I'm glad there are Dev's like all of you to not create a monopoly of having this just run on Windows 8.1

Since last year I was waiting for a browser to open the door to new ways of using this font technology and boy am I pleased that it's Firefox.

All the best and keep up the amazing work.
Shawn W. Dion

Note You need to log in before you can comment on or make changes to this bug.