46700 - change png binary transparency implementation from 8 bit to 1 bit.

Status: RESOLVED FIXED

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

Comment 1

[Eli Goldberg]

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

Comment 2

[pnunn]

yes. This only applies to _binary_ transparency.

Comment 3

[Stuart Ballard]

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.

Comment 4

[tor]

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.

Comment 5

[tor]

The following simplistic approach doesn't work, as apparently PNGs are
allowed to have a tRNS chunk as well as ordinary alpha information.
Adding [e-mail] 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

Comment 6

[Greg Roelofs]

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.

Comment 7

[tor]

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?

Comment 8

[tor]

Attachment: use 1-bit alpha if tRNS chunk indicates binary alpha

Comment 9

[tor]

Taking bug, shifting milestone.

Comment 10

[Greg Roelofs]

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

Comment 11

[Greg Roelofs]

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().

Comment 12

[tor]

Attachment: updated patch incorporating Greg's comments

Comment 13

[tor]

Checked in.  Thanks for your help, Greg.

Comment 14

[Greg Roelofs]

You betcha.  Thanks for taking care of this one.