Closed Bug 476724 Opened 11 years ago Closed 11 years ago

gfxAtsuiFonts needs to recompute mUnderlineOffset after rebuilding font set

Categories

(Core :: Graphics, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: jtd)

References

Details

(Keywords: fixed1.9.1)

Attachments

(3 files, 2 obsolete files)

Summary says it all.  Patch coming up.  This was causing random reftest failures over here depending on the exact timing of user fonts loading.
OS/2 and Pango seem to not set mUnderlineOffset at all, for what it's worth.
Attachment #360377 - Flags: superreview?(roc)
Attachment #360377 - Flags: review?(roc)
Do we want this on 1.9.1, by the way?
Attachment #360377 - Flags: superreview?(roc)
Attachment #360377 - Flags: superreview+
Attachment #360377 - Flags: review?(roc)
Attachment #360377 - Flags: review+
Flags: blocking1.9.1?
Attachment #360377 - Flags: approval1.9.1?
Pushed http://hg.mozilla.org/mozilla-central/rev/8a79415ac146
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
i backed out this to try solving a persistent orange on OS X unit test boxes, where reftests font-face tests were failing
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/download-2.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/src-list-2-big-otf.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/multiple-in-family-1.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/multiple-in-family-1b.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/moz2_slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/sheet-set-switch-1.html |

REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/download-2.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/src-list-format-1.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/src-list-format-2.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/src-list-format-4.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/src-list-format-5.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/multiple-in-family-1.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/multiple-in-family-1b.html | 
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-central-macosx-unittest/build/layout/reftests/font-face/sheet-set-switch-1.html |
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout changesets:
http://hg.mozilla.org/mozilla-central/rev/5b93088b7d66
http://hg.mozilla.org/mozilla-central/rev/729448695308 (merge)

OSX unit boxes have turned green after the backout.
Gah.  Those are exactly the tests I'd had failing without this patch.

I guess I'll try to do some more digging, but the code as written is obviously wrong, and I have no idea how it manages to pass on tinderbox...
What's the effect of this bug, and is it a regression from Firefox 3? Just trying to evaluate blocking status, here ...
The main effect is that when using downloadable fonts (a new feature since Firefox 3) the layout depends on the exact timing of the load compared to the page rendering.  In particular, it's possible to end up with incorrect underline offsets, which leads to 1px errors in layout and painting, possibly leading to painting "turds".
OK, as far as I can tell the issue is just that the various mark* files have different underline offsets or something (which is what leads to the reftest failures).  That plus the fact that we stop at the first "bad" underline offset (so that things depend on the font ordering in the font group) is the only way I can explain the test results I'm getting here.

John, would you mind checking that out?
Assignee: bzbarsky → jdaggett
Summary: [FIX]gfxAtsuiFonts needs to recompute mUnderlineOffset after rebuilding font set → gfxAtsuiFonts needs to recompute mUnderlineOffset after rebuilding font set
On the other hand, ftxdumperfuser claims that markA.ttf and markB.ttf have identical underline metrics ('post' table) and general font metrics ('os/2' table).

So I really have no idea what's going on here.  The current code is clearly wrong, though...
Oh, and _are_ the mark* fonts in fact triggering the "bad underline font" codepaths?  It seems like they are, to me...
And in particular, the download-2 test is failing.  This test just compares the markA.ttf font to the markB.ttf font, right?
It sucks, but I don't think I'd call this a blocker -- it can lead to 1px oddness and turds, but given that it's fairly limited in when it shows up, I think this should be in the "would really like a patch, but not blocker" bucket
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P1
Note that the weirdness here is definitely blocking interruptible reflow passing reftests, no matter what we do on 1.9.1.
OK, so at least one issue is that gfxQuartzFontCache::MakePlatformFont does |new MacOSFontEntry| with the font-id version of the constructor, which uses the no-argument gfxFontEntry constructor, which doesn't initialize mIsBadUnderlineFont.

So we're kinda reading random data.
Blocks: ireflow
John, could you look over those constructor changes?  I wasn't sure what values to pick for some of those booleans...
Attachment #360377 - Attachment is obsolete: true
Attachment #360644 - Flags: superreview?(roc)
Attachment #360644 - Flags: review?(jdaggett)
Attachment #360377 - Flags: approval1.9.1?
Comment on attachment 360644 [details] [diff] [review]
Same as the other patch, but actually initializing all the members

