19283 - Binary PNG transparency does not work correctly on Windows

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Ian Tommins ('thelem')]

Tested in: M11 (1999111520)

Expected behaviour: As at http://www.lemnet.com/trans2.html - the blue
background is 100% transparent.

Actual Behaviour: Transparency incorrectly interpreted. A Black background shows
no transparancy, a white background full transparancy and inbetween varying
amounts. http://www.lemnet.com/trans5.html shows it with a background image.

Comment 1

[Eli Goldberg]

Yup.


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

Comment 2

[Matthew Tuck [:CodeMachine]]

Umm?

From bug #3013:

> note that Mozilla has supported binary PNG transparency since
> the 19980331 release.

Bug #3013 is about 8-bit transparency (ie 256 levels of clearness in a
separate alpha channel).  The summary says this is about binary transparency (ie
making one colour in the palette mean clear).

Comment 3

[Eli Goldberg]

Doh.

Comment 4

[c.z.robertson]

I'm fairly sure that this is a duplicate of bug 13627. Can anyone else confirm
this?

Comment 5

[Ian Tommins ('thelem')]

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

Comment 6

[pnunn]

okey doke.
The background is being anded with the
given png transparency value.

Let's take trans2.html for example.
Change the page bgcolor to #000000.
The blue in the png shows blue. (0000ff)

Change the page bgcolor to #ff0000.
The blue in the png shows violet (ff00ff).

Change the page bgcolor to #ffff00.
The blue in the png shows white (ffffff).

Change the page bgcolor to #ffffff.
The blue in the png shows white. (ffffff)

Change the page bgcolor to #00ff00.
The blue in the png shows bright bluegreen. (00ffff)

Change the page bgcolor to #ff5500.
The blue in the png shows light violet. (presumably ff55ff)

sorry. I got carried away.
kevin:
I think somethings wonky in the gfx mask. 
It may have occurred when we added code for
8 bit depth.
If you guys are buried in bugs, send it back to
me and I'll ask you to review any fixes.
-P

Comment 7

[Kevin McCluskey (gone)]

Reassigning to Don.

Comment 8

[pnunn]

D:
I just thought about something that may have
affected this bug. Hold off until I get a chance
to check my guess.

ps.Why is [BLEND] in the summary field?
-P

Comment 9

[dcone (gone)]

PNG is not supported correctly, Pam Nunn has a bug on this.

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

Comment 10

[Matthew Tuck [:CodeMachine]]

Argh!  Please read the bug first, this was already once marked a duplicate of
bug #3013!

Comment 11

[dcone (gone)]

GIF transparency works.. which means we handle the 1 Bit mask correctly in GFX, 
but the PNG creation of this nsIImage must not be correct.  I did read the bug 
first!!!! the premise was that PNG was being worked on in the area of its mask's 
and this was one of the issues associated with that.

Comment 12

[Matthew Tuck [:CodeMachine]]

OK, that's fair enough, but I would expect this to be mentioned when you're
marking a bug a duplicate of one it has already been said not to be.  Especially
given the fact that it is not a duplicate, rather you're trying to expand the
scope of the other bug which specifically is about 8-bit alpha up until now.

Also, Greg's comments on the other bug are that binary transparency worked, so
there's a good chance this used to work.  Therefore it is surely not the same
issue.  Marking this a duplicate would break the one bug <-> one report issue in
my estimation.  Is there any evidence to suggest this is caused by the same
thing, other than the fact it's in the same area?

Comment 13

[pnunn]

don:
I haven't looked at this in a while.
Transparency in png was working. I suspect
some changes were made that changed the
simple transparency mask in png. I know after
some of Greg's changes, masks on linux were
wonky.

So. gfx is handling masks just dandy. The png module
is not putting the mask in the right buffer or
is doing something horrid to it.

I'll take a look at it as soon as I can.
-P

Comment 14

[Kolbjørn Barmen]

PNG bug gallore in full effect:

