642589 - expose prefs for tweaking DirectWrite/D2D antialiasing behavior

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

This is intended to help with bug 635490 (and the various other complaints about text rendering with HW acceleration) - I've filed it separately because this addresses one specific issue. It will not "solve" everything described there; DWrite/D2D text _will_ still be different from GDI text. But it may help with the perception that FF4 text is different from IE9 text in some cases.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: experimental patch to increase the default EnhancedContrast value

This patch modifies the RenderingParams used with DirectWrite to set the EnhancedContrast value to 1.0 if the user has not used the ClearType Tuner control panel to specifically adjust rendering.

It also implements a new preference, gfx.font_rendering.cleartype_params, which can be used to override the default parameters and experiment with different antialiasing characteristics. This pref expects three semicolon-separated (floating-point) numbers, which will be used for the Gamma, EnhancedContrast, and ClearTypeLevel parameters in CreateCustomRenderingParamss as documented at http://msdn.microsoft.com/en-us/library/dd368190(v=vs.85).aspx. For any of the three that are given negative (invalid) values, defaults will be used instead.

Comment 2

[Jonathan Kew [:jfkthame]]

A tryserver build with this patch will shortly be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-f6f4ee04aded/.

Comment 3

[skylerlaughs]

(In reply to comment #2)
> A tryserver build with this patch will shortly be available at
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-f6f4ee04aded/.

The new defaults in this test build are a huge improvement over the current RC. Much more readable on my screen (still rocking a nice CRT). Will something like this be pushed before Firefox 4 goes final?

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

What range of values CreateCustomRenderingParams() expect? A better description of what the options -do- can be found here:
http://msdn.microsoft.com/en-us/library/aa970267.aspx
... but that article specifies integer ranges, where the function takes floating point values.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to comment #3)
> The new defaults in this test build are a huge improvement over the current RC.
> Much more readable on my screen (still rocking a nice CRT). Will something like
> this be pushed before Firefox 4 goes final?

If you are using a CRT then I would think that you would want to disable subpixel AA altogether. Is that not the case?

Comment 6

[skylerlaughs]

(In reply to comment #5)
> (In reply to comment #3)
> > The new defaults in this test build are a huge improvement over the current RC.
> > Much more readable on my screen (still rocking a nice CRT). Will something like
> > this be pushed before Firefox 4 goes final?
> 
> If you are using a CRT then I would think that you would want to disable
> subpixel AA altogether. Is that not the case?

I have tested every different AA setting on this Windows 7 box that I am aware
of the registry keys for. This includes configuring clear type for a CRT
display instead of a RGB patterned LCD.  Nothing looks better then the default
Windows 7 clear type settings and true GDI+ text.

If the Firefox 4 direct write text could be made to look like Chrome (GDI+ with
Windows 7 clear type) I would probably never open another browser. Disabling
hardware acceleration provides good text quality, but at too much of a cost.
Firefox 4 is snappy with it, without ... not so much.  :)

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to comment #4)
> What range of values CreateCustomRenderingParams() expect? A better description
> of what the options -do- can be found here:
> http://msdn.microsoft.com/en-us/library/aa970267.aspx
> ... but that article specifies integer ranges, where the function takes
> floating point values.

Gamma values of around 1.8-2.2 seem to be typical, which suggests they're 1/1000 of the registry value. I think ClearType Level should range from 0.0 to 1.0, corresponding to registry values of 0 to 100. (But you can experiment with values > 1.0, and create some garish effects!) As for Enhanced Contrast: this seems to be something new, distinct from the Text Contrast mentioned in that article (note that running the ClearType Tuner creates _both_ TextContrastLevel and EnhancedContrastLevel registry keys, with different values).

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Note that setting FontSmoothingGamma to 2200 in the registry makes (GDI) fonts very slim indeed - so I'm confused about what the relation is between the gamma setting used to say, calibrate your monitor, and the gamma setting used for font rendering.

Setting gamma value in gfx.font_rendering.cleartype_params to 2.2 doesn't make the DirectWrite fonts as slim as the results I saw on my system with the registry setting, but that matches what people have reported - that while using the ClearType tuner does affect DirectWrite, it is not affected in the same way as the GDI font rendering used in the tuner (so if you use it to tweak Firefox, you're essentially doing so blindly).

Setting the gamma value in gfx.font_rendering.cleartype_params to 1.0 makes fonts very thick and dark indeed (I've set it to 1.9 for now, which the article I linked to above says is the default value for ClearType, and it looks good to me - but I've been used to DirectWrite font rendering for a while now).

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

Incidentally, the comment in the description of CreateCustomRenderingParams() suggests getting default parameters by calling CreateMonitorRenderingParams() - does Firefox's DW implementation do this? It also suggests some reasonable default values, though that seems like a personal preference.

CreateMonitorRenderingParams():
http://msdn.microsoft.com/en-us/library/dd368199.aspx

Comment 10

[skylerlaughs]

Attachment: arstechnica - Chrome

Comment 11

[skylerlaughs]

Attachment: arstechnica - FF4 RC

Comment 12

[skylerlaughs]

Attachment: arstechnica - FF4 Test Patch 1.0;0.0;1.0

Comment 13

[skylerlaughs]

Attachment: itworld - Chrome

Comment 14

[skylerlaughs]

Attachment: itworld - FF4 RC

Comment 15

[skylerlaughs]

Attachment: itworld - FF4 Test Patch 1.0;0.0;1.0

Comment 16

[skylerlaughs]

Attachment: reddit - Chrome

Comment 17

[skylerlaughs]

Attachment: reddit - FF4 RC

Comment 18

[skylerlaughs]

Attachment: reddit - FF4 Test Patch 1.0;0.0;1.0

Comment 19

[skylerlaughs]

I have tested the available settings quite extensively, and I have found that 1.0;0.0;1.0 provides good results for my eyes/display.

I have added comparison screenshots from Chrome, FF4 RC, and this test build with the 1.0;0.0;1.0 settings for itworld.com, reddit.com, and arstechnica.com. If there is a way to add several attachments at once, please forgive the single attachments.

I hope the examples speak for themselves. This is not just an improvement over the RC, but the text actually compares well to Chrome.

I may also test with 1.0;0.0;0.8 later, in case the any color fringing is too much.

Comment 20

[Emanuel Hoogeveen [:ehoogeveen]]

This is my experience as well. Perhaps the gamma setting is redundant for DirectWrite/hardware accelerated text? Matching the 2.2 gamma of my display certainly doesn't look right - I can compensate using the contrast enhancement, but that changes the color of text and makes light text look too thick (much like using a strong sharpening filter on an image).

The only thing I'm not sure of is the ClearType level - 0.5 has noticeably less fringing than 1.0, but using full ClearType feels sharper from a distance. This might be difficult to get a consensus on.

Comment 21

[Jonathan Kew [:jfkthame]]

Comment on attachment 520021 [details] [diff] [review]
experimental patch to increase the default EnhancedContrast value

So, is this something we want to do? How do we go about deciding on defaults? I suspect the "best" settings are very much a matter of personal preference, as well as perhaps being dependent on display hardware, graphics cards, and drivers.

The patch here currently keeps the defaults for Gamma and ClearType Level, and just boosts Enhanced Contrast; results look ok to my eyes on my screen, but that's hardly an extensive sample!

Comment 22

[John Daggett (:jtd)]

This patch seems to ignore multiple monitor situations (it only looks for ClearType settings for DISPLAY1).  I also think we should have three separate prefs rather than one with xxx;xxx;xxx syntax.  Or just have a single pref for enhanced contrast and skip the others.  I think the key should be whether this appears to match IE9 behavior or not.

What exactly is ClearType level setting?  Is it basically an interpolation factor where 0.0 means no Cleartype and 1.0 means "full" Cleartype and values in between are an average of the two?

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to comment #22)
> This patch seems to ignore multiple monitor situations (it only looks for
> ClearType settings for DISPLAY1). 

Yes, that's a limitation - there's no support for separately-customized per-monitor parameters, so it just looks for the existence of this key to tell whether the user has run the ClearType Tuner tool. (Note that it doesn't actually read the value from the registry, it only cares whether it has been created at all.)

> I also think we should have three separate
> prefs rather than one with xxx;xxx;xxx syntax.  Or just have a single pref for
> enhanced contrast and skip the others.  

From comments above it looks like some people prefer the results they get from adjusting gamma and/or cleartype level, not just contrast.

>I think the key should be whether this
> appears to match IE9 behavior or not.

Comparisons welcome. It seems to me that people's experience of this font rendering stuff is quite variable, depending on lots of factors, which is why providing prefs that people can experiment with seemed like a useful thing.

> What exactly is ClearType level setting?  Is it basically an interpolation
> factor where 0.0 means no Cleartype and 1.0 means "full" Cleartype and values
> in between are an average of the two?

It seems to be, as far as I can tell. Personally, I think I prefer a CT Level of 0.5 rather than 1.0, as the color-fringing effects are much less noticeable. But other people might feel it's less clear.

(You can actually set it to values > 1.0 if you really like colored edges......)

Comment 24

[Emanuel Hoogeveen [:ehoogeveen]]

Presumably 1.0 is what DirectWrite-with-Cleartype was designed for, but the fringing does seem excessive. What I would like to know is what the gamma setting is relative to, or if there's some way of knowing what it should be set to. I'm using a professionally calibrated monitor (with an ICCv2 profile, so Firefox can use it), and setting the gamma to 2.2 to match my monitor makes fonts ridiculously thin. 1.0 looks alright to me (without any contrast enhancement), maybe a little too thick compared to what I'm used to, but that's not particularly objective. I would imagine there's a color response curve it's meant to be matching, but I'm not sure what it is!

Comment 25

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

(In reply to comment #21)
> So, is this something we want to do? How do we go about deciding on defaults? I
> suspect the "best" settings are very much a matter of personal preference, as
> well as perhaps being dependent on display hardware, graphics cards, and
> drivers.

Right now I just want to match IE9 as closely as possible :-)

Comment 26

[Jonathan Kew [:jfkthame]]

Attachment: patch, default EnhancedContrast to 1.0 if ClearType Tuner has not been used

Updated patch to use tabs for indentation in cairo code.

Comment 27

[Bas Schouten (:bas.schouten)]

Comment on attachment 522367 [details] [diff] [review]
patch, default EnhancedContrast to 1.0 if ClearType Tuner has not been used

This patch looks good, but why aren't we using RefPtr<IDWriteRenderingParams> for most of this? That can avoid most of the addref/release code.

Comment 28

[John Daggett (:jtd)]

(In reply to comment #23)
> > I also think we should have three separate
> > prefs rather than one with xxx;xxx;xxx syntax.  Or just have a single pref for
> > enhanced contrast and skip the others.  
> 
> From comments above it looks like some people prefer the results they get from
> adjusting gamma and/or cleartype level, not just contrast.

The current patch still has this xxx;xxx;xxx format for the prefs.  That's really sucky, there's no easy way to document what those values are.  And using sscanf with no validation of the user input sounds a recipe for a user shooting themselves in the foot.  Please at least separate them out if you really feel we *must* have all three.  Match the MS documented names so that it's clear what the connection is.

Why is the gamma logic different (i.e. you don't read the registry value)?  Seems like the logic should be identical for all three, if all three are really necessary.

Comment 29

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

I don't think bolding fonts will fix completely this issues with broken DirectWrite rendering and blurry fonts.

Don't this will be fixed when we simply disable sub-position pixel rendering ?

Comment 30

[John Daggett (:jtd)]

Attachment: cleartype registry trace when running cleartype tuner

Comment 31

[John Daggett (:jtd)]

Attachment: cleartype waterfall testpage

Tests text at various sizes for common web fonts (and some of the C-fonts available on Windows 7).  Includes diabolical test strings like "ililili", etc.  For the diabolical test strings, each is repeated several times with different subpixel offsets.  Our rendering behavior appears almost identical to IE9, the only difference is the effect of variations in subpixel positioning of *elements*, not of the text itself.

Comment 32

[John Daggett (:jtd)]

Attachment: alternative patch, use three override prefs

Based on Jonathan's patch but reworked to use three prefs for gamma, enhanced contrast, and ClearType level instead of a single combined pref.  The only registry call is one to see if the root key for Cleartype settings exists (i.e. HKCU/Software/Microsoft/Avalon.Graphics).  There's no need to read the param values from the registry since DirectWrite does that no matter what.  The individual pref values follow the same format as used for the registry values:

  gamma [1000, 2200] ==> use gamma ranging from 1.0 to 2.2
  contrast [0, 1000] ==> use enhanced contrast value from 0.0 to 10.0
  cleartype level [0, 100] ==> 0 = grayscale, 100 = cleartype

Without running the Cleartype tuner, the defaults are gamma = 2.2 (for my machine), contrast = 0.5 and cleartype level = 1.0.  For all of these parameters, "-1" implies "use default values".

The logic in the patch overrides default value of enhanced contrast to 1.0 when the Cleartype tuner hasn't been run.  The IE9 folks claimed to be doing this but I don't see the evidence of that, they appear to be using defaults from what I've seen.  Increasing the enhanced contrast setting does make up for the "lightness" of text but it does introduce what look like slight artifacts in some glyphs at some sizes.

I omitted setting the Cleartype level to 0.5, I don't see a huge difference other than for intentially diabolical strings (e.g. "ililililil") so I think it's better not to futz with this.

Comment 33

[Jonathan Kew [:jfkthame]]

Attachment: patch, expose five DirectWrite rendering prefs

This version exposes five individual prefs for tweaking DirectWrite/D2D font rendering:

  gfx.font_rendering.cleartype_params.gamma
  gfx.font_rendering.cleartype_params.enhanced_contrast
  gfx.font_rendering.cleartype_params.cleartype_level
  gfx.font_rendering.cleartype_params.pixel_geometry
  gfx.font_rendering.cleartype_params.rendering_mode

See comments in all.js for the meaning of each. Note that they're now using integer values rather than decimals, to match values that users may see if they look in the Registry; thus, typical Gamma would be 2200, and "full" contrast and cleartype-level would be 100 (though these can be set higher to exaggerate the effect).

With DirectWrite enabled and D2D *off*, the effect of tweaking the prefs is immediately visible; with D2D enabled, it is necessary to trigger a refresh by zooming, resizing the window fractionally, etc, in order to see a change.

Note in particular that the rendering_mode pref makes it possible to get "GDI-style" ClearType rendering under D2D, for users who prefer that antialiasing. Setting 2 is "classic" GDI ClearType, and 3 is "natural-widths" GDI ClearType.

Comment 34

[Emanuel Hoogeveen [:ehoogeveen]]

John, have you compared gamma = 1.0, contrast = 0.0 with gamma = 2.2, contrast = 0.5 (or higher)? How do they compare on your setup?

Comment 35

[John Daggett (:jtd)]

(In reply to comment #33)
> See comments in all.js for the meaning of each. Note that they're now using
> integer values rather than decimals, to match values that users may see if they
> look in the Registry; thus, typical Gamma would be 2200, and "full" contrast
> and cleartype-level would be 100 (though these can be set higher to exaggerate
> the effect).

Not quite sure what you mean by "typical" but the default enhanced contrast setting is 50, not 100.

Comment 36

[John Daggett (:jtd)]

Attachment: cleartype waterfall testpage

Added ability to cycle through a set of colors, enter 'c' to cycle the text color, 'b' for the background.  Also added 10 subpixel shifts to show that IE9 shows the same fringing behavior we do.

Comment 37

[John Daggett (:jtd)]

Attachment: cleartype waterfall testpage

Added data-url encoded downloadable fonts (one TrueType, one Postscript CFF) and a slew of Typekit fonts from a variety of foundries.

Comment 38

[John Daggett (:jtd)]

(In reply to comment #33)
> Note in particular that the rendering_mode pref makes it possible to get
> "GDI-style" ClearType rendering under D2D, for users who prefer that
> antialiasing. Setting 2 is "classic" GDI ClearType, and 3 is
> "natural-widths" GDI ClearType.

This is great!  We should also clearly document that "GDI" typically means
setting 2 for all non-DirectWrite browsers when Cleartype is enabled and
that both IE9 and FF4 use setting 5, "Natural Symmetric".  It might just be
simpler to have a "GDI style" boolean pref instead that sets mode 2. 

I think a lot of the issues are for sizes <= 16px when using the basic
sans-serif webfonts.  Times New Roman and Georgia seem much more readable
with DirectWrite rendering than with GDI style.  But the sans-serif core webfonts
are definitely crisper with GDI classic rendering (i.e. Arial, Verdana).

Comment 39

[John Daggett (:jtd)]

Comment on attachment 525796 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs

+	HKEY hKey;
+	if (RegOpenKeyExA(ENHANCED_CONTRAST_REGISTRY_KEY,
+			  0, KEY_READ, &hKey) == ERROR_SUCCESS)

As mentioned in comment 32, there's no need to read these prefs,
DirectWrite is already doing that and supplying them via
Instance()->CreateRenderingParams(&defaultParams).  Just check to see if
the root key for Cleartype settings exists (i.e.
HKCU/Software/Microsoft/Avalon.Graphics) and use that to determine whether
to override the default setting.

+    if (mClearTypeLevel < 0.0) {
+	HKEY hKey;
+	if (RegOpenKeyExA(CLEARTYPE_LEVEL_REGISTRY_KEY,
+			  0, KEY_READ, &hKey) == ERROR_SUCCESS)
+	{
+	    level = defaultParams->GetClearTypeLevel();
+	    RegCloseKey(hKey);
+	} else {
+	    level = 0.5;
+	}

While I think bumping up the enhanced contrast really does help I don't see
any real need to change the ClearType level default right now.  After this
patch lands and gets a lot more real world testing, we can decide whether
to add this or not.

-    if (mUserFontSet && mCurrGeneration != GetGeneration()) {
+    // if user font set is has been updated or font cache has been cleared,
+    // refresh the font list so that we don't use potentially stale fonts
+    if ((mUserFontSet && mCurrGeneration != GetGeneration())) {

Unnecessary parens.

>      if (clearTextFontCaches) {    
> +        gfxTextRunWordCache::Flush();
>          gfxFontCache *fc = gfxFontCache::GetCache();
>          if (fc) {
>              fc->Flush();
>          }
> -        gfxTextRunWordCache::Flush();
>      }

Just curious, why was this needed?  Were fonts not getting flushed because they were still referenced in the word cache?

+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_GAMMA,
+                                                 &value))) {
+            gamma = (FLOAT)value / 1000.0;
+        }
+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_CONTRAST,
+                                                 &value))) {
+            contrast = (FLOAT)value / 100.0;
+        }
+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_LEVEL,
+                                                 &value))) {
+            level = (FLOAT)value / 100.0;
+        }
+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_GEOMETRY,
+                                                 &value))) {
+            geometry = value;
+        }
+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_MODE,
+                                                 &value))) {
+            mode = value;
+        }

