Sign in to follow this  
Mortizar

odd pointer error

Recommended Posts

ok this is a error with a .tga loader function i have written. The problem i am haveing is pointer related, I think. First off is the code (tga struct and function).

#define TGA_RGB		 2		// This tells us it's a normal RGB (really BGR) file
#define TGA_A		 3		// This tells us it's a ALPHA file
#define TGA_RLE		10		// This tells us that the targa is Run-Length Encoded (RLE)

typedef unsigned char byte;	
	struct  TGAFILE {

		unsigned short int ImageWidth;
		unsigned short int ImageHight;
		unsigned short int channels;

		unsigned char *Image;

	};

	TGAFILE *LoadTGA(const char *filename);




TGAFILE *LoadTGA(const char *filename) {

	TGAFILE *		tgafile = NULL;
	std::ifstream	read;
	byte			length, ImageType, bits;
	WORD			width = 0, height = 0;
	int				stride = 0, channels = 0, i = 0;

	read.open(filename, std::ios::binary);

	tgafile = new TGAFILE;
	
	read.read((char *)&length, sizeof(length));
	read.ignore(1);
	read.read((char *)&ImageType, sizeof(ImageType));
	read.ignore(9);//ignore everything until we get to width
	read.read((char *)&width, sizeof(width));
	read.read((char *)&height, sizeof(height));
	read.read((char *)&bits, sizeof(bits));
	read.ignore(length + 1);//now move to the image data

	if(ImageType != TGA_RLE) {
		if(bits == 24 || bits == 32) {

			channels	= bits/8;
			stride		= channels * width;

			tgafile->Image = new byte[stride * height];

			int x, y;

			//read the data into row data
			for(y = 0; y < height; y++) {

				//get the current line of the image we are on
				byte * line = &(tgafile->Image[stride * y]);

				//now read the image data into the line buffer
				for(x = 0; x < stride; x++) {

					read.read((char *)&line[x], sizeof(byte));
				}//end x for

				//now swap the red and blue values so its in RBG format
				for(i = 0; i < stride; i += channels) {

					int temp    = line[i];
					line[i]     = line[i + 2];
					line[i + 2] = temp;
				}//end i for
			}//end y for
		}//end bits == 24 || bits == 32 if

		else if(bits == 16) {
	
			unsigned short pixels = 0;
			int r = 0, g = 0, b = 0;

			channels	= bits/8;
			stride		= channels * width;

			tgafile->Image = new byte[stride * height];

			for(i = 0; i < (width * height); i++) {

				//read data a pixel at a time and then swap the values
				read.read((char *)&pixels, sizeof(pixels));

				b = (pixels & 0x1f) << 3;
				g = ((pixels >> 5) & 0x1f) << 3;
				r = ((pixels >> 10) & 0x1f) << 3;

				//now assigne them to the image data
				tgafile->Image[i * 3 + 0] = r;
				tgafile->Image[i * 3 + 1] = g;
				tgafile->Image[i * 3 + 2] = b;

			}//end i for
		}//end bits == 16 else if

		//if its not any of the above then its a unknown
		// type and we must exit
		else {

			return NULL;
		}//end else
	}//end ImageType != TGA_RLE if

	else {//if it gets here then the file is RLE

		byte rleID = 0;
		int colorsRead = 0;
		channels = bits/8;
		stride = channels * width;
		bool iseof = false;

		tgafile->Image = new byte[stride * height];
		byte * Colors = new byte[channels];

		//now load the data
		while(i < (width * height)) {

			read.read((char *)&rleID, sizeof(rleID));

			if(rleID < 128) {

				rleID++;

				while(rleID) {

					read.read((char *)&Colors, (sizeof(byte) * channels));



					// Store the current pixel in our image array
					tgafile->Image[colorsRead + 0] = Colors[2];
					tgafile->Image[colorsRead + 1] = Colors[1];
					tgafile->Image[colorsRead + 2] = Colors[0];

					// If we have a 4 channel 32-bit image, assign one more for the alpha
					if(bits == 32) {

						tgafile->Image[colorsRead + 3] = Colors[3];
					}//end bits == 32 if

					// Increase the current pixels read, decrease the amount
					// of pixels left, and increase the starting index for the next pixel.
					i++;
					rleID--;
					colorsRead += channels;

				}//end rleID while
			}//end rleID < 128 if

			else {

				// Minus the 128 ID + 1 (127) to get the color count that needs to be read
				rleID -= 127;

				read.read((char *)&Colors, (sizeof(byte) * channels));

				while(rleID) {

					// Assign the current pixel to the current index in our pixel array
					tgafile->Image[colorsRead + 0] = Colors[2];
					tgafile->Image[colorsRead + 1] = Colors[1];
					tgafile->Image[colorsRead + 2] = Colors[0];

					// If we have a 4 channel 32-bit image, assign one more for the alpha
					if(bits == 32)
						tgafile->Image[colorsRead + 3] = Colors[3];

					// Increase the current pixels read, decrease the amount
					// of pixels left, and increase the starting index for the next pixel.
					i++;
					rleID--;
					colorsRead += channels;
				}//end rleID while
			}//end rleID < 128 else
		}//end i while
	}//end ImageType == TGA_RLE else

	read.close();

	tgafile->channels = channels;
	tgafile->ImageWidth = width;
	tgafile->ImageHight = height;

	return tgafile;
	
}//end LoadTGA



