Dynamically allocated struct in function

I have a school project related to bmp and im a bit stuck with the dynamic allocation aspect of things(as I have been asked to use that).
What im trying to do is pass my array using a pointer, so that the array changes its value even after the function ends,which is why i used **. However, the code just ends up crashing because of this single bit(as without it it runs smoothly).Im sure it's my use of * and & incorrectly but I don't know where and how to fix it.

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

void liniarizare(char *filename,pixel **liniar)

{int i;

 ... (i calculate size which is surely correct and declare fin;size=width*height*sizeof(pixel)

*liniar=(pixel*)malloc(size);

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

    {fread(&liniar[i]->b,1,1,fin);

     fread(&liniar[i]->g,1,1,fin);

     fread(&liniar[i]->r,1,1,fin);

     } 

 }

...

int main()

{...

 pixel *liniar

 liniarizare(filename,&liniar);

 ....}

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

Is size the number of bytes or the number of elements in the array?

– Dipstick
Jan 2 at 21:52

1

To simplify things [it may make it more clear], change your function to: pixel * liniarizare(char *filename). At the bottom, do return liniar; And, in main, do: liniar = liniarizare(filename);

– Craig Estey
Jan 2 at 21:55

2

Looks like you allocate an array of pointers with malloc, but never initialize them to point at anything, so when you try to fread through them, it crashes. But you don't include an MVCE, so its impossible to tell.

– Chris Dodd
Jan 2 at 21:56

The combination of malloc(size) and for (i=0;i<size;i++) cannot be correct. You are accessing way past the bounds of allocated memory in the loop

– UnholySheep
Jan 2 at 21:58

@Dipstick It's the number of bytes (i calculate it with size=width * height * sizeof(pixel), as im planning to add width * height pixels in the array)

– valentine george
Jan 2 at 21:59

|
show 7 more comments

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

void liniarizare(char *filename,pixel **liniar)

{int i;

 ... (i calculate size which is surely correct and declare fin;size=width*height*sizeof(pixel)

*liniar=(pixel*)malloc(size);

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

    {fread(&liniar[i]->b,1,1,fin);

     fread(&liniar[i]->g,1,1,fin);

     fread(&liniar[i]->r,1,1,fin);

     } 

 }

...

int main()

{...

 pixel *liniar

 liniarizare(filename,&liniar);

 ....}

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

Is size the number of bytes or the number of elements in the array?

– Dipstick
Jan 2 at 21:52

1

To simplify things [it may make it more clear], change your function to: pixel * liniarizare(char *filename). At the bottom, do return liniar; And, in main, do: liniar = liniarizare(filename);

– Craig Estey
Jan 2 at 21:55

2

Looks like you allocate an array of pointers with malloc, but never initialize them to point at anything, so when you try to fread through them, it crashes. But you don't include an MVCE, so its impossible to tell.

– Chris Dodd
Jan 2 at 21:56

The combination of malloc(size) and for (i=0;i<size;i++) cannot be correct. You are accessing way past the bounds of allocated memory in the loop

– UnholySheep
Jan 2 at 21:58

@Dipstick It's the number of bytes (i calculate it with size=width * height * sizeof(pixel), as im planning to add width * height pixels in the array)

– valentine george
Jan 2 at 21:59

|
show 7 more comments

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

void liniarizare(char *filename,pixel **liniar)

{int i;

 ... (i calculate size which is surely correct and declare fin;size=width*height*sizeof(pixel)

*liniar=(pixel*)malloc(size);

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

    {fread(&liniar[i]->b,1,1,fin);

     fread(&liniar[i]->g,1,1,fin);

     fread(&liniar[i]->r,1,1,fin);

     } 

 }

...

int main()

{...

 pixel *liniar

 liniarizare(filename,&liniar);

 ....}

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

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

void liniarizare(char *filename,pixel **liniar)

{int i;

 ... (i calculate size which is surely correct and declare fin;size=width*height*sizeof(pixel)

*liniar=(pixel*)malloc(size);

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

    {fread(&liniar[i]->b,1,1,fin);

     fread(&liniar[i]->g,1,1,fin);

     fread(&liniar[i]->r,1,1,fin);

     } 

 }

...

int main()

{...

 pixel *liniar

 liniarizare(filename,&liniar);

 ....}

c arrays struct

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

edited Jan 2 at 22:11

asked Jan 2 at 21:49

valentine george

asked Jan 2 at 21:49

valentine george

asked Jan 2 at 21:49

valentine george

Is size the number of bytes or the number of elements in the array?

– Dipstick
Jan 2 at 21:52

1

To simplify things [it may make it more clear], change your function to: pixel * liniarizare(char *filename). At the bottom, do return liniar; And, in main, do: liniar = liniarizare(filename);

– Craig Estey
Jan 2 at 21:55

2

Looks like you allocate an array of pointers with malloc, but never initialize them to point at anything, so when you try to fread through them, it crashes. But you don't include an MVCE, so its impossible to tell.

– Chris Dodd
Jan 2 at 21:56

The combination of malloc(size) and for (i=0;i<size;i++) cannot be correct. You are accessing way past the bounds of allocated memory in the loop

– UnholySheep
Jan 2 at 21:58

@Dipstick It's the number of bytes (i calculate it with size=width * height * sizeof(pixel), as im planning to add width * height pixels in the array)

– valentine george
Jan 2 at 21:59

|
show 7 more comments

Is size the number of bytes or the number of elements in the array?

– Dipstick
Jan 2 at 21:52

1

To simplify things [it may make it more clear], change your function to: pixel * liniarizare(char *filename). At the bottom, do return liniar; And, in main, do: liniar = liniarizare(filename);

– Craig Estey
Jan 2 at 21:55

2

Looks like you allocate an array of pointers with malloc, but never initialize them to point at anything, so when you try to fread through them, it crashes. But you don't include an MVCE, so its impossible to tell.

– Chris Dodd
Jan 2 at 21:56

The combination of malloc(size) and for (i=0;i<size;i++) cannot be correct. You are accessing way past the bounds of allocated memory in the loop

– UnholySheep
Jan 2 at 21:58

@Dipstick It's the number of bytes (i calculate it with size=width * height * sizeof(pixel), as im planning to add width * height pixels in the array)

– valentine george
Jan 2 at 21:59

Is size the number of bytes or the number of elements in the array?

– Dipstick
Jan 2 at 21:52

To simplify things [it may make it more clear], change your function to: pixel * liniarizare(char *filename). At the bottom, do return liniar; And, in main, do: liniar = liniarizare(filename);

– Craig Estey
Jan 2 at 21:55

Looks like you allocate an array of pointers with malloc, but never initialize them to point at anything, so when you try to fread through them, it crashes. But you don't include an MVCE, so its impossible to tell.

– Chris Dodd
Jan 2 at 21:56

The combination of malloc(size) and for (i=0;i<size;i++) cannot be correct. You are accessing way past the bounds of allocated memory in the loop

– UnholySheep
Jan 2 at 21:58

@Dipstick It's the number of bytes (i calculate it with size=width * height * sizeof(pixel), as im planning to add width * height pixels in the array)

– valentine george
Jan 2 at 21:59

|
show 7 more comments

1 Answer
1

active

oldest

votes

Note that this is prefaced by my top comments.

That is, have the function return pixel *. And, use an extra unsigned char variable to prevent reading a byte into an unsigned int

Here's a simplified version that I think should work:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



pixel *

liniarizare(char *filename)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    return liniar;

}



int

main(void)

{

    pixel *liniar;



    liniar = liniarizare(filename);



    return 0;

}

UPDATE:

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice? You said something about linear[i]->b being wrong.

Okay, the simplest/best way to deal with a "return" double star argument (e.g.) whatever **retptr) is to ignore this as much as possible.

That is, the function deals with the simpler whatever *ptr internally. This is faster because there is only a single dereference level and not a double dereference on each statement.

The return value (i.e. the double star pointer) is only set at the end once.

Here's my example reworked to use your original function prototype [but with my other cleanup]. Note that only two lines are changed (the function prototype and the last line of the function):

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare(char *filename,pixel **retval)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;



    liniarizare(filename,&liniar);



    return 0;

}

Sometimes the "return value" pointer needs to be read at the top of a function and set at the bottom.

Here's a "push to tail" function for a singly linked list:

void

push(node **list,node *new)

{

    node *head;

    node *prev;

    node *cur;



    head = *list;



    prev = NULL;

    for (cur = head;  cur != NULL;  cur = cur->next)

        prev = cur;



    if (prev != NULL)

        prev->next = new;

    else

        head = new;



    new->next = NULL;



    *list = head;

}

UPDATE #2:

Okay, now that we've got something working, it's time to optimize it [after a suitable rest period :-)].

