36694 - PNG Alpha Support not implemented on Windows

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

transparent png images are not draw correctly on non-white backgrounds. This is 
tested on a build that did NOT have CAN_SUPPORT_8_BIT_ALPHA enabled.

Transparency is however drawn correcly on text as seen on 
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=2527

Build 2000-04-20-08 on Win98SE

Comment 1

[Kevin McCluskey (gone)]

Don, is this a blender problem? or does this belong to imagelib?

Comment 2

[Greg Roelofs]

Ditto for today's nightly Win32 build, which presumably DOES have
CAN_SUPPORT_8_BIT_ALPHA enabled (per Pam's final comments on bug 3013 yesterday
afternoon). Rendering of http://www.cdrom.com/pub/png/pngs-img.html is screwed
up in all of the transparent areas, regardless of whether the display is 16 or
24 bits deep, and even for the penguin image at the top (which has only GIF-like
binary transparency).  The IceAlpha and MagnoliaAlpha pages are likewise messed
up--there's some correlation with the texture of the background image, but not
with its colors.

Cc'ing Pam since she did the Win32 testing.

Comment 3

[Greg Roelofs]

This may be a duplicate (or extension) of bug 19283.  Alpha blending didn't
exist when that bug was filed, however.

Comment 4

[Chris Hill]

See my comments at the bottom of bug 19283, I probably should have filed a new 
bug but new we have this one.

The masks in nsImageWin.cpp don't work (both 1-bit and 8-bit).  This is not a 
blender issue.  The blender is currently only used for the opacity property, not 
per-pixel alpha.  This is Windows specific.  It is not a PNG decoder bug.

Only if the color data for the transparent pixels of the image is black do 1-bit 
masks work (as it does in GIF).  Over in 19283, I have a workaround patch that 
makes transparent pixels black for PNG to fix 1-bit.  Strictly speaking, this is 
a bug it the Windows code.  The reason it works on white backgrounds is because 
the OR raster op is responsible for this problem when OR is used on top of 
white, the pixel remains white.

8-bit needs to be correctly implemented on Windows, and this means custom code 
as no system functionality is available on all Windows versions exists to draw 
with an 8-bit mask.  The GDK image implementation blends by hand, and something 
similar is needed on Windows.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: proposed patch

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: new patch: fixing AlphaBlend() bug and enabling AlphaBlend() on Win98/2000

Comment 7

[Chris Hill]

I tried the first patch but I couldn't compile because I'm using VC++ 5, which 
probably has some older headers that don't define BLENDFUNC.  I solved it by 
using conditional defines around the use of BLENDFUNC.  I can compile the rest 
of mozilla, so I think there should be an option to compile without AlphaBlend 
support (or copy the definition of the structure so that VC++ 5 can build an 
AlphaBlend compatible version).  Perhaps the conditional use could be tied to 
the WINVER constant. Is there a new one for Win98/Win2k?

The patch looks great on my NT 4.0 system with 32-bit video.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Third patch: adding BLENDFUNCTION for VC++ 5

Comment 9

[Masatoshi Kimura [:emk]]

Please confirm whether VC++ 5 can compile, since I don't have it.

Comment 10

[Chris Hill]

Yes, it compiles now.  Thanks.

Comment 11

[pnunn]

don:
I think this one is mine. And perhaps a duplicate at that.
reassigning to self.
-p

Comment 12

[pnunn]

duplicate of #19283.

*** This bug has been marked as a duplicate of 19283 ***

Comment 13

[pnunn]

I

Comment 14

[pnunn]

I'm reopening bug. And I'm changing the 
Summary to 'banding problems with 8bit 
mask on windows'.

I'm also attaching a test page that displays
a bit more of the problem. 

Unfortunately a patch for this will need alot
of testing on all platforms. What fixes one may
break another.

Thanks for keeping this bug alive, Chris.
-P

Comment 15

[pnunn]

Attachment: Zipped file containing test html and images

Comment 16

[Chris Hill]

The banding problem is Windows specific, right?  It happens not because of the 
decoded PNG data or any XP issues, but because the nsImageWin implementation is 
broken.

The patch from [e-mail] only changes
mozilla/gfx/src/windows/nsImageWin.h and
mozilla/gfx/src/windows/nsImageWin.cpp

It should not have any impact on other platforms. Or did you mean different 
Windows platforms (Win95, Win98, NT 4.0, Win2k)?

Comment 17

[Dawn Endico]

I don't see the banding problem on linux.

I applied the patch and recompiled and things are still fine.

This patch only changes files in mozilla/gfx/src/windows and seems
unliekly to affect other platforms besides windows.

Comment 18

