661471 - Create preference(s) to allow specific font families to be forced to use GDI Classic rendering

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

We seem to have some consensus that forcing the classic "core Web fonts" to use GDI Classic rendering with DirectWrite would be better than the status quo.

Comment 1

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

Attachment: fix

Comment 2

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

Attachment: force GDI Classic for the "core Web fonts"

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 536824 [details] [diff] [review]
force GDI Classic for the "core Web fonts"

>+pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families",
>+     "Arial,Andale Mono,Comic Sans MS,Courier New,Impact,Georgia,Trebuchet MS,Verdana");

What about Times New Roman?

Comment 4

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

Oops, yeah, I missed that one.

Comment 5

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

Attachment: force GDI Classic for the "core Web fonts"

Comment 6

[Dão Gottwald [:dao]]

And Arial Black isn't covered by Arial, is it? Intentionally left out for other reasons?

Comment 7

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

I believe that with DirectWrite, Arial Black is part of the Arial family.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 536823 [details] [diff] [review]
fix

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

Looks like it should be fine, provided tryserver is happy. I think my only substantive question is the one about unconditionally forcing GDI Classic for DW fonts on the GDI backend - is that really desirable?

::: gfx/cairo/cairo/src/cairo-d2d-private.h
@@ +77,5 @@
>  typedef struct _cairo_d2d_device cairo_d2d_device_t;
>  
>  struct _cairo_d2d_surface {
>      _cairo_d2d_surface() : d2d_clip(NULL), clipping(false), isDrawing(false),
> +            textRenderingState(TEXT_RENDERING_UNINITIALIZED)

Should use a real tab rather than 8 spaces here, for consistency with the rest of the cairo code. (There are some other instances of this in the other cairo files, too.)

::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp
@@ +1108,5 @@
>      IDWriteGdiInterop *gdiInterop;
>      DWriteFactory::Instance()->GetGdiInterop(&gdiInterop);
>      IDWriteBitmapRenderTarget *rt;
>  
> +    IDWriteRenderingParams *params = DWriteFactory::RenderingParams(TRUE);

Do we really want to force GDI Classic mode unconditionally here? That seems like it'd prevent non-d2d users ever trying other dwrite modes, which (if they've explicitly turned on dwrite) may be what they actually want.

::: gfx/thebes/gfxDWriteFontList.h
@@ +199,5 @@
>      nsRefPtr<IDWriteFont> mFont;
>      nsRefPtr<IDWriteFontFile> mFontFile;
>      DWRITE_FONT_FACE_TYPE mFaceType;
>  
> +    int mIsCJK;

Maybe use "short" or even "signed char" here, to reduce the size of the structure slightly? It only needs three values - true (1), false (0), and uninitialized (-1).

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 536836 [details] [diff] [review]
force GDI Classic for the "core Web fonts"

You should also include Webdings, I think; it's one of the Core Web Fonts, and shows pretty dramatic differences in rendering between GDI Classic and DW Natural modes.

Comment 10

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