Now the problem is after 18 loops though this part of the code:

while(rleID) {

					read.read((char *)&Colors, (sizeof(byte) * channels));

					iseof = read.eof();

					// Store the current pixel in our image array
					tgafile->Image[colorsRead + 0] = Colors[2];
					tgafile->Image[colorsRead + 1] = Colors[1];
					tgafile->Image[colorsRead + 2] = Colors[0];

					// If we have a 4 channel 32-bit image, assign one more for the alpha
					if(bits == 32) {

						tgafile->Image[colorsRead + 3] = Colors[3];
					}//end bits == 32 if

					// Increase the current pixels read, decrease the amount
					// of pixels left, and increase the starting index for the next pixel.
					i++;
					rleID--;
					colorsRead += channels;

				}//end rleID while


On the 18th loop after the call to read the debugger reads the pointer Colors as being a bad pointer. It also then gives CXX0030 error. personaly i am confused as to how this can happen when the past 17 loops worked fine and despite my best efforts i can come up with any further details or ideas as to what i am doing wrong here and any help is welcome.

Share this post


Link to post
Share on other sites
While it's not your primary problem you never delete Colors; also, I had some trouble following your code since your naming randomly switches between no word seperation and camel notation.

Share this post


Link to post
Share on other sites
I don't think this line does what you think it does:

read.read((char *)&Colors, (sizeof(byte) * channels));

This is writing the data to the address of Colors, and not the address that Colors points to. This is probably trashing the stack. Why it doesn't cause a problem in the first 17 loops is beyond me.

Share this post


Link to post
Share on other sites
Well, it's such horrible code it took me a while to find your problem:

read.read((char *)&Colors, (sizeof(byte) * channels));

Colors is already a pointer. You're not reading into the array Colors points at, your overwriting the Colors pointer itself. Give me a moment and I'll rewrite your code so that it works, is readable, is exception-safe and doesn't leak memory like nobody's business.

EDIT: It's not perfect by a long shot, but it's a lot better than your version. There are probably bugs - I didn't test it very thoroughly:
#include <string>
#include <vector>

// We're more interested in tga as an Image than as a File, so
// name the class to reflect that
class TgaImage
{

public:

// functions which create objects should usually be
// constructors
// std::string is better than char * because it allows
// clients to use either a char * or a std::string
// when creating a TgaImage, rather than only allowing
// a char *
TgaImage(std::string const & filename);

// we don't want clients to arbitrarily change the
// width, height or number of channels without actually
// changing the data correctly, so make those values
// read-only by encapsulating them and providing 'getters'
unsigned short height() const;
unsigned short number_of_channels() const;
unsigned short width() const;

private:

// some helper functions
void read_rle_tga_image(std::istream & reader);
void read_16_bit_raw_tga_image(std::istream & reader);
void read_24_or_32_bit_raw_tga_image(std::istream & reader);

typedef unsigned char byte;

unsigned short width_;
unsigned short height_;
unsigned short channels_;

// vector is a dynamic array class. it's much easier
// to use than raw pointers
std::vector< byte > data_;

};

#include "TgaImage.h"

#include <cassert>
#include <fstream>
#include <stdexcept>

// the unnamed namespace restricts the scope of variables and
// functions to this translation unit (.cpp file) only.
namespace
{

typedef unsigned char byte;

// prefer constants to #defines
// constants respect scope, the preprocessor does not
const byte type_rgb = 2;
const byte type_alpha = 3;
const byte type_rle = 10;

// a simple templated function for binary file input
template < typename Type >
void read_binary(std::istream & reader, Type & value)
{
// prefer the C++ casts (static_cast, const_cast,
// reinterpret_cast and dynamic_cast) to C-style casts
// The C++ casts are more expressive, won't silently
// do the wrong thing depending on context and are easy
// to find
reader.read(reinterpret_cast< char * >(&value), sizeof(Type));

// error checking
if (reader.fail())
{
throw std::runtime_error("error reading file");
}
}

// a specialization of the template for reading into a vector
template < typename Type >
void read_binary(std::istream & reader, std::vector< Type > & vector)
{
// don't want to index into an empty vector
if (vector.empty())
{
return;
}
reader.read(reinterpret_cast< char * >(&vector[0]), vector.size() * sizeof(Type));
if (reader.fail())
{
throw std::runtime_error("error reading file");
}
}

}