http://amiga.nvg.org/

Note how it uses forever to render the page due to the 3x1x1bit png transperant
backdrop (170 secs last time I tried, compare that to the fraction of a second
it take on my amiga browsers). As for transparency, does it only work ok 8bit?

Comment 15

[pnunn]

Thanks for a great test page.
Binary transparency was broken sometime around
revision 1.9 and 1.11. 

The slowness you described is more likely the
affect of the use of a very small image(1x3)
for a background image. We have been trying several
algorithms to quickly duplicate tiling and are
still working on improvements.

In the mean time, if you'd like to speed up loading of
your site, if your bkground image is greater than 8x8
pixels, you should see a large rendering speed improvement.

-p

Comment 16

[pnunn]

This is dependent on bug 16742.
Moving to m16

Comment 17

[pnunn]

Interestingly, binary transparency works on mac and linux.
Just not on wintel.
-P

Comment 18

[Chris Hill]

As far as I can tell, the Windows gfx nsImageWin.cpp drawing code is broken for 
1-bit masks.  I think it was working in 3.52 on Windows NT only using MaskBlt, 
but the MaskBlt code was removed in 3.53.  As it is now, the code draws the mask 
using the AND ROP to black out the opaque pixels on the destination, then in 
uses the OR ROP to draw the image data.  This technique only works if the 
transparent pixels in the image data have been changed to black.  I'd guess that 
this is what is happening with GIF images; by coincidence the transparent data 
is black, allowing the code to work.  That doesn't happen for PNG images, 
leading to incorrect results.

The XP PNG decoder could change the transparent pixels to black to accomidate 
this implementation, but the Windows code is wrong.  

Strangely, 8-bit alpha masks are implemented the same way as 1-bit masks in 
nsImageWin.cpp.  This really won't work because ANDing the 8-bit mask and ORing 
the image data is not blending.  Both steps are wrong.  This definitely requires 
a change in the Windows code, the blending will probably need to be done 
manually like the GDK gfx code does.

Comment 19

[Chris Hill]

The following patch "fixes" the problem on Windows by clearing the color values 
of the transparent pixels (changing them to black).  The patch is in the XP 
scale.cpp.  The patch is mainly intended to demonstrate the problem with the 
Windows mask code.  Fixing the Windows mask code would be more complicated.

Comment 20

[Chris Hill]

Attachment: Patch to scale.cpp to blacken color values of transparent pixels in images with 1-bit masks

Comment 21

[pnunn]

Chris:
I have several changes to scaling and masks queued up for check in.
These changes may affect how binary masks work. After this check in,
I'll see what is broken in binary masks and take a look at your patches.

Thanks for the help.
-pn

Comment 22

[Chris Hill]

pnunn:

Now that 8-bit masks are turned on for bug 3013, it appears that there will be 
no problems with binary transparency because none is being used!  In the current 
code 8-bit alpha will even be used for everything, even if the image is palette 
based with GIF-like transparency (or RGB with multiple transparent colors using 
the iTNS chunk).  We don't want to pay for the extra work required for 8-bit if 
1-bit will do.

From ipng.cpp: 
286     if (channels > 3) {
287         ipng_p->alpharow = NULL;
288 
289 #if defined(CAN_SUPPORT_8_BIT_MASK)
290         if(png_ptr->color_type || PNG_COLOR_MASK_ALPHA )
291                 ic->image->header.alpha_bits = 8/*png_ptr->pixel_depth*/;   
/* 8 */
292         else
293 #endif
294                  ic->image->header.alpha_bits = 1;  
295         ic->image->header.alpha_shift = 0;
296         ic->image->header.is_interleaved_alpha = TRUE;
297     }

What is the intent of line 290?  Shouldn't it be:
if(png_ptr->color_type & PNG_COLOR_MASK_ALPHA )

