Image height = -16

I'm reading header info from a bitmap. I can get the image width and bit-depth fine but the height comes out as -16. I am almost certain that this problem is caused by me reading bytes from the file as char type, but I cant work out how to get it to read as Uint8. Aslo the way I am doing this is probably unnecessarily complicated. Heres my code:

Code:

---

int Image::LoadBMP(char* filename)
{
        ifstream in;
        in.open(filename, ios::binary);
        if(! in.is_open()) return LOAD_IMAGE_INVALID;
       
        char i[4];

        in.seekg(18, ios::beg);
        in.read(i, 4);
        w=(i[3]<<24)+(i[2]<<16)+(i[1]<<8)+i[0];
       
        //in.seekg(22, ios::beg);
        in.read(i, 4);
        h=(i[3]<<24)+(i[2]<<16)+(i[1]<<8)+i[0];

        in.seekg(2, ios::cur);
        in.read(i, 2);
        bpp=(i[1]<<8)+i[0];

        in.close();
        return IMAGE_LOAD_SUCCESS;
}

---

Any suggestions on a working/better way to read this data?

unsigned char i[4] perhaps?

Nah, I already tried that but it wont seem to work. I'd get errors when I call in.read() saying:

Code:

---

error C2664: 'std::basic_istream<_Elem,_Traits>::read' : cannot convert parameter 1 from 'unsigned char [4]' to 'char *'

---

Probably the endian ordering. I had a similar problem when reading dimensions for a terrain file. I had to do shifts each time I read a byte until I recieved the proper value.

Is a BMP really big-endian? Either way, shifting a (signed) char will not give you what you want. Cast the chars to unsigned before you shift them, or make the whole array unsigned and cast it to a regular char * when you call get().

Personally, I'd write a small inline function which takes a char [4] and returns an unsigned 32-bit integer. Call it "unpack32BitBigEndian" or something. No need to duplicate the same code over and over.

Hmm, I thought I sorted that out with all the bitshifting I'm doing.

Also my test image has a height of 240. And -16 as a char would be something like 239 as Uint8.

Quote:

---

Originally Posted by mike_g

Hmm, I thought I sorted that out with all the bitshifting I'm doing.

---

The code you have now is treating the byte sequence as big-endian -- the first byte gets shifted to the MSB. But the real problem is the signed shift.

EDIT: And the bit patterns for (unsigned char)240 and (signed char)-16 are the same bit pattern, so it's definitely the problem.

Thanks brewbuck, that fixed it. I'm casting it to Uint8 just before the bitshifting. As in:

Code:

---

w=((Uint8)i[3]<<24)+((Uint8)i[2]<<16)+((Uint8)i[1]<<8)+(Uint8)i[0];

---

But would there be a cleaner way to write this? It looks kind of messy.

[edit]Oh and how do I get if the machine is big endian or not?

Thanks[/edit]

Quote:

---

Originally Posted by mike_g

But would there be a cleaner way to write this? It looks kind of messy.

---

Like I said, contain the messiness in a function which does this for you.

Quote:

---

Oh and how do I get if the machine is big endian or not?

---

It has nothing to do with the endianness of your machine -- the shifting method protects against your MACHINE'S endianness. It has to do with the endianness of the FILE FORMAT.

Ok cool, i'll just chuck it in a function. and forget about the endian thing then :)

Quote:

---

Originally Posted by mike_g

Ok cool, i'll just chuck it in a function. and forget about the endian thing then :)

---

Well I'm kind of concerned that a Microsoft Windows file format would be big-endian. I don't have the BMP spec or know where to get one, so I don't know for sure, but I suspect the format is actually little-endian, like most Windows computers. I COULD BE WRONG THOUGH.

The difference between reading a big-endian and a little-endian value is simple, the indexes go [3], [2], [1], [0] for big-endian and [0], [1], [2], [3] for little-endian.

Just to be clear, the shifting method makes sure that you get the right value taking into account your machine's endianness. You still have to deal with the endianness of the data source itself.

If your machine is an x86, then endian is unlikely to be the problem.

