Talk:LoadBMPCpp

From GPWiki

I can see a few problems with this file.

Firstly it doesn't take into account bitmaps which are not a width which is a mutiple of 4, ie an image with a size such as 31*32. images like these have padding which is just beening read in as if it was part of the data and it therefore misses some of the data out and will make the data corrupt.

Secondly not all bitmaps need to be flipped, only bitmaps with a height greater than zero have there y order reversed.

Thirdly the line if(pData[0x0]!='B' || pData[0x1]!='M') // BMP ID Bytes, should be 'BM' return IMG_ERR_BAD_FORMAT; does not guarantee that the type is "BM", why not check the magic number for bitmaps is correct (0x4D42 or 19778)

Security/Portability Issues

I see problems too.

- Portability

 This assumes the CPU is in little endian order. Byte-swapping
 should be performed.

- Security

 No checks for integer overflow done.

- Various nitpick

 const incorrect-ness (use const char *szFilename,
 not char *szFilename )

 use RAII-capable standard containers like std::vector
 instead of manual heap management.

Thanks for your input guys.

I do intend to update the code to address these issues. However, given that it works for 99.9% of images I throw at it in its current form, it's hard to prioritise it over other stuff I'm working on. Codehead 09:24, 24 January 2007 (EST)

This code does not work on "99.9%" of images, it works on 25% of images (when width*3 % 4 == 0), and likely less due to the other issues. When it does work, its only luck that your program works while corrupting the last line of pixels, and not segfaulting. This code should either be fixed, or removed entirely to prevent naiive users from using it.

If Codehead can not find the time to update this bitmap loader I will add code to load bitmaps which have padding(Liam 13:23, 4 March 2009 (EST))

Go ahead Liam, this is a wiki after all. Stuff is meant to be tweaked and fixed by the audience.

As my last comment was ~2 years ago, you can see I'm not getting around to sorting it out. TBH, I've never had any issues with the code and I tend to use TGA for textures thesedays anyway.

Codehead 09:46, 5 March 2009 (EST)

I seem to have missed this message. No problems I will get it up sometime today(Liam)

I did actually go back and address some of the issues in this recently (Obviously I haven't uploaded it yet). I split the code up into .h & .cpp files, fixed the 'Magic' check, did some better checking in allocation, etc. However, when I got into the actual data reading, I got tied up in trying to cope with reading some of the rarer BMP formats (OS/2 and the newer V4 and V5 headers). So if you have some thing ready to go Liam, go ahead.

A couple of other points. I'm not going to deal with Endian issues in this example, that would over-complicate the code for beginners. If people want to use it on Motorola or whatever, I'm sure they'll be aware of the gotchas.

Secondly, I didn't use vectors of any of the other containers here because most of the time you'll be passing the image data to OpenGL or another API. These APIs generally expect to receive a char array. Plus the image data is pretty static once it's been loaded so why faff with conversion between types when it's not required?

Finally, this code HAS worked on 99% of images I personally have thrown at it, because I only use ^2 images (textures). If it had exploded on me in the past, I would have fixed it by now.

Codehead 09:38, 13 March 2009 (EDT)

I have code to load 24 bit and 8 bit bmps yet it does not cater for RLE'd images, neither does it cater for endian issue in the file header or info header. The code uses no direct allocation/deallocation instead uses std::vector {Liam 10:50, 13 March 2009 (EDT)}

The code is located at www.liamdevine.co.uk/code/bitmap.php if you want to view it. I have not uploaded it here due to you saying that you are currently fixing the code Codehead {Liam 13:32, 13 March 2009 (EDT)}

Looks good Liam, I won't lose any sleep if you overwrite my page. There's nothing to stop both versions being on the Wiki either if you want to do that. Your code is more C++ compliant than mine. (My Old 'C' habits keep creeping back.) :)
Codehead 04:47, 15 March 2009 (EDT)