I just want to make sure this bug doesn't get combined with the now resolved 
3013 (or potential Windows offspring) because of this problem.  Windows still 
has problems drawing both 1 and 8 bit masks.  The workaround patch I added above 
won't work with for 8-bit masks so hopefully you will read and resolve this 
before experimenting with my earlier patch.

Comment 23

[pnunn]

Thanks for the warning and info. 
I'll be digging into this bug next week.
and yep, I'm aware this bug presents a different
set of issues. It won't be closed with 3013.

-P

Comment 24

[Masatoshi Kimura [:emk]]

>What is the intent of line 290?  Shouldn't it be:
>if(png_ptr->color_type & PNG_COLOR_MASK_ALPHA )

Wrong. Palette based images may require 8-bit mask.
For example:
http://www.cdrom.com/pub/png/img_png/stefan_pal_rgba.png
http://www.cdrom.com/pub/png/img_png/stefan_255_rgba.png

Comment 25

[Chris Hill]

Oops. You're right.  I guess it should be something like this:

#if defined(CAN_SUPPORT_8_BIT_MASK)
        if(png_ptr->color_type & PNG_COLOR_MASK_ALPHA )
                ic->image->header.alpha_bits = 8; 
        else if(png_ptr->color_type==PNG_COLOR_TYPE_PALETTE)
        {
          ic->image->header.alpha_bits = 1;  
          
          for(int i = 0; i < png_ptr->num_trans; i++)
          {
            if((png_ptr->trans[i] != 0) && (png_ptr->trans[i] != 255))
            {
              ic->image->header.alpha_bits = 8;
              break;
            }
          }
        }
        else
#endif

Comment 26

[Greg Roelofs]

With regard to line 290, I submitted a patch last week; it was ignored.  Yes,
line 290 is complete nonsense as it stands.  Chris244's fix is also wrong, at
least as the code currently is written.  For one thing, direct struct access is
going to break, probably later this year, so don't do it.  More important,
though, are two previous lines in ipng.cpp:  one that asserts channels == 3 or
4, and one for the immediate block that makes sure channels > 3.  Ergo, that
block only executes if channels == 4, which means there's a full alpha channel. 
The if/else at line 290 is completely unnecessary. (That, of course, is why the
current line-290 code works--ORing with the always-true mask is effectively a
NOP.)

If you want to modify the earlier code that sets up the transformations so that
palettes are handled separately--and 1-bit vs. 8-bit alpha is only half the
issue; you may as well skip the expansion of normal palettes and grayscale to
RGB, too--then that's the place to do it (i.e., up in the transformation block,
by reading the tRNS chunk with png_get_tRNS()).  But the code gets much more
complex when you start adding special cases for all the image types PNG
supports, and you'll gain a lot more transparency performance by fixing the
compositing logic.

However, the cardinal rule remains:  make it right before you make it fast.  Bug
3195 should be the first priority, and *then* you can start complexifying things
for performance.  (But when you do, don't touch png_struct or info_struct, or
your code *will* break with new versions of libpng.  You've been warned...)

Greg

Comment 27

[Chris Hill]

I may not have been following the rules for access to the data structures and 
the check might be in the wrong place.  My intent is to make sure that if it is 
easily possible to avoid 8-bit alpha (for pallete based images), it is done.  
Would this following better?

// right after png_get_IHDR call
    int trans_depth = 1;
    if((color_type == PNG_COLOR_TYPE_PALETTE)&&
       png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS))
    {
      png_bytep trans;      
      if(png_get_tRNS(png_ptr, info_ptr,&trans, &num_trans, trans_values))
      for(int i = 0; i < num_trans; i++)
      {
        if((trans[i] != 0) && (trans[i] != 255))
        {
          trans_depth = 8;
          break;
        }
      }
    }

// inside channels > 3
#if defined(CAN_SUPPORT_8_BIT_MASK)
        ic->image->header.alpha_bits = 8; 
        if(color_type==PNG_COLOR_TYPE_PALETTE)
           ic->image->header.alpha_bits = trans_depth; 
