PNG Loader Bug and Fix

[borky89]

I found some strange behaviour when I loaded PNGs with an alpha channel.

Example:


Fixed:


The problem was, that the old code tried to just use the ARGB data. The error was: PNGs have RGBA data.

And here's the fix:

Code: Select all

Index: CImageLoaderPNG.cpp
===================================================================
--- CImageLoaderPNG.cpp	(revision 165)
+++ CImageLoaderPNG.cpp	(working copy)
@@ -229,18 +229,18 @@
       for (unsigned int x = 0; x < this->width; x++ ) 
 	  { 
 		  //loop through the row of pixels 
-         if (!alphaSupport) 
-		 { 
-            data[y*width*inc + x*inc] = *(pSrc); //red 
-            data[y*width*inc + x*inc + 1] = *(pSrc+1); //green 
-            data[y*width*inc + x*inc + 2] = *(pSrc+2); //blue 
+         if (!alphaSupport) // RGB -> RGB 
+         { 
+            data[y*width*inc + x*inc + 0] = *(pSrc + 0); // red 
+            data[y*width*inc + x*inc + 1] = *(pSrc + 1); // green 
+            data[y*width*inc + x*inc + 2] = *(pSrc + 2); // blue 
          } 
-		 else 
+         else // RGBA -> ARGB 
 		 { 
-            data[y*width*inc + x*inc] = *(pSrc+2); //red 
-            data[y*width*inc + x*inc + 1] = *(pSrc+1); //green 
-            data[y*width*inc + x*inc + 2] = *(pSrc); //blue 
-            data[y*width*inc + x*inc + 3] = *(pSrc+3); //alpha 
+            data[y*width*inc + x*inc + 0] = *(pSrc + 3); // alpha 
+            data[y*width*inc + x*inc + 1] = *(pSrc + 0); // red 
+            data[y*width*inc + x*inc + 2] = *(pSrc + 1); // green 
+            data[y*width*inc + x*inc + 3] = *(pSrc + 2); // blue 
          } 
 
          pSrc+=inc; //move to next pixel (24 or 32 bits - depends on alpha) 

I hope this will be fixed soon.

Georg

[afecelis]

Nice! thanks! I hope Hybrid sees this one. Or I'll point him to it

[hybrid]

I've done it a little different, but revision 166 should have all things fixed, and also brings support for grey maps, palette colors, and less than 8 bpp format support.

[afecelis]

yes!!!! Gray texture support? finally?

[hybrid]

Well, JPEG already loads gray maps, but both loades just expand those file formats to true color, i.e. 24/32bit. There's still no support for 8bit textures in Irrlicht.

[borky89]

Again there is a bug in revision 172.
The final color mode should be ARGB, not BGRA

Patch:

Code: Select all

Index: CImageLoaderPNG.cpp
===================================================================
--- CImageLoaderPNG.cpp	(revision 172)
+++ CImageLoaderPNG.cpp	(working copy)
@@ -186,9 +186,9 @@
 		(png_uint_32*)&Width, (png_uint_32*)&Height,
 		&BitDepth, &ColorType, NULL, NULL, NULL);
 
-	// Convert RGBA to BGRA
-	if (ColorType==PNG_COLOR_TYPE_RGB_ALPHA)
-		png_set_bgr(png_ptr);
+	// Convert RGBA to ARGB
+	if (ColorType == PNG_COLOR_TYPE_RGB_ALPHA)
+        png_set_swap_alpha(png_ptr);
 
 	// Update the changes
 	png_read_update_info(png_ptr, info_ptr);

Georg

[hybrid]

But this one does not work for me. Do you use a big endian machine?

[borky89]

I do. (PPC Mac OS X) But that shouldn't have anything to do with it.

If there was a endian-bug, I think it has to do with irrlicht, not with libpng. Did my "old" sollution work on LE systems?

Georg

[hybrid]

I don't know, the complete handling in the Irlicht code was strange and did not allow for the other changes. The problem is usually that Irrlicht uses s32 per pixel instead of 4*u8 which avoid big/little endian issues.

[borky89]

I get more and more the feeling, that Irrlicht is based on a buggy core. Hopefully only at the endian issues.
I try to look into it tomorrow. (But I fear I won't have the time I need.)

Georg

[hybrid]

All data layout is usually based on the DirectX data types. That's why some things are not designed for portability.