Fix for PNG height size limits
by Tom Spilman · in Torque Game Engine · 01/14/2007 (1:39 pm) · 5 replies
Ok this bug irked me this morning so i fixed it. It sort of turned out like a lesson on bad coding habits as well as a fix. I apologize in advance for the snark.
The first time one encounters this bug is via a fatal assertion message box...
I fixed this issue on TSE, but the code is pretty much the same on TGE and TGB. You should be able to follow these instructions to fix it on any of the engines.
Ok... all the changes are in engine\dgl\bitmapPng.cc (engine\gfx\bitmapPng.cpp in TSE).
First remove stuff we don't need. Around line 19 you'll see the following:
Remove it... not needed.
Next on to line 226 in GBitmap::readPNG():
Ohhh... an allocation... i may faint. The typical allocation here is small... 256 to 512, but can be as large as 4096. Even then we're talking about a tiny allocation... 16K... there is not good reason just allocate this. Oh one might say fragmentation! But if you look at the code the destination memory for the bitmap is allocated a few lines before this one. So this won't end up being a little gap in free memory. Even GBitmap::readMSBmp() does this exact type of allocation, just for width instead of height.
Just a few lines later, 257, we can now free it.
That's it for reading PNGs... but lets not stop there.
(continued on next post)
The first time one encounters this bug is via a fatal assertion message box...
Quote:Error, cannot load pngs taller than 2048 pixels!... followed by a fatal crash. Looking at the code i don't see a good reason for this artificial limit other than being lazy about memory allocations. The PNG standard defines the width and height values in the header as "four-byte unsigned integers". The GBitmap follows this standard by using U32 for width and height. The only limitation was in the reading and writing code.
I fixed this issue on TSE, but the code is pretty much the same on TGE and TGB. You should be able to follow these instructions to fix it on any of the engines.
Ok... all the changes are in engine\dgl\bitmapPng.cc (engine\gfx\bitmapPng.cpp in TSE).
First remove stuff we don't need. Around line 19 you'll see the following:
static const U32 csgMaxRowPointers = 1 << GBitmap::c_maxMipLevels - 1; ///< 2^11 = 2048, 12 mip levels (see c_maxMipLievels) static png_bytep sRowPointers[csgMaxRowPointers];
Remove it... not needed.
Next on to line 226 in GBitmap::readPNG():
// Set up the row pointers... AssertISV(height <= csgMaxRowPointers, "Error, cannot load pngs taller than 2048 pixels!"); png_bytep* rowPointers = sRowPointers; U8* pBase = (U8*)getBits();This code is freaking evil. Not only is it lazy using a fixed size static, but it crashes fataly... even in release builds... if the image is too tall (notice also width doesn't matter). The side effect of this decision is that the TGBX level builder will crash and you will loose your work if you try to open an image that is too tall. You cannot argue this is a speed optimization either... under normal circumstances the disk access will far and away be slower than the typical allocation for 'rowPointers'. It's just sloppy. Replace it with this...
// Set up the row pointers... png_bytep* rowPointers = new png_bytep[ height ]; U8* pBase = (U8*)getBits();
Ohhh... an allocation... i may faint. The typical allocation here is small... 256 to 512, but can be as large as 4096. Even then we're talking about a tiny allocation... 16K... there is not good reason just allocate this. Oh one might say fragmentation! But if you look at the code the destination memory for the bitmap is allocated a few lines before this one. So this won't end up being a little gap in free memory. Even GBitmap::readMSBmp() does this exact type of allocation, just for width instead of height.
Just a few lines later, 257, we can now free it.
// We're outta here, destroy the png structs, and release the lock // as quickly as possible... //png_read_end(png_ptr, end_info); delete [] rowPointers; // ADDED png_read_end(png_ptr, NULL); png_destroy_read_struct(&png_ptr, &info_ptr, &end_info);
That's it for reading PNGs... but lets not stop there.
(continued on next post)
About the author
Tom is a programmer and co-owner of Sickhead Games, LLC.
#2
01/14/2007 (1:53 pm)
@Tom - check the TXE forum where this started, you'll see my reply about graphics cards not handling graphics that tall...
#3
01/14/2007 (2:00 pm)
@Jonathon - Right... but just because the card cannot allocate a texture of that size doesn't mean that the image loading routines in the core of Torque need to be crippled to match. For instance the TSE Atlas code loads in really large images, but re-arranges them for proper rendering to video cards.
#4
01/14/2007 (4:32 pm)
Good fix, Tom. Issue #2519. 
#5
01/15/2007 (5:25 pm)
Thanks Tom, it's been fixed in our repo. 
Associate Tom Spilman
Sickhead Games
To fix this bug completely move on to GBitmap::_writePNG()... line 265:
#define MAX_HEIGHT 4096 if (height >= MAX_HEIGHT) return (false);Remove it... not needed. Now move further down to line 338. The change should be obvious.
png_write_info(png_ptr, info_ptr); // REMOVED: png_bytep row_pointers[MAX_HEIGHT]; png_bytep* row_pointers = (png_bytep*)FrameAllocator::alloc( height * sizeof( png_bytep ) ); // ADDED for (U32 i=0; i<height; i++) row_pointers[i] = const_cast<png_bytep>(getAddress(0, i));Notice we're using the FrameAllocator here. This is because if you look at all the places _writePNG() is used it is sandwiched between calls to get and set the watermark on the FrameAllocator. I could have used 'new' here, but i took advantage of the FrameAllocator and because of that i don't need to worry about freeing row_pointers... the FrameAllocator will do that for me. To learn more about the FrameAllocator see this article on TDN... highly recommended.
Before i finished though i looked and found that there is another height limit in GBitmap::writeJPEG() at line 147, but in that case there is no fixed size buffer that i can find. JPGs can be 32k x 32k in size, so i believe this is strictly a false limitation. I recommend that it be removed.
And that's it for TGE. For TSE there is another special debug GBitmap::writePNGDebug() function. I posted this fix to the TSE forums over here.
Enjoy!