Clamp values to either documented value ranges (gamma, cleartype level, mode) or a reasonable range (enhanced contrast).

+// PixelGeometry: 0 = flat, 1 = RGB, 2 = BGR
+pref("gfx.font_rendering.cleartype_params.pixel_geometry", -1);

I don't see a use case for this at all.  If the pixel geometry is set the
wrong way by default, *everything* will look bad.  When would someone need
to override this just in the browser?!?

Comment 40

[Jonathan Kew [:jfkthame]]

(In reply to comment #39)
> Comment on attachment 525796 [details] [diff] [review]
> patch, expose five DirectWrite rendering prefs
> 
> +    HKEY hKey;
> +    if (RegOpenKeyExA(ENHANCED_CONTRAST_REGISTRY_KEY,
> +              0, KEY_READ, &hKey) == ERROR_SUCCESS)
> 
> As mentioned in comment 32, there's no need to read these prefs,
> DirectWrite is already doing that and supplying them via
> Instance()->CreateRenderingParams(&defaultParams).  Just check to see if
> the root key for Cleartype settings exists (i.e.
> HKCU/Software/Microsoft/Avalon.Graphics) and use that to determine whether
> to override the default setting.

Are we confident that nothing other than using the ClearType tuner might create registry keys in Avalon.Graphics? That seemed a rather general assumption.

> While I think bumping up the enhanced contrast really does help I don't see
> any real need to change the ClearType level default right now.  After this
> patch lands and gets a lot more real world testing, we can decide whether
> to add this or not.

Fine, I don't have a strong opinion about it (but then, I'm not seeing examples of really awful font rendering on my machine anyway).

