637481 - Cannot load Japanese OTF/WOFF with CFF outline as Web fonts when DirectWrite is disabled

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Make sure 'gfx.font_rendering.directwrite.enabled' is set to false.
2. Open the URL.
Actual result:
「あ」 is displayed with a normal fallback font.
Expected result:
「あ」 should be circled.

It fails even if the WOFF is converted back to raw OTF. It works when I enables DirectWrite. Chrome, IE9, and latest WebKit Nightly Build also work as expected. I heard Firefox for Mac would work either.
AddFontMemResourceEx seems to fail for some unknown reason.

Comment 1

[Masatoshi Kimura [:emk]]

Sorry the subject was wrong.

Comment 2

[Masatoshi Kimura [:emk]]

1. Open type sanitizer broke Japanese OTF with CFF outline.
2. AddFontMemResourceEx added two faces for the font.
When I changed
> -        if (fontRef && numFonts != 1) {
to
> +        if (fontRef && numFonts > 2) {
and turned "gfx.downloadable_fonts.sanitize" to false, the font worked.

Comment 3

[Masatoshi Kimura [:emk]]

Oh, when I turn "gfx.downloadable_fonts.sanitize.preserve_otl_tables" to false, the font works without any source code changes.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to comment #3)
> Oh, when I turn "gfx.downloadable_fonts.sanitize.preserve_otl_tables" to false,
> the font works without any source code changes.

Yes, I see the same behavior. It appears that the presence of the GSUB table in the font is causing AddFontMemResourceEx to fail. At this point I have no idea why that would happen.

(In reply to comment #2)
> 2. AddFontMemResourceEx added two faces for the font.

Are you sure about this? I didn't observe that in my testing so far.

Comment 5

[Masatoshi Kimura [:emk]]

Ok, I've figured out the cause.
If the GSUB table contains vert/vrt2 feature, vhea/vmtx tables must be present. Otherwise AddFontMemResourceEx will fail for OTF with CFF outline.
And if vhea/vmtx tables exist, AddFontMemResourceEx will generate additional face for vertical text ("@faceName").
OTS removes vhea/vmtx tables because it doesn't support those tables. It's the reason why disabling OTS resolves the AddFontMemResourceEx failure.

(In reply to comment #4)
> > 2. AddFontMemResourceEx added two faces for the font.
> Are you sure about this? I didn't observe that in my testing so far.

If the font doesn't support vertical writing (i.e. GSUB vrt2/vhea/vmtx are not present), AddFontMemResourceEx will add only one face.

There are two approaches to fix the problem:
1. Parse GSUB table and remove vert/vrt2 features. It's OK atm because we don't support vertical writing yet (bug 145503).
2. Modify OTS to add a support for vhea/vmtx, and allow two faces after adding font resource if the font supports vertical writing.

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to comment #5)
> Ok, I've figured out the cause.
> If the GSUB table contains vert/vrt2 feature, vhea/vmtx tables must be present.
> Otherwise AddFontMemResourceEx will fail for OTF with CFF outline.
> And if vhea/vmtx tables exist, AddFontMemResourceEx will generate additional
> face for vertical text ("@faceName").

Aha - excellent, thanks for figuring this out!

> There are two approaches to fix the problem:
> 1. Parse GSUB table and remove vert/vrt2 features. It's OK atm because we don't
> support vertical writing yet (bug 145503).
> 2. Modify OTS to add a support for vhea/vmtx, and allow two faces after adding
> font resource if the font supports vertical writing.

I'd prefer to avoid (1), parsing and modifying the GSUB table in OTS, as that seems like a complex and error-prone option. If upstream OTS adds full GSUB support some day, that's great, but in the meantime let's avoid touching it if possible.

So I think we should do (2). It should be fairly easy to add support for vhea/vmtx, as they're similar to hhea/hmtx.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch

Unfortunately I couldn't find a free OTF with CFF outline which is usable for reftest.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: patch to add vhea/vmtx support to OTS

Rather than duplicate the entire parsing and writing code for hhea/hmtx, I think it would be better to share code between the vertical and horizontal tables, as they have the same structure. ISTM that will simplify future maintenance.

Something like this should work, I think (untested) - some of the comments and perhaps some variable naming should also be updated here, this is just to show what I have in mind. Can you try this approach and see if it's OK?

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 516244 [details] [diff] [review]
patch to add vhea/vmtx support to OTS

(In reply to comment #8)
> Rather than duplicate the entire parsing and writing code for hhea/hmtx, I
> think it would be better to share code between the vertical and horizontal
> tables, as they have the same structure. ISTM that will simplify future
> maintenance.
> 
> Something like this should work, I think (untested) - some of the comments and
> perhaps some variable naming should also be updated here, this is just to show
> what I have in mind. Can you try this approach and see if it's OK?
Totally agree.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: patch v2

Your patch works perfectly, so I incorporated it with little modification.
An only change is writing a version field because vhea table may have version 1.1.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 516616 [details] [diff] [review]
patch v2

Looks good. A couple of things before we go ahead, please:

1) It would be good to add comments at the beginning of the hhea and hmtx files to note that they are also used for the vertical versions of these tables, as those have the same structure.

2) Let's get rid of the duplicate struct definitions in hhea.h and hmtx.h, and the reinterpret_cast<...>s in the .cc files. Instead, we can just do

--- a/gfx/ots/src/ots.h
+++ b/gfx/ots/src/ots.h
@@ -183,16 +183,19 @@ class Buffer {
   F(gdef, GDEF) \
   F(gpos, GPOS) \
   F(gsub, GSUB)
 
 #define F(name, capname) struct OpenType##capname;
 FOR_EACH_TABLE_TYPE
 #undef F
 
+#define OpenTypeVHEA OpenTypeHHEA
+#define OpenTypeVMTX OpenTypeHMTX
+
 struct OpenTypeFile {
   OpenTypeFile() {
 #define F(name, capname) name = NULL;
     FOR_EACH_TABLE_TYPE
 #undef F
   }
 
   uint32_t version;

to make the existing struct definitions serve both purposes.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Changes from the previous patch:
* resolved comments.
* make vhea->version validation more strict.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 517393 [details] [diff] [review]
patch v3

r=jfkthame

A couple of tiny changes I'd suggest making before this actually lands:

>diff --git a/gfx/ots/src/hhea.cc b/gfx/ots/src/hhea.cc
>--- a/gfx/ots/src/hhea.cc
>+++ b/gfx/ots/src/hhea.cc
>@@ -9,19 +9,22 @@
> 
> // hhea - Horizontal Header
> // http://www.microsoft.com/opentype/otspec/hhea.htm
>+// vhea - Vertical Header
>+// http://www.microsoft.com/opentype/otspec/vhea.htm
>+// This header is used for both tables because they share the same structures.

Better to say "This file..." rather than "This header...", I think (both here and in hmtx.cc).

>+bool ots_hhea_parse(OpenTypeFile *file, const uint8_t *data, size_t length) {
>+  return ots_Xhea_parse(file, data, length, &file->hhea);
>+}

We could also check that the low word of file->hhea->version is zero here (similar to the check in ots_vhea_parse, except AND with 0xFFFF).

Comment 14

[Jonathan Kew [:jfkthame]]

Also, would you be happy to submit this to Google as a suggested enhancement to the upstream OTS code?

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: patch v4: for check in

Carrying forward r+.

(In reply to comment #13)
> Better to say "This file..." rather than "This header...", I think (both here
> and in hmtx.cc).
Done. Thanks for correcting my poor English.

> We could also check that the low word of file->hhea->version is zero here
> (similar to the check in ots_vhea_parse, except AND with 0xFFFF).
I removed the vhea->version check rather than adding a hhea->version check because we can read the table safely even if we don't know the minor version per <http://www.microsoft.com/typography/otspec/otff.htm> (see "Version Numbers" section). Sorry I didn't notice earlier.

(In reply to comment #14)
> Also, would you be happy to submit this to Google as a suggested enhancement to
> the upstream OTS code?
If we submit this to upstream, we need to develop a GSUB validator. Because if vhea/vmtx exists and GSUB doesn't exist, AddFontMemResourceEx failes to load the OTF with CFF outline. This is the reason why ots_(vhea|vmtx)_should_serialise checks file->preserve_otl.

Comment 16

[Masatoshi Kimura [:emk]]

FYI: I've found a documentation from Adobe.
http://www.adobe.com/devnet/opentype/afdko/topic_tables_win.html
It is consistent with my observation.
> GSUB
> ATM looks for a GSUB table in every OpenType font to determine if vertical
> mode is supported (see description below). (snip) If the FeatureList does
> not contain a 'vrt2' feature, the rest of the GSUB table is ignored.
> vhea
> This table is required for fonts that support vertical mode. ATM uses the
> following fields from the vhea table.
>    * version (must be >= 1.0 and < 2.0)
> vmtx
> This table is required for fonts that support vertical mode.
> vertical mode
> ATM reports a second "face" to the system for fonts that support vertical
> mode:

Comment 17

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/projects/cedar/rev/60bfc795e935

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/60bfc795e935