Thread: Image height = -16

  1. #16
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,057
    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.
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  2. #17
    Registered User
    Join Date
    Apr 2006
    Posts
    2,149
    Quote Originally Posted by mike_g View Post
    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.
    It is too clear and so it is hard to see.
    A dunce once searched for fire with a lighted lantern.
    Had he known what fire was,
    He could have cooked his rice much sooner.

  3. #18
    Dr Dipshi++ mike_g's Avatar
    Join Date
    Oct 2006
    Location
    On me hyperplane
    Posts
    1,218
    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;
    }

  4. #19
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #20
    Dr Dipshi++ mike_g's Avatar
    Join Date
    Oct 2006
    Location
    On me hyperplane
    Posts
    1,218
    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.

  6. #21
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,057
    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?
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  7. #22
    Dr Dipshi++ mike_g's Avatar
    Join Date
    Oct 2006
    Location
    On me hyperplane
    Posts
    1,218
    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.

  8. #23
    Frequently Quite Prolix dwks's Avatar
    Join Date
    Apr 2005
    Location
    Canada
    Posts
    8,057
    Hmm, no, maybe not. My compiler doesn't seem to mind this:
    Code:
    #include <cstring>
    
    int main() {
        return strcmp("name", "NAME");
    }
    dwk

    Seek and ye shall find. quaere et invenies.

    "Simplicity does not precede complexity, but follows it." -- Alan Perlis
    "Testing can only prove the presence of bugs, not their absence." -- Edsger Dijkstra
    "The only real mistake is the one from which we learn nothing." -- John Powell


    Other boards: DaniWeb, TPS
    Unofficial Wiki FAQ: cpwiki.sf.net

    My website: http://dwks.theprogrammingsite.com/
    Projects: codeform, xuni, atlantis, nort, etc.

  9. #24
    Registered User
    Join Date
    Sep 2006
    Posts
    835
    Quote Originally Posted by dwks View Post
    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.)

  10. #25
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by mike_g View Post
    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.

    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.

    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  11. #26
    Dr Dipshi++ mike_g's Avatar
    Join Date
    Oct 2006
    Location
    On me hyperplane
    Posts
    1,218
    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

  12. #27
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    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.

    RAM is dirt cheap nowadays
    Not so cheap that you can afford to waste 75&#37; of the space an image needs.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  13. #28
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by CornedBee View Post
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  14. #29
    Dr Dipshi++ mike_g's Avatar
    Join Date
    Oct 2006
    Location
    On me hyperplane
    Posts
    1,218
    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.

  15. #30
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Bump maps, displacement maps (OK, so probably not both of these), height maps, light maps ...
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Problem reading tiff image files?
    By compz in forum C++ Programming
    Replies: 9
    Last Post: 10-30-2009, 04:17 AM
  2. Image rotation - doesn't always work
    By ulillillia in forum C Programming
    Replies: 12
    Last Post: 05-03-2007, 12:46 PM
  3. SSH Hacker Activity!! AAHHH!!
    By Kleid-0 in forum A Brief History of Cprogramming.com
    Replies: 15
    Last Post: 03-06-2005, 03:53 PM
  4. Binary Search Trees Part III
    By Prelude in forum A Brief History of Cprogramming.com
    Replies: 16
    Last Post: 10-02-2004, 03:00 PM
  5. Replies: 4
    Last Post: 03-02-2003, 09:12 AM