BMP files with a positive height load from bottom to top. ISTR from somewhere that a negative height means that the image loads from top to bottom.

> I don't have the BMP spec or know where to get one
Almost all file formats can be found by typing "foo file format" into a search engine.
http://www.wotsit.org/ for example.

Quote:

---

BMP files with a positive height load from bottom to top. ISTR from somewhere that a negative height means that the image loads from top to bottom.

---

Yes, that's correct. http://www.mathworks.com/access/help...+Height+Values

Quote:

---

Originally Posted by Salem

Almost all file formats can be found by typing "foo file format" into a search engine.
http://www.wotsit.org/ for example.

---

I'm fairly paranoid about file formats. If I can't prove to myself that what I'm looking at is "THE spec" I don't tend to trust it very much. When you're dealing with an MS spec things get even more... interesting.

wotsit.org is very reliable when it comes to file formats.

I've loaded thousands of BMPs and usually when a value in the header is not reading correctly it is caused by byte alignment in the header structure.

I've never come across a negative height for a BMP but keep in mind the BMP format has so many variations it's nearly impossible to account for them all w/o writing an entire library. This is why Windows has functions specifically for loading BMPs.

You could look at some existing code in libraries that read BMP files. The source for SDL_LoadBMP() comes to mind. SDL_LoadBMP() is #defined as a call to SDL_LoadBMP_RW(). The source for it is available here: http://www.google.com/codesearch?hl=...DL_bmp.c#first

Of course, I'm not sure how portable the SDL BMP loading code is, though I should imagine it's pretty good.

Quote:

---

Originally Posted by mike_g

Nah, I already tried that but it wont seem to work. I'd get errors when I call in.read() saying:

Code:

---

error C2664: 'std::basic_istream<_Elem,_Traits>::read' : cannot convert parameter 1 from 'unsigned char [4]' to 'char *'

---

---

This error is usually cause by passing characters arrays to template functions that take a reference. The problem is that if an array is passed by reference then the whole array is passed, not just a pointer. The solution is to either cast the array you are passing to it to a char pointer yourself, or to explicitly specify the template arguments in the function call.

This problem also applies to literal strings.

Thanks for all the help. I'll checkout wotsit.org, for other file format stuff. For now I have managed to get my image class to load and save 24bit, uncompressed, BMPs. I plan on adding features to load: TGAs, PNGs, and JPEGs. Though I have heard JPEGs are hard to do. Anyway, heres the code incase anyone finds a use for it. If you can spot anything that could be done better, let me know :)

Code:

---

#include <cstring>
#include <cctype>
#include <fstream>

typedef unsigned char  Uint8;
typedef unsigned short Uint16;
typedef unsigned int  Uint32;

enum ImageExtensions { NO_EXT, EXT_INVALID, BMP, JPG, PNG, TGA };
enum ImageLoad { LOAD_IMAGE_SUCCESS, LOAD_IMAGE_INVALID, LOAD_IMAGE_UNSUPPORTED_BITDEPTH };

class Image
{
    public:
        Image(){};
        ~Image(){delete[] pixels;}
        bool Load(char*);
        bool Save(char*);
                int GetWidth();
                int GetHeight();
    private:
        int GetExt(char*);
        int LoadBMP(char*);
        int LoadJPG(char*);
        int LoadPNG(char*);
        int LoadTGA(char*);
                int SaveBMP(const char*);
               
                int BytesToInt(char*);
                short BytesToShort(char*);
                void IntToBytes(char*, Uint32);
                void ShortToBytes(char*, Uint16);
        int w, h;
        Uint32 *pixels;
               
};

//------------------------------------------------------------------//
//********************* PUBLIC FUNCTIONS ***************************//
//------------------------------------------------------------------//

bool Image::Load(char* filename)
{
    switch(GetExt(filename))
    {
        case NO_EXT:
            return false;
        case EXT_INVALID:
            return false;
        case BMP:
            LoadBMP(filename);
            break;
        case JPG:
   
            break;
        case PNG:

            break;
        case TGA:

            break;
    }
    return true;
}

