read and write to file in c - compressing matrix

Question

I'm trying to write a compressed bits matrix to a and the read it back. The writing works great but the reading keeps failing and I dont understand why.

    typedef unsigned char BYTE;

    int saveCompressImageToFile(const char *fileName, const int *image, int row, int col)
    {
        FILE *fp = fopen(fileName, "wb");
        if (!fp)
            return 0;
        if (fwrite(&row, sizeof(int), 1, fp) != 1) {
            fclose(fp);
            return 0;
        }
        if (fwrite(&col, sizeof(int), 1, fp) != 1) {
            fclose(fp);
            return 0;
        }
        int byteCount = row * col / 8;
        if (byteCount * 8 < row * col)
            byteCount++;
        BYTE *data = (BYTE *)calloc(sizeof(BYTE), byteCount);
        int dataPos;
        int dataShift;
        for (int i = 0; i < row; i++) {
            for (int j = 0; j < col; j++) {
                dataPos = (i * col + j) / 8;
                dataShift =7 - (i * col + j) % 8;
                data[dataPos] |= (*(image + i * col + j)) << dataShift;
            }
        }
        if (fwrite(data, sizeof(BYTE), byteCount, fp) != byteCount) {
            fclose(fp);
            return 0;
        }
        return 1;
    }
    
    int * getImage_compress(const char * fileName, int * pRow, int * pCol)
{
    FILE* fp = fopen(fileName, "rb");
    if (!fp)
        return NULL;
    if (fread(pRow, sizeof(int), 1, fp) != 1)
    {
        fclose(fp);
        return NULL;
    }
    if (fread(pCol, sizeof(int), 1, fp) != 1)
    {
        fclose(fp);
        return NULL;
    }
    size_t mat_size = *pRow * (*pCol);
    int byteCount = mat_size / 8;
    if (byteCount * 8 < mat_size)
        byteCount++;
    BYTE* data = (BYTE*)malloc(sizeof(BYTE)*byteCount);
    if (!data)
    {
        fclose(fp);
        return NULL;
    }
    if (fread(data, sizeof(BYTE), byteCount, fp) != byteCount)
    {
        free(data);
        fclose(fp);
        return NULL;
    }
    fclose(fp);

    int* mat = (int*)calloc(mat_size, sizeof(int));
    if (!mat)
    {
        free(data);
        fclose(fp);
        return NULL;
    }
    for (int i = 0; i < mat_size; i++)
    {
        for (int j = 0; j < 8; j++)
        {
            mat[i * 8 + j] = (data[i] >> (7 - j)) & 0x1;
        }
    }
    free(data);
    return mat;
}

for some reason the function throws exception in the last line (return mat;) , "Starter.exe has triggered a breakpoint". occurred , what's going on over there? I've tried to debug that code and I can't tell why I can't return this value but I can access the cells in the debugger. any Suggestions?

Test `write()` using `2 x 2` image to file & check file using a hex/binary editor. — जलजनक, May 25 '22 at 12:07
you aren't closing the file in every case so there may be unwritten data. — stark, May 25 '22 at 12:13
Multiple problems: 1. missing `fclose(fp)` in case of successful write. 2. `fread(&data, sizeof(BYTE), byteCount, fp)` has an extra `&` before `data`, causing undefined behavior. 3. the last loop has undefined behavior if `(*pRow * *pCol < byteCount * sizeof(BYTE)` — chqrlie, May 25 '22 at 14:23
@chqrlie's comments, and: 4. you are reassembling the bits backwards from how you wrote them — change it in the last loop to `data[i] >> (7 - j)`, 5. when you say `sizeof(BYTE)`, you get 1, but what you really mean is 8 — use `8`, 6. since you are decompressing all of the bits instead of just rows * cols bits, your allocation of `mat` is up to seven ints too small — allocate with `byteCount * 8` instead of `*pRow * (*pCol)`. — Mark Adler, May 25 '22 at 17:36
Not a bug, but you should use `int byteCount = (row * col + 7) / 8;` instead of needing those if statements. Also use `unsigned` instead of `int` for rows and cols, to avoid signed/unsigned comparisons. Use `size_t` for `byteCount`, and cast to `size_t` when multiplying rows times cols, or cols times i. For speed, you can add `cols` to a row offset for each row, instead of multiplying for every element. — Mark Adler, May 25 '22 at 17:38
@MarkAdler: why can't we reopen this question? I have a gold badge for [c] and you do for [compression] which supposedly allow us to reopen questions single-handedly. — chqrlie, May 25 '22 at 18:30
edited the problem and the functions, would like yall to take a look. — יונתן אליהו, May 30 '22 at 13:58

score 0 · Answer 1 · answered May 26 '22 at 14:30

0

Putting the comments in an answer:

So much wrong.

You need to fclose(fp) when there isn't an error in error in your first function. Otherwise not all of the data will be written. Or maybe nothing will be written.
fread(&data, ... would try to read the data over the pointer to the allocated space! It should just be fread(data, ...
You are reassembling the bits backwards from how you put them in. Change the reassembly to data[i] >> (7 - j). Or assemble them in the other direction.
sizeof(BYTE) is 1. You mean 8. Use 8 for those.
Your allocation of mat when decompressing can be up to seven ints shorter than what you're writing. Either allocate mat large enough, using byteCount * 8, or only decompress rows * columns ints.

answered May 26 '22 at 14:30

Mark Adler

101,978
13
118
158

I don't understand way the mat allocation can be 7 ints shorter' the real number of cells in the matrix in the first place is row*cols and which I read directly from the file. so why it will get shorter ? sec of all, shouldn't I read the byte count times char size ? (has 8 bits in it so every byte or char will hold 8 bits from the actual matrix) – יונתן אליהו May 29 '22 at 07:37
Suppose we have a 3x3 image. Nine pixels. Your `mat` will be allocated nine `int`s. `byteCount` will be two in this case. (Do the calculations yourself.) Those two bytes will have 16 bits. The problem is that your last loop will process _all 16 bits_, and write 16 `int`s to `mat`, when you had allocated only nine `int`s. So the last seven bits processed will write in the memory _after_ `mat`, `mat[9]` through `mat[15]`, which don't exist. This results in undefined behavior, since who knows what's being kept in those 28 bytes of RAM. – Mark Adler May 29 '22 at 17:00
edited the problem and I would like you to take a look . – יונתן אליהו May 30 '22 at 13:57
Now you're _massively_ overwriting eight times the size of the memory actually allocated for `mat`! Also massively overeading eight times the size of the memory allocated for `data`. You're just writing code without thinking. Think. Then write. – Mark Adler May 30 '22 at 15:54
That should have been a new question. You have wiped out any possible value of the answer. You don't keep replacing old questions with new questions. – Mark Adler May 30 '22 at 15:55

read and write to file in c - compressing matrix

1 Answers1