550163 - text/font-related reftest failures on Windows 7

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Running a trunk build on Windows 7, without enabling the new dwrite/d2d features, I currently see 35 unexpected failures (and one unexpected pass), as shown in the attached log.

Most of these are spurious failures caused by subpixel-AA drawing/clipping issues, and might ultimately be resolved when we handle visual overflow areas better. Some are also related to Windows font updates (e.g., fonts that have had kerning added in Windows 7).

Where the failures are not related to the actual purpose of the test, but just artifacts of the updated font rendering, we should adjust them to make them more robust if possible.

Cleaning up these issues will make it easier to isolate and analyze the changes that result when we switch on dwrite and d2d rendering.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: reftest.log for trunk build on windows 7

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: part 1 - resolve Win7 failures in bidi/mixedChartype

The Arabic text with LTR override (test 2) fails because Uniscribe does not shape overridden Arabic correctly; nothing we can do about that. Marking it random as it actually passes on current tinderboxes, but only because of inadequate font support, I think.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: part 2 - resolve Win7 failure in bidi-004 reftest

The Hebrew text is problematic because there's no guarantee that a font will position separate diacritics identically to how they're placed in the precomposed "presentation form" characters.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: part 3 - avoid Win7 subpixel failure in bidi/with-first-letter reftest

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: part 4 - add padding to avoid Win7 subpixel failure in reftest 296361

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: part 5 - use sans-serif and padding to avoid Win7 subpixel failure in reftest 350506

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: part 6 - use sans-serif to avoid Win7 subpixel failure in reftest 351641

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: part 7 - use sans-serif to avoid Win7 subpixel failure in reftest 362901

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: part 8 - use @font-face to ensure we have a font with the fi ligature in reftest 385569

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: part 9 - use sans-serif to avoid Win7 subpixel failure in reftest 389074

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: part 10 - use sans-serif to avoid Win7 subpixel failure in reftest 413292

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: part 11 - use sans-serif to avoid Win7 subpixel failure in reftest 474417

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: part 12 - revise text to avoid Win7 subpixel failure where 'tt' glyphs touch in reftest 495385

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: part 13 - use invisible text to avoid Win7 or Mac subpixel failure in reftest 503399

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: part 14 - use sans-serif to avoid Win7 subpixel failure in mathml/quotes reftest

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: part 15 - use sans-serif to avoid Win7 subpixel failure in reftest 394402

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: part 16 - use sans-serif to avoid Win7 subpixel failure in line-editing-1 reftest

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: part 17 - use sans-serif and revise text to avoid Win7 subpixel failure in capitalize reftests

The text change here is to avoid the word "Question", because even with the sans-serif font we get a problem due to *vertical* subpixel AA occurring at the tip of the stroke of the "Q".

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: part 18 - use bigger text to avoid Win7 subpixel failure in reftest 399636

I don't actually understand *why* this was failing with the original, small text, but using a bigger font makes the problem go away. (Hmm, maybe because it triggers our "quality" path?) We could raise a bug on the issue, though, if we want to investigate further.

Comment 20

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

Switching from serif to sans-serif makes sense in general, I think, but I'm not so keen on just changing the text or its size to avoid bugs. It smells like something that happens to work for the particular systems and fonts we have but won't necessarily be an improvement for others, might in fact be worse for others.

Are any of these other tests fixed by using the 'Ahem' font instead (without breaking the test)? Or 'monospace', which seems even less likely than 'sans-serif' to be affected by subpixel issues? Or can we use padding in some of them, which is really the best solution?

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to comment #20)
> Are any of these other tests fixed by using the 'Ahem' font instead (without
> breaking the test)? Or 'monospace', which seems even less likely than
> 'sans-serif' to be affected by subpixel issues? Or can we use padding in some
> of them, which is really the best solution?

We can't use Ahem to reftest text changes like capitalization (part 17), as it will make them undetectable in the rendered output! I suppose it would be ok in theory for fallback past symbol fonts (18), but in general I'm reluctant to use it where it will make it difficult to see what's happening when actually viewing the test files.

