574907 - [DW] Don't use subpixel positioning for bitmap glyphs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: Screenshot (left is GDI, right is DW)

See the testcase:

> data:text/html;charset=utf-8,<p style="font-family: MS PGothic;"><span style="font-size:15px;">も区間Mir (font-size: 15px;)</span><br><span style="font-size:15.1px;">も区間Mir (font-size: 15.1px;)</span><br><span style="font-size:15.5px;">も区間Mir (font-size: 15.5px;)</span><br><span style="font-size:15.9px;">も区間Mir (font-size: 15.9px;)</span><br><span style="font-size:16px;">も区間Mir (font-size: 16px;)</span><br><span style="font-size:25px;">も区間Mir (font-size: 25px;)</span><br><span style="font-size:25.5px;">も区間Mir (font-size: 25.5px;)</span><br><span style="font-size:26px;">も区間Mir (font-size: 26px;)</span><br></p>

DirectWrite uses subpixel positioning for each glyph. However, it's not good for bitmap glyphs.

See 15.5px case, "も" and "区" are contiguous. And in 15.9px case, "M" and "i" are contiguous.

If we can know what size is rendered as bitmap, we can round the font size. Otherwise, we should always round the font size if the font has bitmap glyph.

Comment 1

[Joe Drew (not getting mail)]

Probably should block, I guess?

Comment 2

[Joe Drew (not getting mail)]

IE9 does this right!

Comment 3

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

Attachment: Patch v1.0

IE9 ignores the decimal of font-size. So, we don't need to honor the decimal.

Our GDI code rounds the decimal, so, I think we should keep our behavior on DW code.

Comment 4

[John Daggett (:jtd)]

I don't think I agree here, it looks like you're fixing the font size for *all* fonts when I think you should be limiting it to *only* bitmap fonts.

The testcase below shows an evenly increasing for decimal-sized waterfall:
http://people.mozilla.org/~jdaggett/tests/decimalfontwaterfalls.html

Both IE9 and DW show this as a smooth waterfall, not as one that jumps at integer fonts sizes.

Comment 5

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

Attachment: testcase

Wow, looks like IE9 does the subpixel positioning only when standards mode, not quirks mode. IE9 also has this bug...

Comment 6

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

I think that there is no API which tells us the glyph will be rendered by bitmap glyph or not. So, we need to look for EBLC table directly. However, e.g., Courier New has both EBLC/EBDT tables but it's not used by both GDI and DW. John-san, do you have any ideas what's the difference of the fonts?

Comment 7

[Joe Drew (not getting mail)]

This doesn't need to block, though I'd still love to see it fixed.

Comment 8

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

Attachment: Patch v2.0

If nobody can fix this bug without blacklist hack, we should use this patch for Fx4.

Comment 9

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

Comment on attachment 501920 [details] [diff] [review]
Patch v2.0

Oh, this doesn't fix this bug completely....

Comment 10

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

Attachment: Patch v2.1

okay, we should return TRUE for such font's gfxFont::ProvidesHintedWidths().

Comment 11

[Jonathan Kew [:jfkthame]]

This looks like it should work, but I'm reluctant to introduce (another) blacklist that we need to maintain - and that will fail to deal with additional fonts (whether locally-installed or downloadable) that have the same issue. So I'm going to try writing a patch to solve this based on probing the font tables instead... if I can't make this work, we could try the blacklist approach.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: alternative patch, probe for bitmap strike sizes in the font

This is an alternative proposal that avoids the need for a blacklist: it looks for the bitmap tables in the font, and rounds the size if there's a bitmap strike for the potential rounded size; if not, it leaves it unrounded.