Keep the now working version around as a reference/cross-check.

fread calls on single bytes are somewhat expensive.

Since your code is doing byte at a time I/O, we can replace the fread calls with fgetc. This should be slightly faster:

for (i = 0; i < count; ++i, ++pix) {

    pix->b = fgetc(fin) & 0xFF;

    pix->g = fgetc(fin) & 0xFF;

    pix->r = fgetc(fin) & 0xFF;

}

However, we'd like to read as much as we can in single chunk. To read the entire image in one fread call would require a temp array of (e.g.) unsigned char image[count];. This is probably too much memory, and reading a large image might run into cache hit/miss issues.

But we could do a row at a time (e.g. unsigned char row[width * 3];). This is more tractable and probably produces as good or better results, so it may be a good compromise.

This may or may not be faster. That's why we keep the other versions around and benchmark to determine the fastest/best.

Note that this code assumes that pixels in the X dimension are physically adjacent [a reasonable possibility], but still works even if the matrix is transposed. In the end, it still reads count pixels in linear order, per your original code:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare_by_row(char *filename,pixel **retval)

{

    int i;

    int yidx;



    int count = width * height;

    int size = sizeof(pixel) * count;

    int w3 = width * 3;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char row[w3];

    for (yidx = 0;  yidx < height;  ++yidx) {

        fread(row,sizeof(row),1,fin);

        for (i = 0;  i < w3;  i += 3, ++pix) {

            pix->b = row[i + 0];

            pix->g = row[i + 1];

            pix->r = row[i + 2]

        }

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;

    pixel *liniar_fast;



    liniarizare(filename,&liniar);

    liniarizare_fast(filename,&liniar_fast);



    return 0;

}

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

add a comment |

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "1"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: true,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: 10,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});

}
});

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fstackoverflow.com%2fquestions%2f54013617%2fdynamically-allocated-struct-in-function%23new-answer', 'question_page');
}
);

