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)

x86
Windows 95
defect

Tracking

()

VERIFIED FIXED

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.
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
Yup. *** This bug has been marked as a duplicate of 3013 ***
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).
Status: VERIFIED → REOPENED
Doh.
Resolution: DUPLICATE → ---
Severity: major → normal
Target Milestone: M14
I'm fairly sure that this is a duplicate of bug 13627. Can anyone else confirm this?
*** Bug 13627 has been marked as a duplicate of this bug. ***
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
Reassigning to Don.
Assignee: kmcclusk → dcone
Status: NEW → ASSIGNED
Summary: Binary PNG transparency does not work → [BLEND]Binary PNG transparency does not work
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
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 ago25 years ago
Resolution: --- → DUPLICATE
Argh! Please read the bug first, this was already once marked a duplicate of bug #3013!
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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
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?
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
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?
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
This is dependent on bug 16742. Moving to m16
Target Milestone: M15 → M16
Interestingly, binary transparency works on mac and linux. Just not on wintel. -P
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.
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.
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
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.
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
>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
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
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
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].
Thanks for your help, Chris. It will be a few days before I can test your patches. -P
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.
Attached patch Proposed patch based on the above idea (deleted) — — Splinter Review
Hmm... png_ptr->color_type should be color_type.
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
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.
*** Bug 36694 has been marked as a duplicate of this bug. ***
Note patch attached to above duplicate.
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
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.
Target Milestone: M16 → M17
Keywords: pp
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not critical to beta2.
Whiteboard: [nsbeta2-]
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-]
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
Blocks: 8415
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.
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!
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
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago24 years ago
Resolution: --- → FIXED
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
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?
...it takes clicking the wrong radio button. ;)
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.
Closing means the product was shipped. eg Mozilla 1.0
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 → ---
smaller->slower
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
>>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
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 → ---
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
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.
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)?
*** Bug 56414 has been marked as a duplicate of this bug. ***
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.
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
If someone tests that change doesn't break mac, then r:pnunn
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.
Keywords: patch, rtm
Whiteboard: [nsbeta2-]fix in hand → [nsbeta2-][rtm+]
I agree. This should go in for rtm. Recommending a ++ to PDT
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++]
was this *not* checked in yesterday? or did the checkin regarding binary pngs have to do with something else?
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.
Checked into the branch.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 years ago
Resolution: --- → FIXED
Verified on build 2000-10-23-09-MN6/
Status: RESOLVED → VERIFIED
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.

Attachment

General

Creator:
Created:
Updated:
Size: