Last Comment Bug 666893 - Windows (ClearType): make all area painted by text be part of text frame's ink overflow area (DirectWrite in GDI mode version)
: Windows (ClearType): make all area painted by text be part of text frame's in...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 Windows 7
: P3 major (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-24 03:15 PDT by Emanuel Hoogeveen [:ehoogeveen]
Modified: 2011-07-02 06:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test file to reliably demonstrate the ClearType rendering glitch (1.28 KB, text/html)
2011-06-24 03:17 PDT, Emanuel Hoogeveen [:ehoogeveen]
no flags Details
screenshot (5.86 KB, image/png)
2011-06-24 03:20 PDT, Emanuel Hoogeveen [:ehoogeveen]
no flags Details
patch, add padding pixels because of ClearType bleed when using GDI Classic mode (2.88 KB, patch)
2011-06-24 15:26 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review

Description Emanuel Hoogeveen [:ehoogeveen] 2011-06-24 03:15:12 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1

Followup for bug 475968 to fix the DirectWrite side of things.

The patch in that bug was made under the assumption that only GDI was affected. However, it appears that the problem is not limited to GDI, but also shows up with the 'GDI Classic' rendering mode set in the DirectWrite API. Bug 642589 gave users the ability to set gfx.font_rendering.cleartype_params.rendering_mode = 2 to force DirectWrite to use this rendering mode, and bug 661471 landing recently ensured that commonly used web fonts always use this mode. As such, the fix for bug 475968 should be extended to work for DirectWrite as well.

Attached are the original testcase from bug 475968 that shows this problem, and a screenshot of the problem happening using DirectWrite.

Reproducible: Always

Steps to Reproduce:
1. Run the testcase using DirectWrite with the force_gdi_classic* options enabled.

Actual Results:  
Vertical lines appear between 'Click here!' and 'Enter'.

Expected Results:  
Space between 'Click here!' and 'Enter' should be blank.
Comment 1 Emanuel Hoogeveen [:ehoogeveen] 2011-06-24 03:17:51 PDT
Created attachment 541647 [details]
test file to reliably demonstrate the ClearType rendering glitch

Test-case
Comment 2 Emanuel Hoogeveen [:ehoogeveen] 2011-06-24 03:20:14 PDT
Created attachment 541648 [details]
screenshot

Screenshot
Comment 3 Emanuel Hoogeveen [:ehoogeveen] 2011-06-24 07:15:04 PDT
Apologies, I thought I had set it to text/html. Must have mis-clicked.
Comment 4 Jonathan Kew (:jfkthame) 2011-06-24 15:26:48 PDT
Created attachment 541831 [details] [diff] [review]
patch, add padding pixels because of ClearType bleed when using GDI Classic mode

This essentially duplicates the GDI patch from bug 475968 in the DWrite font backend, but only when we're using GDI Classic rendering mode; in DW Natural mode, there isn't the same tendency for antialiasing to paint significantly outside the nominal ink bounds - or at least it's so slight that we haven't observed problems arising from it.

Pushed to tryserver to see how the tests fare....
Comment 5 Jonathan Kew (:jfkthame) 2011-06-25 01:30:54 PDT
No problems on unit tests (which is as I'd expect, given that we already do the same thing in some other backends). Tryserver build is available at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-544845e4fae7/try-win32/.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-25 02:50:25 PDT
Comment on attachment 541831 [details] [diff] [review]
patch, add padding pixels because of ClearType bleed when using GDI Classic mode

Review of attachment 541831 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxDWriteFonts.cpp
@@ +627,5 @@
> +    // and the underlying cairo font may be antialiased,
> +    // we can't trust Windows to have considered all the pixels
> +    // so we need to add "padding" to the bounds.
> +    // (see bugs 475968, 439831, compare also bug 445087)
> +    if (aBoundingBoxType == LOOSE_INK_EXTENTS &&

Shouldn't this be aBoundingBoxType != TIGHT_HINTED_OUTLINE_EXTENTS?

Seems like this is needed for TIGHT_INK_EXTENTS as well.
Comment 7 Jonathan Kew (:jfkthame) 2011-06-25 04:29:03 PDT
(In reply to comment #6)
> Comment on attachment 541831 [details] [diff] [review] [review]
> patch, add padding pixels because of ClearType bleed when using GDI Classic
> mode
> 
> Review of attachment 541831 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxDWriteFonts.cpp
> @@ +627,5 @@
> > +    // and the underlying cairo font may be antialiased,
> > +    // we can't trust Windows to have considered all the pixels
> > +    // so we need to add "padding" to the bounds.
> > +    // (see bugs 475968, 439831, compare also bug 445087)
> > +    if (aBoundingBoxType == LOOSE_INK_EXTENTS &&
> 
> Shouldn't this be aBoundingBoxType != TIGHT_HINTED_OUTLINE_EXTENTS?
> 
> Seems like this is needed for TIGHT_INK_EXTENTS as well.

This matches what we do in the GDI font code. For TIGHT_INK_EXTENTS, padding is added to the glyph extents within cairo, so we don't need to do it here. (See _cairo_dwrite_scaled_font_init_glyph_metrics for the DWrite case, or _cairo_win32_scaled_font_init_glyph_metrics for the GDI equivalent. This has to be done within cairo to avoid clipping the glyph images when they're painted.)

So it's only when we're doing "loose" measurement, so we're not getting actual glyph sidebearings etc. from cairo, that we need to add padding at the gfxFont level in case the glyphs at the edges of the run "bleed" slightly beyond their typographic origin/advance.
Comment 8 Emanuel Hoogeveen [:ehoogeveen] 2011-06-25 04:57:22 PDT
I can confirm that the testcase from comment #1 is fixed in the Tryserver build.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-30 06:07:01 PDT
Comment on attachment 541831 [details] [diff] [review]
patch, add padding pixels because of ClearType bleed when using GDI Classic mode

Review of attachment 541831 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 10 Jonathan Kew (:jfkthame) 2011-06-30 23:59:10 PDT
http://hg.mozilla.org/mozilla-central/rev/592403064072
Comment 11 Emanuel Hoogeveen [:ehoogeveen] 2011-07-02 06:45:06 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110702 Firefox/7.0a1

Using today's nightly I no longer see the rendering glitches in the testcase from comment #1; marking as verified fixed. Thanks!

Note You need to log in before you can comment on or make changes to this bug.