0

I am trying to implement a C++ code to read a binary PBM file. Right now, after read the header of the file successfully (magic number, width and height), I got this code to read each character of the file, and extract the individual bits from this character:

    std::vector<pixel> v;

    char c;
    while(input_file.get(c)) {
        for(int i=0; i<8; i++) {
            pixel p;
            p.r = p.g = p.b = (c & (1 << i)) != 0;
            v.push_back(p);
        }
    }

After that, I transfer the data of this vector to a matrix:

    int h = stoi(height), w = stoi(width);

    int counter = 0;
    for(int i=0; i<h; i++) {
        std::vector<pixel> row;
        for(int j=0; j<w; j++) row.push_back(v[counter++]);
        image.push_back(row);
    }

With that, when I try visualize the image on the screen, I got a distorted image, where some pixels seems to be dislocated from its original position. I got the following program that generate a ascii image that shows what I got after read the binary file:

struct Pixel {
    int r, g, b;
};
typedef struct Pixel pixel;

int main(int argc, char *argv[])
{
    if(argc < 3) return 1;

    std::string input = argv[1];
    std::string output = argv[2];

    std::ifstream input_file(input);
    std::ofstream output_file(output);

    std::vector<std::vector<pixel>> image;
    std::vector<pixel> v;

    std::string line_one, line_two, line_pixels;

    char magicNumber;
    std::string width, height;

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

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

    std::cout << magicNumber << std::endl;
    std::cout << width << " " << height << std::endl;

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

        char c;
        while(input_file.get(c)) {
            for(int i=0; i<8; i++) {
                pixel p;
                p.r = p.g = p.b = (c & (1 << i)) != 0;
                v.push_back(p);
            }
        }

        int h = stoi(height), w = stoi(width);

        int counter = 0;
        for(int i=0; i<h; i++) {
            std::vector<pixel> row;
            for(int j=0; j<w; j++) row.push_back(v[counter++]);
            image.push_back(row);
        }
    }

    output_file << "P1" << std::endl << width << " " << height << std::endl;
    for(int i=0; i<stoi(height); i++) {
        for(int j=0; j<stoi(width); j++) {
            output_file << image[i][j].r << " ";
        }
        output_file << std::endl;
    }

    return 0;
}

The image I am trying to read in my tests is this one. Anyone can tell me what's wrong here? How I can get the same image after read the file?

Kleber Mota
  • 8,521
  • 31
  • 94
  • 188
  • 2
    You definitely should be opening your file input in binary mode, `std::ifstream input_file(input, std::ios_base::binary);` – john Dec 14 '22 at 13:01
  • 1
    Open your file in binary mode. Be aware that `char` is signed and that your rgb values are int. – Simon Kraemer Dec 14 '22 at 13:02
  • the file has ascii parts (the header) and binary parts. If I do not specify what mode (`std::ifstream input_file(input);`) it shouldn't be select automatically? – Kleber Mota Dec 14 '22 at 13:03
  • @SimonKraemer But the file has both ascii parts and binary parts. – Kleber Mota Dec 14 '22 at 13:04
  • Even opening the file as binary (`std::ifstream input_file(input, std::ios::binary);` or `std::ifstream input_file(input, std::ios_base::binary);`), I got the same result. – Kleber Mota Dec 14 '22 at 13:10
  • @john got the same result when I open the file in binary mode – Kleber Mota Dec 14 '22 at 13:11
  • @SimonKraemer got the same result when I open the file in binary mode – Kleber Mota Dec 14 '22 at 13:11
  • Is there even binary data in a PBM file? All that I could find about the file format indicates that it's a text-only format, where most of the text is numbers written out in decimal – harold Dec 14 '22 at 13:13
  • @harold I think so, based on what I read here: https://en.wikipedia.org/wiki/Netpbm and the examples available here: https://people.sc.fsu.edu/~jburkardt/data/pbmb/pbmb.html – Kleber Mota Dec 14 '22 at 13:16
  • OK you're right, there is a binary version, and your file uses that version (P4). I'll try to read it myself and see what happens. – harold Dec 14 '22 at 13:18
  • @KleberMota Your code doesn't deal correctly with the case where the width of the image is not a multiple of 8. In that case each row is padded with garbage bits to make up a whole byte. Your code does not deal with this possibility. – john Dec 14 '22 at 13:21
  • @KleberMota And having looked at your test image I can see that the width is not a multiple of 8, so that's an issue. – john Dec 14 '22 at 13:25
  • 1
    @SimonKraemer Whether `char` is signed or unsigned is implementation defined. I've worked on platforms where it was signed and some where it was unsigned - it happens in real life. Also, remember that `char`, `signed char` & `unsigned char` are 3 distinct types in C++, `char` is not simply an alias for one or the other. – Jesper Juhl Dec 14 '22 at 13:32
  • @john I try deal with that this way: https://pastebin.com/hETrdBMZ. But I think I still got something wrong, because the result still the same. – Kleber Mota Dec 14 '22 at 13:33
  • @SimonKraemer I try change the code to account for that: https://pastebin.com/hETrdBMZ. But I think I gstill got something wrong, because still got the same result. – Kleber Mota Dec 14 '22 at 13:34
  • @JesperJuhl You're right, yet on most platforms it is signed and this is in my experience a common source for errors, when interpreting data. Especially when some kind of bit-shifting is involved. May not be the problem in this case, but I found it worth mentioning. – Simon Kraemer Dec 14 '22 at 13:40
  • @KleberMota That code is bugged (as you know). Any code that reads the partial byte at the end of each row should still be inside the outer loop that reads each individual row. But your new code comes after all the rows have been read. So that cannot be right. I would go back to your earlier code, but add a variable that keeps track of how many pixels you have read on the row so far. If that count is >= the width of the row, don't add it to your pixel vector. – john Dec 14 '22 at 13:41
  • @john I change the code again, to that: https://pastebin.com/zuSaLnVe and got this result: https://imgur.com/a/84HaRfL, which it was the closest I got from original image so far. – Kleber Mota Dec 14 '22 at 13:52
  • @KleberMota I think (not certain) that's the issue that harold mentions below, you are processing the bits within each byte in the wrong order. So every eights bits horizontally are flipped around. – john Dec 14 '22 at 14:35
  • @KleberMota If I'm right you can fix that by replacing `p.r = (c & (1 << i)) ? 1 : 0;` with `p.r = (c & (1 << (7 - i))) ? 1 : 0;` or this `p.r = (c & (128 >> i)) ? 1 : 0;` – john Dec 14 '22 at 14:37

