575695 - Firefox trunk no longer kerning text that 3.6 does.

Status: RESOLVED FIXED

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

Description

[Robert Sayre]

See the URL field. At least on Mac OS 10.5 with Minefield, the "AVAST" text is no longer kerned for me. We could also probably use some test coverage if this regression slipped through.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

WFM on trunk.

Comment 2

[Jonathan Kew [:jfkthame]]

It depends on your default sans-serif font - if you have it set to Helvetica, for example, the kerning is lost, but if you have changed it to something like DejaVu Sans then it still works.

How about the "traffic" and "floret" text? I predict that you're not seeing ligatures in those, unless you reset gfx.font_rendering.harfbuzz.level to zero.

This is because harfbuzz only implements OpenType layout, it does not (currently) support the AAT layout tables used in many of Apple's system fonts. So yes, there's expected to be a regression with those fonts.

We probably want to make the Mac font implementation detect fonts that only support AAT and not OT layout, and continue using Core Text with those, but it's nice to get more testing coverage for the harfbuzz back-end for now.

Comment 3

[Robert Sayre]

(In reply to comment #2)
> 
> We probably want to make the Mac font implementation detect fonts that only
> support AAT and not OT layout, and continue using Core Text with those

Yep.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1 - force Mac font back-end to use Core Text for AAT fonts

This checks for fonts that include AAT layout tables (mort or morx) and do NOT have OpenType layout tables (GSUB, GPOS), and forces the use of Core Text for these fonts regardless of the harfbuzz.level setting.

Comment 5

[Robert Sayre]

Comment on attachment 455161 [details] [diff] [review]
patch, v1 - force Mac font back-end to use Core Text for AAT fonts

I don't see a test here. What's the plan for testing this change?

Comment 6

[John Daggett (:jtd)]

Attachment: testcase, check for kerning

Testcase to check kerning on all local fonts.  Needs local privs, so to use first download, then open locally and click 'Allow' when the dialog appears.  The test runs through all fonts available locally, then tags those that are kerning (based on the width difference in one of the two lines of common kerning pairs).

Comment 7

[John Daggett (:jtd)]

So isn't the real bug here that the harfbuzz path isn't handling 'kern' table data?  I don't quite understand the logic of checking for the mort/morx table.

For example, I have a bunch of mplus fonts installed, they show kerning on 3.6 but not under harfbuzz.  They lack mort/morx tables.

http://mplus-fonts.sourceforge.jp/

Comment 8

[John Daggett (:jtd)]

Same for a local install of Gentium, has a 'kern' table but no mort/morx table.  Kerns fine on 3.6 but not using harfbuzz.

Comment 10

[Jonathan Kew [:jfkthame]]

So there are actually two issues to be fixed:
(a) The harfbuzz path does not support legacy truetype 'kern' tables, because the relevant callback is not implemented in gfxHarfBuzzShaper. I'll work up a patch for this and post to this bug.
(b) The harfbuzz path does not support AAT tables (mort/morx), which means that ligatures, etc, will break in AAT-only fonts; we should fix that by using the Core Text path in those cases. I've filed Bug 577380 for this aspect.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - support old-style truetype kerning in the harfbuzz shaper

This implements the callback needed for harfbuzz to apply the old 'kern' table. It doesn't support the more exotic forms of 'kern' subtable that Apple defined for use with AAT, but examples of these are very rare, and as harfbuzz doesn't support AAT shaping anyway I don't think that's a high priority.

The results of kerning in RTL text are not quite correct with this patch alone because of how glyph advance and xoffset are handled, but that will be fixed by a following patch to modify how harfbuzz applies the kerning.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - modify how kerning is applied in RTL text, to simplify glyph positioning

With this modification to how kerning values are applied, we get correct results in RTL as well.

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - tweak reftest to avoid subpixel discrepancy

Fix for reftest that fails because of slight discrepancies in glyph positioning.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: add a reftest specifically for the legacy 'kern' table

This uses a copy of one of the M+ fonts where the OpenType/AAT tables have been stripped out, to ensure that we are testing support for the legacy 'kern' table regardless of the state of OpenType support in the build.

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - support truetype kerning in the harfbuzz shaper backend

Updated the processing of glyph positions in gfxHarfBuzzShaper::SetGlyphsFromRun to properly use the x_offset and advance information from harfbuzz.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.

Comment 17

[John Daggett (:jtd)]

Testing with the attached kerning testcase on 0SX 10.5 there are a few fonts that still lose kerning from 3.6 to trunk w/ patch.

Apple kern table format 1, subtable format 2 (not supported by new code):

  Skia
  Apple Chancery

Kern under 3.6 but not with patch:

  Arial Black
  Trebuchet MS
  Verdana

If Skia is the only one not kerning, meh, but the last three probably need to have correctly kerned text.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2.1 - support truetype kern table in gfxHarfBuzzShaper - refreshed

Refreshed patch for current trunk. (Apple subtable format 2 is still not implemented, but note that Skia now kerns anyway because bug 577380 means we now use the Core Text path again for it.)

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - patch harfbuzz to try fallback kerning if no 'kern' feature in GPOS

This modifies the harfbuzz shaper to use fallback positioning (i.e. the old truetype kern table) if no 'kern' feature was present in the GPOS table; resolves the problem of MS fonts like Verdana and Trebuchet not kerning.

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: patch, v3 - also support apple format 2 and 3 kern subtables

This adds support for Apple subtable format 2, as used in Apple Chancery, and format 3 as used in Skia.

(Tested by hacking gfxMacFont.cpp to use harfbuzz for these fonts even though they normally require AAT. These subtable formats are pretty rare "in the wild".)

Comment 21

[John Daggett (:jtd)]

Comment on attachment 460271 [details] [diff] [review]
patch, v3 - also support apple format 2 and 3 kern subtables

+            PRUint16 coverage = st0->coverage;
+            if (!(coverage & KERN0_COVERAGE_HORIZONTAL)) {
+                // we only care about horizontal kerning (for now)
+                continue;
+            }
+            if (coverage & KERN0_COVERAGE_CROSS_STREAM) {
+                // we don't support cross-stream kerning
+                continue;
+            }
+            if (coverage & KERN0_COVERAGE_RESERVED) {
+                // reserved bits should be zero; ignore the subtable if not
+                continue;
+            }

No real reason for three if statements here, combine.

+            switch (coverage >> 8) {

Maybe a local variable to document the code a bit better?

  format = (coverage >> 8)

+                if (coverage & KERN1_COVERAGE_VERTICAL) {
+                    // we only care about horizontal kerning (for now)
+                    continue;
+                }
+                if (coverage & KERN1_COVERAGE_CROSS_STREAM) {
+                    // we don't support cross-stream kerning
+                    continue;
+                }
+                if (coverage & KERN1_COVERAGE_VARIATION) {
+                    // we don't support variations
+                    continue;
+                }
+                if (coverage & KERN1_COVERAGE_RESERVED) {
+                    // reserved bits should be zero; ignore the subtable if not
+                    continue;
+                }

Same here, combine.

+                    NS_WARNING("unknown kern subtable format");
+                    break;

Maybe put in debug-only code to dump the font name and the format?

Comment 22

[John Daggett (:jtd)]

Comment on attachment 459759 [details] [diff] [review]
part 2 - patch harfbuzz to try fallback kerning if no 'kern' feature in GPOS

+  // compile lookup list; return true if kerning handled here,
+  // false if fallback kerning should be attempted
+  bool compile (hb_face_t *face,

Maybe compile_lookup would be a better name?  Just 'compile' is a bit generic.

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to comment #21)

> +            PRUint16 coverage = st0->coverage;
> +            if (!(coverage & KERN0_COVERAGE_HORIZONTAL)) {
> +                // we only care about horizontal kerning (for now)
> +                continue;
> +            }
> +            if (coverage & KERN0_COVERAGE_CROSS_STREAM) {
> +                // we don't support cross-stream kerning
> +                continue;
> +            }
> +            if (coverage & KERN0_COVERAGE_RESERVED) {
> +                // reserved bits should be zero; ignore the subtable if not
> +                continue;
> +            }
> 
> No real reason for three if statements here, combine.

I combined the 2nd and 3rd; left the first one separate as I think the code is much clearer that way than writing a bit-masking expression that tests both positive and negative flags at once. The compiler should be able to optimize this stuff anyhow.

Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/bf9b760b359b [reftest]
http://hg.mozilla.org/mozilla-central/rev/3dc9955d7e0b [gfxHarfBuzzShaper]
http://hg.mozilla.org/mozilla-central/rev/c64b4e0368b5 [HB kerning fallback]