reading ppm into vector

Oct 14, 2019 at 5:36pm
Hi, I'm running a test on my code to see if I can view the image I read in after I save it into a different file, and it's failing with an exit code. Can anyone point out the error(s) in my code please?

Thank you.

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
#include "stacker.h"
#include <vector>
#include <fstream>
#include <iostream>
using namespace std;

void stacker::readFile(string s, int count){

    ifstream fin;

    fin.open(s.c_str());

    fin >> magic_number;
        fin.get();
        fin >> width;
        fin >> height;
        fin >> max_color;

    while (fin) {

        fin >> color[count].r;
        fin >> color[count].g;
        fin >> color[count].b;

    }


}

void stacker::saveImage(string d, int sCount){

    ofstream fileIn;

    fileIn.open(d.c_str());

    fileIn << magic_number << endl; // adds back in discarded info
    fileIn << width << " " << height << endl;
    fileIn << max_color << " " << endl;

    while(fileIn){

        fileIn << color[sCount].r << " ";
        fileIn << color[sCount].g << " ";
        fileIn << color[sCount].b << " ";

    }

    fileIn.close();
}


*edit*
My main
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include "stacker.h"

using namespace std;

int main() {

    stacker a;

    a.readFile("orion_001.ppm", 0);
    a.saveImage("firstsave.ppm", 0);

    return 0;
}


*edit*
my header
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
#include <string>
#include <vector>

#ifndef IMAGEVECTOR_STACKER_H
#define IMAGEVECTOR_STACKER_H


class stacker {

private:
    std::string magic_number;
    int width, height, max_color;

    struct pixel {
        int r, g, b;
    };
    struct stddev {
        double red, green, blue;
    };

    std::vector<pixel> color;
    std::vector<stddev> devColor;

public:
    void readFile(std::string s, int count);
    void saveImage(std::string d, int sCount);
};


#endif //IMAGEVECTOR_STACKER_H 
Last edited on Oct 14, 2019 at 11:20pm
Oct 14, 2019 at 10:27pm
I have been messing with it, and I believe it's a memory leak.
I think the first issue lies in the read file function, but I'm not sure how to fix it.
Oct 14, 2019 at 11:06pm
> I believe it's a memory leak.
way off target, ¿how did you reach such a conclusion?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
//read
    while (fin) {

        fin >> magic_number; //<--- this is the header, ¿why is it inside the loop?
        fin.get();
        fin >> width;
        fin >> height;
        fin >> max_color;    //<---

        fin >> color[count].r; // you never change `count' so you are reading into the same pixel
        fin >> color[count].g;
        fin >> color[count].b;

    }

//write
    while(fileIn){ //¿when will this end

        fileIn << color[sCount].r << " "; // always writing the same pixel
        fileIn << color[sCount].g << " ";
        fileIn << color[sCount].b << " ";

    }

std::vector<pixel> color;
that vector is empty, ¿do you ever change its size?


apart, ¿what's the difference between your `pixel' and `stddev' classes?
¿why do you pass a `sCount' parameter to your read and write functions?
Oct 14, 2019 at 11:14pm
>> how did you reach such a conclusion

I googled the error code, and while I didn't find mine specifically, others that were similar were about memory leaks.

>> this is the header, why inside loop.

I have since moved the header out of the loop. That was a shot in the dark tracer hoping to get something to stick that I forgot to change back. Thank you for pointing it out.

>>That vector is empty, do you ever change its size.

To be honest, I thought that the vector class did the resizing and capacity tracking automatically.


>> What's the difference between pixel and stddev

pixel takes in the raw data from 10 shots of the same image.

stddev holds the averages of the 10 shots to produce a clean, hi resolution image.
Oct 14, 2019 at 11:42pm
This is my attempt at fixing some of the issues you pointed out that I could sort of understand.

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
#include "stacker.h"
#include <vector>
#include <fstream>
#include <iostream>
using namespace std;

void stacker::readFile(string s){

    ifstream fin;

    fin.open(s.c_str());

    color.push_back(pixel());

    while (fin) {

        fin >> magic_number;
        fin.get();

        fin >> width;
        fin >> height;
        fin.get();

        fin >> max_color;
        fin.get();

        

        for(int i = 0; i < width;  i++){ 
            fin >> color[i].r;
            fin >> color[i].g;
            fin >> color[i].b;

            color.push_back(pixel()); 
        }
    }
     
    fin.close();
}

void stacker::saveImage(string d){

    ofstream fileIn;

    fileIn.open(d.c_str());

    fileIn << magic_number << endl;
    fileIn << width << " " << height << endl;
    fileIn << max_color << " " << endl;

    while(fileIn){

        for(int i = 0; i < width;  i++) {

            fileIn << color[i].r << " ";
            fileIn << color[i].g << " ";
            fileIn << color[i].b << " ";

        }
    }

    fileIn.close();
}



Last edited on Oct 15, 2019 at 12:40am
Oct 15, 2019 at 8:47am
Line 29 processes only the width. What about the height?