1 Answers1

1

I was able to load the image:

Decoded image

The main issue, as far as I see it, is that your code reads every byte from the least significant bit up, but the pixels are stored from the most significant bit down (which is admittedly strange).

You can for example change (c & (1 << i)) != 0 to (c & (1 << (i ^ 7))) != 0

Reading the extra padding bits at the end of rows that are not a multiple of 8 wide shouldn't be an issue by itself, but when you "de-linearize" the pixels into a 2D grid you must take care that your vector contains the padding bits: w is not the width of your rows, it's the width of the "useful part" of the image.

To be specific, it's fine to have this code:

for(int i=0; i<8; i++) {
    pixel p;
    p.r = p.g = p.b = (c & (1 << (i ^ 7))) != 0;
    v.push_back(p);
}

But then you should be careful to use the width rounded up to a multiple of 8 here:

for(int i=0; i<h; i++) {
    std::vector<pixel> row;
    for(int j=0; j < ((w + 7) & -8); j++) row.push_back(v[counter++]);
    image.push_back(row);
}

((w + 7) & -8) rounds up w to a multiple of 8, there are other ways to do that. The loop that outputs the result should use w though, and not the rounded-up w.

Chars being signed or not shouldn't make any difference, c may be negative but the bits that you're actually reading would not be affected.

harold
  • 61,398
  • 6
  • 86
  • 164
  • With this suggested change, I got this code: https://pastebin.com/NR4Eu412, which still got issues with the result. Do you made another change in the original code? – Kleber Mota Dec 14 '22 at 13:38
  • @KleberMota well, I didn't use that code at all, I wrote my own, I'll check if there are more differences. BTW can you post your resulting image in a viewable format? – harold Dec 14 '22 at 13:40
  • the resulting image, with the changed code, is: https://imgur.com/a/u4M94Am – Kleber Mota Dec 14 '22 at 13:44
  • @KleberMota in `1 << (i ^ 7)` change `i` to `k`, you're using `k` for that loop now. The skew is because the rows are padded to a multiple of 8. – harold Dec 14 '22 at 13:47
  • You can't loop (in the `j`-loop) to `w/8`, the extra byte (with padding bits) is there at the end of the row whether we like it or not. You can loop to `(w+7)/8`. You could stop the `k` loop when it goes out of bounds, or not, up to you, just don't put the padding pixels in the output – harold Dec 14 '22 at 13:52
  • this version: https://pastebin.com/zuSaLnVe got me very close to the original version, but I still got some issues. – Kleber Mota Dec 14 '22 at 13:53
  • that is the result for this version: https://imgur.com/a/84HaRfL – Kleber Mota Dec 14 '22 at 13:54
  • I also tried this, which uses the change you suggested: https://pastebin.com/3MDpU5TB. – Kleber Mota Dec 14 '22 at 13:57
  • OK but wow there are a dozen different versions spread across pastebin now. I'll put some stuff in my answer now. – harold Dec 14 '22 at 14:00