class atributes being corrupted after initialization of object

klebermo (44)
I have trying implement this class class Bitmap : public Netpbm, where the subclass Bitmap has a read_file method which read data from a file (to atributes inherited from superclass), and in the superclass Netpbm I have the toArray which return an 1d array for the data read from the file.

In my main function, I will have something like that:

1
2
3
 
Bitmap image(file_name.c_str());
Surface * view = new Surface("image", image.getWidth(), image.getHeight());
view->getRenderer()->setImage(image.toArray(), image.getWidth(), image.getHeight());


After the initialization (image(file_name.c_str())), I checked each atribute and everything is fine. But when I try execute image.toArray() in the third line, some data from the object is corrupted.

I verified that every time, it's always the first bytes of the object (something like 2-6 bytes from the start), and some bytes of the dynamically allocated atribute. For instance, I have this atributes for the class:

1
2
3
4
 
  char magicNumber;
  int width;
  int height;
  Matrix<pixel> * pixels;


magicNumber and width will have corrupted data, height will be ok, and for pixels (which, as the name implies, encapsulates a 2d array), only the first line of the matrix will have some corrupted data. This happens every time I run the program.

Looking in the code for the classes below, can anyone give me a hint of what's happening here, or something to fix this issue?

NetPBM class

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
 
struct Pixel {
  float r, g, b;
};
typedef struct Pixel pixel;

class Netpbm {
protected:
  char magicNumber;
  int width;
  int height;
  Matrix<pixel> * pixels;
public:
  Netpbm();
  ~Netpbm();

  char getMagicNumber();
  int getWidth();
  int getHeight();

  virtual void read_file(char * file_name) = 0;
  virtual void write_file(char * file_name) = 0;
  
  float * toArray();
};


Matrix class

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
 
template <class T>
class Matrix {
private:
    int height;
    int width;
    T ** values;
public:
    Matrix(int height, int width) {
        values = new T*[height];
        for(int i=0; i<height; i++)
            values[i] = new T[width];
        this->height = height;
        this->width = width;
    }

    ~Matrix() {
        for(int i=0; i<height; i++)
            delete[] values[i];
        delete[] values;
    }

    T * operator[](int index) {
        return values[index];
    }
};


Bitmap class

1
2
3
4
5
6
7
8
 
class Bitmap : public Netpbm {
public:
  Bitmap(std::string file_name) {
    read_file(file_name);
  }
  void read_file(std::string file_name);
  void write_file(std::string file_name);
};


toArray implementation

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
float * Netpbm::toArray() {
  float * result = new float[width * height * 5];

  int count = 0;
  for(int i=0; i<height; i++) {
    for(int j=0; j<width; j++) {
      float x = j/width, y=i/height;
      result[count++] = -1 + (2 * x);
      result[count++] = 1 - (2 * y);
      result[count++] = (*pixels)[i][j].r;
      result[count++] = (*pixels)[i][j].g;
      result[count++] = (*pixels)[i][j].b;
    }
  }

  return result;
}


read_file implementation

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
 
void Bitmap::read_file(std::string file_name) {
  std::ifstream file(file_name);
  std::string line_one, line_two, line_pixels;

  while(getline(file, line_one)) {
    if(line_one.size() > 0 && line_one.at(0) != '#') {
      this->magicNumber = line_one.at(1);
      break;
    }
  }

  while(getline(file, line_two)) {
    if(line_two.size() > 0 && line_two.at(0) != '#') {
      std::string width, height;
      std::stringstream ss(line_two);

      if(getline(ss, width, ' '))
        this->width = stoi(width);
      
      if(getline(ss, height, ' '))
        this->height = stoi(height);
      
      break;
    }
  }

  pixels = new Matrix<pixel>(height, width);

  if(magicNumber == '1') {
    std::vector<pixel> v;

    while(getline(file, line_pixels)) {
      if(line_pixels.size() > 0 && line_pixels.at(0) != '#') {
        std::stringstream ss(line_pixels);
        std::string value;
        while(getline(ss, value, ' ')) {
          pixel p;
          p.r = p.g = p.b = stoi(value);
          v.push_back(p);
        }
      }
    }

    int counter = 0;
    for(int i=0; i<height; i++)
      for(int j=0; j<width; j++)
        (*pixels)[i][j] = v[counter++];
  }

  if(magicNumber == '4') {
    std::vector<pixel> v;

    while(!file.eof()) {
      unsigned char data[1];
      file.read((char*)data, 1);
      for(int i=0; i<8; i++) {
        pixel p;
        if(data[0]&(1<<i))
          p.r = p.g = p.b = 1;
        else
          p.r = p.g = p.b = 0;
        v.push_back(p);
      }
    }

    int counter = 0;
    for(int i=0; i<height; i++)
      for(int j=0; j<width; j++)
        (*pixels)[i][j] = v[counter++];
  }
}



