Firefox trunk no longer kerning text that 3.6 does.

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Robert Sayre, Assigned: jfkthame)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(URL)

Attachments

(4 attachments, 6 obsolete attachments)

3.43 KB, text/html
Details
369.58 KB, patch
jtd
: review+
Details | Diff | Splinter Review
6.14 KB, patch
jtd
: review+
Details | Diff | Splinter Review
26.71 KB, patch
jtd
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
WFM on trunk.
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

8 years ago
Created attachment 455161 [details] [diff] [review]
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.
Assignee: nobody → jfkthame
Attachment #455161 - Flags: review?(jdaggett)
(Reporter)

Comment 5

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

8 years ago
Created attachment 455417 [details]
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

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

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

Updated

8 years ago
Duplicate of this bug: 576414

Updated

8 years ago
Attachment #455161 - Flags: review?(jdaggett)
blocking2.0: ? → beta2+
(Assignee)

Comment 10

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

Comment 11

8 years ago
Created attachment 457804 [details] [diff] [review]
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.
Attachment #455161 - Attachment is obsolete: true
Attachment #457804 - Flags: review?(jdaggett)
(Assignee)

Comment 12

8 years ago
Created attachment 457805 [details] [diff] [review]
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.
Attachment #457805 - Flags: review?(jdaggett)
(Assignee)

Comment 13

8 years ago
Created attachment 457807 [details] [diff] [review]
part 3 - tweak reftest to avoid subpixel discrepancy

Fix for reftest that fails because of slight discrepancies in glyph positioning.
Attachment #457807 - Flags: review?(jdaggett)
(Assignee)

Comment 14

8 years ago
Created attachment 457812 [details] [diff] [review]
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.
Attachment #457812 - Flags: review?(jdaggett)
(Assignee)

Comment 15

8 years ago
Created attachment 458410 [details] [diff] [review]
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.
Attachment #457804 - Attachment is obsolete: true
Attachment #457805 - Attachment is obsolete: true
Attachment #457807 - Attachment is obsolete: true
Attachment #458410 - Flags: review?(jdaggett)
Attachment #457804 - Flags: review?(jdaggett)
Attachment #457805 - Flags: review?(jdaggett)
Attachment #457807 - Flags: review?(jdaggett)
(Assignee)

Updated

8 years ago
Attachment #457812 - Attachment description: part 4 - add a reftest specifically for the legacy 'kern' table → add a reftest specifically for the legacy 'kern' table
Moving to betaN - requires beta coverage, no specific beta milestone targetted at this time.
blocking2.0: beta2+ → betaN+
(Assignee)

Updated

8 years ago
Blocks: 580863

Comment 17

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

Comment 18

8 years ago
Created attachment 459758 [details] [diff] [review]
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.)
Attachment #458410 - Attachment is obsolete: true
Attachment #459758 - Flags: review?(jdaggett)
Attachment #458410 - Flags: review?(jdaggett)
(Assignee)

Comment 19

8 years ago
Created attachment 459759 [details] [diff] [review]
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.
Attachment #459759 - Flags: review?(jdaggett)
(Assignee)

Comment 20

8 years ago
Created attachment 460271 [details] [diff] [review]
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".)
Attachment #459758 - Attachment is obsolete: true
Attachment #460271 - Flags: review?(jdaggett)
Attachment #459758 - Flags: review?(jdaggett)

Updated

8 years ago
Attachment #457812 - Flags: review?(jdaggett) → review+

Comment 21

8 years ago
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?
Attachment #460271 - Flags: review?(jdaggett) → review+

Comment 22

8 years ago
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.
Attachment #459759 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 23

8 years ago
(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]
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
No longer blocks: 580863
Depends on: 605043
Depends on: 608876
Depends on: 661157
You need to log in before you can comment on or make changes to this bug.