Last Comment Bug 650723 - add ClearType parameter info to about:support
: add ClearType parameter info to about:support
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows 7
: P3 normal (vote)
: ---
Assigned To: John Daggett (:jtd)
:
Mentors:
Depends on: 1212982 1219925 634286
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-17 21:53 PDT by John Daggett (:jtd)
Modified: 2016-02-01 17:18 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, add ClearType parameter info to about:support (14.66 KB, patch)
2011-04-18 01:05 PDT, John Daggett (:jtd)
no flags Details | Diff | Review
patch, add ClearType parameter info to about:support v2 (14.16 KB, patch)
2011-04-18 01:20 PDT, John Daggett (:jtd)
no flags Details | Diff | Review
patch, add ClearType parameter info to about:support v3 (17.19 KB, patch)
2011-04-18 23:39 PDT, John Daggett (:jtd)
jmuizelaar: review+
gavin.sharp: review+
Details | Diff | Review
screenshot, cleartype params listed for system with two displays (29.86 KB, image/png)
2011-04-19 01:16 PDT, John Daggett (:jtd)
no flags Details

Description John Daggett (:jtd) 2011-04-17 21:53:32 PDT
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) 2011-04-18 01:00:17 PDT
Slight tweak to output:
ClearType ParametersGamma: 2200 Pixel Structure: RGB Enhanced Contrast: 50 ClearType Level: 100
Comment 2 John Daggett (:jtd) 2011-04-18 01:05:40 PDT
Created attachment 526671 [details] [diff] [review]
patch, add ClearType parameter info to about:support
Comment 3 John Daggett (:jtd) 2011-04-18 01:20:49 PDT
Created attachment 526675 [details] [diff] [review]
patch, add ClearType parameter info to about:support v2

cleanup and add missing stubs to OSX, Android branches
Comment 4 John Daggett (:jtd) 2011-04-18 23:39:35 PDT
Created attachment 526937 [details] [diff] [review]
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) 2011-04-19 01:16:09 PDT
Created attachment 526944 [details]
screenshot, cleartype params listed for system with two displays
Comment 6 John Daggett (:jtd) 2011-04-20 17:00:26 PDT
(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] 2011-04-25 08:08:21 PDT
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) 2011-04-25 14:49:49 PDT
(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: gavin@gavinsharp.com] 2011-04-27 08:13:57 PDT
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] 2011-04-29 20:19:21 PDT
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) 2011-05-08 23:02:02 PDT
Pushed to trunk, with changes based on review comments:
http://hg.mozilla.org/mozilla-central/rev/d34dd7156b4d
Comment 12 John Daggett (:jtd) 2011-05-08 23:06:14 PDT
(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) 2011-05-08 23:08:09 PDT
(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) 2011-05-09 00:02:36 PDT
Drat, missed including the X11 stubs!  Backed out.
Comment 15 John Daggett (:jtd) 2011-05-10 17:37:11 PDT
Relanded on trunk, with additional stub for GfxInfoX11.
http://hg.mozilla.org/mozilla-central/rev/0ed4f22648e3
Comment 16 Benjamin Peng 2015-10-08 11:48:39 PDT
(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 Benjamin Peng 2015-10-08 11:53:29 PDT
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

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