Scaling a bitmap image getting segfault

Question

Sup guys, learning C and working on a C programming assignment where I am to scale a given bitmap image and I have been stuck on this all day. this is my code thus far but I am getting a segfault and can't figure out why. I've been tracing through the code all day and am just stuck. here is my code of the function to scale, any help would be appreciated

int enlarge(PIXEL* original, int rows, int cols, int scale, 
    PIXEL** new, int* newrows, int* newcols) 
{
  int ncols, nrows;
  ncols = cols * scale;
  nrows = rows * scale;
  double xratio =(double) rows / nrows;
  double yratio =(double) cols / ncols;
  int px, py;
  int auxw, cnt;
  int i, j;

 *new = (PIXEL*)malloc(nrows * ncols * sizeof(PIXEL));
  for (i = 0; i < nrows; i++){
    auxw = 0;
    cnt = 0;
    int m = i * 3;
     for (j = 0; j < ncols; j++){
         px = (int)floor( j * xratio);
         py = (int)floor( i * yratio);

         PIXEL* o1 = original + ((py*rows + px) *3);
         PIXEL* n1 = (*new) + m*ncols + j + auxw;
         *n1 = *o1;

         PIXEL* o2 = original + ((py*rows + px) *3) + 1;
         PIXEL* n2 = (*new) + m*ncols + j + 1 + auxw;
         *n2 = *o2;

         PIXEL* o3 = original + ((py*rows + px) *3) + 2;
         PIXEL* n3 = (*new) + m*ncols + j + 2 + auxw;
         *n3 = *o3;

         auxw += 2;
         cnt++;
       }
     }
 return 0;
}

using the GDB, i get the following :

Program received signal SIGSEGV, Segmentation fault.
0x00000000004013ff in enlarge (original=0x7ffff7f1e010, rows=512, cols=512, scale=2,                   new=0x7fffffffdeb8, 
newrows=0x7fffffffdfb0, newcols=0x0) at maind.c:53
53       *n3 = *o3;

however, I can't understand what exactly the problem is

thanks

EDIT: Working off code our professor provided for us, a PIXEL is defined as such:

typedef struct {
  unsigned char r;
  unsigned char g;
  unsigned char b;
} PIXEL;

From my understanding i have a 2 dimensional array where each element of that array contains a 3 element PIXEL array. Also, when tracing my code on paper, I added the auxw logic in order to advance down the array. It works somewhat in the same way as multiplying by 3.

Answer 1

Is your array a cols X rows array of PIXEL objects -- or is it actually an cols X rows X 3 array of PIXEL objects where what you call a pixel is actually really a component channel of a pixel? Your code isn't clear. When accessing the original array, you multiply by 3, suggesting an array of 3 channels:

     PIXEL* o1 = original + ((py*rows + px) *3);

But when accessing the (*new) array there is no multiplication by 3, instead there's some logic I cannot follow with auxw:

     PIXEL* n1 = (*new) + m*ncols + j + auxw;

     auxw += 2;

Anyway, assuming that what you call a pixel is actually a channel, and that there are the standard 3 RGB channels in each pixel, you need to allocate 3 times as much memory for your array:

     *new = (PIXEL*)malloc(nrows * ncols * 3*sizeof(PIXEL));

Some additional issues:

1. int* newrows and int* newcols are never initialized. You probably want to initialize them to the values of nrows and ncols

2. If PIXEL is really a CHANNEL, then rename it to correctly express its meaning.

3. Rather than copying logic for multidimensional array pointer arithmetic all over the place, protect yourself from indexing off your pixel/channel/whatever arrays by using a function:

   #include "assert.h"
   
   PIXEL *get_channel(PIXEL *pixelArray, int nRows, int nCols, int nChannels, int iRow, int iCol, int iChannel)
   {
       if (iRow < 0 || iRow >= nRows)
       {
           assert(!(iRow < 0 || iRow >= nRows));
           return NULL;
       }
       if (iCol < 0 || iCol >= nCols)
       {
           assert(!(iRow < 0 || iRow >= nRows));
           return NULL;
       }
       if (iChannel < 0 || iChannel >= nChannels)
       {
           assert(!(iChannel < 0 || iChannel >= nChannels));
           return NULL;
       }
       return pixelArray + (iRow * nCols + iCol) * nChannels + iChannel;
   }
   

   Later, once your code is fully debugged, if performance is a problem you can replace the function with a macro in release mode:

   #define get_channel(pixelArray, nRows, nCols, nChannels, iRow, iCol, iChannel)\
       ((pixelArray) + ((iRow) * (nCols) + (iCol)) * (nChannels) + (iChannel))
   

