Closed Bug 574907 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: masayuki, Assigned: jfkthame)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(5 files, 6 obsolete files)

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.
Blocks: dwrite
No longer depends on: dwrite
Probably should block, I guess?
blocking2.0: ? → final+
IE9 does this right!
Assignee: nobody → bas.schouten
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #468979 - Flags: review?(jdaggett)
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.
Attached file testcase
Wow, looks like IE9 does the subpixel positioning only when standards mode, not quirks mode. IE9 also has this bug...
Attachment #468979 - Attachment is obsolete: true
Attachment #468979 - Flags: review?(jdaggett)
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?
Assignee: bas.schouten → jfkthame
This doesn't need to block, though I'd still love to see it fixed.
blocking2.0: final+ → -
Attached patch Patch v2.0 (obsolete) — Splinter Review
If nobody can fix this bug without blacklist hack, we should use this patch for Fx4.
Attachment #501920 - Flags: review?(jfkthame)
Attachment #501920 - Flags: review?(jdaggett)
Comment on attachment 501920 [details] [diff] [review]
Patch v2.0

Oh, this doesn't fix this bug completely....
Attachment #501920 - Flags: review?(jfkthame)
Attachment #501920 - Flags: review?(jdaggett)
Attachment #501920 - Flags: review-
Attached patch Patch v2.1 (obsolete) — Splinter Review
okay, we should return TRUE for such font's gfxFont::ProvidesHintedWidths().
Attachment #501920 - Attachment is obsolete: true
Attachment #501945 - Flags: review?(jfkthame)
Attachment #501945 - Flags: review?(jdaggett)
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.
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).
Attachment #504753 - Flags: review?
Attachment #504753 - Flags: feedback?(jdaggett)
Attachment #504753 - Flags: review? → review?(masayuki)
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.
Attached file additional testcase
Note that the start X position of each line causes different positioning of each glyph.
Attachment #504993 - Attachment mime type: text/html → text/html;charset=utf-8
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.
Attached patch Merged patch (obsolete) — Splinter Review
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.
Attachment #505030 - Flags: review?(jfkthame)
Attachment #505030 - Flags: review?(jdaggett)
Attachment #504753 - Attachment is obsolete: true
Attachment #504753 - Flags: review?(masayuki)
Attachment #504753 - Flags: feedback?(jdaggett)
Attachment #501945 - Attachment is obsolete: true
Attachment #501945 - Flags: review?(jfkthame)
Attachment #501945 - Flags: review?(jdaggett)
And I'm afraid that doesn't the table check cause performance regression for bitmap fonts?
(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.
(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?
(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.
The gasp table of msgothic.ttc is:

header:
version: 0
numRanges: 2

ranges:
rangeMaxPPEM, rangeGaspBehaviour:
24,    0x0001
65535, 0x0003
(Note that this is based on top of the patch in bug 624310 - it won't apply cleanly to current trunk without that.)
Attachment #505040 - Flags: review?(masayuki)
Attachment #505040 - Flags: feedback?(jdaggett)
(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 on attachment 505040 [details] [diff] [review]
updated to avoid subpixel positioning when (potentially) using bitmaps

Looks okay for me except the waterfall.
Attachment #505040 - Flags: review?(masayuki) → review+
(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.
(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 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.
Attachment #505040 - Flags: feedback?(jdaggett) → approval2.0?
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)
Attachment #505040 - Flags: approval2.0? → approval2.0+
(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.
Includes all fonts on Windows 7 JA containing bitmap font data, along with CK text for comparison.
Attachment #504996 - Attachment is obsolete: true
Attachment #505030 - Attachment is obsolete: true
Attachment #505030 - Flags: review?(jfkthame)
Attachment #505030 - Flags: review?(jdaggett)
Pushed patch and tests:
http://hg.mozilla.org/mozilla-central/rev/e0c9841ac3dd
http://hg.mozilla.org/mozilla-central/rev/e0ea4e4d401f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Thank you very much!
Depends on: 627840
You need to log in before you can comment on or make changes to this bug.