Post as a guest

Name

Required, but never shown

1 Answer
1

active

oldest

votes

1 Answer
1

active

oldest

votes

Note that this is prefaced by my top comments.

That is, have the function return pixel *. And, use an extra unsigned char variable to prevent reading a byte into an unsigned int

Here's a simplified version that I think should work:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



pixel *

liniarizare(char *filename)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    return liniar;

}



int

main(void)

{

    pixel *liniar;



    liniar = liniarizare(filename);



    return 0;

}

UPDATE:

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice? You said something about linear[i]->b being wrong.

Okay, the simplest/best way to deal with a "return" double star argument (e.g.) whatever **retptr) is to ignore this as much as possible.

That is, the function deals with the simpler whatever *ptr internally. This is faster because there is only a single dereference level and not a double dereference on each statement.

The return value (i.e. the double star pointer) is only set at the end once.

Here's my example reworked to use your original function prototype [but with my other cleanup]. Note that only two lines are changed (the function prototype and the last line of the function):

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare(char *filename,pixel **retval)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;



    liniarizare(filename,&liniar);



    return 0;

}

Sometimes the "return value" pointer needs to be read at the top of a function and set at the bottom.

Here's a "push to tail" function for a singly linked list:

void

push(node **list,node *new)

{

    node *head;

    node *prev;

    node *cur;



    head = *list;



    prev = NULL;

    for (cur = head;  cur != NULL;  cur = cur->next)

        prev = cur;



    if (prev != NULL)

        prev->next = new;

    else

        head = new;



    new->next = NULL;



    *list = head;

}