#else
        ic->image->header.alpha_bits = 1;
#endif

I don't mind expanding palettes and grayscale to RGB. I'm concerned about the 
performance of the drawing operation, not decoding.  It is much more expensive 
to draw 8-bit alpha unless there is hardware available.  

I submitted a patch for 3195 and [this bug/36694], someone else submitted a 
patch to fix 8-bit alpha drawing on Windows [the 8-bit part of 36694].

Comment 28

[pnunn]

Thanks for your help, Chris.
It will be a few days before I can
test your patches.
-P

Comment 29

[Masatoshi Kimura [:emk]]

Chris' second suggestion is right (except direct access internal structure), 
and last is worse.
png_set_expand() and png_read_update_info() will create the 3 (RGB) channels 
even if images are not true color, and they will create the alpha channel for 
images with tRNS even if they have no alpha.
We should check png_color_type inside channels > 3, or images with tRNS would 
use 8-bit alpha ineffectively.

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: Proposed patch based on the above idea

Comment 31

[Masatoshi Kimura [:emk]]

Hmm... png_ptr->color_type should be color_type.

Comment 32

[Greg Roelofs]

Chris, your proposed code is cleaner, but if I understood your original intent,
it's not as efficient as you want.  That is, the transformations at the top of
the info_callback() function will always expand paletted images to RGB or RGBA,
and your code doesn't change that.  If that's OK and all you want is to check
whether the (expanded) alpha channel consists of other than 0 and 255 values,
then your code is fine, except I think it will still render binary-transparency
RGB and grayscale images (that is, RGB or grayscale with tRNS) with full 8-bit
processing.  If you do this after png_get_IHDR() instead:

    int trans_depth = 1;

#if defined(CAN_SUPPORT_8_BIT_MASK)
    if (color_type & PNG_COLOR_MASK_ALPHA)
      trans_depth = 8;

    if (color_type == PNG_COLOR_TYPE_PALETTE &&
        png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS))
    {
      png_bytep trans;      
      if (png_get_tRNS(png_ptr, info_ptr, &trans, &num_trans, trans_values))
      { 
        for (int i = 0; i < num_trans; i++)
        { 
          if ((trans[i] != 0) && (trans[i] != 255))
          { 
            trans_depth = 8;
            break;
          }
        }
      }
    }
#endif /* CAN_SUPPORT_8_BIT_MASK */

...then you can get rid of the ifdef near line 290 entirely:

  ic->image->header.alpha_bits = trans_depth;

That's even cleaner and will take care of both types of "real alpha"
transparency in PNG images.

To fix the inefficiency of always expanding to 3 or 4 bytes per pixel, you would
have to make more extensive changes in info_callback(), row_callback(), and the
imagelib code that acts as a sink for PNG image data (scale.cpp and whatnot).  I
think it's probably not worth the effort unless someone does some benchmarking
first (which demonstrates that the expansion significantly slows down the
rendering of "typical" PNG-infested web pages).  Even then, there are probably
better places to optimize the PNG decoding, but those would span imagelib, the
compositor, layout, and probably some of the frontend code.

Greg

Comment 33

[Chris Hill]

There's a lot of versions of the code here, so I'll just say what I think should 
be done instead of posting yet another version. 

Here's what I want: (specifications are BEFORE expansion)

Anything with an alpha channel (channels > 3) -> 8-bit
RGB and grayscale images (channels == 3) with tRNS -> 1-bit
Palette based images:
     with tRNS which contains values other than 0 and 255  -> 8-bit
     with tRNS which contains at most 0 and 255 -> 1-bit
Everything else -> no alpha

If transparency of any kind is required, the information is decoded as RGBA.  
That is fine with me.  When constructing a container for the image that will be 
used to draw with, the minimum size alpha buffer that is true to the intent of 
the PNG file and that meets the requirements of the target platform should be 
used.