TgaImage::TgaImage(std::string const & filename)
{
// restrict using statements to the smallest scope possible
using namespace std;

// initialize at time of construction - it's often more
// efficient and never less so.
ifstream reader(filename.c_str(), ios::binary);

// it is reasonable to expect loading a tga file to succeed
// therefore an exception is a sensible approach if the
// unexpected happens
if (!reader)
{
throw std::runtime_error(("could not find tga file \'" + filename + "\'").c_str());
}

// delay declaration of variables until they are needed
// this helps keep the source more readable
byte length;
read_binary(reader, length);

// i would prefer to use a symbolic constant here, but I don't
// know tga files so I haven't a clue what that '1' is!
reader.ignore(1);
byte image_type;
read_binary(reader, image_type);
reader.ignore(9);

// error checking - make sure that unsigned char really is
// 16 bits. still not sufficient, since a byte on this
// platform may not be eight bits, but it'll do
assert(sizeof(width_) == 2 && sizeof(height_) == 2);
read_binary(reader, width_);
read_binary(reader, height_);
byte bpp;
read_binary(reader, bpp);
reader.ignore(length + 1);

if (bpp != 16 && bpp != 24 && bpp != 32)
{
throw std::runtime_error("could not load tga file - bits-per-pixel must be 16, 24 or 32");
}

// since 16 bpp is a packed format with three channels we
// have to correct this line. since the meaning is not
// apparent from just reading the source a comment is
// essential
channels_ = std::max< byte >(bpp, 24) / 8;
unsigned int stride = width_ * channels_;

// allocate our image data array. vector is an RAII wrapper
// it handles allocation and deallocation for us, so we can
// just 'allocate and forget'.
data_.resize(stride * height_);
if (image_type == type_rle)
{
// can't handle 16 bpp rle files, so error out
if (bpp == 16)
{
throw std::runtime_error("could not load tga file - rle image bits-per-pixel must be 24 or 32");
}
// this is a logical place to break the function and stop
// it getting too big and difficult to understand as a
// whole
read_rle_tga_image(reader);
}
else
{
if (bpp == 16)
{
read_16_bit_raw_tga_image(reader);
}
else
{
read_24_or_32_bit_raw_tga_image(reader);
}
}
}

unsigned short TgaImage::height() const
{
return height_;
}

unsigned short TgaImage::number_of_channels() const
{
return channels_;
}

unsigned short TgaImage::width() const
{
return width_;
}

void TgaImage::read_rle_tga_image(std::istream & reader)
{
for (unsigned int i = 0; i < static_cast< unsigned int >(width_ * height_);)
{
byte rle_type;
read_binary(reader, rle_type);
if (rle_type < 128)
{
byte pixel_count = rle_type + 1;
reader.read(reinterpret_cast< char * >(&data_[i * channels_]), pixel_count * channels_);
if (reader.fail())
{
throw std::runtime_error("error reading file");
}
for (unsigned int pixel = i * channels_; pixel < (i + pixel_count) * channels_; pixel += channels_)
{
std::swap(data_[pixel], data_[pixel + 2]);
}
i += pixel_count;
}
else
{
byte run_length = rle_type - 127;
byte colours[4];
reader.read(reinterpret_cast< char * >(&colours), channels_);
if (reader.fail())
{
throw std::runtime_error("error reading file");
}
for (byte pixel = 0; pixel < run_length; ++pixel)
{
data_[(i * channels_) + 0] = colours[2];
data_[(i * channels_) + 1] = colours[1];
data_[(i * channels_) + 2] = colours[0];
if (channels_ == 4)
{
data_[(i * channels_) + 3] = colours[3];
}
i += channels_;
}
}
}
}

void TgaImage::read_16_bit_raw_tga_image(std::istream & reader)
{
for (unsigned int i = 0; i < static_cast< unsigned int >(width_ * height_); ++i)
{
unsigned short packed_pixel;
read_binary(reader, packed_pixel);

// red component
data_[(i * channels_) + 0] = ((packed_pixel >> 10) & 0x1f) << 3;

// green component
data_[(i * channels_) + 1] = ((packed_pixel >> 5) & 0x1f) << 3;

// blue component
data_[(i * channels_) + 2] = ((packed_pixel >> 0) & 0x1f) << 3;
}
}

void TgaImage::read_24_or_32_bit_raw_tga_image(std::istream & reader)
{
read_binary(reader, data_);
for (unsigned int i = 0; i < data_.size(); i += channels_)
{
std::swap(data_[i], data_[i + 2]);
}
}


Σnigma

[Edited by - Enigma on June 12, 2006 5:22:37 PM]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this