UPDATE #2:

Okay, now that we've got something working, it's time to optimize it [after a suitable rest period :-)].

Keep the now working version around as a reference/cross-check.

fread calls on single bytes are somewhat expensive.

Since your code is doing byte at a time I/O, we can replace the fread calls with fgetc. This should be slightly faster:

for (i = 0; i < count; ++i, ++pix) {

    pix->b = fgetc(fin) & 0xFF;

    pix->g = fgetc(fin) & 0xFF;

    pix->r = fgetc(fin) & 0xFF;

}

But we could do a row at a time (e.g. unsigned char row[width * 3];). This is more tractable and probably produces as good or better results, so it may be a good compromise.

This may or may not be faster. That's why we keep the other versions around and benchmark to determine the fastest/best.

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare_by_row(char *filename,pixel **retval)

{

    int i;

    int yidx;



    int count = width * height;

    int size = sizeof(pixel) * count;

    int w3 = width * 3;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char row[w3];

    for (yidx = 0;  yidx < height;  ++yidx) {

        fread(row,sizeof(row),1,fin);

        for (i = 0;  i < w3;  i += 3, ++pix) {

            pix->b = row[i + 0];

            pix->g = row[i + 1];

            pix->r = row[i + 2]

        }

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;

    pixel *liniar_fast;



    liniarizare(filename,&liniar);

    liniarizare_fast(filename,&liniar_fast);



    return 0;

}

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

add a comment |

Note that this is prefaced by my top comments.

That is, have the function return pixel *. And, use an extra unsigned char variable to prevent reading a byte into an unsigned int

Here's a simplified version that I think should work:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



pixel *

liniarizare(char *filename)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    return liniar;

}



int

main(void)

{

    pixel *liniar;



    liniar = liniarizare(filename);



    return 0;

}

UPDATE:

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice? You said something about linear[i]->b being wrong.

Okay, the simplest/best way to deal with a "return" double star argument (e.g.) whatever **retptr) is to ignore this as much as possible.

That is, the function deals with the simpler whatever *ptr internally. This is faster because there is only a single dereference level and not a double dereference on each statement.

The return value (i.e. the double star pointer) is only set at the end once.

Here's my example reworked to use your original function prototype [but with my other cleanup]. Note that only two lines are changed (the function prototype and the last line of the function):

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare(char *filename,pixel **retval)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;



    liniarizare(filename,&liniar);



    return 0;

}

Sometimes the "return value" pointer needs to be read at the top of a function and set at the bottom.

Here's a "push to tail" function for a singly linked list:

void

push(node **list,node *new)

{

    node *head;

    node *prev;

    node *cur;



    head = *list;



    prev = NULL;

    for (cur = head;  cur != NULL;  cur = cur->next)

        prev = cur;



    if (prev != NULL)

        prev->next = new;

    else

        head = new;



    new->next = NULL;



    *list = head;

}

UPDATE #2:

Okay, now that we've got something working, it's time to optimize it [after a suitable rest period :-)].

Keep the now working version around as a reference/cross-check.

fread calls on single bytes are somewhat expensive.

Since your code is doing byte at a time I/O, we can replace the fread calls with fgetc. This should be slightly faster:

for (i = 0; i < count; ++i, ++pix) {

    pix->b = fgetc(fin) & 0xFF;

    pix->g = fgetc(fin) & 0xFF;

    pix->r = fgetc(fin) & 0xFF;

}

But we could do a row at a time (e.g. unsigned char row[width * 3];). This is more tractable and probably produces as good or better results, so it may be a good compromise.

This may or may not be faster. That's why we keep the other versions around and benchmark to determine the fastest/best.

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare_by_row(char *filename,pixel **retval)

