549190 - Underline drawn inconsistently with direct2d

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Haas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100226 Minefield/3.7a2pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a2pre) Gecko/20100226 Minefield/3.7a2pre (.NET CLR 3.5.30729)

After enabling direct2d rendering, text-decoration:underline sometimes is connected to the letters and sometimes not. Going to attach testcase and screenshot.

Reproducible: Always

Steps to Reproduce:
1. Load testcase with and without gfx.font_rendering.directwrite.enabled
2. Observe difference

Comment 1

[Jonathan Haas]

Attachment: Testcase

Comment 2

[Jonathan Haas]

Attachment: Screenshot

Comment 3

[Josh Tumath]

Could this have something to do with the text height being incorrectly shown? When I highlight text, the highlight is sometimes 1 or 2 pixels lower than it should be.

Comment 4

[u88484]

Confirming:

Windows 7 64-bit
NVIDIA GeForce 7150M / nForce 630M
Drivers: 7.15.11.7967

I see a lot of this in gmail.

Comment 5

[Jonathan Haas]

Attachment: Testcase 2

It's interesting to note, that giving the parent element an underline results in a currect underline.

In this testcase, the parent element (div) produces a red underline, while the child elements (links) have a light gray color with underline that /should/ cover the previous underline, making the red underline invisible. For some strange reason although the link underline is in some lines misplaced so you can see the red underline.

You could probably turn this into some sort of reftest if you set the link color to white. Then the resulting document should be completely white. With the current direct2d rendering, there are some red lines visible (depending on font size).

Comment 6

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

Normally we try to snap the text baseline to a pixel boundary (see GetSnappedBaselineY in nsTextFrameThebes.cpp). Some more rounding happens in nsCSSRendering::GetTextDecorationRectInternal ... in particular, the underline-offset is rounded to a device pixel there. That should be enough to ensure that offset is visually constant, so I'm not sure what's going wrong here.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #6)
> Normally we try to snap the text baseline to a pixel boundary (see
> GetSnappedBaselineY in nsTextFrameThebes.cpp). Some more rounding happens in
> nsCSSRendering::GetTextDecorationRectInternal ... in particular, the
> underline-offset is rounded to a device pixel there. That should be enough to
> ensure that offset is visually constant, so I'm not sure what's going wrong
> here.

I guess it could happen that they round in such a way that the underline is rounded in a different direction than the baseline? Causing inconsistent spacing?

Comment 8

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

We round the offset, and the offset value should be the same for every line, so the rounded value should be the same, so the spacing should be consistent.

Someone's just going to have to dive in and debug in nsCSSRendering::GetTextDecorationRectInternal I think.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Every height between underline and next line's glyph is 3px. So, the height of lines whose underline is too near is 1px narrower than the others.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to comment #9)
> Every height between underline and next line's glyph is 3px. So, the height of
> lines whose underline is too near is 1px narrower than the others.

So this does indeed seem to be a problem with this being the first case of sub-pixel vertical positioning?

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like gfxDWriteFont::ComputeMetrics() computes the metrics in very small unit. I guess that they should be rounded to pixels because the DW mode line-height is shorter than GDI's. This difference makes incompatibility with other Windows' web browsers (including our old versions) especially if some pages are designed tightly.

Comment 12

[Jonathan Kew [:jfkthame]]

I've experimented with rounding the font metrics in gfxDWriteFont::ComputeMetrics, and this does appear to result in consistent underlines (as well as resolving a bunch of reftest failures with DWrite enabled, such as those triggered by first-letter, where the presence of the first-letter frame seems to be affecting the rounding of the text baseline position).

So the "quick fix" here would be to apply rounding to DWrite font metrics; this would probably also help to maintain more consistency across platforms where some are already giving us pre-rounded metrics.

The alternative is to decide that we want to work with high-precision line-spacing metrics when available, and try to resolve the layout issues that this exposes - at least underlining and first-letter, and possibly others. Is this something we want to tackle at the moment?

Comment 13

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

Let's take the quick fix and consider high-precision line spacing as a separate bug.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: round the maxAscent/Descent values to get more consistent spacing

This makes two changes to the gfxDWriteFont metrics: first, it ensures that emAscent + emDescent = emHeight, which I think is a generally-expected relation. Second, it rounds maxAscent and maxDescent to integer values, which appears to resolve the underline-spacing issue.