Monospace is not particularly "safer" than sans; it does avoid kerning, which is why I used it for some of the bidi tests where Windows applies kerning to "natural" text but not to direction-override text. But as far as subpixel AA is concerned, it is just as susceptible to problems. I suppose if we used a monospace font, AND changed the text to be entirely "narrow" glyphs such as "i" and "l", it might be ok - though typical "typewriter" fonts give these very long serifs so that even they almost fill the glyph cell.

It's true that changing the text to avoid subpixel issues is not guaranteed to be a universal solution. But by understanding where the problems are occurring, I think it's possible to choose test characters that are much less likely to cause problems. In Part 17 (capitalization), the problem is the stroke of the Q, which descends slightly below the baseline and gets vertical AA on the bottom; this gets clipped inconsistently because the test and reference do their drawing in different chunks. So choosing text that doesn't have a downward-projecting point like this improves our chances. (Properly handling ink overflow areas should eliminate issues like this, as we'll be able to pad our drawing area to include the AA pixels without the risk of affecting layout.)

In part 12, the problem occurs where we have a "tt" in the text, and one of the test lines draws this in two separate operations, breaking between the t's. Because the "arms" of the letters come very close together in typical fonts, their subpixel AA areas end up overlapping, and we get a slightly different result from combining the two separate drawing operations than ClearType gives us for a continuous string. This isn't really a bug, IMO, it's just a fact of life. We can't use padding to give extra space here, as we're comparing to a continuous string of text, so the simplest solution is to use text where the glyphs at the "break" don't have elements that project so close to their neighbors.

So I think the textual changes in parts 12 and 17 are reasonable, and unlikely to make things worse anywhere. The most questionable one, IMO, is the size change in part 18. There's nothing "wrong" with this as such - the size of the text is completely irrelevant to the test - but I don't understand the original failure, which does bother me.

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 430566 [details] [diff] [review]
part 2 - resolve Win7 failure in bidi-004 reftest

Simon, could you see if you think the changes to the bidi-004 reftests are ok. I've changed the literal characters to entities, to make it easier to tell what's actually present, but more significantly, I've also removed the ALEPH + PATAH pair from the test.

The underlying issue here is that with the available fonts on Windows 7 (at least), the result of ALEPH + PATAH (as separate characters) positions the PATAH glyph slightly differently from how it appears in the presentation form glyph for U+FB2E, and so our comparison fails. The same discrepancy can be seen if I put the sequence <ALEPH, PATAH> and the presentation form <ALEPH WITH PATAH> into WordPad; it's a font issue, not a Gecko error.

In general, reftesting by comparing the rendering of a unicode combining sequence and a precomposed character is as much a test of the fonts and the OS text system as it is a test of Gecko; but it happens that this test works OK with the default Windows fonts if we remove that particular element from it.

Comment 23

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

