implement support for Windows 8.1 colored emoji font

RESOLVED FIXED in mozilla32

Status

()

Core
Graphics: Text
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: jfkthame, Assigned: m_kato)

Tracking

(Depends on: 1 bug)

unspecified
mozilla32
x86
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

(Reporter)

Description

4 years ago
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
When Google will create yet another color emoji spec for Android? :(
(Reporter)

Comment 2

4 years ago
(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/
For D2D, the new D2D1_DRAW_TEXT_OPTIONS_ENABLE_COLOR_FONT flag will enable drawing color emoji on Windows 8.1.
OS: Windows 8 → Windows 8.1
(Reporter)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

3 years ago
Created attachment 8388310 [details] [diff] [review]
WIP
(Assignee)

Comment 6

3 years ago
TODO
- add more fallback to GetCommonFallbackFonts.
- cairo implementation
- D2D1.1 implementation
(Reporter)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
for OTS.  https://code.google.com/p/chromium/issues/detail?id=354315
(Reporter)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
Created attachment 8401808 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Attachment #8388310 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8401809 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL (WIP)

This is WIP
(Assignee)

Comment 14

3 years ago
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.
(Reporter)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
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.
(Reporter)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
Created attachment 8411538 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Assignee: nobody → m_kato
(Assignee)

Comment 19

3 years ago
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:
Attachment #8401808 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Created attachment 8411539 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL
Attachment #8401809 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8411540 [details] [diff] [review]
Part 3. reftest
Attachment #8402289 - Attachment is obsolete: true
(Reporter)

Comment 22

3 years ago
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.
(Assignee)

Comment 23

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
more investigating reftest.  after that, I will review it with comment #22
(Assignee)

Comment 25

3 years ago
Created attachment 8424616 [details] [diff] [review]
Part 1. Add Segoe UI Emoji to fallback font list
Attachment #8411538 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8424617 [details] [diff] [review]
Part 2. Render color glyph using COLR/CPAL
Attachment #8411539 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8424618 [details] [diff] [review]
Part 3. Add Preference for color font
(Assignee)

Comment 28

3 years ago
Created attachment 8424619 [details] [diff] [review]
Part 4. reftest
Attachment #8411540 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
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+
Attachment #8424616 - Flags: review?(jfkthame)
(Assignee)

Comment 30

3 years ago
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.
Attachment #8424617 - Flags: review?(jfkthame)
(Assignee)

Comment 31

3 years ago
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.
Attachment #8424618 - Flags: review?(jfkthame)
(Assignee)

Updated

3 years ago
Attachment #8424619 - Flags: review?(jfkthame)
(Reporter)

Comment 32

3 years ago
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/
Attachment #8424617 - Flags: review?(jfkthame) → review+
(Reporter)

Updated

3 years ago
Attachment #8424616 - Flags: review?(jfkthame) → review+
(Reporter)

Comment 33

3 years ago
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.)
Attachment #8424618 - Flags: review?(jfkthame) → review+
(Reporter)

Comment 34

3 years ago
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.
I don't think we need a pref for this.
(Assignee)

Comment 36

3 years ago
(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.
(Assignee)

Comment 37

3 years ago
Created attachment 8428570 [details] [diff] [review]
Reftest

Roc says, pref is unnecessary.  I remove prefs.
Attachment #8424619 - Attachment is obsolete: true
Attachment #8424619 - Flags: review?(jfkthame)
Attachment #8428570 - Flags: review?(jfkthame)
(Reporter)

Comment 38

3 years ago
Comment on attachment 8428570 [details] [diff] [review]
Reftest

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

OK, omitting the pref is fine with me.
Attachment #8428570 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 39

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8be55236cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80b51a801ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb3006c6d6f
https://hg.mozilla.org/mozilla-central/rev/6f8be55236cb
https://hg.mozilla.org/mozilla-central/rev/c80b51a801ce
https://hg.mozilla.org/mozilla-central/rev/8eb3006c6d6f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Comment 41

3 years ago
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?
(Reporter)

Comment 42

3 years ago
Yes, it will.

Updated

3 years ago
Depends on: 1066933

Comment 43

3 years ago
@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

Updated

a month ago
Depends on: 1366598
You need to log in before you can comment on or make changes to this bug.