Closed
Bug 476724
Opened 15 years ago
Closed 15 years ago
gfxAtsuiFonts needs to recompute mUnderlineOffset after rebuilding font set
Categories
(Core :: Graphics, defect, P1)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: jtd)
References
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 2 obsolete files)
9.25 KB,
patch
|
jtd
:
review+
roc
:
superreview+
vlad
:
approval1.9.1+
|
Details | Diff | Splinter Review |
19.39 KB,
image/png
|
Details | |
10.90 KB,
patch
|
vlad
:
review+
vlad
:
approval1.9.1+
|
Details | Diff | Splinter Review |
Summary says it all. Patch coming up. This was causing random reftest failures over here depending on the exact timing of user fonts loading.
Reporter | ||
Comment 1•15 years ago
|
||
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)
Reporter | ||
Comment 2•15 years ago
|
||
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+
I think we do.
Reporter | ||
Updated•15 years ago
|
Flags: blocking1.9.1?
Reporter | ||
Updated•15 years ago
|
Attachment #360377 -
Flags: approval1.9.1?
Reporter | ||
Comment 4•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/8a79415ac146
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 5•15 years ago
|
||
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 → ---
Comment 6•15 years ago
|
||
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.
Reporter | ||
Comment 7•15 years ago
|
||
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...
Comment 8•15 years ago
|
||
What's the effect of this bug, and is it a regression from Firefox 3? Just trying to evaluate blocking status, here ...
Reporter | ||
Comment 9•15 years ago
|
||
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".
Reporter | ||
Comment 10•15 years ago
|
||
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
Reporter | ||
Comment 11•15 years ago
|
||
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...
Reporter | ||
Comment 12•15 years ago
|
||
Oh, and _are_ the mark* fonts in fact triggering the "bad underline font" codepaths? It seems like they are, to me...
Reporter | ||
Comment 13•15 years ago
|
||
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
Reporter | ||
Comment 15•15 years ago
|
||
Note that the weirdness here is definitely blocking interruptible reflow passing reftests, no matter what we do on 1.9.1.
Reporter | ||
Comment 16•15 years ago
|
||
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
Reporter | ||
Comment 17•15 years ago
|
||
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+
Reporter | ||
Comment 19•15 years ago
|
||
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.
Assignee | ||
Comment 20•15 years ago
|
||
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.
Assignee | ||
Comment 21•15 years ago
|
||
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+
Assignee | ||
Comment 22•15 years ago
|
||
So as part of the checkin, I think we should disable this reftest completely until it can be reworked.
Assignee | ||
Comment 23•15 years ago
|
||
Hmmm, even changing the reference test to not use MarkXMark2Y, I see the same problem: reference: body { font-family: "Mark2B", "MarkC"; } <body>ABC</body>
Assignee | ||
Comment 24•15 years ago
|
||
Trim unused fields and move Windows-specific fields appropriately.
Reporter | ||
Comment 25•15 years ago
|
||
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.
Reporter | ||
Comment 26•15 years ago
|
||
Oh, and it stuck this time: green unit tests.
Assignee | ||
Comment 27•15 years ago
|
||
Only use mUnicodeFont and mSymbolFont under Windows, not the WinCE/FT2 flavor.
Attachment #360683 -
Attachment is obsolete: true
Assignee | ||
Comment 28•15 years ago
|
||
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-
Assignee | ||
Comment 30•15 years ago
|
||
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+
Assignee | ||
Comment 32•15 years ago
|
||
pushed follow-on cleanup patch http://hg.mozilla.org/mozilla-central/rev/4c66a7e1fdf2
Attachment #360644 -
Flags: approval1.9.1+
Attachment #361913 -
Flags: approval1.9.1+
Assignee | ||
Comment 33•15 years ago
|
||
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: 15 years ago → 15 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•