458847 - colordepth.html TEST-UNEXPECTED-FAIL on my Windows 2000: 24<->32 bit colors mismatch

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1223339223.1223343129.18930.gz&fulltext=1
Win2k3 comm-central dep unit test on 2008/10/06 17:27:03

REFTEST TEST-PASS | file:///d:/builds/slave/comm-central-win32/build/mozilla/modules/libpr0n/test/reftest/colordepth.html | 

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20081004 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

See log in bug 458844.

NB: My setting is "32 bit colors".

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080929201016 Minefield/3.1b1pre] (home, optim default) (W2Ksp4)

FF fails too.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Could you explain what this bug report is?  It's not possible to tell from a bunch of disconnected pasted URLs and quotations what the problem is or what you expect to be fixed.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)

Obviously (I would have think), this report is about a test that fails whereas it seems to be expected to succeed.
So I'm reporting it, in hope it could be "fixed"...

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081009 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

(still fails)

Comment 4

[Jeff Muizelaar [:jrmuizel]]

The test checks for 24bit color depth and windows will often have 32bit color depth. In fact, I can't even choose 24bit on my windows box. I think the test should perhaps check for 24bit or 32bit.

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/5d37678a2482
+http://hg.mozilla.org/comm-central/rev/892e9f783d9e)

(In reply to comment #4)
> In fact, I can't even choose 24bit on my windows box.

I can't either: 8, 16 or 32 bit only.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #4)
> The test checks for 24bit color depth and windows will often have 32bit color
> depth. In fact, I can't even choose 24bit on my windows box. I think the test
> should perhaps check for 24bit or 32bit.

Really, perhaps the underlying the DOM accessor that we use, or perhaps even the widget code underneath it, should convert any values of 32 on Windows to be 24.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> See log in bug 458844.
> 
> NB: My setting is "32 bit colors".

That's it: the (failing) 'test' image displays as "ERROR: color depth is 32.".

First, colordepth.html was added by bug 414720:
at least, to warn about unsupported/not-enough 16-bit color Windows (tinderboxes).

(In reply to comment #6)
> Really, perhaps the underlying the DOM accessor that we use, or perhaps even
> the widget code underneath it, should convert any values of 32 on Windows to be
> 24.

Then, there was bug 424386.
The current bug looks very much like it, or a missing followup to it:
I'm a little puzzled about what (old 24 or new 32) Windows behavior was actually intended there.

At least, it seems colordepth.html was not updated to match that change and it is the cause of the current bug.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Allow 32 on Windows

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/5d37678a2482
+http://hg.mozilla.org/comm-central/rev/892e9f783d9e)

Fix the test to match current code.
Or, maybe, we could simply check for |>= 24| ?

NB: Fixing the code, if need be, would be for someone else.

Comment 9

[Justin Dolske [:Dolske]]

(In reply to comment #6)
> Really, perhaps the underlying the DOM accessor that we use, or perhaps even
> the widget code underneath it, should convert any values of 32 on Windows to be
> 24.

I'd tend to think this is the right fix. I'm not sure that there's any difference between 24 and 32 so far as web content is concerned.

That said, the original purpose of the test was just to make sure something made obvious noise if running reftests with insufficient color depth (because other tests would fail, and the cause wouldn't otherwise be obvious). Allowing 32 in the test would be fine for that purpose.

Comment 10

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

I think we should take this patch, but I also think we should return 24 to the DOM instead of 32.

Comment 11

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

Er, I don't think we should take this patch. I think we should just be testing >= 24.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Allow higher values, on all platforms

Av1, with comment 11 suggestion(s).

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 349973 [details] [diff] [review]
(Av2) Allow higher values, on all platforms

>  * Almost all of the image decoding reftests require the display to be in
>  * 24-bit color mode, or else the rendered images will have subtle color
>- * variations and will fail. The Windows test boxes have a tendancy to flip to
>- * 16-bit color mode (see bug 414720), so this test will explicit check the
>- * color depth to make it more obvious when that happens.
>+ * variations and will fail.
>+ * This test will explicitly check the color depth to make it more obvious when
>+ * that requirement is not met. (See bug 414720.)
>+ * 24-bit is only a minimum, allow higher values too. (See bug 458847.)

Please don't remove the comment that refers to the Windows test boxes flipping to 16-bit.  (Though feel free to fix the spelling of "tendency" and change "explicit" to "explicitly".)

>  */
>+
> var colorDepth = window.screen.colorDepth;
> 
>-if (colorDepth != 24)
>-    document.write("ERROR: color depth is " + colorDepth + ".");
>-
>+if (colorDepth < 24)
>+  document.write("ERROR: color depth is only " + colorDepth + ".");

Please leave this as 4-space indent.

r=dbaron

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I filed bug 466669 on changing the Windows widget code.

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a) Allow higher values, on all platforms [Checkin: Comment 17 & 18]

Av2, with comment 13 suggestion(s).

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 349984 [details] [diff] [review]
(Av2a) Allow higher values, on all platforms
[Checkin: Comment 17 & 18]

"approval1.9.1b2=?":
Zero risk: test only and doesn't affect tinderboxes.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 349984 [details] [diff] [review]
(Av2a) Allow higher values, on all platforms
[Checkin: Comment 17 & 18]

http://hg.mozilla.org/mozilla-central/rev/90f89770b9b6

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 349984 [details] [diff] [review]
(Av2a) Allow higher values, on all platforms
[Checkin: Comment 17 & 18]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/62e06c96a379