(In reply to comment #21)
> In part 12, the problem occurs where we have a "tt" in the text, and one of the
> test lines draws this in two separate operations, breaking between the t's.
> Because the "arms" of the letters come very close together in typical fonts,
> their subpixel AA areas end up overlapping, and we get a slightly different
> result from combining the two separate drawing operations than ClearType gives
> us for a continuous string. This isn't really a bug, IMO, it's just a fact of
> life. We can't use padding to give extra space here, as we're comparing to a
> continuous string of text, so the simplest solution is to use text where the
> glyphs at the "break" don't have elements that project so close to their
> neighbors.

What if we use letter-spacing?

Comment 24

[Simon Montagu :smontagu]

Comment on attachment 430566 [details] [diff] [review]
part 2 - resolve Win7 failure in bidi-004 reftest

The only problem with doing this is that we end up without sufficient testing for cases that we care about. I take your point that the test as it stands is as much a test of the font as the rendering system, but we need to think about a better way to test diacritic placement.

Comment 25

[Simon Montagu :smontagu]

BTW, it might be worth experimenting with enabling bidi-004.html on other platforms. IIRC, the reason that I disabled it on Mac was that the ALEF WITH PATAH was being displayed in a different font, so maybe it will work there now.

Comment 26

[Jonathan Kew [:jfkthame]]

(In reply to comment #25)
> BTW, it might be worth experimenting with enabling bidi-004.html on other
> platforms. IIRC, the reason that I disabled it on Mac was that the ALEF WITH
> PATAH was being displayed in a different font, so maybe it will work there now.

Right; with this change, it now "unexpectedly" passes on Mac, at least on tryserver. I realize removing the ALEF WITH PATAH weakens the test somewhat, but we do still have several other characters there, so it's better than nothing.

Comment 27

[Jonathan Kew [:jfkthame]]

Attachment: part 12, v2 - use sans-serif and letter-spacing to avoid Win7 subpixel failure where 'tt' glyphs touch in reftest 495385

Yes, using letter-spacing is a neat alternative for part 12; by adding this, we avoid the antialiasing of the adjacent glyphs interfering with each other and thus becoming dependent on the exact details of the painting sequence.

Comment 28

[Jonathan Kew [:jfkthame]]

Attachment: part 17, v2 - use letter-spacing to avoid Win7 subpixel failure in capitalize reftests

Letter-spacing can also be used in part 17, allowing us to stay with the default (serif) font and thus avoid the vertical antialiasing problem that occurs with the sans-serif capital Q.

(The fact that we don't get any vertical AA issues with the serif font and this particular text is of course just a lucky break. Until we really fix the paint overflow areas, this stuff will remain rather fragile and system-/font-dependent.)

Comment 29

[Jonathan Kew [:jfkthame]]

Attachment: part 17, v2.1 - use letter-spacing to avoid Win7 subpixel failure in capitalize reftests

Oops, inadvertently included hacks to reftest.list in the previous version of the patch. Sorry for the spam.

Comment 30

[Jonathan Kew [:jfkthame]]

Attachment: part 18, v2 - force "quality" text mode to ensure consistent kerning in reftest 399636

In part 18 (reftest bug-399636), the issue is that the reference files use the "fast" text path, without kerning, etc, but the testcases are hitting the "quality" path and on Win7, the font includes kerning that affects the glyph positions. That's why enlarging the text resolved the failure: at larger sizes, we always use the quality path.

So an alternative fix is to add text-rendering:optimizeLegibility to the styles, so that we get consistent kerning. I'm not actually sure why the testcases (where font fallback is occurring) are triggering the quality path even at smaller sizes - is that a bug, or expected behavior?

Comment 31

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

Comment on attachment 430988 [details] [diff] [review]
part 18, v2 - force "quality" text mode to ensure consistent kerning in reftest 399636

I don't know why the testcases are taking the quality path.

Comment 32

[Jonathan Kew [:jfkthame]]

Pushed patches 1-18:
http://hg.mozilla.org/mozilla-central/rev/578d4f095c88 - part 1
http://hg.mozilla.org/mozilla-central/rev/f68c19561c3a - part 2
http://hg.mozilla.org/mozilla-central/rev/557412abf1e8 - part 3
http://hg.mozilla.org/mozilla-central/rev/57465f48b37b - part 4
http://hg.mozilla.org/mozilla-central/rev/60c07a0b53d2 - part 5
http://hg.mozilla.org/mozilla-central/rev/ed14603cb310 - part 6
http://hg.mozilla.org/mozilla-central/rev/73d4c624a4d6 - part 7
http://hg.mozilla.org/mozilla-central/rev/974400ad6c10 - part 8
http://hg.mozilla.org/mozilla-central/rev/d95bfa9fd44e - part 9
http://hg.mozilla.org/mozilla-central/rev/ad8a95f3f32b - part 10
http://hg.mozilla.org/mozilla-central/rev/198053a9f9ef - part 11
http://hg.mozilla.org/mozilla-central/rev/23f996bcbc31 - part 12
http://hg.mozilla.org/mozilla-central/rev/1f79e295c7d7 - part 13
http://hg.mozilla.org/mozilla-central/rev/f55b591447fb - part 14
http://hg.mozilla.org/mozilla-central/rev/59a2511ba0d2 - part 15
http://hg.mozilla.org/mozilla-central/rev/6b2c89119e4e - part 16
http://hg.mozilla.org/mozilla-central/rev/7c23a0ba12af - part 17
http://hg.mozilla.org/mozilla-central/rev/9e3f1b33eb54 - part 18

Leaving this open for now, as I still have a couple of outstanding failures to investigate and possibly patch or mark in some way.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: part 8 followup - add padding to try and fix intermittent Rd failure in reftest 385569-1.

After pushing part 8, we're seeing an intermittent failure on tinderbox machines; the issue is clipping of a few antialiasing pixels on the top of the curve of the 'f'. This has only happened on Windows Debug machines; presumably has something to do with how the page gets updated when the font loads, and the Opt builds don't show it because they run faster.

As a possible fix, I suggest we add some padding to the testcase in the hope that this will prevent the clipping. (I can't reproduce it locally or on tryserver for testing.)

Comment 34

[Jonathan Kew [:jfkthame]]

Log showing the failure on Windows debug reftest:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268055646.1268058279.9999.gz

Comment 35

[Jonathan Kew [:jfkthame]]

Hmm, also happened on Opt reftest, although we've had several successful runs:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268061222.1268062200.24044.gz

Comment 36

[Jonathan Kew [:jfkthame]]

Comment on attachment 431094 [details] [diff] [review]
part 8 followup - add padding to try and fix intermittent Rd failure in reftest 385569-1.

Landed this per ted's advice on irc, as we had several (intermittent) failures - hoping it will help.
http://hg.mozilla.org/mozilla-central/rev/1041bbdd97ea

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: part 19 - add vertical bars to work around ClearType clipping issues in reftest 386339

This feels rather hacky, but ought to be ok as a workaround (until we fix ink overflow properly!) -- the reftest fails because of *vertical* antialiasing overflow at the tips of diagonal strokes. This isn't normally a problem, but the use of bidi override controls causes separate direction runs to be created, and the glyphs are then painted in isolation and clipped too tightly.

I've seen the same failure on Vista, too; it's not new with Win7. But it's likely to become an issue on tinderbox when we get Win7 test boxes.

This patch works around the problem by adding vertical bar (|) characters that do not affect the bidi behavior or mirroring (which is what's being tested here), but ensure that the tips of the < > glyphs are not the vertical extremes of the run that's being painted.

(Note that the issue doesn't occur if you view the reftest in a normal browser window; it only happens on the Cairo fallback path that gets used in the reftest framework. See bug 559879.)

Comment 38

[Jonathan Kew [:jfkthame]]

Pushed part 19: http://hg.mozilla.org/mozilla-central/rev/cdc8bf25220e

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: part 20 - use Calibri font if available (for Win7) in CGJ reftest

The reftest text/cgj-01.html fails on Windows 7 because the default font used has a visible glyph for Combining Grapheme Joiner.

The test is currently mixing two issues: (1) CGJ should not be visible; (2) presence of CGJ should not affect the advance of the preceding cluster.

I suggest modifying the test to specify the Calibri font (if available - this will only be relevant on Windows), as this has an invisible glyph for CGJ and thus allows us to test only that CGJ does not affect the surrounding glyph placement.

We should then have a separate bug and testcase (which will currently fail on Win7) about ensuring that display of CGJ is always suppressed, even if the font has a visible glyph.

Comment 40

[Jonathan Kew [:jfkthame]]

Pushed http://hg.mozilla.org/mozilla-central/rev/cd01fa67467b (part 20)

Comment 41

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

If all patches landed, shouldn't we close this ? ;)

Comment 42

[Jonathan Kew [:jfkthame]]

Yes, looks like we're done here.