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

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

Trunk
mozilla5
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

8 years ago
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.
Assignee

Comment 1

8 years ago
Sorry the subject was wrong.
Summary: Cannot load Japanese OTF/WOFF with CFF outline as Web fonts when DirectWrite is enabled → Cannot load Japanese OTF/WOFF with CFF outline as Web fonts when DirectWrite is disabled
Assignee

Comment 2

8 years ago
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.
Assignee

Comment 3

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

Updated

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

Comment 5

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

Comment 7

8 years ago
Posted patch patch (obsolete) — Splinter Review
Unfortunately I couldn't find a free OTF with CFF outline which is usable for reftest.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #516238 - Flags: review?(jfkthame)
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?
Attachment #516244 - Flags: feedback?(VYV03354)
Assignee

Comment 9

8 years ago
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.
Attachment #516244 - Flags: feedback?(VYV03354) → feedback+
Assignee

Comment 10

8 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #516238 - Attachment is obsolete: true
Attachment #516238 - Flags: review?(jfkthame)
Attachment #516616 - Flags: review?(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.
Assignee

Comment 12

8 years ago
Posted patch patch v3 (obsolete) — Splinter Review
Changes from the previous patch:
* resolved comments.
* make vhea->version validation more strict.
Attachment #516616 - Attachment is obsolete: true
Attachment #516616 - Flags: review?(jfkthame)
Attachment #517393 - Flags: review?(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).
Attachment #517393 - Flags: review?(jfkthame) → review+
Also, would you be happy to submit this to Google as a suggested enhancement to the upstream OTS code?
Assignee

Comment 15

8 years ago
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.
Attachment #517393 - Attachment is obsolete: true
Attachment #517417 - Flags: review+
Assignee

Updated

8 years ago
Blocks: post2.0
Assignee

Updated

8 years ago
No longer blocks: post2.0
Depends on: post2.0
Assignee

Comment 16

8 years ago
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:
Assignee

Updated

8 years ago
Keywords: checkin-needed

Comment 18

8 years ago
http://hg.mozilla.org/mozilla-central/rev/60bfc795e935
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
Assignee

Updated

8 years ago
Blocks: 643460
Assignee

Updated

8 years ago
No longer blocks: 643460
You need to log in before you can comment on or make changes to this bug.