{

    int i;

    int yidx;



    int count = width * height;

    int size = sizeof(pixel) * count;

    int w3 = width * 3;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char row[w3];

    for (yidx = 0;  yidx < height;  ++yidx) {

        fread(row,sizeof(row),1,fin);

        for (i = 0;  i < w3;  i += 3, ++pix) {

            pix->b = row[i + 0];

            pix->g = row[i + 1];

            pix->r = row[i + 2]

        }

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;

    pixel *liniar_fast;



    liniarizare(filename,&liniar);

    liniarizare_fast(filename,&liniar_fast);



    return 0;

}

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

add a comment |

Note that this is prefaced by my top comments.

That is, have the function return pixel *. And, use an extra unsigned char variable to prevent reading a byte into an unsigned int

Here's a simplified version that I think should work:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



pixel *

liniarizare(char *filename)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    return liniar;

}



int

main(void)

{

    pixel *liniar;



    liniar = liniarizare(filename);



    return 0;

}

UPDATE:

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice? You said something about linear[i]->b being wrong.

Okay, the simplest/best way to deal with a "return" double star argument (e.g.) whatever **retptr) is to ignore this as much as possible.

That is, the function deals with the simpler whatever *ptr internally. This is faster because there is only a single dereference level and not a double dereference on each statement.

The return value (i.e. the double star pointer) is only set at the end once.

Here's my example reworked to use your original function prototype [but with my other cleanup]. Note that only two lines are changed (the function prototype and the last line of the function):

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare(char *filename,pixel **retval)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;



    liniarizare(filename,&liniar);



    return 0;

}

Sometimes the "return value" pointer needs to be read at the top of a function and set at the bottom.

Here's a "push to tail" function for a singly linked list:

void

push(node **list,node *new)

{

    node *head;

    node *prev;

    node *cur;



    head = *list;



    prev = NULL;

    for (cur = head;  cur != NULL;  cur = cur->next)

        prev = cur;



    if (prev != NULL)

        prev->next = new;

    else

        head = new;



    new->next = NULL;



    *list = head;

}

UPDATE #2:

Okay, now that we've got something working, it's time to optimize it [after a suitable rest period :-)].

Keep the now working version around as a reference/cross-check.

fread calls on single bytes are somewhat expensive.

Since your code is doing byte at a time I/O, we can replace the fread calls with fgetc. This should be slightly faster:

for (i = 0; i < count; ++i, ++pix) {

    pix->b = fgetc(fin) & 0xFF;

    pix->g = fgetc(fin) & 0xFF;

    pix->r = fgetc(fin) & 0xFF;

}

But we could do a row at a time (e.g. unsigned char row[width * 3];). This is more tractable and probably produces as good or better results, so it may be a good compromise.

This may or may not be faster. That's why we keep the other versions around and benchmark to determine the fastest/best.

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare_by_row(char *filename,pixel **retval)

{

    int i;

    int yidx;



    int count = width * height;

    int size = sizeof(pixel) * count;

    int w3 = width * 3;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char row[w3];

    for (yidx = 0;  yidx < height;  ++yidx) {

        fread(row,sizeof(row),1,fin);

        for (i = 0;  i < w3;  i += 3, ++pix) {

            pix->b = row[i + 0];

            pix->g = row[i + 1];

            pix->r = row[i + 2]

        }

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;

    pixel *liniar_fast;



    liniarizare(filename,&liniar);

    liniarizare_fast(filename,&liniar_fast);



    return 0;

}

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

Note that this is prefaced by my top comments.

That is, have the function return pixel *. And, use an extra unsigned char variable to prevent reading a byte into an unsigned int

Here's a simplified version that I think should work:

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



pixel *

liniarizare(char *filename)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    return liniar;

}



int

main(void)

{

    pixel *liniar;



    liniar = liniarizare(filename);



    return 0;

}

UPDATE:

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice? You said something about linear[i]->b being wrong.

Okay, the simplest/best way to deal with a "return" double star argument (e.g.) whatever **retptr) is to ignore this as much as possible.

That is, the function deals with the simpler whatever *ptr internally. This is faster because there is only a single dereference level and not a double dereference on each statement.

The return value (i.e. the double star pointer) is only set at the end once.

Here's my example reworked to use your original function prototype [but with my other cleanup]. Note that only two lines are changed (the function prototype and the last line of the function):

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare(char *filename,pixel **retval)