Ganado (6784)
First thing is
1
2
 
    if(line_one.size() > 0 && line_one.at(0) != '#') {
      this->magicNumber = line_one.at(1);

You are checking to make sure line_one.size() > 0, good, but that doesn't mean line_one.at(1) exists. Of course, this would throw an exception if it didn't exist... so it's not the issue here, but still a bit weird.

Second thing I see is that you're using looping on !file.eof() on line 53. I suggest looping more like:
1
2
3
4
5
 
unsigned char data[1];
while (file.read((char*)data, 1)
{
    ...
}


What library is the Surface/getRenderer stuff?

Edit: Also, your Netpbm destructor should be virtual.
https://stackoverflow.com/questions/461203/when-to-use-virtual-destructors
Last edited on
TheIdeasMan (6783)
Hi

The Bitmap::read_file(std::string file_name) function sets height and width, but these are never initialised in the base class, so when you try to use them with Netpbm::toArray(), they will have default values. This looks like object slicing. Do height and width need to belong to NetBpm? See comment below about protected variables. The job of the constructor is to initialise all the varibales in the class.

Have you used a debugger?

Were there any warnings when you compiled? How did you compile exactly?

The code looks like the infamous mixture of C and C++, mainly C.

In C++ we try to not use pointers or new, prefer to use a STL container instead, they do memory management for you. In Netpbm::toArray() it looks like you have a memory leak - there isn't a delete to match the new or is the destructor not shown for brevity? Using a STL container avoids all this hassle.

Functions should only do one conceptual thing. If you find yourself writing functions that do several things, split them up. Notably Bitmap::read_file(std::string file_name)

protected variables are a bad idea, in the same way that public variables are. One can have private variables with protected functions to access / mutate them.

You have functions:

1
2
3
 
char getMagicNumber();
  int getWidth();
  int getHeight();

Are they used anywhere? Do you need them?

I hope all this helps :+)
mbozzi (3911)
First sentences of the OP sound like object slicing, but my experience is that object slicing is more likely a problem in the usage code that isn't shown here. Other problems could be (repeating everyone)
- Lack of a virtual destructor, or
- Lack of copy construction & assignment operator

Try a lifetime sanitizer;
Try a undefined behavior sanitizer;
Try placing a watchpoint on the first byte of the object that becomes corrupted.

This:
float * result = new float[width * height * 5];
Looks like it might have integer overflow problems.

Can you make a small, complete example of the problem and post it here?
Last edited on
seeplus (6458)
The Matrix and Netpbm classes also need a copy constructor and an operator = . They use dynamic memory so need a deep copy for these operations rather than the default shallow copy.

Why is pixels a pointer?

As a matter of style, I'd advise against having a member function with params or local variables with the same name as member variables. At some point using the same name will bite.
klebermo (44)
In the example, where I should add the copy construction & assignment operator, as sugested? In the base class or in the sub class?
Last edited on
klebermo (44)
I have tried add the virtual destructor and a copy constructor. After adding the copy constructor (both in the superclass and in the subclass), I keep getting no errors or warnings as before, but now all the atributes get their values corrupted in the toArray method (in the read_file method everything gets the correct one).

the copy constructor:

1
2
3
4
5
 
Netpbm::Netpbm(const Netpbm &other) {
  this->magicNumber = other.magicNumber;
  this->width = other.width;
  this->height = other.height;
}


the virtual destructor:

declaration:

 
 
virtual ~Netpbm() = 0;


implementation:

1
2
3
 
Bitmap::~Bitmap() {
  delete pixels;
}

klebermo (44)
For the record, the complete current code for the Main function is this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
 
#include "2D/Surface.hpp"

#include "netpbm.hpp"

int main(int argc, char ** argv) {
    if (argc < 2) {
        std::cout << "Usage: game <file_name>" << std::endl;
        return 0;
    }

    if (SDL_Init(SDL_INIT_VIDEO) >= 0) {
        std::string file_name(argv[1]);
        std::string extension;
        
        std::stringstream ss(file_name);
        while(getline(ss, extension, '.'));

        if(extension == "pbm") {
            Bitmap image(file_name.c_str());
            Surface * view = new Surface("image", image.getWidth(), image.getHeight());
            view->getRenderer()->setImage(image.toArray(), image.getWidth(), image.getHeight());
            view->loop();
            delete view;
        }
        
        if(extension == "pgm") {
            Graymap image(file_name.c_str());
            Surface * view = new Surface("image", image.getWidth(), image.getHeight());
            view->getRenderer()->setImage(image.toArray(), image.getWidth(), image.getHeight());
            view->loop();
            delete view;
        }

        if(extension == "ppm") {
            Pixmap2 image(file_name.c_str());
            Surface * view = new Surface("image", image.getWidth(), image.getHeight());
            view->getRenderer()->setImage(image.toArray(), image.getWidth(), image.getHeight());
            view->loop();
            delete view;
        }

        if(extension == "jpg" || extension == "jpeg") {
            //
        }

        if(extension == "png") {
            //
        }

        if(extension == "mpg" || extension == "mpeg") {
            //
        }

        SDL_Quit();
    }

    return 1;
}


link: https://github.com/klebermo/game
Last edited on
Ganado (6784)
Also probably not the actual issue, but in your toArray call, if you call new float[...], then you should be deleting with [ ] as well.

If you allocate with ptr = new(...), you should delete ptr;
if you allocate with ptr = new[...], you should delete[] ptr;
Last edited on
seeplus (6458)
As a first refactor, consider (NOT tried):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
 
#include <algorithm>
#include <string>
#include <vector>
#include <sstream>
#include <fstream>

using pxArray = std::vector<float>;

template <class T>
class Matrix {
private:
	size_t height {};
	size_t width {};
	T** values {};

public:
	Matrix() = default;

	Matrix(size_t h, size_t w) : height(h), width(w), values(new T* [height]) {
		for (size_t ht {}; ht < h; ++ht)
			values[ht] = new T[width] {};
	}

	Matrix(const Matrix& old) : Matrix(old.height, old.width) {
		for (size_t ht {}; ht < height; ++ht)
			std::copy_n(old.values[ht], width, values[ht]);
	}

	Matrix(Matrix&& old) noexcept {
		swap(old);
	}

	Matrix& operator=(Matrix old) noexcept {
		swap(old);
		return *this;
	}

	size_t getHeight() const {
		return height;
	}

	size_t getWidth() const {
		return width;
	}

	~Matrix() {
		clear();
	}

	void swap(Matrix& old) noexcept {
		std::swap(height, old.height);
		std::swap(width, old.width);
		std::swap(values, old.values);
	}

	void clear() {
		for (size_t i {}; i < height; ++i)
			delete[] values[i];

		delete[] values;
		height = width = 0;
		values = nullptr;
	}

	T* operator[](size_t index) {
		return values[index];
	}

	const T* operator[](size_t index) const {
		return values[index];
	}
};

struct Pixel {
	float r {}, g {}, b {};
};

class Netpbm {
protected:
	char magicNumber {};
	Matrix<Pixel> pixels;

public:
	virtual ~Netpbm() {}

	char getMagicNumber() const {
		return magicNumber;
	}

	size_t getWidth() const {
		return pixels.getWidth();
	}

	size_t getHeight() const {
		return pixels.getHeight();
	}

	pxArray toArray() const;

	virtual bool read_file(const std::string& file_name) = 0;
	//virtual void write_file(const std::string& file_name) = 0;
};

class Bitmap : public Netpbm {
public:
	Bitmap(const std::string& file_name) {
		readOK = read_file(file_name);
	}

	// OR use throw with exception handling
	bool openOK() const {
		return readOK;
	}

	//void write_file(const std::string& file_name) override;

private:
	bool readOK {};
	bool read_file(const std::string& file_name) override;
};

pxArray Netpbm::toArray() const {
	pxArray result(pixels.getWidth() * pixels.getHeight() * 5);

	for (size_t i {}, count {}; i < pixels.getHeight(); ++i)
		for (size_t j {}; j < pixels.getWidth(); ++j) {
			// NOTE THIS IS INTEGER DIVISION!!!!
			const auto x { float(j) / pixels.getWidth()}, y {float(i) / pixels.getHeight()};

			result[count++] = - 1 + (2 * x);
			result[count++] = 1 - (2 * y);
			result[count++] = pixels[i][j].r;
			result[count++] = pixels[i][j].g;
			result[count++] = pixels[i][j].b;
		}

	return result;
}

bool Bitmap::read_file(const std::string& file_name) {
	std::ifstream file(file_name);

	if (!file)
		return false;

	size_t h {}, w {};

	for (std::string line_one; std::getline(file, line_one); )
		if (line_one.size() >= 2 && line_one[0] != '#') {
			magicNumber = line_one[1];
			break;
		}

	for (std::string line_two; std::getline(file, line_two); )
		if (!line_two.empty() && line_two[0] != '#') {
			std::stringstream ss(line_two);

			ss >> w >> h;
			pixels = Matrix<Pixel>(h, w);
			break;
		}

	// Make sure we're read some values!
	if (magicNumber == char {} || h == 0 || w == 0)
		return false;

	if (magicNumber == '1' || magicNumber == '4') {
		std::vector<Pixel> v;

		if (magicNumber == '1') {
			for (std::string line_pixels; std::getline(file, line_pixels); )
				if (!line_pixels.empty() && line_pixels[0] != '#') {
					std::stringstream ss(line_pixels);

					for (float p; ss >> p; v.emplace_back(p, p, p));
				}
		} else
			for (unsigned char data {}; file.read(reinterpret_cast<char*>(&data), 1); )
				for (int i {}; i < 8; ++i)
					if (data & (1 << i))
						v.emplace_back(1.f, 1.f, 1.f);
					else
						v.emplace_back(0.f, 0.f, 0.f);

		if (v.size() < h * w)
			return false;

		for (size_t i {}, counter {}; i < h; ++i)
			for (size_t j {}; j < w; ++j)
				pixels[i][j] = v[counter++];
	}

	return true;
}


which only uses dynamic memory within the Matrix class.
Last edited on
Topic archived. No new replies allowed.