4. Another reason to use a standard get_channel() function is that your pointer arithmetic is inconsistent:

    PIXEL* o1 = original + ((py*rows + px) *3);
    PIXEL* n1 = (*new) + m*ncols + j + auxw;
   

   to access the original pixel, you do array + iCol * nRows + iRow, which looks good. But to access the *new array, you do array + iCol * nCols + iRow, which looks wrong. Make a single function to access any pixel array, debug it, and use it.

Update

Given your definition of the PIXEL struct, it is unnecessary for you to be "adding those +1 and +2 values allowed me to reach the second and third element of the PIXEL struct." Since PIXEL is a struct, if you have a pointer to one you access its fields using the -> operator:

            PIXEL *p_oldPixel = get_pixel(old, nRowsOld, nColsOld, iRowOld, iColOld);
            PIXEL *p_newPixel = get_pixel(*p_new, nRowsNew, nColsNew, iRowNew, iColNew);

            p_newPixel->r = p_oldPixel->r;
            p_newPixel->g = p_oldPixel->g;
            p_newPixel->b = p_oldPixel->b;

Or, in this case you can use the assignment operator to copy the struct:

            *p_newPixel = *p_oldPixel;

As for indexing through the PIXEL array, since your pointers are correctly declared as PIXEL *, the C compiler's arithmetic will multiply offsets by the size of the struct.

Also, I'd recommend clarifying your code by using clear and consistent naming conventions:

1. Use consistent and descriptive names for loop iterators and boundaries. Is i a row or a column? Why use i in one place but py in another? A consistent naming convention helps to ensure you never mix up your rows and columns.

2. Distinguish pointers from variables or structures by prepending "p_" or appending "_ptr". A naming convention that clearly distinguishes pointers can make instances of pass-by-reference more clear, so (e.g.) you don't forget to initialize output arguments.

3. Use the same syllable for all variables corresponding to the old and new bitmaps. E.g. if you have arguments named old, nRowsOld and nColsOld you are less likely to accidentally use nColsOld with the new bitmap.

Thus your code becomes:

#include "assert.h"

typedef struct _pixel {
    unsigned char r;
    unsigned char g;
    unsigned char b;
} PIXEL;

PIXEL *get_pixel(PIXEL *pixelArray, int nRows, int nCols, int iRow, int iCol)
{
    if (iRow < 0 || iRow >= nRows)
    {
        assert(!(iRow < 0 || iRow >= nRows));
        return NULL;
    }
    if (iCol < 0 || iCol >= nCols)
    {
        assert(!(iRow < 0 || iRow >= nRows));
        return NULL;
    }
    return pixelArray + iRow * nCols + iCol;
}

int enlarge(PIXEL* old, int nRowsOld, int nColsOld, int scale, 
    PIXEL **p_new, int *p_nRowsNew, int *p_nColsNew) 
{
    int nColsNew = nColsOld * scale;
    int nRowsNew = nRowsOld * scale;
    double xratio =(double) nRowsOld / nRowsNew;
    double yratio =(double) nColsOld / nColsNew;
    int iRowNew, iColNew;

    *p_new = malloc(nRowsNew * nColsNew * sizeof(PIXEL));
    *p_nRowsNew = nRowsNew;
    *p_nColsNew = nColsNew;
    for (iRowNew = 0; iRowNew < nRowsNew; iRowNew++){
        for (iColNew = 0; iColNew < nColsNew; iColNew++){
            int iColOld = (int)floor( iColNew * xratio);
            int iRowOld = (int)floor( iRowNew * yratio);

            PIXEL *p_oldPixel = get_pixel(old, nRowsOld, nColsOld, iRowOld, iColOld);
            PIXEL *p_newPixel = get_pixel(*p_new, nRowsNew, nColsNew, iRowNew, iColNew);

            *p_newPixel = *p_oldPixel;
        }
    }

    return 0;
}

I haven't tested this code yet, but by using consistent naming conventions one can clearly see what it is doing and why it should work.