> Unnecessary parens.

Oops, residue from experiments that involved other code here!

> >      if (clearTextFontCaches) {    
> > +        gfxTextRunWordCache::Flush();
> >          gfxFontCache *fc = gfxFontCache::GetCache();
> >          if (fc) {
> >              fc->Flush();
> >          }
> > -        gfxTextRunWordCache::Flush();
> >      }
> 
> Just curious, why was this needed?  Were fonts not getting flushed because they
> were still referenced in the word cache?

Also residue, I think. I still haven't figured out why the screen doesn't refresh properly in the D2D case, though; any inspiration on that would be welcome. It does a reflow, as can be seen by the changed spacing if you switch between "classic" and "natural" widths, for example, but the actual glyph bitmaps don't refresh until you touch the window size. Something is still cached somewhere. (But this doesn't happen in the DWrite-on-GDI case; that refreshes the glyphs immediately.)

> Clamp values to either documented value ranges (gamma, cleartype level, mode)
> or a reasonable range (enhanced contrast).

The code should already ignore unrecognized mode values, IIRC.

Where is the range for ClearType Level documented? (I think I know what the values are supposed to mean, but http://msdn.microsoft.com/en-us/library/dd371285(v=VS.85).aspx doesn't specify.) What's the point in coding an arbitrary "reasonable range" here? If people choose to try huge values, and get ugly text as a result - well, they're free to play around, no?

> +// PixelGeometry: 0 = flat, 1 = RGB, 2 = BGR
> +pref("gfx.font_rendering.cleartype_params.pixel_geometry", -1);
> 
> I don't see a use case for this at all.  If the pixel geometry is set the
> wrong way by default, *everything* will look bad.  When would someone need
> to override this just in the browser?!?

It's obscure, I agree, but at least the "flat" setting may be interesting to play around with. E.g. as another way to reduce color-fringing, or if someone has multiple displays with different characteristics (as we don't do separate tuned-per-device rendering).