looks OK, but there must be another bug here where the fontID version of the constructor is not initializing things properly.
Attachment #360644 - Flags: superreview?(roc) → superreview+
Neither of the MacOSFontEntry constructor really initializes much of the parent struct.... they were both leaving all sorts of stuff (e.g. mUnicodeFont, mSymbolFont, mTrueType, mIsType1) uninitialized.

That's where I'd like John's review.
With this patch, one of the reftests fails:

layout/reftests/font-face/sheet-set-switch-1.html

Without the patch, the failure does not occur.  The only real difference is that with the patch, the mUnderlineOffset can change after re-initializing the font list, since the underline offset of the font group is based on the first font in the list.  I'm going to dig into this and figure what exactly is causing this difference.

The screenshot shows an enlarged version of the test on the left, the reference on the right, the reference is one pixel higher than the test.  Probably the same thing seen on Linux, bug 468217.

The patch looks good.  Some of those flags should have been cleaned out or are only used on Windows.  Looking over each of these there's no uninitialized use currently.  Same for the FamilyName and mFamily member of MacOSFontEntry, that code is unused and should be cleaned out.  I'll set up a follow-on patch to do the cleanup once this patch lands.
Comment on attachment 360644 [details] [diff] [review]
Same as the other patch, but actually initializing all the members

So I think the reftest is the problem here, it ends up comparing text
runs which contain fonts with differing metrics, specifically the
underline offset is different:

testcase:

  body { font-family: "MarkA", "Mark2B", "MarkC"; }

  <body>ABC</body>

reference:

  body { font-family: "MarkXMark2Y"; }

  <body>AXY</body>
  
Logging font matching and metrics calculations for each of the fonts
involved here, the testcase vs. the reference produces this log:

  http://pastebin.mozilla.org/616047

The key here is that the underlineOffset for MarkA, MarkC and
MarkXMark2Y is -1 but for Mark2B it's 1.  So a comparison like the one
above is a bit of an apples vs. oranges comparison.  The glyphs used
should match but the line metrics may not.
Attachment #360644 - Flags: review?(jdaggett) → review+
So as part of the checkin, I think we should disable this reftest completely until it can be reworked.
Hmmm, even changing the reference test to not use MarkXMark2Y, I see the same problem:

reference:

  body { font-family: "Mark2B", "MarkC"; }

  <body>ABC</body>
Trim unused fields and move Windows-specific fields appropriately.
Pushed my patch as http://hg.mozilla.org/mozilla-central/rev/c84b63d503df

Leaving open so John can decide what he wants to do with his cleanup patch.
Oh, and it stuck this time: green unit tests.
No longer blocks: ireflow
Blocks: ireflow
Only use mUnicodeFont and mSymbolFont under Windows, not the WinCE/FT2 flavor.
Attachment #360683 - Attachment is obsolete: true
Comment on attachment 361913 [details] [diff] [review]
patch, follow-on font entry cleanup, v.0.2

cleaning out unused flags and member vars
Attachment #361913 - Flags: review?(vladimir)
Comment on attachment 361913 [details] [diff] [review]
patch, follow-on font entry cleanup, v.0.2

Hmm, mUnicodeFont/mSymbolFont get used in gfxWindowsFonts.h at least -- http://mxr.mozilla.org/mozilla-central/search?string=mUnicodeFont

So maybe move it into the windows-specific FontEntry?
Attachment #361913 - Flags: review?(vladimir) → review-
Vlad, can you take another look at this?  The patch is only trimming mUnicodeFont/mSymbolFont out of gfxFont.h and gfxFT2Fonts.h, and moving them into gfxWindowsFont.h, since both of these are specific to Windows-only, non-FT2 code.
Comment on attachment 361913 [details] [diff] [review]
patch, follow-on font entry cleanup, v.0.2

Erm.  Somehow I completely missed over the hunk that added them to gfxWindowsFonts.h, sorry about that!
Attachment #361913 - Flags: review- → review+
pushed follow-on cleanup patch
http://hg.mozilla.org/mozilla-central/rev/4c66a7e1fdf2
pushed to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d5c097c5693a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e9c9f33f463
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.