Closed
Bug 19283
Opened 25 years ago
Closed 24 years ago
Binary PNG transparency does not work correctly on Windows
Categories
(Core :: Graphics: ImageLib, defect, P3)
Tracking
()
VERIFIED
FIXED
M18
People
(Reporter: ian, Assigned: pnunn)
References
()
Details
(Keywords: platform-parity, Whiteboard: [nsbeta2-][rtm++])
Attachments
(2 files)
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.
Updated•25 years ago
|
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → DUPLICATE
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Comment 2•25 years ago
|
||
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).
Updated•25 years ago
|
Status: VERIFIED → REOPENED
Comment 3•25 years ago
|
||
Doh.
Updated•25 years ago
|
Resolution: DUPLICATE → ---
Comment 4•25 years ago
|
||
I'm fairly sure that this is a duplicate of bug 13627. Can anyone else confirm
this?
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
Assignee: pnunn → kmcclusk
Status: REOPENED → NEW
Updated•25 years ago
|
Status: NEW → ASSIGNED
Summary: Binary PNG transparency does not work → [BLEND]Binary PNG transparency does not work
Updated•25 years ago
|
Target Milestone: M14 → M15
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•25 years ago
|
||
PNG is not supported correctly, Pam Nunn has a bug on this.
*** This bug has been marked as a duplicate of 3013 ***
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → DUPLICATE
Comment 10•25 years ago
|
||
Argh! Please read the bug first, this was already once marked a duplicate of
bug #3013!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 11•25 years ago
|
||
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.
Assignee: dcone → pnunn
Status: REOPENED → NEW
Summary: [BLEND]Binary PNG transparency does not work → Binary PNG transparency does not work
Comment 12•25 years ago
|
||
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?
Assignee | ||
Comment 13•25 years ago
|
||
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
Status: NEW → ASSIGNED
Comment 14•25 years ago
|
||
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?
Assignee | ||
Comment 15•25 years ago
|
||
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
Assignee | ||
Comment 16•25 years ago
|
||
This is dependent on bug 16742.
Moving to m16
Target Milestone: M15 → M16
Assignee | ||
Comment 17•25 years ago
|
||
Interestingly, binary transparency works on mac and linux.
Just not on wintel.
-P
Comment 18•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
Assignee | ||
Comment 21•25 years ago
|
||
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•25 years ago
|
||
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.
Assignee | ||
Comment 23•25 years ago
|
||
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•25 years ago
|
||
>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•25 years ago
|
||
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•25 years ago
|
||
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•25 years ago
|
||
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].
Assignee | ||
Comment 28•25 years ago
|
||
Thanks for your help, Chris.
It will be a few days before I can
test your patches.
-P
Comment 29•25 years ago
|
||
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•25 years ago
|
||
Comment 31•25 years ago
|
||
Hmm... png_ptr->color_type should be color_type.
Comment 32•25 years ago
|
||
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•25 years ago
|
||
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.
Assignee | ||
Comment 34•25 years ago
|
||
*** Bug 36694 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 35•25 years ago
|
||
Note patch attached to above duplicate.
Comment 36•25 years ago
|
||
Changing summary since bug 36694 was a (8bit-)alpha transparency bug.
Summary: Binary PNG transparency does not work → Binary/Alpha PNG transparency does not work correctly on Windows
Comment 37•25 years ago
|
||
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 39•24 years ago
|
||
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...
Whiteboard: [nsbeta2-] → [nsbeta2-] PLEASE reconsider for nsbeta+.
Whiteboard: [nsbeta2-] PLEASE reconsider for nsbeta+. → [nsbeta2-]
Assignee | ||
Comment 40•24 years ago
|
||
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
Reporter | ||
Comment 41•24 years ago
|
||
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•24 years ago
|
||
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•24 years ago
|
||
Changing back the summary since bug 36694 was reopened.
Summary: Binary/Alpha PNG transparency does not work correctly on Windows → Binary PNG transparency does not work correctly on Windows
Assignee | ||
Comment 44•24 years ago
|
||
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 24 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 45•24 years ago
|
||
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?).
Status: RESOLVED → VERIFIED
Comment 46•24 years ago
|
||
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•24 years ago
|
||
...it takes clicking the wrong radio button. ;)
Reporter | ||
Comment 48•24 years ago
|
||
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•24 years ago
|
||
Closing means the product was shipped. eg Mozilla 1.0
Comment 50•24 years ago
|
||
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 51•24 years ago
|
||
smaller->slower
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 52•24 years ago
|
||
>>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•24 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 54•24 years ago
|
||
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".
Whiteboard: [nsbeta2-] → [nsbeta2-]fix in hand
Target Milestone: M17 → M18
Comment 55•24 years ago
|
||
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.
Reporter | ||
Comment 56•24 years ago
|
||
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.
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•24 years ago
|
||
*** Bug 56414 has been marked as a duplicate of this bug. ***
Comment 59•24 years ago
|
||
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•24 years ago
|
||
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.
QA Contact: elig → tpreston
Assignee | ||
Comment 61•24 years ago
|
||
If someone tests that change doesn't break mac, then
r:pnunn
Comment 62•24 years ago
|
||
pinkerton reports that macos still looks fine with this patch.
sr=tor and checked into the trunk (third party patch from Chris244@aol.com).
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•24 years ago
|
||
I agree. This should go in for rtm. Recommending a ++ to PDT
Comment 64•24 years ago
|
||
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++]
Whiteboard: [nsbeta2-][rtm+] → [nsbeta2-][rtm++]
Comment 65•24 years ago
|
||
was this *not* checked in yesterday? or did the checkin regarding binary pngs
have to do with something else?
Comment 66•24 years ago
|
||
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•24 years ago
|
||
Checked into the branch.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 69•24 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•