Closed Bug 580863 Opened 14 years ago Closed 14 years ago

OTF font positioning renderings text incorrectly

Categories

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

x86
Windows 7
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: kpdecker, Assigned: jfkthame)

References

()

Details

Attachments

(7 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

Under OSX FF 4.0b1 the font rendering with the given font causes the characters to overlap and generally be unreadable.

Reproducible: Always

Steps to Reproduce:
1. View http://incaseofstairs.com/tests/font-face-render/
Actual Results:  
The text is rendered incorrectly.

Expected Results:  
The text is readable and utilizes the user defined font.

I have tested this on the Mozilla 4.0b1 builds for Win and Ubuntu 64bit and they both appear to render correctly. The 3.6 branch appears to render correctly as well.
It renders correctly with 
gfx.font_rendering.harfbuzz.level set to '0'

(the default on Linux and Windows, iirc)
Blocks: 569531
Status: UNCONFIRMED → NEW
Ever confirmed: true
Although this isn't an actual dup of bug 575695, it will be fixed by the same patch (because it improves the way we process positioning data from harfbuzz).
Depends on: 575695
(In reply to comment #2)
> Although this isn't an actual dup of bug 575695, it will be fixed by the same
> patch (because it improves the way we process positioning data from harfbuzz).

Hmm, this was not actually fixed by the patch in bug 575695; I must have messed up my prefs settings or something when I was testing previously.
No longer depends on: 575695
So the issue here is that CGFontGetUnitsPerEm appears to always return 1000 for OpenType/CFF fonts, even if the 'head' table says otherwise. This means that when we read and scale the glyph advances from the 'hmtx' table, we end up getting incorrect metrics. In this particular case, unitsPerEm is supposed to be 512, so when CoreGraphics "normalizes" this to 1000 it means that we end up computing glyph advances as roughly half of what they should be.
This fixes the problem by reading the 'head' table directly in InitMetrics, only falling back to CGFontGetUnitsPerEm if this fails (which should only happen for non-OpenType fonts such as bitmaps or old Type 1 fonts).
Assignee: nobody → jfkthame
Attachment #460890 - Flags: review?(jdaggett)
I think this should block, as the unreadable rendering shown by the page in the URL field (above) is a pretty bad regression.

Most current OpenType/CFF fonts happen to use a 1000-unit em-square and therefore do not suffer from this problem, but (in addition to the example here) I am aware of some new fonts from Adobe and other major foundries where this will be an issue.
blocking2.0: --- → ?
Attachment #460890 - Flags: review?(jdaggett) → review+
http://hg.mozilla.org/mozilla-central/rev/bbefb7bcb41e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reopening this, as the issue is not completely fixed: comparing Minefield rendering with Safari, we can see that although we're now getting metrics correct for the printing glyphs, we're using incorrect word spacing for CFF fonts with a non-1000 unit em square.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note the excessively large word spacing in the Minefield display, compared to Safari's version. (Ignore the weight difference; the test page uses a <h1> tag, so we are applying synthetic bold - which is correct behavior.)
For comparison, this screenshot shows Firefox 3.6 and Safari rendering; here, FF has correct word spacing.
The difference between the testcase rendering in Safari/FF3.6/Minefield has to do with synthetic bolding issues, *not* with metric handling.  Without synthetic bolding, existing trunk code renders identically to Safari/FF3.6.

Trunk code does not adjust the advance when synthetic bolding is used.  Looks like bug 593564 is not completely fixed.  

If there's still a bug here, a new testcase demonstrating the problem is needed.
Er, one more time with latest trunk...

Looks like the fix for synthetic bolding is fine.  Difference with Safari is incorrect handling of synthetic bold in Safari.
Attachment #478150 - Attachment is obsolete: true
John, I think you must be testing under OS X 10.5.x, which doesn't have the CGFont "feature" of rescaling metrics for CFF fonts to a standard 1000-unit em square. Therefore, on 10.5 this issue doesn't occur; but the attached screenshot from 10.6 clearly shows the incorrect word spacing in Minefield (nightly from 2010-09-23) compared to Safari.
Comment on attachment 477478 [details] [diff] [review]
patch, part 2 - use CoreGraphics idea of unitsPerEm when scaling values from CG APIs in InitMetrics

Ok, testing on 10.6 I see the problem.

+    // For CFF fonts, when scaling values read from CGFont* APIs, we need to
+    // use CG's idea of unitsPerEm, which may differ from the "true" value in
+    // the head table of the font (see bug 580863)
+    gfxFloat cgConvFactor;
+    CFDataRef cffData =
+        ::CGFontCopyTableForTag(mCGFont, TRUETYPE_TAG('C','F','F',' '));
+    if (cffData) {
+        cgConvFactor = mAdjustedSize / ::CGFontGetUnitsPerEm(mCGFont);
+        ::CFRelease(cffData);
+    } else {
+        cgConvFactor = mFUnitsConvFactor;
+    }

While it might be correct, using CGFontCopyTableForTag to check whether a table exists or not seems completely wrong from an efficiency perspective.  CFF tables can be large (e.g. the CFF table of Hiragino Mincho Pro is 9MB).  Plus you're doing this on the gfxFont subclass which means this code will get executed lots and lots of times for every size instantiation.

A better way would be to simply add a method "IsCFF" to the MacOSFontEntry that initializes a boolean on the first access.  When initializing it would only need to call ATSFontGetTable with a zero buffer size to establish whether the table exists or not, you don't actually need to load the table.
Attachment #477478 - Flags: review?(jdaggett) → review-
(In reply to comment #15)

> While it might be correct, using CGFontCopyTableForTag to check whether a table
> exists or not seems completely wrong from an efficiency perspective.  CFF
> tables can be large (e.g. the CFF table of Hiragino Mincho Pro is 9MB).  Plus
> you're doing this on the gfxFont subclass which means this code will get
> executed lots and lots of times for every size instantiation.
> 
> A better way would be to simply add a method "IsCFF" to the MacOSFontEntry that
> initializes a boolean on the first access.  When initializing it would only
> need to call ATSFontGetTable with a zero buffer size to establish whether the
> table exists or not, you don't actually need to load the table.

I believe CGFontCopyTableForTag is not actually as expensive as it sounds: it does not copy the actual table data, it returns a reference to a (shared) CFData object, so all it's really doing is looking up the object and bumping the Core Foundation retain count. (On my laptop, checking for the CFF table in this way seems to typically take around 30-40 microseconds in the case when it is actually present; less than 10 microseconds when it is not.)

You're right, though, that it would make sense to check this in the font entry rather than the gfxFont, so that it's not repeated on each instantiation. I'll update along those lines.

(Also note that although we still have ATSFontGetTable in the Mac font entry class, we're trying to get away from the ATS APIs, as they have such a history of flakiness, and are considered legacy, non-recommended APIs by Apple. But that's a separate matter, not for this bug.)
This now uses an IsCFF() method in MacOSFontEntry.
Attachment #477478 - Attachment is obsolete: true
Attachment #478801 - Flags: review?(jdaggett)
Comment on attachment 478801 [details] [diff] [review]
patch, part 2 - fix InitMetrics for CFF fonts on 10.6 - updated for review comments

Looks good.  Need a simple reftest for this.  Also, I'm assuming this doesn't impact behavior on 10.5.
Attachment #478801 - Flags: review?(jdaggett) → review+
Whiteboard: [needs reftest and landing]
(In reply to comment #18)
> Looks good.  Need a simple reftest for this. 

It's a bit tricky to reftest this, as it requires use of a CFF font with a non-1000 em square, which is not very common. The original reported testcase is using a copyrighted font from a commercial vendor, so not really suitable to put into our tree.

To allow us to test it, I used FontForge to convert a copy of DejaVu Sans Mono from TrueType to CFF format, retaining the 2048-unit em square. With this, we can use a test based on the input-text-size reftest, which relies on the font's character width to size the input field and thus control how much of the text is visible.

> Also, I'm assuming this doesn't
> impact behavior on 10.5.

Yes, there's no impact on 10.5.
Attached patch reftestSplinter Review
This includes a CFF version of DejaVu Sans Mono with 2048-unit em square, and then uses the sizing of an input field to test whether the space width is correct. Without the patch here, the font's spaceWidth becomes approximately half of what it should be, and as a result an extra glyph becomes visible within the field.
Attachment #479194 - Flags: review?(jdaggett)
One more thing: this broke font-size-adjust slightly (shown by the font-size-adjust-02 reftest). It needs to recalculate the scaling factor after applying the size adjustment in InitMetrics. Fixed in this version of the patch.
Attachment #478801 - Attachment is obsolete: true
Comment on attachment 479194 [details] [diff] [review]
reftest

Clever.
Attachment #479194 - Flags: review?(jdaggett) → review+
OS: Mac OS X → Windows 7
Whiteboard: [needs reftest and landing] → [needs landing] (part 2 & reftest)
http://hg.mozilla.org/mozilla-central/rev/5a676445a050 (patch, part 2)
http://hg.mozilla.org/mozilla-central/rev/e9a5a378c850 (reftest)
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] (part 2 & reftest)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: