650723 - add ClearType parameter info to about:support

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[John Daggett (:jtd)]

To give us a better idea of how ClearType parameters affect users who dislike DirectWrite rendering, add ClearType parameter info to the about:support page.  This includes pixel geometry, ClearType level, enhanced contrast, and gamma.  These are registry settings set by the ClearType tuner utility which prompts the user to tune text rendering to their liking (none of the registry keys exist by default).

I'm going to add code that adds a line like this:

Direct2D Enabled      true
DirectWrite Enabled   true (6.1.7601.17563)
ClearType Parameters  GammaLevel: 2200 PixelStructure: 1 ClearTypeLevel: 100 EnhancedContrastLevel: 50

Comment 1

[John Daggett (:jtd)]

Slight tweak to output:
ClearType ParametersGamma: 2200 Pixel Structure: RGB Enhanced Contrast: 50 ClearType Level: 100

Comment 2

[John Daggett (:jtd)]

Attachment: patch, add ClearType parameter info to about:support

Comment 3

[John Daggett (:jtd)]

Attachment: patch, add ClearType parameter info to about:support v2

cleanup and add missing stubs to OSX, Android branches

Comment 4

[John Daggett (:jtd)]

Attachment: patch, add ClearType parameter info to about:support v3

Add in support for multiple monitors.  Assume that the display subkey name is the same for the HKLM and HKCU values.

Comment 5

[John Daggett (:jtd)]

Attachment: screenshot, cleartype params listed for system with two displays

Comment 6

[John Daggett (:jtd)]