pixel takes in the raw data from 10 shots of the same image.

stddev holds the averages of the 10 shots to produce a clean, hi resolution image.
Is there really a situation where each rgb value is more than 0...255?

Actually what you named pixel is the color. A pixel usually has more data such as the position. stddev seems not to be a descriptive name.
I would thinkg that color should be 2d vector. And it is more pixels than color.
Oct 15, 2019 at 12:43pm
> To be honest, I thought that the vector class did the resizing and capacity
> tracking automatically.
.push_back() or .insert() will expand the size and reallocate if needed
but trying to access out of bounds is an error
1
2
3
4
5
std::vector<int> v;
v.resize(5);
v.push_back(42); //now .size() is 6
v[13] = 54; //error
v[-1] = 0; //error 


on your last code
1
2
3
    while (fin) {

        fin >> magic_number; //still inside the loop 

1
2
3
4
5
6
7
        for(int i = 0; i < width;  i++){ 
            fin >> color[i].r; //out of bounds access
            fin >> color[i].g;
            fin >> color[i].b;

            color.push_back(pixel()); //appending an empty element
        }


1
2
3
4
5
6
7
8
9
10
    while(fileIn){ //¿when will this end?

        for(int i = 0; i < width;  i++) {

            fileIn << color[i].r << " ";
            fileIn << color[i].g << " ";
            fileIn << color[i].b << " ";

        }
    }
and as coder777 said, the loops just ignore height

didn't realise that `stddev' members were of another type
but I agree, a terrible name
Oct 15, 2019 at 6:01pm
Okay. I'll go another round with it.
I appreciate the input y'all.

*edit*

>> should be 2d

I agree. It would be easier for me to understand. I have to keep it 1d. I already asked the professor that.

>> while(fileIn){ //¿when will this end?

Last semester, I was pretty sure I remember a lecture about while(file) being better than while(!eof) since it checks for valid file, end of file, and something else I don't remember. However, I must be remembering it incorrectly because that's not how it's used. I'm going to take out the while loop completely. The for loop should be enough if I multiply by height and width.

>> color is pixels, stddev is a bad name (paraphrasing)

I can't change stddev, but I'm changing color to pixels. Thanks!

1
2
3
4
5
6
7
for(int i = 0; i < width;  i++){ 
            fin >> color[i].r; //out of bounds access
            fin >> color[i].g;
            fin >> color[i].b;

            color.push_back(pixel()); //appending an empty element
        }


Should I do a v.resize() on height * width before I start appending?

Is the color.push_back unnecessary then?

Thanks!
Last edited on Oct 15, 2019 at 6:56pm
Oct 15, 2019 at 11:26pm
It works!!
Granted, this may be bad coding, but I exit with an exit code 0 and it saves the image in question.

If something needs to be improved, please, feel free to be as blunt and nasty as needed.

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
//
// Created by jeffj on 10/11/2019.
//

#include "stacker.h"
#include <vector>
#include <fstream>
#include <iostream>
using namespace std;

void stacker::readFile(string s){

    ifstream fin;

    fin.open(s.c_str());

    pixels.emplace_back();

    fin >> magic_number;
    fin.get();

    fin >> width;
    fin >> height;
    fin.get();

    fin >> max_color;
    fin.get();

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

        fin >> pixels[i].r;
        fin >> pixels[i].g;
        fin >> pixels[i].b;

        pixels.emplace_back();

    }

    fin.close();

}

void stacker::saveImage(string d){

    ofstream fileIn;

    fileIn.open(d.c_str());

    fileIn << magic_number << endl;
    fileIn << width << " " << height << endl;
    fileIn << max_color << " " << endl;

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

        fileIn << pixels[i].r << " ";
        fileIn << pixels[i].g << " ";
        fileIn << pixels[i].b << " ";

    }

    fileIn.close();

}



*edit*
Changed push_back to emplace_back
Last edited on Oct 16, 2019 at 3:02am
Oct 16, 2019 at 3:17am
1
2
3
4
5
6
7
8
9
    for(int i = 0; i < width * height;  i++){

        fin >> pixels[i].r;
        fin >> pixels[i].g;
        fin >> pixels[i].b;

        pixels.emplace_back();

    }
that has the same problem as before

you may write
1
2
3
pixels.resize(width*height);
for(int i = 0; i < width * height;  i++)
   fin >> pixels[i].r >> pixels[i].g >> pixels[i].b;
or instead
1
2
3
4
5
6
pixels.clear() //in case that it had another value before
for(int i = 0; i < width * height;  i++){
   pixel p;
   fin >> p.r >> p.g >> p.b;
   pixels.push_back(p); //would be easier if `pixel' had a constructor
}
Oct 16, 2019 at 4:10am
I changed it.
I really appreciate the help y'all. Especially since y'all pointed out why the code was bad and how I should be thinking about it instead of just providing code. (at least until you knew I had working code that wasn't complete trash.)

Thanks!

The rest should be doable I believe.
Topic archived. No new replies allowed.