-3

This is my code:

#include <stdio.h>
#include <stdlib.h>

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

int main ()
{
    int width, height, i, j, opCode;

    struct pixel **img;

    printf("opCode = ");
    scanf("%d", &opCode);

    printf("Numarul de coloane = ");
    scanf("%d", &width);

    printf("Numarul de linii = ");
    scanf("%d", &height);

    img = malloc(height * width * sizeof(int *));

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

             img[i][j].r = scanf("%d ", &img[i][j].r);
             img[i][j].g = scanf("%d ", &img[i][j].g);
             img[i][j].b = scanf("%d ", &img[i][j].b);

        }
      }

    free(*img);

    return 0;
}

When I read opCode, height and width it works just fine. But after I try to read the first element of the matrix img, it gives me a segmentation fault (core dumped). I tried to use Valgrind to figure out what's wrong, but I can't figure out the problem.

Cœur
  • 37,241
  • 25
  • 195
  • 267
Telehoi Alex
  • 1

4 Answers4

1

Your problems are

  1. You have not allocated the memory correctly for a 2D array.
  2. Your use of scanf is incorrect. Please check the manual page.

Here is (not perfect) code that might help. You need to check the return values of scanf and malloc and take appropriate measures. I leave that bit to you.

#include <stdio.h>
#include <stdlib.h>

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

int main ()
{
    int width, height, i, j, opCode;

    struct pixel **img;

    printf("opCode = ");
    scanf("%d", &opCode);

    printf("Numarul de coloane = ");
    scanf("%d", &width);

    printf("Numarul de linii = ");
    scanf("%d", &height);

    img = malloc(height * sizeof(struct pixel *)); // Creates the array of pointers to an array or pixcels

    for ( i = 0; i < height; i++)
      {
         img[i] = malloc(sizeof(struct pixel) * width)); // Creates an array of pixels
         for ( j = 0; j < width; j++)
        {

             scanf("%d ", &img[i][j].r); // Should really check the return value here? Perhaps error checking
             scanf("%d ", &img[i][j].g); // Ditto
             scanf("%d ", &img[i][j].b);

        }
      }

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

        free(img[i]);
     }
     free(img);
    return 0;
}
Ed Heal
  • 59,252
  • 17
  • 87
  • 127
0

You have used the wrong 'malloc' 

    //img = malloc(height * width * sizeof(int *));
      img = (int **)malloc(height*sizeof(int *)); // First Init the double pointer with the number of height 

for ( i = 0; i < height; i++)
  {
     img[i] = (struct pixel *)malloc(width*sizeof(struct pixel)); 
     for ( j = 0; j < width; j++)
    {

         img[i][j].r = scanf("%d ", &img[i][j].r);
         img[i][j].g = scanf("%d ", &img[i][j].g);
         img[i][j].b = scanf("%d ", &img[i][j].b);

    }
  }

Now free will be changed, you need to free first the width portion of structure for each height.

for (i = 0 ; i < height; i++)
  free(img[i]);

free(img);
Sohil Omer
  • 1,171
  • 7
  • 14
0
#include <stdio.h>
#include <stdlib.h>

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

int main ()
{
    int width, height, i, j, opCode;

    struct pixel **img;

    printf("opCode = ");
    scanf("%d", &opCode);

    printf("Numarul de coloane = ");
    scanf("%d", &width);

    printf("Numarul de linii = ");
    scanf("%d", &height);


    /*Try to understand starting from here*/
    img = malloc(height * sizeof(struct pixel *));

    for(i= 0; i < height; i++)
    {
        img[i] = malloc(width*sizeof(struct pixel));
    }

    for(i = 0; i < height; i++)
    {
        for (j = 0; j < width; j++)
        {
            /*This part was wrong in your code*/
            scanf("%d ", &img[i][j].r);
            scanf("%d ", &img[i][j].g);
            scanf("%d ", &img[i][j].b);
        }
    }

    /*This is for you to see*/
    for(i = 0; i < height; i++)
    {
        for(j = 0; j < width; j++)
        {
            printf("%d ", img[i][j].r);
            printf("%d ", img[i][j].g);
            printf("%d ", img[i][j].b);
            printf("\n");
        }
    }

    /*And lastly free allocated spaces*/
    for(i = 0; i < height; i++)
    {
        free(img[i]);
    }

    free(img);

    return 0;
}
aeb-dev
  • 313
  • 5
  • 16
  • 1
    Code-only answers are not helpful. And you also don't show a 2D array. – too honest for this site Dec 01 '16 at 13:47
  • That is nonsense! You can use an `int(*)[COLS]` and `malloc` a 2D array. See the link I provided in a comment to the question. And "buffer" is not a datatype, but a certain usage for an array. It is an inappropriate term for this question. – too honest for this site Dec 01 '16 at 14:07
  • @Olaf This code handles what OP wants. There is no indication that forces people to use 2D array as you want. It solves the problem then it is 'a' single solution. If you have better one you can suggest it. Im not even goint bother to argue with you anymore. – aeb-dev Dec 01 '16 at 14:14
  • OP clearly wants a 2D array, although he does not explicitly use that name, he apparently allocates one and uses the pointer-to-pointer like as if it was one. Your approack is way too complicated for what is required, and typically less performant due to the double indirection. It also implements a less common pattern, so the compiler might have trouble optimising it (vector-instructions, loop-unrolling). All this adds to discouraging this approach. – too honest for this site Dec 01 '16 at 16:42
