err, stupid question, but this is specifically for _binary_ transparency, right? ;)

yes. This only applies to _binary_ transparency.

This is closely related to bug 36002, in that they both affect many of the same performance issues. Specifically, of the following three issues: 1) Slow scrolling and redraw when binary-transparent PNGs are involved 2) Slow scrolling when alpha-transparent PNGs are involved 3) Slow performance when binary-transparent PNGs are involved in dynamic pages This bug applies to 1 and 3; 36002 applies to 1 and 2. In other words, a fix for this would provide half a fix for 36002 and vice versa.

This has become important because the new modern skin is using (abusing) PNGs with alpha channels. PNGs with binary tRNS chunks are being converted to 8-bit alpha images internally, which forces them through the slow compositing DrawImage paths.

The following simplistic approach doesn't work, as apparently PNGs are allowed to have a tRNS chunk as well as ordinary alpha information. Adding newt@pobox.com for his insight. Index: ipng.cpp =================================================================== RCS file: /cvsroot/mozilla/modules/libimg/pngcom/ipng.cpp,v retrieving revision 1.24 diff -u -r1.24 ipng.cpp --- ipng.cpp 2000/08/07 22:09:02 1.24 +++ ipng.cpp 2000/09/04 04:34:59 @@ -295,7 +295,8 @@ ipng_p->alpharow = NULL; #if defined(CAN_SUPPORT_8_BIT_MASK) - if(png_ptr->color_type || PNG_COLOR_MASK_ALPHA ) + if ((png_ptr->color_type || PNG_COLOR_MASK_ALPHA) && + !(png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS))) ic->image->header.alpha_bits = 8/*png_ptr->pixel_depth*/; /* 8 */ else #endif

It's late enough that I'm a little fuzzy and not willing to go pore over CVS sources and libpng documentation at the moment, but two comments come to mind right off: (1) If by "allowed to have a tRNS chunk as well as ordinary alpha" you mean the *same* PNG, then no--tRNS is specifically disallowed in PNG images with an alpha channel. (It would have made sense in the same way that PLTE is allowed in RGB PNGs for the purpose of suggested palettes, but that's a completely separate issue.) (2) I'm not 100% certain, but I think the pre-png_update_info() transformations nullify a lot of the png_get_valid() stuff that would have worked earlier in the code. That is, until you do png_update_info(), the description of the image returned by the png_get_valid() calls corresponds to the actual, untransformed image--any transformation calls you may have made are just setting bits in the PNG struct, not actually doing anything to the image or affecting png_get_valid() info. But *after* png_update_info(), which I believe is where the code in the patch lives, all previously set transformations are treated as though they're in effect. So unless something else has changed upstream since I wrote the transformation code a year ago, all knowledge of PLTE and tRNS is gone at line 295; the image is either 24-bit RGB or 32-bit RGBA at this point. If it's just a matter of flagging that the 8-bit alpha channel really consists only of 0 and 255 values, then it should be possible to do that up above (i.e., set a flag up there and use it in this if-statement). Note, however, that you'll have to get all of the tRNS data--potentially 255 bytes--and check whether it includes any values other than 0 and 255; if it does, you won't be able to use the 1-bit code path. Sorry if I'm restating the obvious here.

Thanks for the explanation - that pointed out some of my misunderstandings regarding tRNS. I'm attaching a patch which makes libimg use 1-bit alpha if the tRNS information indicates binary transparency. Could you review it?

Taking bug, shifting milestone.

Yes, that looks correct. (I'm not currently set up to compile the beast, so I haven't tested it to be sure.) But please change "png_ptr->color_type" to "color_type" in the outer if-test. And purely from a readability standpoint, I think the innermost if-test would be easier to understand if written as: if ((trans[i] != 0) && (trans[i] != 255)) { That's aesthetics, though. Greg

One other comment: since you're not using trans_values, you can eliminate the variable entirely and use "NULL" as the last argument to png_get_tRNS().

Checked in. Thanks for your help, Greg.

You betcha. Thanks for taking care of this one.