(In reply to comment #8)
> ::: gfx/cairo/cairo/src/cairo-d2d-private.h
> @@ +77,5 @@
> >  typedef struct _cairo_d2d_device cairo_d2d_device_t;
> >  
> >  struct _cairo_d2d_surface {
> >      _cairo_d2d_surface() : d2d_clip(NULL), clipping(false), isDrawing(false),
> > +            textRenderingState(TEXT_RENDERING_UNINITIALIZED)
> 
> Should use a real tab rather than 8 spaces here, for consistency with the
> rest of the cairo code. (There are some other instances of this in the other
> cairo files, too.)

I don't think this is actually required. I discussed it with Carl Worth a long time ago :-).

> ::: gfx/cairo/cairo/src/cairo-dwrite-font.cpp
> @@ +1108,5 @@
> >      IDWriteGdiInterop *gdiInterop;
> >      DWriteFactory::Instance()->GetGdiInterop(&gdiInterop);
> >      IDWriteBitmapRenderTarget *rt;
> >  
> > +    IDWriteRenderingParams *params = DWriteFactory::RenderingParams(TRUE);
> 
> Do we really want to force GDI Classic mode unconditionally here? That seems
> like it'd prevent non-d2d users ever trying other dwrite modes, which (if
> they've explicitly turned on dwrite) may be what they actually want.

Good point, will fix.

The _dwrite_draw_glyphs_to_gdi_surface_d2d path fails to set RenderingParams at all. I won't fix that in this bug, but I've added an XXX comment pointing out the issue.

> ::: gfx/thebes/gfxDWriteFontList.h
> @@ +199,5 @@
> >      nsRefPtr<IDWriteFont> mFont;
> >      nsRefPtr<IDWriteFontFile> mFontFile;
> >      DWRITE_FONT_FACE_TYPE mFaceType;
> >  
> > +    int mIsCJK;
> 
> Maybe use "short" or even "signed char" here, to reduce the size of the
> structure slightly? It only needs three values - true (1), false (0), and
> uninitialized (-1).

Made it a PRInt8.

Comment 11

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

Attachment: fix v2

Comment 12

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

Attachment: force GDI Classic for the "core Web fonts", v2

Comment 13

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

I've pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=24a3a50f5d85

Jonathan expects there to be some minor reftest failures (and passes) to paper over.

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to comment #13)
> I've pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=24a3a50f5d85
> 
> Jonathan expects there to be some minor reftest failures (and passes) to
> paper over.

For which usable wallpaper can be found in attachment 536575 [details] [diff] [review] at bug 652141 (I expect the issues to be mostly the same).

Comment 15

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

I'll probably just land this on trunk tomorrow morning.

Comment 16

[JP Rosevear [:jpr]]

(In reply to comment #15)
> I'll probably just land this on trunk tomorrow morning.

John is at the CSS font working group F2F in Japan, can we wait a couple days for him to come back to review the patch first?

Comment 17

[Jonathan Kew [:jfkthame]]

If we want to fix this for Fx5 - which I think we should! - then we need to get it landed pretty soon so as to have a chance of seeking approval for beta and getting into a beta build (as I understand those only happen weekly) before the train is completely out of sight.

Another possible reviewer might be Bas, if John's not available.

Comment 18

[John Daggett (:jtd)]

Comment on attachment 536865 [details] [diff] [review]
fix v2

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

+        gfxDWriteFontEntry *fe =
+            static_cast<gfxDWriteFontEntry*>(mFontEntry.get());
+        cairo_dwrite_scaled_font_set_force_GDI_classic(mCairoScaledFont,
+                                                       fe->GetForceGDIClassic());

+DWRITE_MEASURING_MODE
+gfxDWriteFont::GetMeasuringMode()
+{
+    return static_cast<gfxDWriteFontEntry*>(mFontEntry.get())->GetForceGDIClassic()
+        ? DWRITE_MEASURING_MODE_GDI_CLASSIC
+        : gfxWindowsPlatform::GetPlatform()->DWriteMeasuringMode();
+}

The logic in both these cases should be based on size, there's absolutely no reason to use
GDI classic rendering at larger sizes, it only makes the text look jaggy.

Something like:

  if (size < 16px) {
    use classic
  } else {
    use default
  }
  
Testpage here: http://people.mozilla.org/~jdaggett/tests/cleartype-waterfall.html

Comment 19

[John Daggett (:jtd)]

Comment on attachment 536866 [details] [diff] [review]
force GDI Classic for the "core Web fonts", v2

+// Currently we apply this setting to all the classic Microsoft "core Web fonts".
+pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families",
+     "Andale Mono,Arial,Comic Sans MS,Courier New,Georgia,Impact,Times New Roman,Trebuchet MS,Verdana,Webdings");

I would prefer to limit this to the sans-serif faces that are at the
heart of the complaints.  The serif faces in this list are actually more
readable with subpixel antialiasing at small sizes.

I would limit this to:

Arial,Courier New,Trebuchet MS,Verdana

Comment 20

[Dão Gottwald [:dao]]

> I would prefer to limit this to the sans-serif faces that are at the
> heart of the complaints.  The serif faces in this list are actually more
> readable with subpixel antialiasing at small sizes.

At small sizes, yes. So ideally, shouldn't this be font-size dependent as well? If I remember correctly, DW also improves sans-serif text at small sizes, so I'm not sure why you're drawing a line between serif and sans-serif.

Comment 21

[John Daggett (:jtd)]

(In reply to comment #20)
> > I would prefer to limit this to the sans-serif faces that are at the
> > heart of the complaints.  The serif faces in this list are actually more
> > readable with subpixel antialiasing at small sizes.
> 
> At small sizes, yes. So ideally, shouldn't this be font-size dependent as
> well? If I remember correctly, DW also improves sans-serif text at small
> sizes, so I'm not sure why you're drawing a line between serif and
> sans-serif.

Not exactly, it's the sans-serif fonts that look "crisp" in GDI classic mode because they are placed on pixel boundaries.  With serif faces placement on pixel boundaries doesn't assure "crispness" nearly as much.

Compare FF4 to Chrome with this testcase:

http://people.mozilla.org/~jdaggett/tests/cleartype-waterfall.html

Comment 22

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to comment #18)
> Something like:
> 
>   if (size < 16px) {
>     use classic
>   } else {
>     use default
>   }

It's also been suggested that at very small sizes, subpixel positioning will start to look better again as glyph line widths become significantly smaller than a pixel. Given the simplicity of the above, is that something that could be done here or would that take more research?

Comment 23

[John Daggett (:jtd)]

(In reply to comment #22)
> (In reply to comment #18)
> > Something like:
> > 
> >   if (size < 16px) {
> >     use classic
> >   } else {
> >     use default
> >   }
> 
> It's also been suggested that at very small sizes, subpixel positioning will
> start to look better again as glyph line widths become significantly smaller
> than a pixel. Given the simplicity of the above, is that something that
> could be done here or would that take more research?

By very small you mean <9px?  It's hard to have optimal settings for this range as they will be *very* dependent on the font as to whether it's usable at all at these sizes.

Comment 24

[John Daggett (:jtd)]

(In reply to comment #19)
> I would limit this to:
> 
> Arial,Courier New,Trebuchet MS,Verdana

Sorry, I meant to write:

Arial,Courier New,Tahoma,Trebuchet MS,Verdana

Comment 25

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to comment #23)
> By very small you mean <9px?  It's hard to have optimal settings for this
> range as they will be *very* dependent on the font as to whether it's usable
> at all at these sizes.

Something like that - I don't remember if an exact size range was suggested, or if it was based on anything substantial. It does stand to reason though that at some point, snapping glyphs to the pixel grid is going to leave them unacceptably deformed - subpixel positioning is no guarantee of readability, but it's better than nothing.

Comment 26

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

Attachment: Part 1: implement GDI-classic family list pref

Comment 27

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

Attachment: Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size

Comment 28

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

Comment on attachment 537038 [details] [diff] [review]
Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size

Whoever gets to it first...

Comment 29

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

Attachment: Part 3: Apply 'GDI Classic' prefs to @font-face local()

There was a bug in part 1 which caused reftest failures. We weren't forcing "GDI Classic" for @font-face local() fonts.

Comment 30

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

Comment on attachment 537039 [details] [diff] [review]
Part 3: Apply 'GDI Classic' prefs to @font-face local()

Whoever gets to it first

Comment 31

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

Attachment: Part 4: Force DirectWrite to use 'GDI Classic' rendering for sans-serif 'core Web fonts' of size < 16 pixels

This is exactly what John asked for.

Comment 32

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

Attachment: Part 5: Fix reftests

This is actually Jonathan's patch that I stole and reviewed :-)

Comment 33

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

New tryserver push: http://tbpl.mozilla.org/?tree=Try&rev=28df502c24ee

Comment 34

[John Daggett (:jtd)]

Comment on attachment 537040 [details] [diff] [review]
Part 4: Force DirectWrite to use 'GDI Classic' rendering for sans-serif 'core Web fonts' of size < 16 pixels

+// Currently we apply this setting to the sans-serif Microsoft "core Web fonts".
+pref("gfx.font_rendering.cleartype_params.force_gdi_classic_for_families",
+     "Arial,Courier New,Trebuchet MS,Verdana");

Need Tahoma in the list here.

Comment 35

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

Attachment: Part 4 v2

Comment 36

[John Daggett (:jtd)]

Comment on attachment 537038 [details] [diff] [review]
Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size

review+ with changes in part 4v2

Comment 37

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

Thanks! I'll land this once my tryserver results come in.

Comment 38

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

Tryserver got a green reftest run, so I checked this in.

http://hg.mozilla.org/mozilla-central/rev/79f718b57c49
http://hg.mozilla.org/mozilla-central/rev/c79148810af9
http://hg.mozilla.org/mozilla-central/rev/3ad038dfdd82
http://hg.mozilla.org/mozilla-central/rev/077b3514036d
http://hg.mozilla.org/mozilla-central/rev/051b1bd21ae7

Comment 39

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

Very shortly we should apply for aurora and beta approval for this bug and for bug 624589 (which this depends on).

Comment 40

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

Er, bug 642589.

Comment 41

[Kai Liu]

Perhaps Segoe UI could be added to the list as well, so that text in chrome would be covered?  Should a separate bug be filed for that?

Comment 42

[[Baboo]]

Do the font sizes in pixel here refer to what the designer specifies or to how it is in fact displayed, ie. after zooming and such?

Comment 43

[John Daggett (:jtd)]

(In reply to comment #42)
> Do the font sizes in pixel here refer to what the designer specifies or to
> how it is in fact displayed, ie. after zooming and such?

The font sizes here are at the graphics layer level so they're all in real pixels (i.e. after zoom).  So zoom = 200% + font-size: 8px == 16px at the graphics layer.

Comment 44

[[Baboo]]

OK, that's good.

Comment 45

[Kai Liu]

(In reply to comment #41)
> Perhaps Segoe UI could be added to the list as well, so that text in chrome
> would be covered?  Should a separate bug be filed for that?

Filed bug 661869.

Comment 46

[neb1236]

I believe it's due to this bug so I will say it here, tell me if I need to file a new bug:
This new pref seems to break about everything when Cleartype is turned off: http://tinyurl.com/6gfkgu7

And even more when we use font implied before for the UI (Tahoma used here instead of the default SegoeUI): http://tinyurl.com/6xqc99g

Turning off hardware acceleration of course resolve the problem.

Comment 47

[Kai Liu]

(In reply to comment #46)
> This new pref seems to break about everything when Cleartype is turned off:

Please see bug 662115.

The patches in this bug did not create the problem that you describe, since that problem pre-dates this bug.  It merely exposed a problem that already existed.

Comment 48

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

Attachment: Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides

The problem is that the changes in this bug make things break with default prefs and Cleartype disabled. This patch fixes that.

It doesn't perfectly handle disabling of Cleartype without restarting the browser, but we already don't handle that perfectly, and I think that's OK.

Comment 49

[Jonathan Kew [:jfkthame]]

Comment on attachment 537479 [details] [diff] [review]
Part 6: If Cleartype is disabled, just use default rendering parameters and ignore GDI_CLASSIC overrides

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

If ClearType is disabled, we should probably use DirectWrite's default rendering parameters - i.e. from IDWriteFactory::CreateRenderingParams - rather than our mRenderingParams, which will still fail if the cleartype_params prefs are set inappropriately. I don't have time to test it tonight, but will try this out tomorrow (if you don't get to it and post an update before then).

Comment 50

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

Attachment: Part 6 alternative

is this what you want?

Comment 51

[Jonathan Kew [:jfkthame]]

Attachment: part 6- another alternative approach

I was hoping we could do something along these lines, and actually handle the case where font smoothing is disabled (or enabled) while the browser is running; I don't like the fact that it's currently possible to end up in a state where basically no text gets drawn. (E.g., with your "alternative" patch, go to the system control panel and disable smoothing, then zoom the browser window to enlarge text: virtually all of it disappears.)

However, this version still doesn't handle all situations correctly, so some further followup is needed if we're going to handle on-the-fly changes.

Comment 52

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

I wrote a patch like this, but it still doesn't handle dynamic changes properly because RenderingParams() doesn't get called very often (which is a good thing!). Surfaces initialized while Cleartype is enabled continue to use the old settings after Cleartype is disabled.

Personally I don't see that handling dynamic Cleartype changes cleanly without restarting should be a high priority compared to the big issues we're grappling with here.

Comment 53

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

Comment on attachment 537525 [details] [diff] [review]
part 6- another alternative approach

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

So I think this as acceptable as the others.

Comment 54

[Jonathan Kew [:jfkthame]]

Comment on attachment 537492 [details] [diff] [review]
Part 6 alternative

OK, I suggest we take this patch for now, as the simplest/cleanest solution that should fix the basic issue for users with ClearType disabled.

I'd really like to handle dynamic changes without getting into states with missing text - after all, the rest of the Windows environment responds immediately to changes in font-smoothing settings - but we could do that as a followup bug; it's more urgent to fix the 'static' issue here.

Comment 55

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

Attachment: Part 6.1: Expose cairo_win32_get_system_text_quality

Comment 56

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

Attachment: Part 6.2: Handle dynamic changes to Cleartype enabled/disabled

I believe this is what you ordered :-). With this patch, changes to the Cleartype setting are reflected immediately in all browser windows.

The patch ensures that all gfxFonts are recreated when the Cleartype setting changes. So when Cleartype is disabled, the antialias_mode in every cairo_dwrite_scaled_font_t defaults to something other than CAIRO_ANTIALIAS_SUBPIXEL. This patch ignores attempts to set "GDI classic" mode for fonts that don't have CAIRO_ANTIALIAS_SUBPIXEL.

Comment 57

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

Attachment: Part 6.2 v2

Actually, here's just the part that handles dynamic changes to the Cleartype setting.

Comment 58

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

Attachment: Part 6.2 v3

... and with a crash fix if you happen to change the Cleartype setting just as a page is being torn down.

Comment 59

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

Attachment: Part 6.3: Allow force_gdi_classic only if Cleartype is enabled

Comment 60

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

Attachment: Part 7: Only use force_gdi_classic_for_families when rendering_mode parameter is -1

I think this fixes the issue John raised.

Comment 61

[Jonathan Kew [:jfkthame]]

Parts 6.1 - 6.3 definitely make things better, but we still have the issue that if you turn off font smoothing at the system level while the rendering_mode is explicitly set, most text disappears.

Comment 62

[Jonathan Kew [:jfkthame]]

Attachment: part 6.3 alternative - handle rendering modes and cleartype setting more carefully

Here's an alternative implementation of part 6.3 (based on the earlier patch that created distinct "custom", "gdi classic" and "default" rendering params); I think this fixes the disappearing text issues, by ensuring we use DW's default parameters instead of our custom ClearType params if CT is disabled.

Comment 63

[Jonathan Kew [:jfkthame]]

Comment on attachment 537716 [details] [diff] [review]
Part 6.1: Expose cairo_win32_get_system_text_quality

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

Comment 64

[Jonathan Kew [:jfkthame]]

Comment on attachment 537722 [details] [diff] [review]
Part 6.2 v3

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

::: layout/base/nsPresContext.cpp
@@ +776,3 @@
>  {
> +  if (!mShell)
> +    return;

style guide calls for {braces}

::: widget/src/windows/nsWindow.cpp
@@ +4470,5 @@
> +    Preferences::GetBool(kPrefName, PR_FALSE);
> +  Preferences::SetBool(kPrefName, !fontInternalChange);
> +}
> +
> +static PRBool CleartypeSettingChanged()

ClearType normally gets an uppercase T, I think.

@@ +4836,5 @@
> +        ForceFontUpdate();
> +        gfxFontCache *fc = gfxFontCache::GetCache();
> +        if (fc) {
> +          fc->Flush();
> +        }

Do we need to explicitly flush the textrun cache as well, to make sure textruns will be created afresh (with possibly different metrics)?

Comment 65

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

(In reply to comment #61)
> Parts 6.1 - 6.3 definitely make things better, but we still have the issue
> that if you turn off font smoothing at the system level while the
> rendering_mode is explicitly set, most text disappears.

That's what part 7 is about.

Comment 66

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

Oh, I see what you mean. You want the rendering mode that forces Cleartype to still render something when Cleartype is disabled. I don't care *too* much about that myself...

Comment 67

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

(In reply to comment #64)
> ::: layout/base/nsPresContext.cpp
> @@ +776,3 @@
> >  {
> > +  if (!mShell)
> > +    return;
> 
> style guide calls for {braces}

I don't think it does around "return", "break" and "continue".

> ::: widget/src/windows/nsWindow.cpp
> @@ +4470,5 @@
> > +    Preferences::GetBool(kPrefName, PR_FALSE);
> > +  Preferences::SetBool(kPrefName, !fontInternalChange);
> > +}
> > +
> > +static PRBool CleartypeSettingChanged()
> 
> ClearType normally gets an uppercase T, I think.

OK.

> @@ +4836,5 @@
> > +        ForceFontUpdate();
> > +        gfxFontCache *fc = gfxFontCache::GetCache();
> > +        if (fc) {
> > +          fc->Flush();
> > +        }
> 
> Do we need to explicitly flush the textrun cache as well, to make sure
> textruns will be created afresh (with possibly different metrics)?

gfxTextRunWordCache observes pref changes to "font.*". gfxTextRunCache does need to be flushed, but currently nothing flushes it even for font changes for which it definitely should be flushed already! I will fix that in a separate patch.

Comment 68

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

Comment on attachment 537873 [details] [diff] [review]
part 6.3 alternative - handle rendering modes and cleartype setting more carefully

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

Comment 69

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

Comment on attachment 537724 [details] [diff] [review]
Part 6.3: Allow force_gdi_classic only if Cleartype is enabled

Obsoleted by Jonathan's part 6.3

Comment 70

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

Comment on attachment 537725 [details] [diff] [review]
Part 7: Only use force_gdi_classic_for_families when rendering_mode parameter is -1

Obsoleted by Jonathan's part 6.3

Comment 71

[Kai Liu]

Comment on attachment 537873 [details] [diff] [review]
part 6.3 alternative - handle rendering modes and cleartype setting more carefully

>     DWRITE_RENDERING_MODE renderingMode =
>         mRenderingMode >= DWRITE_RENDERING_MODE_DEFAULT && mRenderingMode <= DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC ?
>-            (DWRITE_RENDERING_MODE)mRenderingMode : defaultParams->GetRenderingMode();
>+            (DWRITE_RENDERING_MODE)mRenderingMode : DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL;

I think that should be mDefaultRenderingParams->GetRenderingMode() instead of DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL.  A rendering mode initializes to a default of DWRITE_RENDERING_MODE_DEFAULT, which happens to render like DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL under most circumstances, but the two are not technically equivalent.

Comment 72

[John Daggett (:jtd)]

(In reply to comment #71)
> Comment on attachment 537873 [details] [diff] [review] [review]
> part 6.3 alternative - handle rendering modes and cleartype setting more
> carefully
> 
> >     DWRITE_RENDERING_MODE renderingMode =
> >         mRenderingMode >= DWRITE_RENDERING_MODE_DEFAULT && mRenderingMode <= DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL_SYMMETRIC ?
> >-            (DWRITE_RENDERING_MODE)mRenderingMode : defaultParams->GetRenderingMode();
> >+            (DWRITE_RENDERING_MODE)mRenderingMode : DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL;
> 
> I think that should be mDefaultRenderingParams->GetRenderingMode() instead
> of DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL.  A rendering mode initializes to
> a default of DWRITE_RENDERING_MODE_DEFAULT, which happens to render like
> DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL under most circumstances, but the
> two are not technically equivalent.

Actually, DWRITE_RENDERING_MODE_DEFAULT maps to a mode based on the size used:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-d2d-surface.cpp#3630

But I agree with your point, setting DWRITE_RENDERING_MODE_CLEARTYPE_NATURAL for out-of-bounds parameters doesn't make sense here.

Comment 73

[Joe Drew (not getting mail)]

There's really a lot of patch here.

My recommendation is to let the full set of patches bake on mozilla-central for a little while (a week?) before pushing the whole lot to Aurora.

Comment 74

[Jonathan Kew [:jfkthame]]

(In reply to comment #70)
> Comment on attachment 537725 [details] [diff] [review] [review]
> Part 7: Only use force_gdi_classic_for_families when rendering_mode
> parameter is -1
> 
> Obsoleted by Jonathan's part 6.3

I don't think part 7 is obsoleted by that patch; at least it wasn't expected to be. We still need an additional change to make sure that an explicitly-set mode overrides the GDI Classic setting for fonts in the force_classic list. But AFAICT the part 7 patch didn't fully work; if I set the render_mode pref to 5, it looked like it was correctly using Natural glyphs for these fonts, but laying out with GDI metrics, resulting in irregular spacing.

So I think we still want a version of Part 7, but it needs further work...

Comment 75

[Jonathan Kew [:jfkthame]]

(In reply to comment #67)
> > style guide calls for {braces}
> 
> I don't think it does around "return", "break" and "continue".

AFAICS, there's no such exception mentioned, though I know lots of existing code omits them. I'll leave it to your judgment... there are better things to spend our time on!

> gfxTextRunWordCache observes pref changes to "font.*". gfxTextRunCache does
> need to be flushed, but currently nothing flushes it even for font changes
> for which it definitely should be flushed already! I will fix that in a
> separate patch.

OK, thanks.

Comment 76

[Jonathan Kew [:jfkthame]]

Attachment: part 7: only use force_gdi_classic if an explicit rendering mode has not been set

Updated version of part 7 - I think this handles the interaction now, so that the force-GDI list only applies if the user has not explicitly forced a rendering mode in the prefs.

Comment 77

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

Comment on attachment 537969 [details] [diff] [review]
part 7: only use force_gdi_classic if an explicit rendering mode has not been set

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

Can you land this lot? :-)

Comment 78

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

Comment on attachment 537969 [details] [diff] [review]
part 7: only use force_gdi_classic if an explicit rendering mode has not been set

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

Can you land this lot? :-)

Comment 79

[Jonathan Kew [:jfkthame]]

(In reply to comment #78)
> Can you land this lot? :-)

OK - to be on the safe side, I'll push them to try this evening, then I can land them tomorrow morning my time.

Comment 80

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/63c8f645cc1c
http://hg.mozilla.org/integration/mozilla-inbound/rev/3eb99af46b93
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ccc676a3a2c
http://hg.mozilla.org/integration/mozilla-inbound/rev/5607cca3f7bf

Comment 81

[Jonathan Kew [:jfkthame]]

Argh - I just pushed these to mozilla-central without realizing you'd landed them on mozilla-inbound .... somehow missed the relevant bugmail.

Apologies to whoever handles the m-i -> m-c merge. :( There will be a couple of minor conflicts, as I tweaked the comments in all.js and also addressed comments #71-72 here.

http://hg.mozilla.org/mozilla-central/rev/3329c9d9a4ae (pt 6.1)
http://hg.mozilla.org/mozilla-central/rev/8524ae3117e4 (pt 6.2)
http://hg.mozilla.org/mozilla-central/rev/91c6afbd9046 (pt 6.3)
http://hg.mozilla.org/mozilla-central/rev/5795ed49996d (pt 7)

Comment 82

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

You should probably back them out of mozilla-inbound.

Comment 83

[Jonathan Kew [:jfkthame]]

OK, backed out all 4 changesets in comment #80 from mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/f0472d3bf28a

AFAICS this should allow the inbound->central merge to proceed harmlessly.

Comment 84

[John Bez]

So after landing of pt. 7 it's not possible to use 'natural symmetric' (gfx.font_rendering.cleartype_params.rendering_mode = 5) and retain the ability to force GDI classic rendering for some font families?

Comment 85

[Jonathan Kew [:jfkthame]]

(In reply to comment #84)
> So after landing of pt. 7 it's not possible to use 'natural symmetric'
> (gfx.font_rendering.cleartype_params.rendering_mode = 5) and retain the
> ability to force GDI classic rendering for some font families?

That's true.

I think (IIRC) that the default behavior (with gfx.font_rendering.cleartype_params.rendering_mode = -1) will be to use natural symmetric for text above a certain size, in accordance with MS recommendations. So it doesn't seem like there should be much need to explicitly select it.

Comment 86

[John Bez]

AFAIK it's only for fonts sizes over 16ppem, and i prefer natural symmetric for all sizes because at smaller sizes 'natural' rendering is either too light or, if turn up the contrast, too jagged (especially obvious on letters like S).
Should i file a new bug asking for ability to change rendering mode AND to force GDI classic rendering for some font families or the chances of it getting fixed are close to none so it's pointless?

Comment 87

[Jonathan Kew [:jfkthame]]

I don't know what the chances would be..... might depend how complex it makes things. How would you suggest that should be specified in prefs?

Comment 88

[[Baboo]]

Revert it to not care about whether gfx.font_rendering.cleartype_params.rendering_mode is set to -1 and introduce an separate setting for turning it on/off?

Comment 89

[Kai Liu]

(In reply to comment #88)
> introduce an separate setting for turning it on/off?

You can just set force_gdi_classic_max_size to 0 (or clear out the font list) to turn off the GDI classic fallback.

Comment 90

[Fanolian]

I agree with John Bez that some fonts are rendered better in natural_symmetric even in small size. That is why some of us explicitly set gfx.font_rendering.cleartype_params.rendering_mode to 5 instead of leaving it to -1 because we want to have natural_symmetric at all font sizes for those fonts, and set force_gdi_classic_for_families to use GDI_classic for fonts that suit.
Part 7 of this patch is breaking this practice.

Bug 652141 is discussing changing the default setting for gfx.font_rendering.cleartype_params.rendering_mode to GDI classic. So either bug 652141 is a WONTFIX, or part 7 of this patch is redundant.

Comment 91

[John Bez]

(In reply to comment #87)
> I don't know what the chances would be..... might depend how complex it
> makes things. How would you suggest that should be specified in prefs?

Baboo and Kai Liu already said it all.
The easiest solution would be to backout pt. 7, because anyone who knows how to change rendering mode will also know how to clear out gfx.font_rendering.cleartype_params.force_gdi_classic_for_families if he or she wants all fonts rendered the same.
A little harder solution would be to add pref for forcing forcing GDI classic for certain font families, something like boolean gfx.font_rendering.cleartype_params.force_force_gdi_classic_for_families :)

Comment 92

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

I'll leave it up to John Daggett to decide whether we want patch 7 or not.

Comment 93

[Kai Liu]

Two more fonts to consider for the fallback list:

1) Microsoft Sans Serif -- This is the default font used for form controls (it's what MS Shell Dlg maps to).  Since it is an adaptation of the old MS Sans Serif raster font, a GDI-style rendering is arguably closest to its "intended" appearance.  (And it looks better that way, too.)

2) Consolas -- It is somewhat widely used as a code font (developer.mozilla.org's fancy code formatting thing uses it, and so does the MSDN library).  The fuzzier appearance of the natural rendering of this font at 10pt (esp. with color) was one of the reasons Microsoft backed down on using natural in VS2010.  Also, as a monospace font, the positioning fidelity of natural seems less relevant.


(I could file a new bug if that's preferred, but I got the impression from the recent round of patches is that the preference is to keep all the related stuff consolidated together in this bug.)

Comment 94

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

Let's not add anything more to this bug. It was my mistake to do that in the first place.

Comment 95

[Kai Liu]

(In reply to comment #94)
> Let's not add anything more to this bug.

Okay, I filed that request as bug 664055.

Comment 96

[John Bez]

Any news from John Daggett on patch 7?

Comment 97

[Jonathan Kew [:jfkthame]]

Attachment: parts 1-6 rebased to mozilla-aurora tip

This is a roll-up of parts 1 to 6.3, merged to the current tip of mozilla-aurora ready for landing there. (Carrying forward r+ from the m-c patches.)

I'm keeping part 7 separate as there's a pending question of whether we really want to keep this, or revert to the behavior we had without it.

Comment 98

[Jonathan Kew [:jfkthame]]

Attachment: part 7 rebased to mozilla-aurora tip

Part 7 for aurora, if we decide to keep it (see comments #84 and following). The alternative is to back it out on central - we shouldn't leave the two with different behavior.

Comment 99

[Joe Drew (not getting mail)]

Jonathan, have you done a crash-stats and input check to make sure that these patches haven't caused any regressions?

Comment 100

[Jonathan Kew [:jfkthame]]

I don't see any evidence in crash-stats (looking at the topcrashes for the past week on 7.0a1) that these have had any effect (positive or negative) there.

I did notice someone on input.mozilla.com remarked that fonts are better on nightlies, which is what we'd expect; otherwise, just the usual complaints about blurriness (referring to the DW rendering in release versions, I assume).

So in short, I didn't see any indication of regressions. (My searching may not be particularly expert, however!)

Comment 101

[LegNeato]

Asa, can you weigh in on this form a product standpoint? This seems like too risky a change to take 1/2 way through aurora but if product concerns trump the risk we can look to take it.

Comment 102

[John Daggett (:jtd)]

(In reply to comment #92)
> I'll leave it up to John Daggett to decide whether we want patch 7 or not.

One problem I see now is that changing the two force_xxx prefs still don't appear to be live while the rendering mode pref is.  This is confusing and it makes it hard to test differences in rendering without restarting.

So I guess I would be fine with allowing the force_gdi prefs to override the rendering mode pref as long as the force_xxx prefs were live.  But that should also probably happen in a separate bug.

Comment 103

[Joe Drew (not getting mail)]

(In reply to comment #101)
> Asa, can you weigh in on this form a product standpoint? This seems like too
> risky a change to take 1/2 way through aurora but if product concerns trump
> the risk we can look to take it.

In Firefox 4, we introduced what is seen as a major regression in font rendering by many (in the form of DirectWrite's natural (symmetric) rendering mode); specifically, they think it looks blurry and/or lighter. We know this has been a major issue for our users because it's come up a lot on SUMO, especially when Firefox 4 was first released.

This series of patches fixes the major cause of that regression by using DirectWrite's "GDI classic" rendering mode for the common web fonts at small sizes, where DirectWrite's "blurriness" is most apparent.

If we wait for Firefox 7 for these patches, we're worried that we're going to bleed lots of users to alternate browsers. This is already an issue, obviously, but the longer it remains unresolved, the more users we'll lose.

I hope this is an adequate explanation of the issue!

Comment 104

[Asa Dotzler [:asa]]

Sorry I didn't get to this sooner. I think this is a pretty serious regression for quite a few Windows users. I'd hoped when we shipped 4 that people would be more accepting of the change because IE and other Windows software sees some of the same problems. Unfortunately that wasn't the case and there's been quite a bit of unrest. 

The only question I have, given how late in the game we are here, is whether it would be safer to take a "flip all fonts" pref rather than introducing code to only change it for certain fonts at certain sizes. I think we should certainly do something, either theses patches for selective falling back to GDI or a patch to flip all fonts back.

Comment 105

[Jonathan Kew [:jfkthame]]

(In reply to comment #104)
> Sorry I didn't get to this sooner. I think this is a pretty serious
> regression for quite a few Windows users. I'd hoped when we shipped 4 that
> people would be more accepting of the change because IE and other Windows
> software sees some of the same problems. Unfortunately that wasn't the case
> and there's been quite a bit of unrest. 
> 
> The only question I have, given how late in the game we are here, is whether
> it would be safer to take a "flip all fonts" pref rather than introducing
> code to only change it for certain fonts at certain sizes. I think we should
> certainly do something, either theses patches for selective falling back to
> GDI or a patch to flip all fonts back.

The latter option was filed as bug 652141. I posted a trivial patch there to flip the default value of the rendering-mode pref, but it was not accepted (see comments 28 and following).

Comment 106

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

And no reason was given why this patch can't land. Only some misleading facts like dev team fight and some devs pushing new font rendering...

Also I see no reason, why we still keeping default rendering as Natural, because 99,9% of web is using fonts in size about 8-14 and in this sizes the best results (sharp and clear) provide only GDI Classic. Even internal interface look better, look on screen shots posted in this bug #652141

Comment 107

[Asa Dotzler [:asa]]

(In reply to comment #106)

Virtual_ManPL, I appreciate your passion about this issue but your commentary is not helpful here. The Development and Product teams are fully aware of the issues and the various solutions. Continued commentary is just noise that makes it harder for us to do the work that needs to be done to improve Firefox. If you're not adding new data, please don't comment. Thanks.

Comment 108

[Kai Liu]

(In reply to comment #104)
> The only question I have, given how late in the game we are here, is whether
> it would be safer to take a "flip all fonts" pref rather than introducing
> code to only change it for certain fonts at certain sizes.

No, it would not be safer.

A wholesale switch away from DWRITE_RENDERING_MODE_DEFAULT *must* be accompanied by the patches in this bug (in particular, the part 6 patches) or else we will run into bug 662115.  So there really isn't an option here: you have to take this bug.  (Either that, or spin a new patch to fix bug 662115, but that doesn't make sense from a risk perspective.)

Comment 109

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

Kai is correct.

Comment 110

[Asa Dotzler [:asa]]

Comment on attachment 539812 [details] [diff] [review]
parts 1-6 rebased to mozilla-aurora tip

After considerable discussion, I've concluded that this is not an appropriate change for Aurora. No one wants to see this fixed more than me, but given the size of the changes and the fact that no more than 10-15% of our users are going to be affected by the regression, I can't see how it warrants putting the Firefox 6 release at risk to get it a fix to users six weeks sooner.

Comment 111

[[Baboo]]

Please notice that Microsoft has now released an update for Arial, Tahoma and Verdana apparently for improving the rendering with DirectWrite at small sizes:
http://support.microsoft.com/kb/2545698/en-us?fr=1

Comment 112

[Kai Liu]

Attachment: KB2545698.png, before and after

(In reply to comment #111)
> Please notice that Microsoft has now released an update for Arial, Tahoma
> and Verdana apparently for improving the rendering with DirectWrite at small
> sizes:
> http://support.microsoft.com/kb/2545698/en-us?fr=1

The three fonts from KB2545698, at the sizes mentioned in the article, at 96DPI.

left=before, right=after, top=Firefox (GDI fallback disabled), bottom=IE9

Comment 113

[John Daggett (:jtd)]

(In reply to comment #112)
> Created attachment 542624 [details]
> KB2545698.png, before and after
>
> The three fonts from KB2545698, at the sizes mentioned in the article, at
> 96DPI.
> 
> left=before, right=after, top=Firefox (GDI fallback disabled), bottom=IE9

Rendered with what gamma/enhanced contrast settings?  See about:support for this info.

Comment 114

[Kai Liu]

(In reply to comment #113)
> Rendered with what gamma/enhanced contrast settings?

Default.  (-1)

Comment 115

[John Daggett (:jtd)]

(In reply to comment #114)
> (In reply to comment #113)
> > Rendered with what gamma/enhanced contrast settings?
> 
> Default.  (-1)

What shows up under Cleartype parameter settings, as listed in about:support in a nightly build?  "Default" is dependent upon whether you've run the Cleartype tuner or not.

Comment 116

[Kai Liu]

(In reply to comment #115)
> What shows up under Cleartype parameter settings, as listed in about:support
> in a nightly build?  "Default" is dependent upon whether you've run the
> Cleartype tuner or not.

Parameters not found; I never ran the system's CT tuner on this particular machine.

Comment 117

[Csaba Kozák [:WonderCsabo]]

(In reply to comment #111)
> Please notice that Microsoft has now released an update for Arial, Tahoma
> and Verdana apparently for improving the rendering with DirectWrite at small
> sizes:
> http://support.microsoft.com/kb/2545698/en-us?fr=1

Rendering these fonts with GDI Classic is still necessary? IMHO it is not.

Comment 118

[David E. Ross]

Please clarify:  Has this been implemented only for Windows 7?  I ask this because core fonts for a given operating system and platform are not necessarily the core fonts for other operating systems and platforms.  See <http://www.codestyle.org/css/font-family/sampler-CombinedResultsFull.shtml>.

Comment 119

[[Baboo]]

DirectWrite rendering is only available for Windows Vista and newer.