Mostly, it just seemed like if we're going to expose these controls so that people can experiment with font rendering, and see if that leads us towards some changed defaults that we think will be more generally liked, we might as well expose everything that's tweakable. We're not putting this into primary UI, after all!

Comment 41

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to comment #40)
> Where is the range for ClearType Level documented?
http://msdn.microsoft.com/en-us/library/aa970267.aspx has some information on valid ranges.

Comment 42

[Jonathan Kew [:jfkthame]]

Attachment: patch, expose five DirectWrite rendering prefs - updated

Updated patch following jdaggett's comments. Now only boosts EnhancedContrast by default; also ignores out-of-valid(or reasonable)-range pref values.

(Sorry about the size of the patch. It's not really that extensive, but there's a bunch of cleanup in the cairo-dwrite/d2d files - tabs and trailing whitespace - that got done when I was cleaning up the whitespace in my actual changes. It should be fairly easy to skim past those chunks, though.)

Comment 43

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

(In reply to comment #40)
> Mostly, it just seemed like if we're going to expose these controls so that
> people can experiment with font rendering, and see if that leads us towards
> some changed defaults that we think will be more generally liked, we might as
> well expose everything that's tweakable. We're not putting this into primary
> UI, after all!

Yeah I agree!

Comment 44

[John Daggett (:jtd)]

Comment on attachment 526618 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - updated

+        if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_CONTRAST,
+                                                 &value))) {
+            if (value >= 0 && value <= 100) {
+                contrast = (FLOAT)value / 100.0;
+            }
+        }