bool Image::Save(char* filename)
{
        SaveBMP(filename);
        return 0;
}
int Image::GetWidth()
{
        return w;
}
int Image::GetHeight()
{
        return h;
}

//------------------------------------------------------------------//
//********************* PRIVATE FUNCTIONS **************************//
//------------------------------------------------------------------//

int Image::GetExt(char* filename)
{
    //This function gets a file extension and checks if it is valid
    char ext[10]="\0";
    char *e_ptr = ext;
    char *f_ptr = filename;
    Uint16 count = 0;
    Uint16 index = 0;

    //Get index of last full stop
    while(*f_ptr++)
    {
        count++;
        if(*f_ptr == '.') index = count;     
    }   
    if(!index) return NO_EXT;
    f_ptr = filename+index;
   
    //Copy The extension converting to upper case
    while(*e_ptr++ = toupper(*f_ptr++));

    //Check against valid extensions   
    if(!strcmp(ext, ".BMP"))  return BMP;
    if(!strcmp(ext, ".JPG"))  return JPG;
    if(!strcmp(ext, ".JPEG")) return JPG;
    if(!strcmp(ext, ".PNG"))  return PNG;
    if(!strcmp(ext, ".TGA"))  return TGA;
    if(!strcmp(ext, ".TARGA"))return TGA;
    return EXT_INVALID;
}

int Image::LoadBMP(char* filename)
{
        ifstream in;
        in.open(filename, ios::binary);
        if(! in.is_open()) return LOAD_IMAGE_INVALID;
       
        char bytes[4];

        in.seekg(18, ios::beg);
        in.read(bytes, 4);
        w=BytesToInt(bytes);
       
        in.read(bytes, 4);
        h=BytesToInt(bytes);

        in.seekg(2, ios::cur);
        in.read(bytes, 2);
        short bpp=BytesToShort(bytes);
        if(bpp != 24) return LOAD_IMAGE_UNSUPPORTED_BITDEPTH;

       
        //Allocate space for image data
        pixels = new Uint32[w*h];
        Uint32 *px = pixels;

        in.seekg(54, ios::beg);
        for(int y=h; y>0; y--)
        {
                for(int x=0; x<w; x++)
                {               
                        in.read(bytes, 3);
                        *px=((Uint8)bytes[2]<<16)+((Uint8)bytes[1]<<8)+(Uint8)bytes[0];
                        px++;
                }
        }
        in.close();
        return true;
}

int Image::BytesToInt(char* i)
{
        return ((Uint8)i[3]<<24)+((Uint8)i[2]<<16)+((Uint8)i[1]<<8)+(Uint8)i[0];
}
short Image::BytesToShort(char* i)
{
        return ((Uint8)i[1]<<8)+(Uint8)i[0];
}
int Image::SaveBMP(const char* filename)
{
        ofstream out;
        out.open(filename, ios::binary);
        if(! out.is_open()) return 1;
       
        char bytes[4]={'B','M','B', 'M'};

        //-------HEADER--------------//
        out.write(bytes, 2);                                //Write bitmap ID

        IntToBytes(bytes, w*h*3+54);
        out.write(bytes, 4);                                //Uint32 File Size In Bytes

        IntToBytes(bytes, 0);
        out.write(bytes, 4);                                //Uint16 reserved 1 and 2

        IntToBytes(bytes, 44);
        out.write(bytes, 4);                                //Int Offset To Image Data

        //------- FILE INFO --------//

        IntToBytes(bytes, 40);
        out.write(bytes, 4);                                //Uint32 Header Size In Bytes
       
        IntToBytes(bytes, (Uint32)w);
        out.write(bytes, 4);                                //Int Image Width
        IntToBytes(bytes, (Uint32)h);
        out.write(bytes, 4);                                //Int Image Height

        ShortToBytes(bytes, 1);
        out.write(bytes, 2);                                //Unsigned Short Planes = 1
        ShortToBytes(bytes, 24);
        out.write(bytes, 2);                                //Unsigend Short Bit Depth = 24
        IntToBytes(bytes, 0);
        out.write(bytes, 4);                                //Unsigned Int Compression = 0 (No compression)
        IntToBytes(bytes, w*h*3);
        out.write(bytes, 4);                                //Unsigned Int Image Size In Bytes
        IntToBytes(bytes, 2835);
        out.write(bytes, 4);
        out.write(bytes, 4);                                //Int X_Res, Y_Rex (pixes per meter)
        IntToBytes(bytes, 0);
        out.write(bytes, 4);                                //Uint32 number of paletted colours =0
        out.write(bytes, 4);                                //Uint32 Important colours

        //------ IMAGE DATA --------//
        Uint32 *px = pixels;
        for(int y=h; y>0; y--)
        {
                for(int x=0; x<w; x++)
                {
                        IntToBytes(bytes, *px);
                        out.write(bytes, 3);
                        px++;
                }
        }
        out.close();
        return 0;
}