I have no interest in avoiding the expansion from palette/grayscale to RGB[A].  
The modifications would not be worth the time it would take to implement them.  
I agree that avoiding expansion should not be a priority considering some of the 
other issues that need to be fixed.  Sure, special case code could be made for 
all the combinations (grayscale alone allows 5 different depths!) but then the 
code between the decoder and the display would get more complicated (and 
probably more buggy).

Checking the palette on palette based tRNS images is something that can happen 
without any structural changes and is localized to this one section of code.

Comment 34

[pnunn]

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

Comment 35

[pnunn]

Note patch attached to above duplicate.

Comment 36

[Masatoshi Kimura [:emk]]

Changing summary since bug 36694 was a (8bit-)alpha transparency bug.

Comment 37

[Chris Hill]

I'm not sure this is a duplicate of Bug 36694.  The patch attached to 36694 
fixes 8-bit masks.  AFAIK it doesn't solve the problem with 1-bit masks. Of 
course if only 8-bit masks are used for all PNG transparency purposes, it 
doesn't really matter except that is hiding the problem with 1-bit masks on 
Windows.  If 1-bit is to be used at all, either the 1-bit Windows mask code 
needs to change or the patch I supplied earlier in this bug could be used.

To test if 1-bit masks are working, don't define CAN_SUPPORT_8_BIT_MASK and see 
if the dithered alpha version draws correctly over a non-white background with 
the 36694 patch.

Comment 38

[leger]

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

Comment 39

[Chris Nelson]

pnunn,

would it be possible to get the patch from 36694 checked in soon (pre nsbeta2)? 
It would greatly benefit skinnability, as png's 8-bit alpha blending seems to 
only work correctly on white backgrounds...

Comment 40

[pnunn]

I've reopened #36694. It is a different bug. 
Will dig into this one as soon as I take care
of some reload policy problems.
-P

Comment 41

[Ian Tommins ('thelem')]

Restored testcase to lemnet.com server. The area labelled transparent colour is 
the same as the real transparent colour, except it has 239 blue instead of 231.

Comment 42

[Eric Murphy]

Any updates on this? Seems important to me, as I am also having problems.

Also, please bump this up to severe, because it is holding people back on 
things they are trying to do.

Thanks!

Comment 43

[Masatoshi Kimura [:emk]]

Changing back the summary since bug 36694 was reopened.

Comment 44

[pnunn]

Fixed.

Comment 45

[Ian Tommins ('thelem')]

Looks great to me. Tested with my 3 testcases on Win2kLooks great to me. Tested
with my 3 testcases on Win2k at 32-bit and  256 colours. I like verifying a bug
I reported, it is kinda satisfing (sp?).

Comment 46

[Greg Roelofs]

Just for completeness, was this in fact a duplicate of bug 36694 after all, as
it now appears?  19283 is fixed in a Win32 nightly I downloaded immediately
after the 36694 patch was applied, so unless somebody had previously applied
some other patch and didn't update this bug, it looks like the 36694 patch fixed
both the 1-bit and 8-bit transparency problems.

Btw, what does it take to CLOSE a bug?  Does that ever happen?

Comment 47

[Eli Goldberg]

...it takes clicking the wrong radio button. ;)

Comment 48

[Ian Tommins ('thelem')]

But what does close mean? 

As for being a duplicate of 36694, is this possible (36694 being newer, surely 
that should be a duplicate of an expanded version of this bug).

There was talk of using 8-bit code to handle 1-bit transparency, and this is 
probably what has happened.

BTW If you have seen the 8-bit code working, you should have verified that bug. 
I have done this.

Comment 49

[timeless]

Closing means the product was shipped. eg Mozilla 1.0

Comment 50

[Masatoshi Kimura [:emk]]

Not fixed. Turn off CAN_SUPPORT_8_BIT_MASK to confirm.
Using 8-bit code to handle 1-bit transparency is very inefficient. It wastes a 
lot of memory and much smaller than 1-bit code.