+    // For EnhancedContrast, we override the default if the user has not set it
+    // in the registry (by using the ClearType Tuner).
+    FLOAT contrast;
+    if (mEnhancedContrast >= 0.0 && mEnhancedContrast <= 1.0) {
+	contrast = mEnhancedContrast;
+    } else {

The upper limit is wrong here.  The enhanced contrast doesn't have a
documented upper limit.  When running the Cleartype tuner, the first step
is used to determine pixel orderings, the second and third to determine
Cleartype level and the fourth to determine enhanced contrast.  For this
six settings are shown, representing in visual order:

    0.0   0.5   1.0
    2.0   3.0   4.0

You can verify this by looking at the registry values stored after
selecting each.  So I set the upper limit to 10.0 in my patch, although
DirectWrite may restrict this to a narrower range.

+// ClearType tuning parameters for directwrite/d2d.
+// Negative values mean use system or ClearType Tuner defaults.
+// GammaLevel * 1000, typical values 1000-2200 (1.0-2.2):
+pref("gfx.font_rendering.cleartype_params.gamma", -1);
+// EnhancedContrast * 100 (percentage)
+pref("gfx.font_rendering.cleartype_params.enhanced_contrast", -1);
+// ClearTypeLevel * 100 (percentage)
+pref("gfx.font_rendering.cleartype_params.cleartype_level", -1);
+// PixelGeometry: 0 = flat, 1 = RGB, 2 = BGR
+pref("gfx.font_rendering.cleartype_params.pixel_geometry", -1);
+// ClearTypeMode: 0 = default, 1 = aliased, 2 = GDI Classic, 3 = GDI Natural, 4 = Natural, 5 = Natural Symmetric
+pref("gfx.font_rendering.cleartype_params.rendering_mode", -1);

I think it would be helpful to mention that these are mimicing registry
values and include the URLs that describe the registry values.  The alt
patch lists those URLs.

Comment 45

[Jonathan Kew [:jfkthame]]

Attachment: patch, expose five DirectWrite rendering prefs - address comment #44

Updated as per comment #44 - included comments from alt patch in all.js, corrected range of EnhancedContrast value.

Bas, do you want to review this, or just leave it to John?

Comment 46

[Bas Schouten (:bas.schouten)]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44

This patch changes a whole bunch of whitespace in code that it doesn't touch, that seems like a bad idea to me! Other than that I'm fine with jdaggett reviewing it.

Comment 47

[Jonathan Kew [:jfkthame]]

(In reply to comment #46)
> Comment on attachment 526666 [details] [diff] [review]
> patch, expose five DirectWrite rendering prefs - address comment #44
> 
> This patch changes a whole bunch of whitespace in code that it doesn't touch,

(As noted in comment 42.) I can try to strip that out if you/jdaggett prefer, but it was easier and more reliable to fix up whitespace globally in the files I was touching than to try and fix it in the fragments I touched but leave it inconsistent/wrong elsewhere in the file.

Comment 48

[John Daggett (:jtd)]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44


> @@ -1153,19 +1153,19 @@ _cairo_d2d_create_strokestyle_for_stroke
>      RefPtr<ID2D1StrokeStyle> strokeStyle;
>      sD2DFactory->CreateStrokeStyle(D2D1::StrokeStyleProperties(line_cap, 
>  							       line_cap,
>  							       line_cap, 
>  							       line_join, 
>  							       (FLOAT)style->miter_limit,
>  							       dashStyle,
>  							       (FLOAT)style->dash_offset),
> -							        dashes,
> -							        style->num_dashes,
> -							        &strokeStyle);
> +								dashes,
> +								style->num_dashes,
> +								&strokeStyle);

This white-space cleanup is inconsistent, the lines above it all have the
same style as the one you're fixing here.

Otherwise, the patch looks fine.  A small inconsistency is that the
ClearType parameters do not apply to the non-DirectWrite case, even though
some of these settings have meaning in that case (i.e. setting the
underlying registry value will affect ClearType rendering).  But that's a
codepath we'll eventually deprecate so I don't see that it's important to
consider right now.

We also don't handle multiple monitors correctly, since the ClearType tuner
allows for different settings for each monitor.  This patch and other portions
of our existing code only appears to use the default display settings.

Comment 49

[Jonathan Kew [:jfkthame]]

(In reply to comment #48)

> This white-space cleanup is inconsistent, the lines above it all have the
> same style as the one you're fixing here.

Oops - the original indentation was inconsistent, and so entabbing the run of 8 spaces still left it inconsistent. Fixed in my queue now, for when I push it (after a final check on tryserver).

> Otherwise, the patch looks fine.  A small inconsistency is that the
> ClearType parameters do not apply to the non-DirectWrite case, even though
> some of these settings have meaning in that case (i.e. setting the
> underlying registry value will affect ClearType rendering).  But that's a
> codepath we'll eventually deprecate so I don't see that it's important to
> consider right now.

Agreed - I don't think it's worth complicating the GDI backend with this. That's a legacy rendering path which just keeps working the way it always has - and we haven't been hearing demands for adjustments there.

> We also don't handle multiple monitors correctly, since the ClearType tuner
> allows for different settings for each monitor.  This patch and other portions
> of our existing code only appears to use the default display settings.

Yes - it's arguably a bug that we use the same rendering options across all displays, but that's a separate bug from this one. When we do something about that, we'll need to consider per-display prefs as well, I suppose.

Comment 50

[Jonathan Kew [:jfkthame]]

Pushed patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/fd8e217b2c84

This causes a couple of reftest oranges:
REFTEST TEST-UNEXPECTED-PASS | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/bidi/413928-1.html | image comparison (==)
REFTEST TEST-UNEXPECTED-PASS | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/bidi/413928-2.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/379316-2.html | image comparison (==)

The 413928-1 and 413928-2 tests were failing due to tiny subpixel rendering discrepancies, and apparently with the tweaked EnhancedContrast we get lucky and they pass. Accordingly, I removed the FAILS annotation from these.

379316-2 is a test for rendering text with partial opacity, which already fails on Android and OS X; the tweaked AA setting now gives us a visually-imperceptible discrepancy around the edges of the glyphs. So I marked this as failing on Win7. As long as we use different rendering techniques for opaque vs transparent surfaces, this test will be hard to fix "right" for all platforms. See also bug 379786.

Updated reftest manifests:
http://hg.mozilla.org/mozilla-central/rev/46fdf12082d4

Comment 51

[DB Cooper]

Can this be expanded to add prefs (at least for rendering mode) for the chrome? It would be nice to be able to keep the default DW rendering for content but use classic cleartype for chrome (and get rid of the rainbow effect in the URL bar).

Comment 52

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

I see no option for "Outline" in gfx.font_rendering.cleartype_params.rendering_mode.
Setting it for 6 didn't work. It was intentionally omitted ?



"DWRITE_RENDERING_MODE_OUTLINE
Specifies that rendering should bypass the rasterizer and use the outlines directly. This is typically used at very large sizes."

source - http://msdn.microsoft.com/en-us/library/dd368118.aspx

Comment 53

[Jonathan Kew [:jfkthame]]

When I tried this, it didn't work in some circumstances (in the sense that text would completely fail to display), so I decided it was better not to accept this setting at all rather than allow people to shoot themselves in the foot with it.

From looking at the results it gave (in the cases where it worked for me), I don't believe anyone would really have wanted to use it anyway.

Comment 54

[Emanuel Hoogeveen [:ehoogeveen]]

In the this post, a user claims he "got the best result, for some particular Chinese fonts, from AA-tuner's setting AAmode greyscale, Rendering Mode Outline.": http://forums.mozillazine.org/viewtopic.php?p=10717403#p10717403

Comment 55

[Fanolian]

Is there a reason behind the omission of DWRITE_RENDERING_MODE_OUTLINE?

I do find that Latin characters do not look best in Outline mode among others. However some common CJK fonts improve a lot in this mode. For example, iLiHei/LiHei Pro are two very popular Mac fonts that are used in Windows system in the Chinese community, especially those who want to improve their browsing experience by changing to a better font than the default. MS PGothic, the default font for Japanese in firefox in Windows, also greatly benefit from Outline mode. Other Gothic/sans-serif fonts that come with Windows have a similar vast improvement using Outline mode.

IMO, people who change their font in the browser are the one who most likely notice (care about) the difference among each rendering mode, eager to change the settings, and find out they are missing a feature, particularly crucial to the Chinese community (perhaps Japanese and Korean too, but I cannot speak for them), that are supposed to deliver with this bugfix, as DWRITE_RENDERING_MODE_OUTLINE is in the specification but is not included in the fix.

Comment 56

[Jonathan Kew [:jfkthame]]

(In reply to comment #55)
> Is there a reason behind the omission of DWRITE_RENDERING_MODE_OUTLINE?

See comment 53. If we had a patch that made it work reliably in all cases I'd be happy to include it, but a setting that causes some text to vanish does not seem very useful.

Comment 57

[Fanolian]

(In reply to comment #56)

Oops sorry, I wasn't aware you were replying to comment #52.

Comment 58

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

@ Jonathan Kew - could you please look on bug #652141

Comment 59

[Jonathan Kew [:jfkthame]]

(In reply to comment #58)
> @ Jonathan Kew - could you please look on bug #652141

I'm aware of it - as you can tell by the fact that I commented there. No need to spam this bug with a reminder.

(FWIW, I think Kai Liu's comments in that bug provide the most constructive input we've seen on this.)

Comment 60

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

Yep, but branching is in a week for Firefox 6 and nice will be when this will be fixed before it.

Comment 61

[Jonathan Kew [:jfkthame]]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44

Requesting approval to land on mozilla-beta for Fx5.

The change to font rendering on Windows for users with D2D-capable hardware (using subpixel positioning and "natural" instead of gridfitted antialiasing), is perceived by many as a serious regression in Fx4 compared to earlier versions (and competing browsers).

(See, for example, bug 635490, bug 652141, comments at http://input.mozilla.com/en-US/search?product=firefox&version=--&q=blurry, http://input.mozilla.com/en-US/search?product=firefox&version=--&q=fuzzy, or the 50+ pages of comments at http://forums.mozillazine.org/viewtopic.php?f=23&t=2060933.)

While we can endlessly debate the merits of different font rasterization techniques, and the context in which each should or should not be used, the current reality is that many users detest the text rendering in Fx4, and as such this is a serious regression in their view.

In the longer term, we're hoping to solicit more detailed feedback on font-rendering preferences (e.g. via a Test Pilot study), and enhance the code so that we can potentially apply different rendering modes depending on the specific font/size/context as appropriate. However, in the meantime we risk losing substantial numbers of users who simply dislike the appearance of text in Fx4 compared to older versions and other products.

As such, we should treat this as a regression, and until we have a better fix ready to deploy, we should revert to pre-regression behavior; this is the safest approach given that we do not currently have a better fix to offer.

The brute-force approach to this would be to revert to non-accelerated (GDI-only) rendering, but that would lose important performance gains. This patch makes it possible to revert the appearance of text _without_ disabling acceleration. It has been on trunk (and now Aurora) for several weeks without problems, and has been enthusiastically received by users on those channels who are now able to revert font rendering in the browser to GDI-style. I think we should ship this in Fx5 as a basis for fixing the perceived regression from Fx4.

(This patch does not actually "fix" the issue by itself, it merely provides a preference that can be used to switch to the GDI-style text rendering. Bug 652141 deals with changing the default setting that we ship, and will be nominated for landing in addition as soon as the patch there is reviewed.)

Comment 62

[JP Rosevear [:jpr]]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44

Exposing this doesn't help us enough for 5.0 because these are for investigation.

Comment 63

[Jonathan Kew [:jfkthame]]

(In reply to comment #62)
> Comment on attachment 526666 [details] [diff] [review] [review]
> patch, expose five DirectWrite rendering prefs - address comment #44
> 
> Exposing this doesn't help us enough for 5.0 because these are for
> investigation.

But _creating_ the switch is a prerequisite for making _any_ change to the font rendering. This would enable us to flip a switch (in bug 652141, a one-line pref change) so as to get GDI Classic rendering. Without this, we have no code to support that, and therefore will continue to alienate users. :(

Comment 64

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

I filed bug 642589 to cover the creation of a pref to force "GDI Classic" rendering for particular font families (and attached patches).

Comment 65

[Dão Gottwald [:dao]]

(In reply to comment #64)
> I filed bug 642589 to cover the creation of a pref to force "GDI Classic"
> rendering for particular font families (and attached patches).

bug 661471

Comment 66

[Asa Dotzler [:asa]]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44

renominating per roc's comment and the intention that Bug 661471 become a 5 nomination as well.

Comment 67

[LegNeato]

Comment on attachment 526666 [details] [diff] [review]
patch, expose five DirectWrite rendering prefs - address comment #44

We don't want to take this on beta at this point but we'd like it on aurora so we can point users to it and see if it fixes their issue.

Comment 68

[Jonathan Kew [:jfkthame]]

(In reply to comment #67)
> Comment on attachment 526666 [details] [diff] [review] [review]
> patch, expose five DirectWrite rendering prefs - address comment #44
> 
> We don't want to take this on beta at this point but we'd like it on aurora
> so we can point users to it and see if it fixes their issue.

AFAICT, this is on aurora already, having landed on m-c on 04/21 during the mozilla-6 cycle.