void Image::IntToBytes(char* bytes, Uint32 num)
{
        bytes[3] = num >> 24;
        bytes[2] = (num >> 16) & 255;
        bytes[1] = (num >> 8) & 255;
        bytes[0] = num & 255;
}
void Image::ShortToBytes(char* bytes, Uint16 num)
{
        bytes[1] = num >> 8;
        bytes[0] = num & 255;
}

---

A few comments:
1. you use const inconsistantly. Surely ALL of your filenames are always const?

2. If you ever plan on extending your image to allow inheritance, you may want to make getwidth, getheight, load and save virtual member functions.

3. Why not pass in a reference to the stream that has been already opened and checked that it's a valid stream to your LoadFMT function, rather than duplicating the open and error checking multiple times.

4. I think BytesToInt, IntToBytes, etc are not supposed to be member functions - they have no reason to be part of the class, they don't do things to a class member, they don't (really) operate on something specific to the class. And I also think the numbers should be unsigned also on the return values.

5. Eventually, you'll probably want to modify the pixels within the image, so you will need methods to get one or sets of pixeles, along with something like getStride() that returns the number of bytes per row of pixels.

6.

Code:

---

char ext[10]="\0";

---

is the same as

Code:

---

char ext[10]="";

---

for the use that you have of that variable.

7. I think you may want to use a void * for your pixels, as Uint32 * indicates that the pixels are 32-bits, which may not be the case. If it's a void pointer, it certainly forbids any direct use, so there's no temptation of trying to use the pixels directly in an incorrect manner. You can always use a local variable that is a copy of pixels in any code that accesses the pixels.

8.

Code:

---

    //Get index of last full stop
    while(*f_ptr++)
    {
        count++;
        if(*f_ptr == '.') index = count;     
    }   
    if(!index) return NO_EXT;
    f_ptr = filename+index;

---

can be done with

Code:

---

    //Get index of last full stop
    f_ptr = strrchr(filename, '.');
    if(!f_ptr) return NO_EXT;

---

9. There is a "magic" number in the bitmap format that says "this is a BMP file" besides the extension (I can quite easily take a text file and rename it to .bmp, and your application would probably fall over in a heap). Check the first two bytes, it should be "BM".

10. You may want some more sanity checking and error checking to see that the values you get are "not completely crazy". For example, there is a size of the image in bytes, and a dimension in pixels wide and high. Multiplied by bitdepth, those should match up.

--
Mats

Thanks for the feedback matsp. To some of the points you mentioned:

1. Yeah I'll change all instances of filenames passed into functions to constant char. Something I hadent got around to doing.

2. Err, I don't know what a virtual member function is, guess I should look it up. On the few occasions I used inheritance, I just changed all my private stuff to protected and it seemed to do what I wanted.

3. Sounds good.

4. Yeah, I dont know, It kind of keeps everything used as part of the class though. I'm not too fussed about that really.

5. I'll definitely be doing that.

7. I think I am just going to have it set at 32bit. Nothing uses lower bit depths nowadays anyway.

8. Cool, I dident know that.

9/10. Youre right I should really check that stuff first.

Anyway, I'll work on this some more tomorrow. Right now I'm off to bed. Nite.

Correct me if I'm wrong, but shouldn't the code use std::strcmp() etc since <cstring> rather than <string.h> is included, and there are no using directives?

