293586 - Navigation icons change their shade of grey after time

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050510
Firefox/1.0+

1. New profile, start firefox
2. Open some tabs: www.google.com www.neowin.net
http://www.mozilla.org/products/firefox/central.html www.slashdot.org
3. Focus the www.slashdot.org tab
4. Nip outsite for a cigarette / wait a length of time (~3 mins)
5. Return to firefox, and select all the other tabs.

Actual: Notice that any of firefox's greyed out navigation icons have changed
colour, from being a light grey to a dark grey
Expected: Navigation icons don't change their shade of grey.

Picture: http://syskin.is.dreaming.org/b0rk.png

I suspect bug 292051 caused this regression.

Comment 1

[ArronM (:paper)]

Steve, can you tell me your resolution (specifically color mode.. 32 bit, 16
bit, 256 color, etc)?

Also, anyone seeing this on a Seamonkey build?

If I don't figure this out in the next day or two, I'll back out 292051 (for
b2).  By first guess is ConvertDDBtoDIB is somehow converting the DDB back to a
DIB as a greyscale image.

Comment 2

[José Jeria]

I have 32 bit and see this

Comment 3

[Radek 'sysKin' Czyz]

No the problem is not greyscale, the problem is in alpha channel: after some
time, the weightned pixel values suddenly get darker, and the darkness level
depends on the alpha value. 

On the picture, this is especially visible wth the Forecast icons - the suns are
nicely anti-aliased, but with this bug they get a black outline.

The icons are gray because they are in disabled state, which gives them
semi-transparent look.

The funny thing is, not all pictures are always affected - for example right
now, a Up-Down autoscroller is very dark (it's semi transparent) while
Up-Down-Left-Right autoscroller is white as it should be. If I wait some more,
it'll become black too.

This might be a png-only problem - quite difficuilt to tell since only pngs have
such alpha channel.
Transparent png files from websites are also affected.

Comment 4

[Peter van der Woude [:Peter6]]

Attachment: screenshot

gradual degradation.
from the top to bottom is ~30 minutes

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050510
Firefox/1.0+
Matrox G200 / 32-bit color

Comment 5

[Steve England [:stevee]]

32bit, 1280x1024 @ 85Hz, nVidia GF4 Ti4400, Forceware 76.41

Comment 6

[ArronM (:paper)]

Thanks, I can reproduce now.  Only affects 8 bit alphas (PNGs)

..I hope ;)

Comment 7

[ArronM (:paper)]

I have a fix for this, but I'll sleep on it before attaching the patch.

The problem is we pre-multiply the imagebits by the alpha when optimizing
(required by GDI).  After 60s, we un-optimize and get back the bits (still
pre-multiplied).  When the image needs drawing again, we re-optimize,
pre-multiplying the image again.  This ends up in a layer effect of lowering
opacity over time.

The solution is to not pre-multiply the imagebits if the imagebits are already
pre-multiplied.

Comment 8

[ArronM (:paper)]

*** Bug 293665 has been marked as a duplicate of this bug. ***

Comment 9

[ArronM (:paper)]

Testcase:

1) Load any PNG with 8-bit alpha
(http://mozilla.animecity.nu/IMGTest/images/Mozilla.png for example)

2) Wait 60s (or 70s just to be sure).  Don't touch mouse, move from page, etc

3) Press the Reload button (or find some other way to update the image, like
hovering another window over it, or switching tabs)

4) Image will be darker.

Repeat steps 2 and 3 and image will continually get darker

Comment 10

[ArronM (:paper)]

Attachment: Don't premultiply image with alpha if it's already premultiplied

Patch that worked last night on a very dirty tree of mine.  I'm in the process
of compiling a clean tree.

Comment 11

[djpohly]

On that patch, did you mean to type mImagePreMultipled or mImagePreMultipl*i*ed?

Comment 12

[ArronM (:paper)]

Attachment: Don't premultiply image with alpha if it's already premultiplied

Fixed spelling error that Devin noticed.

The scary thing is that I had to have mispelled it 4 times when writing the
patch, yet I spelled it right on the bug comments

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183219 [details] [diff] [review]
Don't premultiply image with alpha if it's already premultiplied

a=shaver to un-regress from a previous approval.

Comment 14

[ArronM (:paper)]

Checked-in.  Thanks for the help, everyone!  I'll wait for reports before
resolving the bug

Checking in gfx/src/windows/nsImageWin.cpp;
/cvsroot/mozilla/gfx/src/windows/nsImageWin.cpp,v  <--  nsImageWin.cpp
new revision: 3.149; previous revision: 3.148
done
Checking in gfx/src/windows/nsImageWin.h;
/cvsroot/mozilla/gfx/src/windows/nsImageWin.h,v  <--  nsImageWin.h
new revision: 3.66; previous revision: 3.65
done

Comment 15

[Radek 'sysKin' Czyz]

No more problems for me, thank you!

Comment 16

[Radek 'sysKin' Czyz]

Oh, two minutes after I sent that it's back. It might be because I have an older
build. Will try again, but in the meantime scratch my confirmation.

Sorry for the bugspam.

Comment 17

[Radek 'sysKin' Czyz]

AronM, is it just me or your code only changes truecolour images with alpha,
while 256-colour images are still multiplied just like before?

Comment 18

[ArronM (:paper)]

(In reply to comment #17)
> AronM, is it just me or your code only changes truecolour images with alpha,
> while 256-colour images are still multiplied just like before?

You are right, but the function (CreateImageWithAlphaBits) only gets called on
images with 8 bit alphas, and we never never create an 8 bit alpha image with a
palette (256 colors).  That function really needs a cleanup (on my list of TODOs
sometime)

Comment 19

[Michael Apel]

fixed for me with latest beast build

Comment 20

[Jason Daly]

Can anyone else confirm that this is fixed?

Comment 21

[Tim Meader]

Yup... it's fixed since yesterday's builds. Closing.

Comment 22

[Peter van der Woude [:Peter6]]

(In reply to comment #21)
> Yup... it's fixed since yesterday's builds. Closing.
doesn't comment #18 imply this bug isn't completely fixed yet ?

Comment 23

[Tim Meader]

It simply mentions a later function cleanup from what I can tell. The problem
this bug was opened for though is indeed fixed in the nightlies. Did the patch
author intend to keep this bug open until some unspecified later date that he
can cleanup the code?

Comment 24

[ArronM (:paper)]

Thanks again everyone.  I was only leaving the bug open until I had a few more
verifies (which it now does!).  Comment #18 is indeed a cleanup bug not related
to this bug.