For Courier New (or similar fonts) where the bitmap strike only contains a couple of glyphs, it seems better to leave the size untouched (and DW doesn't appear to use the few bitmaps that exist, anyway).

Comment 13

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

Comment on attachment 504753 [details] [diff] [review]
alternative patch, probe for bitmap strike sizes in the font

This patch cannot fix this bug completely.

See my patch, we need to return TRUE at ProvidesHintedWidths() because if this returns FALSE, each glyph widths are computed from outline glyph by DW, but if this returns TRUE, we compute them by GetGdiCompatibleGlyphMetrics() in gfxDWriteFont::GetHintedGlyphWidth(). Then, we can get same result as GDI.

Comment 14

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

Attachment: additional testcase

Note that the start X position of each line causes different positioning of each glyph.

Comment 15

[John Daggett (:jtd)]

Attachment: msgothic variants waterfall with meiryo for comparison

Glyph collisions that don't occur in Firefox 3.6 occur in multiple places.  With Jonathan's patch is better but doesn't fully resolve the regression.

Comment 16

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

Attachment: Merged patch

merged both patches.

1. Check whether the computed size is larger than 10px and less than 20px because DirectWrite ignores other size's bitmap glyphs.
2. Return TRUE at ProvidesHintedWidths() if the font uses bitmap glyph. This means the each glyph size is computed as GDI compatible.

Comment 17

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

And I'm afraid that doesn't the table check cause performance regression for bitmap fonts?

Comment 18

[Jonathan Kew [:jfkthame]]

(In reply to comment #17)
> And I'm afraid that doesn't the table check cause performance regression for
> bitmap fonts?

I doubt it makes a significant difference. The check only happens when the font is instantiated, not during every rendering operation, so it won't slow down ongoing drawing.

If you find that the added cost during font creation is actually detectable, we could address this by moving the check into the fontEntry class, and caching the results there so that we don't have to repeat it when the same font is instantiated again. But I really don't think it'll be an issue.

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to comment #16)
> 1. Check whether the computed size is larger than 10px and less than 20px
> because DirectWrite ignores other size's bitmap glyphs.

Is this documented somewhere, or how do we know it? Is it dependent on other factors such as the size ranges and rendering options in the 'gasp' table?

Comment 20

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

(In reply to comment #19)
> (In reply to comment #16)
> > 1. Check whether the computed size is larger than 10px and less than 20px
> > because DirectWrite ignores other size's bitmap glyphs.
> 
> Is this documented somewhere, or how do we know it? Is it dependent on other
> factors such as the size ranges and rendering options in the 'gasp' table?

I don't know. I just find the behavior by the actual behavior with Japanese system fonts. If we give up the waterfall for such sizes, the check isn't needed.

Comment 21

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

The gasp table of msgothic.ttc is:

header:
version: 0
numRanges: 2

ranges:
rangeMaxPPEM, rangeGaspBehaviour:
24,    0x0001
65535, 0x0003

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: updated to avoid subpixel positioning when (potentially) using bitmaps

(Note that this is based on top of the patch in bug 624310 - it won't apply cleanly to current trunk without that.)

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #16)
> > > 1. Check whether the computed size is larger than 10px and less than 20px
> > > because DirectWrite ignores other size's bitmap glyphs.
> > 
> > Is this documented somewhere, or how do we know it? Is it dependent on other
> > factors such as the size ranges and rendering options in the 'gasp' table?
> 
> I don't know. I just find the behavior by the actual behavior with Japanese
> system fonts.

Hmm. It may not be quite as simple as that. Yes, the MS PGothic waterfall appears to use bitmaps only from 10px to 20px (even though the font has bitmaps from 7px to 22px). But John's sample with Hiragino Kaku Gothic Pro appears to be using bitmaps at 9px as well. It's not clear to me how DW is making the decision - it doesn't seem to be related to the 'gasp' settings, nor does it seem to be a simple hard-coded pixel size.

So I'd prefer to omit the size-range limit from our code, and simply base our behavior on the sizes of bitmaps present in the font. If in some cases, DW decides not to use them even though they're present, no great harm is done; it'll just mean that we've rounded the size in a case where perhaps we need not have done.

BTW, I think there's no need for a new boolean flag in the gfxDWriteFont; we can just use the existing mUsingClearType flag, which already controls whether subpixel positioning will be used. I've renamed it to mUseSubpixelPositions here, to better reflect its significance.

Comment 24

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

Comment on attachment 505040 [details] [diff] [review]
updated to avoid subpixel positioning when (potentially) using bitmaps

Looks okay for me except the waterfall.

Comment 25

[Jonathan Kew [:jfkthame]]

(In reply to comment #24)
> Looks okay for me except the waterfall.

What do you consider is "not okay"? It's true that the result is different from FF3.6/GDI rendering in that we don't round the font size to an integer in cases where no bitmap strike is available, and so the waterfall looks different in these cases, but I would say this is the correct behavior. We do not expect DirectWrite text rendering to be identical to GDI in all situations.

Comment 26

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

(In reply to comment #25)
> (In reply to comment #24)
> > Looks okay for me except the waterfall.
> 
> What do you consider is "not okay"?

I meant that the patched build doesn't have the problem which the glyphs are contacted or overlapped than Fx3.6. In other words, the rendering result is same as GDI version when bitmap glyphs aren't ignored. And the patch doesn't have problem. But it failed on the waterfall demo when the bitmap glyphs are ignored by DW but it's not as important as this bug.

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 505040 [details] [diff] [review]
updated to avoid subpixel positioning when (potentially) using bitmaps

(In reply to comment #26)
> I meant that the patched build doesn't have the problem which the glyphs are
> contacted or overlapped than Fx3.6. In other words, the rendering result is
> same as GDI version when bitmap glyphs aren't ignored. And the patch doesn't
> have problem. But it failed on the waterfall demo when the bitmap glyphs are
> ignored by DW but it's not as important as this bug.

OK. I don't think we can do anything about the fact that DirectWrite seems to ignore bitmap glyphs in certain cases; there doesn't appear to be any way for us to control this, or even to determine reliably when it will happen.

Requesting approval-2.0; this fixes a text-rendering regression in CJK fonts where bitmaps are provided for certain sizes. It'd be good to get this into a beta soon in order to get the modified rendering in front of a wider audience.

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 505040 [details] [diff] [review]
updated to avoid subpixel positioning when (potentially) using bitmaps

a=me so long as we add a test (if it's testable)

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to comment #28)

> a=me so long as we add a test (if it's testable)

We can include a Windows-only reftest; this is Windows-only code, so we can just rely on standard Windows system fonts in the test. I'll work something up.

Comment 30

[John Daggett (:jtd)]

Attachment: embedded bitmap font waterfall

Includes all fonts on Windows 7 JA containing bitmap font data, along with CK text for comparison.

Comment 31

[Jonathan Kew [:jfkthame]]

Pushed patch and tests:
http://hg.mozilla.org/mozilla-central/rev/e0c9841ac3dd
http://hg.mozilla.org/mozilla-central/rev/e0ea4e4d401f

Comment 32

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

Thank you very much!