I dont know. Would that be important? I added 'using namespace std' in other files included in the project, so thats why I might not notice anything going wrong here.

Hmm, no, maybe not. My compiler doesn't seem to mind this:

Code:

---

#include <cstring>

int main() {
    return strcmp("name", "NAME");
}

---

Quote:

---

Originally Posted by dwks

Hmm, no, maybe not. My compiler doesn't seem to mind this:

Code:

---

#include <cstring>

int main() {
    return strcmp("name", "NAME");
}

---

---

http://cboard.cprogramming.com/showthread.php?t=86711

(I think you were the one who refreshed my memory by pointing out this thread to me a while back.)

Quote:

---

Originally Posted by mike_g

2. Err, I don't know what a virtual member function is, guess I should look it up. On the few occasions I used inheritance, I just changed all my private stuff to protected and it seemed to do what I wanted.

---

Definitely try to learn how virtual works. It is an important part of object orientation, and it's definitely necessary for polymorphism - I think your class actually should make use of polymorphism in the sense that you probably should have one base-class for "Image", and then multiple inherited classes that implement for example BMP, JPEG, PNG, etc.

Quote:

---

4. Yeah, I dont know, It kind of keeps everything used as part of the class though. I'm not too fussed about that really.

---

Well, that obviously depends on what you are trying to achieve, but one of the bases for class design is that a "class only contains what it needs". It is very easy to try to make "swiss knife classes", which incorporates too much to be meaningful.

In this case the functions I pointed out are not dealing with anything inside the class, they are just converting data from one format to another. This is typically best done with a static function in the class implementation file. It shouldn't be part of the class declaration tho'.

The rule should be "if it doesn't have to be in the class, don't put it in the class". Classes tend to grow too large eventually anyways, without having extra stuff added that don't need to be there.

Quote:

---

7. I think I am just going to have it set at 32bit. Nothing uses lower bit depths nowadays anyway.

---

And that applies for all image formats that you intend to support? What if you were to support 16-bit per pixel TIFF, or perhaps GIF that is (AFAIK) always 8BPP?

There are also plenty of BMP's that are 1BPP because they are black and white or 8BPP grayscale - if it's not in colour, it's no point in spending 4 times the data-size to store it, is there?

I'm not trying to clank down on your code - I'm just trying to get it right before you've written too much of it and then have to make massive changes.

--
Mats

Yeah I'll check out virtual functions and maybe try and break my class down a bit as it would end up pretty big. If you haven't noticed yet I still kind of suck at OOP. I guess taking out the non-essential functions wouldn't hurt either.

As for supporting lower bitdepths, I'm still not bothered about it. Bitmasks are useful but I would probably make a separate class for them. PNG is better than GIF in nearly every respect, so I'm not planning GIF support. As for greyscale; I would be wasting space, but hey RAM is dirt cheap nowadays :)

Quote:

---

one base-class for "Image", and then multiple inherited classes that implement for example BMP, JPEG, PNG, etc.

---

That's not a good design. The serialization format is a property of the image loader/writer, not of the image itself. Once you've loaded an image, whatever format it was is irrelevant.

Quote:

---

RAM is dirt cheap nowadays

---

Not so cheap that you can afford to waste 75&#37; of the space an image needs.

Quote:

---

Originally Posted by CornedBee

That's not a good design. The serialization format is a property of the image loader/writer, not of the image itself. Once you've loaded an image, whatever format it was is irrelevant.

---

Yes, I agree. Perhaps what I described wasn't quite the right way to go about things, but I still think the class should be separated into a common part, and a part that understands a particular image format (e.g. bmp, jpeg, etc).

--
Mats

Quote:

---

Not so cheap that you can afford to waste 75&#37; of the space an image needs.

---

Ok, maybe think about it this way: How many greyscale images is anyone ever likely to have simultaneously loaded in RAM? Probably not many. The only sort of thing like that I can thing of would be bump map data, which fits snuggly in the high 8 bits of an image.

Bump maps, displacement maps (OK, so probably not both of these), height maps, light maps ...