-1

You messed up your malloc and free.

  1. You allocate too less memory here:

    img = malloc(height * width * sizeof(int *));

    because the struct pixel at least requires the size of three single integers. (The sizeof(int*) is wrong, because it's just the size of a pointer)

  2. The free(*img) should be free(img) accordingly to your malloc() call.

  3. The access to a single pixel value by using img[i][j].r cannot work, because you don't have a 2-dim array as a storage.

You can simplify your double pointer to pixel by a single pointer (and allocate everything in a continuous memory block):

int main() {
    int width, height, i, j, opCode;

    printf("opCode = ");
    scanf("%d", &opCode);

    printf("Numarul de coloane = ");
    scanf("%d", &width);

    printf("Numarul de linii = ");
    scanf("%d", &height);

    struct pixel* img = malloc(height * width * sizeof(pixel));

    for (i = 0; i < height; i++) {
        for (j = 0; j < width; j++) {
            int val;
            scanf("%d ", &val);
            img[i*width + j].r = val;
            scanf("%d ", &val);
            img[i*width + j].g = val;
            scanf("%d ", &val);
            img[i*width + j].b = val;
        }
    }
    free(img);
    return 0;
}
cwschmidt
  • 1,194
  • 11
  • 17
  • Any reason you emulate a 2D array with error-prone manual code? Let the compiler do the index-calculations and use a 2D array – too honest for this site Dec 01 '16 at 13:48
  • @Olaf: No particular reason in this case, in general it has a better performance (because of less `malloc` calls and continousity of the data in memory). – cwschmidt Dec 01 '16 at 14:13
  • How does this have less than the single call for a 2D array? Or better performance than letting the compiler do the indexing? Have a look at my other comments here, I certainly do **not** talk about a jagged array (which I agree is the worst of three approaches). A pointer-to-pointer is no 2D array, nor can it point to one or represent one! – too honest for this site Dec 01 '16 at 16:44
  • @Olaf: Was just the first thing, that came into my mind (As I remember it was inspired from that video https://www.youtube.com/watch?v=06OHflWNCOE). In contrast to some of other answers here it does not need to call `malloc` within a loop. I don't say that's the best or even only solution to do it. – cwschmidt Dec 01 '16 at 17:10
  • @Olaf: _OP clearly wants a 2D array,_ - Is what you said in some comment above. Just for curiousity: From which statement of the OP do you conclude that? – cwschmidt Dec 01 '16 at 17:14
  • Youtube videos are far from a good or authoritative reference. There are still far too many ppl stuck in the 1990ies with C90 or even K&R-C. (both allowed multidimensional arrays, though) You might want to read the standard and use modern (i.e. the 1999 release) C. There is neither a reason for a loop, not manual index-calculations which are error-prone. – too honest for this site Dec 01 '16 at 17:15
  • From how he allocates the array and uses it and from the context of the code. 2D Images are **typically** 2D and rectangular. It also would be require the least changes to the original code. Also "matrix" is the mathematical name of a 2D array. I never saw a matrix in mathematics represented as a vector nor a vector of pointers to vectors. – too honest for this site Dec 01 '16 at 17:17
  • @Olaf: I really don't want to argue with you (because most of your posting are really helpful), but in this case, the OP said, he is "new to programming". How can you conclude his intention from his code (which even is not working) on one hand and tell on the other hand that youtube videos are not an authorative reference. Of course those videos they are not authorative, but neither are blogs or even stackoverflow comments/answers. According to your logic (whom to trust, only the spec? That's hard.), that makes even stackoverflow obsolete then. – cwschmidt Dec 01 '16 at 17:33
  • 1
    @Olaf: As I said, my answer is one possible solution, that solves the problem of the seg-fault. It may neither be the best nor the most elegant. However, I agree with you that there is no reason for a for loop, otherwise my example wouldn't work, too. – cwschmidt Dec 01 '16 at 17:33