(In reply to comment #1)
> ClearType ParametersGamma: 2200 Pixel Structure: RGB Enhanced Contrast: 50
> ClearType Level: 100

One thing to note for those reviewing this, the string above is hard-coded as English.  I think it's better to leave it this way as these reflect registry settings that have similar names independent of system language/locale.  In the multiple monitor case the "DISPLAY1", "DISPLAY2" strings are also subkeys in the registry.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 526937 [details] [diff] [review]
patch, add ClearType parameter info to about:support v3

You may want to use nsWindowsRegKey if that will make the registry handling cleaner.

Comment 8

[John Daggett (:jtd)]

(In reply to comment #7)
> Comment on attachment 526937 [details] [diff] [review]
> patch, add ClearType parameter info to about:support v3
> 
> You may want to use nsWindowsRegKey if that will make the registry handling
> cleaner.

This doesn't, it just adds a layer of abstraction that leads to inefficient registry API usage.  This is what I discovered when looking at font API usage at startup and is why I rewrote the GetFontSubstitutes method to use registry calls directly.  Do a trace on registry calls using that API and you'll see what I mean (e.g. run ProcessMonitor with Firefox 3.6 at startup).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 526937 [details] [diff] [review]
patch, add ClearType parameter info to about:support v3

>diff --git a/toolkit/content/aboutSupport.js b/toolkit/content/aboutSupport.js

>+    var cleartypeParams = "";
>+    try {
>+      cleartypeParams = gfxInfo.cleartypeParameters;
>+    } catch(e) {
>+      cleartypeParams = bundle.GetStringFromName("clearTypeParametersNotFound");
>+    }

Jeff and Benoit and I talked about transitioning the GfxInfo API to always avoid throwing and have the fallback behavior be handled in the implementation of GfxInfo itself rather than by the JS callers (bug 651981). It might be nice to use this bug as an opportunity to test that approach out for this new property. But I suppose that would introduce an inconsistency in the API, and you may not want to muck with the rest of this patch at the moment, so that's OK to defer.

>diff --git a/toolkit/locales/en-US/chrome/global/aboutSupport.properties b/toolkit/locales/en-US/chrome/global/aboutSupport.properties

>+# LOCALIZATION NOTE In the following string, "Direct2D", "DirectWrite" and "ClearType" 

While you're touching this anyways, this should say "following strings", not "following string", since it applies to multiple strings in this file.

I didn't review any of the gfx/ or widget/ code.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 526937 [details] [diff] [review]
patch, add ClearType parameter info to about:support v3

>+
>+        ClearTypeParameterInfo ctinfo;
>+        ctinfo.displayName.Assign(displayName);
>+
>+        DWORD subrv, value;
>+        bool foundData = false;
>+
>+        swprintf_s(subkeyName, NS_ARRAY_LENGTH(subkeyName),
>+                   L"Software\\Microsoft\\Avalon.Graphics\\%s", displayName);

Seems like subkeyName should be at least sizeof(displayName) + sizeof(L"Software\\Microsoft\\Avalon.Graphics\\%s");

>+
>+        // subkey for gamma, pixel structure
>+        subrv = RegOpenKeyExW(HKEY_LOCAL_MACHINE,
>+                              subkeyName, 0, KEY_QUERY_VALUE, &subKey);
>+
>+        if (subrv == ERROR_SUCCESS) {
>+            size = sizeof(value);
>+            subrv = RegQueryValueExW(subKey, L"GammaLevel", NULL, NULL,
>+                                     (LPBYTE)&value, &size);

Would it be better if the QueryValue functions specified a type instead of using NULL?

Comment 11

[John Daggett (:jtd)]

Pushed to trunk, with changes based on review comments:
http://hg.mozilla.org/mozilla-central/rev/d34dd7156b4d

Comment 12

[John Daggett (:jtd)]

(In reply to comment #10)
> Seems like subkeyName should be at least sizeof(displayName) +
> sizeof(L"Software\\Microsoft\\Avalon.Graphics\\%s");

Those arrays are defined with max sizes in mind, I think it's simpler to use
an arbitrarily large size and be careful to use size-restricted calls to deal
with "won't happen" situations.

> >+            subrv = RegQueryValueExW(subKey, L"GammaLevel", NULL, NULL,
> >+                                     (LPBYTE)&value, &size);
> 
> Would it be better if the QueryValue functions specified a type instead of
> using NULL?

The type is an output parameter, not an input parameter, so I added a check
to be sure the type returned is REG_DWORD.

Comment 13

[John Daggett (:jtd)]

(In reply to comment #9)
> Jeff and Benoit and I talked about transitioning the GfxInfo API to
> always avoid throwing and have the fallback behavior be handled in the
> implementation of GfxInfo itself rather than by the JS callers (bug
> 651981). It might be nice to use this bug as an opportunity to test
> that approach out for this new property. But I suppose that would
> introduce an inconsistency in the API, and you may not want to muck
> with the rest of this patch at the moment, so that's OK to defer.

That makes sense, I left the code in the same style as the surrounding code.

> >+# LOCALIZATION NOTE In the following string, "Direct2D", "DirectWrite" and "ClearType" 
> 
> While you're touching this anyways, this should say "following
> strings", not "following string", since it applies to multiple
> strings in this file.

Done.

Comment 14

[John Daggett (:jtd)]

Drat, missed including the X11 stubs!  Backed out.

Comment 15

[John Daggett (:jtd)]

Relanded on trunk, with additional stub for GfxInfoX11.
http://hg.mozilla.org/mozilla-central/rev/0ed4f22648e3

Comment 16

[fireattack [:fireattack]]

(In reply to John Daggett (:jtd) from comment #15)
> Relanded on trunk, with additional stub for GfxInfoX11.
> http://hg.mozilla.org/mozilla-central/rev/0ed4f22648e3

If I read it correctly, if my Gamma level in Reg is 2200, shouldn't it be shown as 2.2?

   1.119              if (value >= 1000 && value <= 2200) {
   1.120 -                gamma = (FLOAT)value / 1000.0;
   1.121 +                gamma = FLOAT(value / 1000.0);
   1.122              }


But in my computer (win 10), it shows 2200.

ClearType Parameters	Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 200 

(Same problem for other parameters, for sure)

Comment 17

[fireattack [:fireattack]]

Same for Pixel Structure... it should be either RGB or BGR, no idea what "R" came from.

http://hg.mozilla.org/mozilla-central/rev/0ed4f22648e3#l10.52