1738700 - Some glyphs are vertically shifted with COLR fonts

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Petru-Mugurel Lingurar [:petru]]

From github: https://github.com/mozilla-mobile/fenix/issues/22232.

Steps to reproduce

The page https://www.amirifont.org/fatiha-colored.html uses a font with COLR and CPAL tables for color glyphs.

Expected behaviour

The rendering should match https://www.amirifont.org/fatiha.html sans the colors.

Actual behaviour

On Android the mark glyphs (colored red) as well as the numbers (colored green inside the oval shapes) are shifted up.

Device name

Nokia 7.2

Android version

Android 10

Firefox release type

Firefox

Firefox version

93.2.0

Device logs

No response

Additional information

On Firefox desktop it renders correctly. This is a regression since the page used to show fine, but I don't remember exactly last time I checked it.

Change performed by the Move to Bugzilla add-on.

Comment 1

[Petru-Mugurel Lingurar [:petru]]

Attachment: image.png

Side by side of chrome stable and latest GVE:

Comment 2

[Jonathan Kew [:jfkthame]]

Identifying when this regressed would be really helpful.

Also, have all desktop platforms been tested? I wonder if it's specific to a particular font backend.

Comment 3

[Khaled Hosny]

Windows and macOS are fine, but the same issue is reproducible on Linux.

Comment 4

[Jonathan Kew [:jfkthame]]

Useful to know, thanks -- that suggests this is probably an issue in our use of FreeType.

Comment 5

[Jonathan Kew [:jfkthame]]

This appears to be a webrender issue; on my Linux system it regressed with the Nightly build of 2019-05-27, where bug 1554251 enabled WR; but if I manually enabled WR on an earlier build, it shows up there too. And if I force-disable webrender on a more recent version, the problem goes away again.

Comment 6

[Jonathan Kew [:jfkthame]]

I don't understand quite where/why this is going wrong, but it seems to have something to do with COLR table entries that refer to glyph IDs that themselves also have COLR entries.

In a case like

    <ColorGlyph name="uni0623">
      <layer colorID="65535" name="aAlf.isol.l0"/>
      <layer colorID="2" name="hamza.above.l0"/>
    </ColorGlyph>

we have a base glyph (aAlf.isol.l0) rendered in the current color, and a mark (hamza.above.l0) in color 2. This works fine; the two components are rendered at the expected positions. Note that neither of the components have entries of their own in the COLR table.

But when there's an entry like

    <ColorGlyph name="uni064E">
      <layer colorID="0" name="uni064E"/>
    </ColorGlyph>

where the color-layer glyph is itself a glyph with a COLR table entry (it's in fact the same glyph ID -- we might call this a "self-referential" color table entry -- although this does not seem to be significant; any glyph that has its own COLR entry shows similar issues), webrender ends up drastically misplacing the glyph.

AFAICT the text-drawing commands being sent to webrender have the correct glyph positions; the misplacement of the glyph image relative to the requested position is happening entirely on the WR side.

It kind of looks as though webrender might be losing track of the proper glyph origins; maybe something to do with caching of rasterized glyphs mishandles the sidebearings in some way? That's pure speculation, though.

I can work around the bug by stripping out the COLR table from the font data that is passed to webrender; if I do this (e.g. in UnscaledFontFreeType::GetFontFileData) the colored marks render at the proper positions. So it's clearly the presence of the COLR table that is causing the misplacement. That's an ugly hack, though.

Lee, any idea why webrender might be mishandling the glyphs in this case?

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: suppress-colr-table-for-wr.patch

Here's a proof-of-concept patch that crudely "hides" the COLR table when passing font data over to WR; this makes the Amiri example render fine on Linux. (And presumably on Android.)

However, if we can fix the issue within WR itself and avoid the need to munge the font data (note that additional work would be needed to account for locally-installed fonts as well), that would be far preferable.

Comment 8

[Jonathan Kew [:jfkthame]]

Aha! If I remove the FT_LOAD_COLOR flag from the flags we pass to FT_Load_Glyph, the problem goes away.

We don't need FreeType to try and handle color layers here, because we already did so on the Gecko side; in WebRender we're just painting each colored layer individually.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1738700 - Don't use FT_LOAD_COLOR for COLR-format fonts within WebRender, because color-font layers are handled at the Gecko level. r=lsalzman

Comment 10

[Khaled Hosny]

Wouldn’t not settling FT_LOAD_COLOR stop FreeType from also loading color bitmaps (sbix or CBDT)?

Comment 11

[Jonathan Kew [:jfkthame]]

Hmm, yes -- that may be an issue. I'll test it....

Comment 12

[Jonathan Kew [:jfkthame]]

You're right, that doesn't work so well. With Noto Color Emoji, for example, it does still load glyphs -- but as grayscale bitmaps rather than color, which is not at all the desired look.

Comment 13

[Jonathan Kew [:jfkthame]]

OK, modified the patch so that we set FT_LOAD_COLOR if bitmap strikes are present, and not otherwise. With this version, both the colored Amiri example and Noto Color Emoji render nicely.

(In theory this could still fail, if a font contains both bitmaps and COLR-layered glyphs, and uses glyphs that have COLR entries as component layers of themselves or of other glyphs, but that seems like a highly obscure edge-case -- and it wouldn't be any worse than current behavior.)

Comment 14

[Lee Salzman [:lsalzman]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)

OK, modified the patch so that we set FT_LOAD_COLOR if bitmap strikes are present, and not otherwise. With this version, both the colored Amiri example and Noto Color Emoji render nicely.

(In theory this could still fail, if a font contains both bitmaps and COLR-layered glyphs, and uses glyphs that have COLR entries as component layers of themselves or of other glyphs, but that seems like a highly obscure edge-case -- and it wouldn't be any worse than current behavior.)

Seems reasonable.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdfd26e1439d
Don't use FT_LOAD_COLOR for COLR-format fonts within WebRender, because color-font layers are handled at the Gecko level. r=lsalzman

Comment 16

[Iulian Moraru]

Backed out for causing reftest failures on 1738700-1.html.

Push with failures

Failure log

Backout link

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e09abd58c15
Don't use FT_LOAD_COLOR for COLR-format fonts within WebRender, because color-font layers are handled at the Gecko level. r=lsalzman

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d53746fc547
Backed out changeset 6e09abd58c15 for causing failures at bugs/1738700-1.html. CLOSED TREE

Comment 19

[Atila Butkovits]

Backed out for causing failures at bugs/1738700-1.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/9d53746fc5470b7e28f7189d4ae9b07b4edc4c88

Push where the failures started: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Kh_Usv5UTlWQ4WYNOfeFyA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=r8&revision=1c739ce689ee92c9923d53007796b92c50aec7d4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=357531513&repo=autoland&lineNumber=13480

Comment 20

[Jonathan Kew [:jfkthame]]

Ugh -- not sure how that happened (presumably I messed up with phabricator somehow, though I don't know how...), but the two font files in the patch apparently ended up as empty! No wonder the test fails.

Will restore the fonts and then re-land.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bd4e0246b82
Don't use FT_LOAD_COLOR for COLR-format fonts within WebRender, because color-font layers are handled at the Gecko level. r=lsalzman

Comment 22

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/3bd4e0246b82