[pnunn]

Kevin:

I reassigning this to you since you are the
module owner for gfx. The attached patch would
need your blessing before it could be checked in.
(I have not had time to test out the patch.)

My take on the alphachannel banding problem on
windows is due to incorrect values used as the
8 bit mask....maybe for some reason windows expands
the grayscale values to 24 bit, but only uses the value on
R and not G&B..or something like that. This is just a 
guess after looking at the test urls.

-pn

Comment 19

[Chris Hill]

The banding is because the 8-bit mask is combined with the destination using the 
SRCAND raster operation, as would be done with a 1-bit mask, before laying down 
the color data with SRCPAINT (and OR op).

The mask must be used to blend the source color with the destination color, 
there is no simple raster op to blend.  The patch looks for OS support for 
blending and uses it if present.  If it is not present, it blends manually.

Comment 20

[Chris Hill]

Does anybody know when this patch might be checked in?

Comment 21

[Kevin McCluskey (gone)]

Don, could you look at the patch? This is in an area that you implemented.

Comment 22

[dcone (gone)]

Checking out the patch currently.. and if everything is ok, I will check in on 
m17.

Comment 23

[leger]

Putting on [nsbeta2-] radar.  Not critical to beta2.

Comment 24

[dcone (gone)]

This really needs to get in.. this is why PNG was invented.  If not beta2 then 
beta 3 is really important...

Comment 25

[/\/\arcio Galli]

Adding a simple testcase with "alpha balls" and R,G,B,Y,C,M,White backgrounds
to help visualization:

http://www.geckonnection.com/wed2/alphatest.html

Comment 26

[Greg Roelofs]

Marcio's web server is misconfigured, so I'm not sure his test case is going to
be useful:

> HEAD http://www.geckonnection.com/wed2/images/binocle2.png
200 OK
Connection: close
Date: Thu, 22 Jun 2000 18:18:13 GMT
Accept-Ranges: bytes
Server: Apache/1.2.6 FrontPage/3.0.4
Content-Length: 10832
Content-Type: text/plain                       <-- should be image/png
ETag: "19add6-2a50-39484ff0"
Last-Modified: Thu, 15 Jun 2000 03:39:28 GMT
Client-Date: Thu, 22 Jun 2000 18:18:22 GMT
Client-Peer: 192.41.26.206:80

Also, unless I'm going blind, the test page has a useless SCRIPT tag in it with
no closing tag.  Might be good to run it past the W3C validator...

Greg

Comment 27

[/\/\arcio Galli]

I just fixed mime-type and <script> tag of testcase Greg, thanks! I just 
included this example page because it's easy to visualize a problem with 
blending in the png image that is over the pure-green background. Tested with: 
20000623/Win98.

Comment 28

[Stuart Ballard]

Recent discussion on n.p.m.seamonkey (well, a staircase thread between myself
and Phil Peterson under the subject "Suggestion: moz-help keyword...") suggests
that just because the bug is nsbeta2- does not mean that the patch can't be
checked in (although it is, of course, subject to code review and approval by
the module owner). It doesn't sound like it would be a "destabilizing" patch.

Judging by the number of cc's and votes on this bug, quite a lot of people would
like to see it fixed... is there any chance that the patch could be checked in
soon?

Comment 29

[dcone (gone)]

The patch will be checked in when the tree opens again.. this will make the 
final ship...

Comment 30

[Thomas Swan]

Thank you...

Comment 31

[pnunn]

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

Comment 32

[[e-mail]]

Is this in? Tree is now open, check-em-in if you got 'em.
If it's in please mark "FIXED" and await confirmation.

Comment 33

[dcone (gone)]

No this is not in yet.. the tree has to be branched -- post beta 2 before I can 
check this in.

Comment 34

[Chris Nelson]

Post beta2? Is it that unstable? PNGs really can't be used for skins if this 
isn't fixed. And aren't skins a key point of the second beta? You would think 
that the powers that be would be a bit sensible regarding this.

Comment 35

[Stuart Ballard]

According to Phil Peterson on n.p.m.seamonkey, you don't *have* to wait until
after beta2 branches to put this in, since the patch was written by an external
contributor.

Please add the "patch" keyword to this bug as it contains an externally
contributed patch. I just tried but I don't have the permissions.

The rules for checkins of external contributor's patches are different than
those for NS employees - "code review by module owner, approval by Waterson or
Brendan". And NS employees *are* allowed to perform the checkin subject to these
rules, if the patch was written by an external contributor. Please try to get
the patch in by *these* rules instead of the more restrictive rules for patches
by NS employees. Phil just posted a message in n.p.m.seamonkey specifically
addressing this issue (subject: bugs with PATCH keyword).