{

    int i;



    int count = width * height;

    int size = sizeof(pixel) * count;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char byte;

    for (i = 0; i < count; ++i, ++pix) {

        fread(&byte,1,1,fin);

        pix->b = byte;



        fread(&byte,1,1,fin);

        pix->g = byte;



        fread(&byte,1,1,fin);

        pix->r = byte;

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;



    liniarizare(filename,&liniar);



    return 0;

}

Sometimes the "return value" pointer needs to be read at the top of a function and set at the bottom.

Here's a "push to tail" function for a singly linked list:

void

push(node **list,node *new)

{

    node *head;

    node *prev;

    node *cur;



    head = *list;



    prev = NULL;

    for (cur = head;  cur != NULL;  cur = cur->next)

        prev = cur;



    if (prev != NULL)

        prev->next = new;

    else

        head = new;



    new->next = NULL;



    *list = head;

}

UPDATE #2:

Okay, now that we've got something working, it's time to optimize it [after a suitable rest period :-)].

Keep the now working version around as a reference/cross-check.

fread calls on single bytes are somewhat expensive.

Since your code is doing byte at a time I/O, we can replace the fread calls with fgetc. This should be slightly faster:

for (i = 0; i < count; ++i, ++pix) {

    pix->b = fgetc(fin) & 0xFF;

    pix->g = fgetc(fin) & 0xFF;

    pix->r = fgetc(fin) & 0xFF;

}

But we could do a row at a time (e.g. unsigned char row[width * 3];). This is more tractable and probably produces as good or better results, so it may be a good compromise.

This may or may not be faster. That's why we keep the other versions around and benchmark to determine the fastest/best.

typedef struct pixel {

    unsigned int r;

    unsigned int g;

    unsigned int b;

} pixel;



void

liniarizare_by_row(char *filename,pixel **retval)

{

    int i;

    int yidx;



    int count = width * height;

    int size = sizeof(pixel) * count;

    int w3 = width * 3;

    pixel *liniar = malloc(size);



    pixel *pix = liniar;

    unsigned char row[w3];

    for (yidx = 0;  yidx < height;  ++yidx) {

        fread(row,sizeof(row),1,fin);

        for (i = 0;  i < w3;  i += 3, ++pix) {

            pix->b = row[i + 0];

            pix->g = row[i + 1];

            pix->r = row[i + 2]

        }

    }



    *retval = liniar;

}



int

main(void)

{

    pixel *liniar;

    pixel *liniar_fast;



    liniarizare(filename,&liniar);

    liniarizare_fast(filename,&liniar_fast);



    return 0;

}

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

edited Jan 3 at 0:32

answered Jan 2 at 22:22

Craig Estey

15.3k21131

answered Jan 2 at 22:22

Craig Estey

15.3k21131

answered Jan 2 at 22:22

Craig Estey

15.3k21131

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

add a comment |

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

Miraculously enough it works. The only problem is that i need to be able to pass the array by "reference" in the function and have the function provide back the modified array,which is why i insisted on using ** and a void. Do you have any idea what could be wrong in my code with your advice?You said something about linear[i]->b being wrong.

– valentine george
Jan 2 at 22:38

It works!! Honestly thanks a lot this answer saved me from spending even more hours trying to figure out why it would crash.

– valentine george
Jan 2 at 23:15

You're welcome! We all work for "points" here :-) So, please consider upvote/accept: stackoverflow.com/help/someone-answers

– Craig Estey
Jan 2 at 23:19

add a comment |

draft saved

draft discarded

Thanks for contributing an answer to Stack Overflow!

Please be sure to answer the question. Provide details and share your research!

But avoid …

Asking for help, clarification, or responding to other answers.

Making statements based on opinion; back them up with references or personal experience.

To learn more, see our tips on writing great answers.

draft saved

draft discarded

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Sign up or log in

StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});

Post as a guest

Name

Required, but never shown

Name

Required, but never shown

Name

Required, but never shown

This page is only for reference, If you need detailed information, please check here

Search This Blog

Ufyukyu