1219925 - ClearType parameters info to about:support are wrongly formatted

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[fireattack [:fireattack]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20151026170526

Steps to reproduce:

I mentioned it in bug 650723 but non responded so I assume it's worth to create a new bug ticket.

My current ClearType parameters for example:
Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 200 

It *as least* has two errors:

1. Gamma level should be displayed as 2.2:

http://hg.mozilla.org/mozilla-central/file/0ed4f22648e3/gfx/thebes/gfxWindowsPlatform.cpp

   921         if (NS_SUCCEEDED(aPrefBranch->GetIntPref(GFX_CLEARTYPE_PARAMS_GAMMA,

   922                                                  &value))) {

   923             if (value >= 1000 && value <= 2200) {

   924                 gamma = FLOAT(value / 1000.0);


But it's not displayed as 2200;

2. The Pixel Structure is supposed to be "RGB", not just "R":

http://hg.mozilla.org/mozilla-central/file/0ed4f22648e3/widget/src/windows/GfxInfo.cpp

   146         swprintf_s(valStr, NS_ARRAY_LENGTH(valStr),

   147                    L"Pixel Structure: %s ",

   148                    (params.pixelStructure == PIXEL_STRUCT_RGB ?

   149                       L"RGB" : L"BGR"));

Comment 1

[John Daggett (:jtd)]

Sorry, don't know the answer here.

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

http://hg.mozilla.org/mozilla-central/file/ec20b463c04f/widget/windows/GfxInfo.cpp#l100

100        outStr.AppendPrintf("Pixel Structure: %s ",
101                   (params.pixelStructure == PIXEL_STRUCT_RGB ?
102                      L"RGB" : L"BGR"));

The problem of only displaying 'R' seems like an issue with the narrow format specifier. Changing %s to %S, fixes that issue.

As for the display of gamma issue, the code above referencing gfxWindowsPlatform.cpp is for the creation of a surface, not the code for displaying the current settings. Regardless, the Gamma is used in the registry and gfxInfo interface as an integer, so I'm not sure if that should be changed to a floating point. It could be changed to display as a float in about:support.

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Patch for PixelStructure display issue.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8760995 [details] [diff] [review]
Patch for PixelStructure display issue.

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

Rather than switching the format to %S, I'd suggest just dropping the L prefix on the "RGB"/"BGR" strings.

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

So that was my thought too, but dropping the L prefix caused an invalid access in the printf function. Looking into it deeper now, it seems like MSVC is interpreting the single "" as a va_list for the overloaded printf function, not a '...' type argument. It shouldn't do this because "" should be a const char *, not a char * (which is what va_list is a macro for). sigh, that's gross.

We can get around that by defining a 'const char *' variable before the printf and uninline that call, and that seemed to fix it.

I also noticed that there is another issue in this function with an incorrect format specifier (params.displayName.get() is a wide string, and it's using a %s not a %S, causing the same behavior as Pixel Structure).

Because all of the strings are wide in this function, including the destination, it might make sense to just use UTF16 string constants here, so we don't mix and match %s, %S. This would also get around the weird va_list issue with MSVC.

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Possible solution to wide string issues.

Comment 7

[fireattack [:fireattack]]

About the gamma, I'm sorry if I interpreted the code wrong. 

But I still feel it should be *displayed* as 2.2, since that is the actual gamma, instead of 2200 (which is just how Windows stores it in Registry).

Comment 8

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Outputs the current gamma in the correct converted form.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8761323 [details] [diff] [review]
Possible solution to wide string issues.

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

OK, if this keeps the compiler happy it seems fine.

::: widget/windows/GfxInfo.cpp
@@ +98,5 @@
>            params.pixelStructure == PIXEL_STRUCT_BGR)
>        {
>          outStr.AppendPrintf("Pixel Structure: %S ",
> +                              (params.pixelStructure == PIXEL_STRUCT_RGB ?
> +                                MOZ_UTF16("RGB") : MOZ_UTF16("BGR")));

Style nit: while you're touching these lines anyway, please adjust the indent so that the open-paren of `(params.pixelStructure == ...` lines up with the open-double-quote above; it's a separate arg to AppendPrintf(), not a continuation or subsidiary of the first, so shouldn't have extra indentation. And then wrap the line before (rather than after) the ? operator.

(See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators.)

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8761352 [details] [diff] [review]
Outputs the current gamma in the correct converted form.

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

::: widget/windows/GfxInfo.cpp
@@ +89,4 @@
>  
>      if (params.gamma >= 0) {
>        foundData = true;
> +      outStr.AppendPrintf("Gamma: %.1lf ", params.gamma / 1000.0);

Given how the value is represented, it could have up to 3 decimal digits of precision; %.1f will truncate that to a single decimal digit, which seems unfortunate (params.gamma of 1234 would result in "1.2").

Changing this to use %.3f would be better, but that will give us trailing zeros in common cases like 2200 (-> "2.200"), which isn't good for readability.

So I suggest using %.4g as the format, which should give us the tidiest output (2200 -> "2.2", but 1234 -> "1.234") for all common cases; the only downside would be that it'll switch to scientific notation for huge values, but that's unlikely to occur.

Comment 11

[Jonathan Kew [:jfkthame]]

...the above is assuming that AppendPrintf behaves just like standard printf w.r.t. the various format characters; I didn't actually confirm that.

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Fix for indentation.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8761679 [details] [diff] [review]
Fix for indentation.

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

(No real need to re-request r? in a case like this, where I'd given r+ already and just asked for some cosmetic clean-up; you can update the patch and note that you're "carrying forward r=jfkthame".)

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8761352 [details] [diff] [review]
Outputs the current gamma in the correct converted form.

# HG changeset patch
# User rhunt <[e-mail]>

Format cleartype gamma in converted form.


diff --git a/widget/windows/GfxInfo.cpp b/widget/windows/GfxInfo.cpp
index f4219a0..64702c7 100644
--- a/widget/windows/GfxInfo.cpp
+++ b/widget/windows/GfxInfo.cpp
@@ -89,7 +89,7 @@ GfxInfo::GetCleartypeParameters(nsAString & aCleartypeParams)
 
     if (params.gamma >= 0) {
       foundData = true;
-      outStr.AppendPrintf("Gamma: %d ", params.gamma);
+      outStr.AppendPrintf("Gamma: %.4g ", params.gamma / 1000.0);
     }
 
     if (params.pixelStructure >= 0) {

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

Ah, didn't mean to comment that.

Comment 16

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: gamma.patch

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8942d876c668
Fix for format specifiers in getCleartypeParameters. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/87f46214c5a6
Format cleartype gamma in converted form. r=jfkthame

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8942d876c668
https://hg.mozilla.org/mozilla-central/rev/87f46214c5a6