Sign in to follow this  

odd pointer error

This topic is 4204 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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

This topic is 4204 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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