If you have any other reason for delaying this then that's a different issue.
But I don't think you "have to" delay it.

Thanks!

Comment 36

[dcone (gone)]

I would rather an external contributor check this in if thats the route you want 
to go.  There are issues with this loophole.. and I really don't want to explore 
these so close to beta.  

Comment 37

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

I have installed the latest patch and everything appears to be working 
perfectly.

Comment 38

[dcone (gone)]

can you check the patch in.. get Kevin McClusky to review.. or tell him I 
(Don Cone reviewed it).. then ask for Mozilla.org permission to check in?

Comment 39

[Brendan Eich [:brendan]]

There is no "loophole".  If you, dcone, can argue convincingly that there is too 
much risk in taking this particular patch (not just any patch) right now, please 
do so.  You have final say as module owner.  On the other hand, if you would 
rather delegate the checkin to [e-mail] so he takes cvsblame, that's 
fine too.  What's not fine is to default control of the Mozilla open source 
project's CVS trunk to netscape.com exclusively.  Sorry if I'm preaching to the 
choir.

Robert, are you ready to check in?  Has kmmclusk given an r= (not clear whether 
that's necessary given dcone's r=)?  Should you assign this bug to yourself, 
since you're checking in the fix?

Anyway, a=[e-mail].

Comment 40

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

Sorry, I'd rather not check it in. I'm afraid of my Mozilla CVS password being 
sniffed.

Comment 41

[pnunn]

here is another test url donated by Paluba Michal <[e-mail]>
http://www.spsselib.hiedu.cz/~zaruba/Temp/AlphaBlended_PNG-test.html
-p

Comment 42

[Brendan Eich [:brendan]]

Robert, [e-mail] says he'll get you SSH cvs access tonight.

/be

Comment 43

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

Cool, thanks.

That test looks nice.

Comment 44

[Eli Goldberg]

[Note to petersen or self --- when verifying this bug, be sure to check all test 
cases, including those in 40916.]

Comment 45

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

OK, I am now fully CVS and SSH enabled (thanks dmose) :-).

Kevin, please review the patch:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=7864
Checkin has already been approved by mozilla.org. When Kevin says OK, I will 
check in. This bug can be assigned to me if dcone is tired of it :-).

Comment 46

[dcone (gone)]

Kevin.. I have been running with this patch for a while.. with no problems.
Reassigning to Robert so he can check this in if Kevin approves.

Comment 47

[Kevin McCluskey (gone)]

Robert: Looks good. - Kevin

Comment 48

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

Patch checked in. Thanks all!

Comment 49

[Thomas Swan]

Works for me as well... as of build 2000072008 (Win2K)

Thanks a million!!!!

Comment 50

[Chris Nelson]

Visit this test URL mentioned in the comments:

http://www.geckonnection.com/wed2/alphatest.html

Scroll down and then back up. On Win98, 7-20-00 build, the top 3, and 
occasionally 4 "circles" will become garbled. Now, switch focus to another app 
that covers the garbled circles. Switch back to Mozilla. The circles now display 
correctly.

Is this a problem with the PNG rendering, or a more serious problem affecting 
all images? I don't notice this on any other sites with transparent GIFs.

Comment 51

[Thomas Swan]

I cannot duplicate the results mentioned by Chris Nelson on 2000-07-20 at 
12:31.  Works flawlessly for me.  All repaints and scrolling actions do not 
produce the "garbled" result.  (Win2K, 1600x1200x32b)

Comment 52

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

I cannot reproduce with NT4 SP5.

Comment 53

[Chris Nelson]

To: those who can't reproduce the problem I mentioned above...

Are the circles in question scrolling offscreen? My screensize is 1024x768, the 
top 4 circles scroll offscreen, and they're the ones that become garbled.

Comment 54

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

I'm running 1024x768, and the circles are certainly being scrolled offscreen and 
back on.

Comment 55

[John Dobbins]

Tested with build 2000072008 On Win 98
I can reproduce this on Win 98. Win 9x only?
Res is 1024 x 768. When I scroll all the way down the top 4 images scroll off
the top and are garbled when I scroll back down. The 4th image scrolls off the
bottom when I move the scroll bar all the way to the top. scrolling back down
the 4th image is ungarbled when it is scrolled back from the into view from the
bottom.

Comment 56

[John Dobbins]

Farther tests. I tried lower resoulations and less than full screen size so I
could scroll more images off the top and bottom of the window. All images are
broken when scrolled off the top of the window. All images are fixed if they are
scrolled off the bottom, then scrolled back into view, or if they are hidden by
another ap then brought back into view. 

