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

RESOLVED FIXED in mozilla7

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
mozilla7
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5-)

Details

Attachments

(12 attachments, 13 obsolete attachments)

29.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.81 KB, patch
jtd
: review+
Details | Diff | Splinter Review
1.35 KB, patch
jtd
: review+
Details | Diff | Splinter Review
18.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.62 KB, patch
jtd
: review+
Details | Diff | Splinter Review
3.98 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
5.77 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
13.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
65.84 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
5.86 KB, patch
Details | Diff | Splinter Review
20.67 KB, image/png
Details
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.
Created attachment 536823 [details] [diff] [review]
fix
Assignee: nobody → roc
Attachment #536823 - Flags: review?(jfkthame)
Attachment #536823 - Flags: review?(jdaggett)
Created attachment 536824 [details] [diff] [review]
force GDI Classic for the "core Web fonts"
Attachment #536824 - Flags: review?(jfkthame)
Attachment #536824 - Flags: review?(jdaggett)
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?
Oops, yeah, I missed that one.
Created attachment 536836 [details] [diff] [review]
force GDI Classic for the "core Web fonts"
Attachment #536824 - Attachment is obsolete: true
Attachment #536824 - Flags: review?(jfkthame)
Attachment #536824 - Flags: review?(jdaggett)
Attachment #536836 - Flags: review?(jfkthame)
Attachment #536836 - Flags: review?(jdaggett)
And Arial Black isn't covered by Arial, is it? Intentionally left out for other reasons?
I believe that with DirectWrite, Arial Black is part of the Arial family.
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 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.
Attachment #536836 - Flags: review?(jfkthame) → review+
(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.
Created attachment 536865 [details] [diff] [review]
fix v2
Attachment #536823 - Attachment is obsolete: true
Attachment #536823 - Flags: review?(jfkthame)
Attachment #536823 - Flags: review?(jdaggett)
Attachment #536865 - Flags: review?(jfkthame)
Attachment #536865 - Flags: review?(jdaggett)
Created attachment 536866 [details] [diff] [review]
force GDI Classic for the "core Web fonts", v2
Attachment #536866 - Flags: review?(jdaggett)
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.
Attachment #536865 - Flags: review?(jfkthame) → review+
(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).
I'll probably just land this on trunk tomorrow morning.
Whiteboard: [needs landing]

Comment 16

6 years ago
(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?
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

6 years ago
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

6 years ago
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
> 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

6 years ago
(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
(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

6 years ago
(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

6 years ago
(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
(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.
Created attachment 537036 [details] [diff] [review]
Part 1: implement GDI-classic family list pref
Attachment #536865 - Attachment is obsolete: true
Attachment #536865 - Flags: review?(jdaggett)
Attachment #537036 - Flags: review+
Created attachment 537038 [details] [diff] [review]
Part 2: Add a pref to limit the forcing of 'GDI Classic' to a maximum font size
Attachment #537038 - Flags: review?(jdaggett)
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...
Attachment #537038 - Flags: review?(jfkthame)
Created attachment 537039 [details] [diff] [review]
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.
Attachment #537039 - Flags: review?(jdaggett)
Comment on attachment 537039 [details] [diff] [review]
Part 3: Apply 'GDI Classic' prefs to @font-face local()

Whoever gets to it first
Attachment #537039 - Flags: review?(jfkthame)
Created attachment 537040 [details] [diff] [review]
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.
Attachment #537040 - Flags: review+
Created attachment 537041 [details] [diff] [review]
Part 5: Fix reftests

This is actually Jonathan's patch that I stole and reviewed :-)
Attachment #537041 - Flags: review+
Attachment #536836 - Attachment is obsolete: true
Attachment #536836 - Flags: review?(jdaggett)
Attachment #536866 - Attachment is obsolete: true
Attachment #536866 - Flags: review?(jdaggett)
New tryserver push: http://tbpl.mozilla.org/?tree=Try&rev=28df502c24ee
Whiteboard: [needs landing]

Comment 34

6 years ago
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.
Created attachment 537064 [details] [diff] [review]
Part 4 v2
Attachment #537040 - Attachment is obsolete: true
Attachment #537064 - Flags: review?(jdaggett)

Updated

6 years ago
Attachment #537039 - Flags: review?(jdaggett) → review+

Updated

6 years ago
Attachment #537064 - Flags: review?(jdaggett) → review+

Comment 36

6 years ago
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
Attachment #537038 - Flags: review?(jdaggett) → review+
Thanks! I'll land this once my tryserver results come in.
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Very shortly we should apply for aurora and beta approval for this bug and for bug 624589 (which this depends on).
Er, bug 642589.

Comment 41

6 years ago
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?

Updated

6 years ago
tracking-firefox5: --- → ?

Comment 42

6 years ago
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

6 years ago
(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

6 years ago
OK, that's good.

Updated

6 years ago
Blocks: 661869

Comment 45

6 years ago
(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.

Updated

6 years ago
Blocks: 661917
Attachment #537038 - Flags: review?(jfkthame)
Attachment #537039 - Flags: review?(jfkthame)
Depends on: 642589

Comment 46

6 years ago
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

6 years ago
(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.
Created attachment 537479 [details] [diff] [review]
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.
Attachment #537479 - Flags: review?(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).
Created attachment 537492 [details] [diff] [review]
Part 6 alternative

is this what you want?
Attachment #537492 - Flags: review?(jfkthame)
Created attachment 537525 [details] [diff] [review]
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.
Attachment #537525 - Flags: feedback?(roc)
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 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.
Attachment #537525 - Flags: feedback?(roc) → feedback+
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.
Attachment #537492 - Flags: review?(jfkthame) → review+

Updated

6 years ago
tracking-firefox5: ? → -
Created attachment 537716 [details] [diff] [review]
Part 6.1: Expose cairo_win32_get_system_text_quality
Attachment #537716 - Flags: review?(jfkthame)
Created attachment 537717 [details] [diff] [review]
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.
Attachment #537479 - Attachment is obsolete: true
Attachment #537492 - Attachment is obsolete: true
Attachment #537525 - Attachment is obsolete: true
Attachment #537479 - Flags: review?(jfkthame)
Attachment #537717 - Flags: review?
Created attachment 537718 [details] [diff] [review]
Part 6.2 v2

Actually, here's just the part that handles dynamic changes to the Cleartype setting.
Attachment #537718 - Flags: review?(jfkthame)
Created attachment 537722 [details] [diff] [review]
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.
Attachment #537717 - Attachment is obsolete: true
Attachment #537718 - Attachment is obsolete: true
Attachment #537717 - Flags: review?
Attachment #537718 - Flags: review?(jfkthame)
Attachment #537722 - Flags: review?(jfkthame)
Created attachment 537724 [details] [diff] [review]
Part 6.3: Allow force_gdi_classic only if Cleartype is enabled
Attachment #537724 - Flags: review?
Attachment #537724 - Flags: review? → review?(jfkthame)
Created attachment 537725 [details] [diff] [review]
Part 7: Only use force_gdi_classic_for_families when rendering_mode parameter is -1

I think this fixes the issue John raised.
Attachment #537725 - Flags: review?(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.
Created attachment 537873 [details] [diff] [review]
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.
Attachment #537873 - Flags: review?(roc)
Comment on attachment 537716 [details] [diff] [review]
Part 6.1: Expose cairo_win32_get_system_text_quality

Review of attachment 537716 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #537716 - Flags: review?(jfkthame) → review+
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)?
(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.
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...
(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 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]:
-----------------------------------------------------------------
Attachment #537873 - Flags: review?(roc) → review+
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
Attachment #537724 - Attachment is obsolete: true
Attachment #537724 - Flags: review?(jfkthame)
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
Attachment #537725 - Attachment is obsolete: true
Attachment #537725 - Flags: review?(jfkthame)

Comment 71

6 years ago
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

6 years ago
(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.
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.
(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...
(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.
Attachment #537722 - Flags: review?(jfkthame) → review+
Created attachment 537969 [details] [diff] [review]
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.
Attachment #537969 - Flags: review?(roc)
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? :-)
Attachment #537969 - Flags: review?(roc) → review+
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? :-)
(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.
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
Whiteboard: [inbound]
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)
You should probably back them out of mozilla-inbound.
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.
Whiteboard: [inbound]

Comment 84

6 years ago
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?
(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

6 years ago
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?
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

6 years ago
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

6 years ago
(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

6 years ago
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

6 years ago
(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 :)
I'll leave it up to John Daggett to decide whether we want patch 7 or not.

Comment 93

6 years ago
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.)
Let's not add anything more to this bug. It was my mistake to do that in the first place.

Updated

6 years ago
Blocks: 664055

Comment 95

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

Okay, I filed that request as bug 664055.

Comment 96

6 years ago
Any news from John Daggett on patch 7?
Created attachment 539812 [details] [diff] [review]
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.
Attachment #539812 - Flags: review+
Attachment #539812 - Flags: approval-mozilla-aurora?
Created attachment 539814 [details] [diff] [review]
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.
Attachment #539814 - Flags: approval-mozilla-aurora?
Jonathan, have you done a crash-stats and input check to make sure that these patches haven't caused any regressions?
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

6 years ago
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 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.
(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

6 years ago
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.
(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).
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

6 years ago
(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

6 years ago
(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.)
Kai is correct.

Comment 110

6 years ago
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.
Attachment #539812 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #539814 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Target Milestone: --- → mozilla7

Comment 111

6 years ago
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

6 years ago
Created attachment 542624 [details]
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
(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

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

Default.  (-1)
(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

6 years ago
(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.

Updated

6 years ago
Blocks: 668162
(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

6 years ago
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

6 years ago
DirectWrite rendering is only available for Windows Vista and newer.

Updated

6 years ago
Depends on: 675383

Updated

6 years ago
Depends on: 691157

Updated

6 years ago
Depends on: 700671

Updated

5 years ago
Depends on: 715461

Updated

5 years ago
Depends on: 783381

Updated

5 years ago
No longer depends on: 783381

Updated

4 years ago
Blocks: 651789

Updated

3 years ago
Depends on: 1078021
Blocks: 703625
You need to log in before you can comment on or make changes to this bug.