Comment 51

[Masatoshi Kimura [:emk]]

smaller->slower

Comment 52

[pnunn]

>>Using 8-bit code to handle 1-bit transparency is very inefficient. It wastes a 
>>lot of memory and much smaller than 1-bit code.

Closing this bug. Functionally, transparency works and there are other bugs
that need focus now.

Opening a new bug #46700 to cover changing png transparency implementation from 
8 bit to 1 bit.

-p

Comment 53

[Masatoshi Kimura [:emk]]

Reopening.
This bug came back again since bug 46700 was fixed (that is, 1-bit code path is 
used now).
However, a fix is already proposed.
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=7722

Comment 54

[Masatoshi Kimura [:emk]]

Why is this ignored?
We cannot use png with 1-bit transparency until this bug is fixed. Tor?
Changing ongoing milestone and adding "fix in hand".

Comment 55

[tor]

VYV03354: I don't have a win32 box for mozilla development, so I haven't
seen this problem and can't test if attachment 7722 [details] [diff] [review] fixes it.  However
the patch looks fine.  Adding roc in case he has some time to check this
out.

Anyone have an updated test URL for this bug?  The current one is dead.

Comment 56

[Ian Tommins ('thelem')]

Appologies about the url, the server stopped giving dir listings.
Change trans.html to trans2.html or trans3.html for coloured (red) or black 
backgrounds respectively.

Comment 57

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

I tried the patch and it fixes the bug.

PS, computing tmprand for all pixels must really slow down this code. Can't you 
branch around it whenever tmprand is going to be irrelevant (i.e. src[3] <= 1   
or src[3] >= 202)?

Comment 58

[tor]

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

Comment 59

[Chris Hill]

Are we waiting for something here?  My two line patch has been sitting here for 
months.  

Robert: I don't know about calculating tmprand, it looks like a possible 
optimization.  Are we waiting for a response on this?  I didn't write the 
dithering code.  

I think it is more important to get this fix than to worry about a little speed.  
As I understand it, this won't be in NS6 which is really sad.  I see many bugs 
in the database which are well understood, have trivial fixes, have a contained 
and low risk patch present, have a few votes, and yet nothing happens.  I just 
hope there is a sweep through the database to fix these issues in 6.1 before 
people start implementing new features with new bugs.

Comment 60

[tor]

I'm willing to take the patch as it is now.  Tweaking the actual dithering
calculation should be the subject of another bug.

pnunn, any estimate on when we could get the module owner's r=?  Thanks.

Reassigning QA from elig to default imagelib QA.

Comment 61

[pnunn]

If someone tests that change doesn't break mac, then 
r:pnunn

Comment 62

[tor]

pinkerton reports that macos still looks fine with this patch.

sr=tor and checked into the trunk (third party patch from [e-mail]).

Nominating for rtm as this is a localized, low risk patch for a problem which
makes PNGs with binary alpha useless on MS-Windows.

Comment 63

[Gagan]

I agree. This should go in for rtm. Recommending a ++ to PDT

Comment 64

[Phil Peterson]

This bug is really not critical, but if you all think it will improve PNG, we'll
take the patch (this week) since even if it regresses something, only PNGs get
hurt. Marking [rtm++]

Comment 65

[Chris Nelson]

was this *not* checked in yesterday? or did the checkin regarding binary pngs 
have to do with something else?

Comment 66

[tor]

It was checked into the trunk yesterday, and indeed the latest trunk bits from
mozilla.org no longer show the problem.

I'll be checking it into the release branch after it reopens from verification
later today.

Comment 67

[tor]

Checked into the branch.

Comment 68

[Terri Preston]

Verified on build 2000-10-23-09-MN6/   

Comment 69

[ratman]

using win32 installer mtrunk build 2001040904, but seen since 2001040404.

this bug is back, as a regression - png images in the thinice chrome are no
longer trasnparent.  either this bug or bug 50974 should be reopened.