Comment 57

[Masatoshi Kimura [:emk]]

The problem mentioned above occurs on:
2000072008 nightly build on Windows 98 (1024x480).

Doesn't occur on:
2000072008 nightly build on Windows 2000 (1024x768).

Probably this is specific to Win98.

Comment 58

[Chris Nelson]

http://www.geckonnection.com/wed2/alphatest.html

Another twist. If you see the garbled images described in my previous test,  
clicking on the garbled image once it is fully on the screen will fix it 
properly -- it will be re-rendered and appear in the proper position.

However, if you click on the garbled image while it is partially scrolled off 
the top of the viewport, it is re-rendered, however, it is not rendered in the 
proper position. Mozilla acts as if the top of the viewport is the proper top of 
the image (even though the "proper" top of the image is scrolled out of view), 
and renders the image there, cropping the bottom of the image at its "proper" 
bottom.

John Dobbins, you were able to reproduce the initial problem. Maybe you could 
confirm this and describe it better.

Comment 59

[John Dobbins]

Confirming Chris' last obervation. In addition I noticed that if 3 or more
images are garbled sometime clicking on one will cause 2 of them to repaint. I
could not find any pattern. Killed the cash and this disapeared. only 1 image is
"fixed" with no mem or disk cash. seems to be a repaint problem with top of
window on Win 9.x . Can someone test this on Win95 to see if this is a general
9.x problem or it's Win 98 only. 

Comment 60

[John Dobbins]

Another test. To check if this problem existed before fix was checked in and
just not noticed. I tried build 20001910. There is no sign of this problem in
yesterday's build.

Comment 61

[Masatoshi Kimura [:emk]]

This is a AlphaBlend() implementation bug on Windows 98. We can fix this by 
disabling AlphaBlend() on Win98.
Windows 95/NT4 have no problem since their system have no AlphaBlend() support.
I confirmed this problem doesn't occur on Windows 95 (res: 800x600).
Can anyone test to see if this is fixed on Windows Me?

Comment 62

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

Ooops, I checked in the wrong version of the patch (minus the VC5 compatibility 
code). I will check in the VC5 compatibility code when the tree opens. It's very 
minor and will not affect VC6 builds:

#if !defined(AC_SRC_OVER)
#define AC_SRC_OVER                 0x00
#define AC_SRC_ALPHA                0x01
#pragma pack(1)
typedef struct {
    BYTE   BlendOp;
    BYTE   BlendFlags;
    BYTE   SourceConstantAlpha;
    BYTE   AlphaFormat;
}BLENDFUNCTION;
#pragma pack()
#endif

Comment 63

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

Do I need to get approval again?

Comment 64

[Brendan Eich [:brendan]]

Not really, a= still applies, along with the r= provided reviewers saw the code 
you intended to check in.

/be

Comment 65

[Masatoshi Kimura [:emk]]

Attachment: A patch: disable using AlphaBlend() on Win98

Comment 66

[Masatoshi Kimura [:emk]]

I heard AlphaBlend() bug is fixed on Windows Me RC1, so I disabled it only on 
Winsows 98.

Comment 67

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

OK, Kevin and Brendan, can I get r= and a= to check in
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=11754
and the VC5 compatibility snippet? Thanks.

Comment 68

[Kevin McCluskey (gone)]

Reviewed http://bugzilla.mozilla.org/showattachment.cgi?attach_id=11754 - looks 
good. 

Comment 69

[Brendan Eich [:brendan]]

a=[e-mail].

/be

Comment 70

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

Checked in VC5 build fix and patch to disable AlphaBlend() on Win98.

Comment 71

[Ian Tommins ('thelem')]

The w3 url looks fine to me. Win2k running at 32-bit colour.

Comment 72

[Eli Goldberg]

Could this possibly be also checked on Win98 prior to formally marking Verified? 
(I'll volunteer if nobody would like to.) 

Comment 73

[Eli Goldberg]

this space intentionally left blank.

Comment 74

[/\/\arcio Galli]

Folks, this is just to let you know:

Maybe not related with this but I just filed today a bug:46607
PNG+alpha images inside DIV areas (with opacity 0<value<1 ) results 
in rendering problems or even a crash.

Comment 75

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Verifying for Win98SE where the AlphaBlend() bug existed before the fix so I'm
verifying the bug.

Thank you guys, this is Beauuuuuuuuuuuuutiful =). Really love the png-over-text
testcase, cool to resize the window and se stuff moving around below the png.

Comment 76

[Daniel Veditz [:dveditz]]

Adding keyword to bugs which already show a nsbeta2 triage value in the status 
whiteboard so the queries don't get screwed up.