Comment 15

[Bas Schouten (:bas.schouten)]

Comment on attachment 436307 [details] [diff] [review]
round the maxAscent/Descent values to get more consistent spacing

>+#define ROUND(x) floor((x)+0.5)
>+
Perhaps it's better to have this at the top of the file than right in between two functions?

Comment 16

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

Better still, include nsMathUtils.h and use NS_round.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Is round using right? I guess that the values should be always rounded up. When you cut a jot, couldn't it cause glyph overflow? And I think that maxHeight should be sum of maxAscent and maxDescent.

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to comment #17)
> Is round using right? I guess that the values should be always rounded up. When
> you cut a jot, couldn't it cause glyph overflow?

I wondered about this. The idea of using rounding rather than rounding-up was to stay as close as possible to the "true" metrics; rounding up would have a tendency to spread lines slightly more at times. But maybe that's better than risking the possibility of (very slight) glyph overflow when the rounding is downwards. I'll try a few tests and see if I can notice a difference.

> And I think that maxHeight
> should be sum of maxAscent and maxDescent.

Yes, that makes sense. I'll check what effect (if any) that has.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: CJK characters are tighter than alphabets because they don't have the x-height idea, so, they spread in ascent area as far as possible.

Comment 20

[Jonathan Haas]

(In reply to comment #18)
> (In reply to comment #17)
> The idea of using rounding rather than rounding-up was
> to stay as close as possible to the "true" metrics; rounding up would have a
> tendency to spread lines slightly more at times.

I actually think spreading the lines more is a good thing. Our current DW rendering has a much smaller line height than the classic GDI rendering (see attachment 429393 [details]). When we round up, we hopefully will match the classic rendering and display the line height as it is intended by the website designers.

Comment 21

[Jonathan Kew [:jfkthame]]

Attachment: round ascent/descent and leading metrics upwards

We're unlikely ever to match the GDI metrics exactly, as I believe they may be affected by hinting, whereas with DWrite we are getting "ideal" metrics and then scaling them to the font size. But rounding up both the ascent/descent values and the leading metrics seems to give the most consistent results in my comparisons. 

Marking for re-review, as this does change the behavior slightly from the previous iteration. It actually gets us a couple more reftest passes thanks to the rounding of leading, apparently. (Overall, this patch cuts dwrite reftest failures from 99 to 50 in my local testing.)

Comment 22

[Bas Schouten (:bas.schouten)]

Comment on attachment 436472 [details] [diff] [review]
round ascent/descent and leading metrics upwards

I think perhaps we could calculate ascent after maxAscent/maxHeight and save ourselves a slight bit of arithmetic. Other than that it looks good to me!

Comment 23

[Jonathan Kew [:jfkthame]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4365eabf7fb0

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the invariants that layout code expects font metrics to maintain are:
  emHeight = (em)Ascent + (em)Descent
  maxHeight = maxAscent + maxDescent
  internalLeading = maxHeight - emHeight
though I'm not sure this is documented anywhere (but it probably should be if it's correct).

The rounding in this patch looks like it slightly violates the third of these invariants.  Not sure if that would show up as a real problem, though.

I'm also a little curious why emAscent isn't rounded.

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to comment #24)
> So I think the invariants that layout code expects font metrics to maintain
> are:
>   emHeight = (em)Ascent + (em)Descent
>   maxHeight = maxAscent + maxDescent
>   internalLeading = maxHeight - emHeight
> though I'm not sure this is documented anywhere (but it probably should be if
> it's correct).
> 
> The rounding in this patch looks like it slightly violates the third of these
> invariants.  Not sure if that would show up as a real problem, though.

Ok, that's at least something to keep in mind; I have a feeling we're not finished with all this. (Actually, I'd really like to try a common, cross-platform approach to font metrics, based on the patch in bug 532533, but that's for another day.....)

> I'm also a little curious why emAscent isn't rounded.

IIRC, I experimented with that at one point, and couldn't tell that it was actually affecting any results (either visually or in reftest). So I left it alone, with the general aim of tweaking as little as possible in order to resolve the problem here.

Comment 26

[u88484]

Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100407 Minefield/3.7a4pre

Even fixed all of the portions of cutoff text, like the bottoms of letters like g,y,p,and q.