While loop is causing a stack dump and the program to crash in C++

Question

I am just getting back into programming in C++. I wrote a program that is supposed to read hex colors from a file, mathematically compare them to an array of colors identified within the program to determine which one is the closest, and then write the original color and closest color to a file. For some reason after writing about 62,000 lines or so the program craps out with a stack dump. The file I'm reading from has about 16 million colors in it. I was hoping someone might be able to point me in the right direction with my code to resolve this issue.

The code is below, I did not paste the arrays for red, green, blue, or pantonehexcode; but you can assume they are array with numerical and hexadecimal string values respectively.

    string line;
string hexcolor, r_hex, g_hex, b_hex;

const char delim[] = " ;";
float *cie1 = new float[3];
float *cie2 = new float[3];

float r ;
float g ;
float b ;
float currentClosestVal = 1000000;
float challengeClosestVal;
int currentClosestIndex;

    ifstream file ("hexcolormaplist.txt");
if (file.fail()) 
        {cout << "Error opening infile file"; return 0;}
ofstream ofile("tpxmap.txt");
if (ofile.fail()) 
        {cout << "Error opening ofile file"; return 0;}

bool newline = true;
//Comparing colors variables
int i, k;
double Kl, K1, K2, Sl, SC, SH, dL, dA, dB, dC, dH, c1, c2;

getline (file,line);
char * cline = new char [line.length()+1];



while(newline == true){

    currentClosestVal = 1000000;
    std::strcpy (cline ,line.c_str());
    hexcolor = strtok(cline, delim);
    r_hex = strtok(NULL, delim);
    g_hex = strtok(NULL, delim);
    b_hex = strtok(NULL, delim);


    r = (float)atof(r_hex.c_str());
    g = (float)atof(g_hex.c_str());
    b = (float)atof(b_hex.c_str());

    cie1 = rgb2lab (r, g, b);



    for (i = 0; i < 2100; i++)
    {
        cie2 = rgb2lab (red[i], green[i], blue[i]);
        //challengeClosestVal = pow(cie1[0] - cie2[0], 2.0) + pow(cie1[1] - cie2[1], 2.0) + pow(cie1[2] - cie2[2], 2.0);
        dL = cie1[0] - cie2[0];
        dA = cie1[1] - cie2[1];
        dB = cie1[2] - cie2[2];
        c1 = sqrt(cie1[1] + cie1[2]);
        c2 = sqrt(cie2[1] + cie2[2]);
        dC = c1 - c2;
        dH = sqrt(pow(dA, 2) + pow(dB,2) - pow(dC,2));
        Kl = 2;
        K1 = .048;
        K2 = .014;
        Sl = 1;
        SC = 1 + K1*c1;
        SH = 1 + K2*c1;

        challengeClosestVal = sqrt(pow(dL/(Kl*Sl), 2) + pow(dC/(Kl*SC),2) + pow(dH/(Kl*SH), 2));


        if(challengeClosestVal < currentClosestVal){
            currentClosestIndex = i;
            currentClosestVal = challengeClosestVal;
        }
    }

    ofile << hexcolor <<"; " << pantoneHexCodes[currentClosestIndex] <<";"<<endl; // prints The pantone color comparator
    line = "";
    newline = getline (file,line);

}//end of while loop

//close files
file.close();
ofile.close();
return 0;
}

float *rgb2lab (float r, float g, float b){

    float var_r, var_g, var_b;
    double X, Y, Z, var_X, var_Y, var_Z;
    float ref_X =  95.047;  //Observer= 2°, Illuminant= D65
    float ref_Y = 100.000;
    float ref_Z = 108.883;
    double cieL, cieA, cieB;
    float *cie = new float[3];



    //Convert RGB to XYZ
    //First set RGB values between 0-1
    var_r = r/255;
    var_g = g/255;
    var_b = b/255;

    if ( var_r > 0.04045 )
            var_r = pow( ( var_r + 0.055 ) / 1.055 , 2.4);
    else
            var_r = var_r / 12.92;

    if ( var_g > 0.04045 )
            var_g = pow( ( var_g + 0.055 ) / 1.055 , 2.4);
    else
            var_g = var_g / 12.92;
    if ( var_b > 0.04045 )
            var_b = pow( ( var_b + 0.055 ) / 1.055 , 2.4);
    else
            var_b = var_b / 12.92;

    var_r = var_r * 100;
    var_g = var_g * 100;
    var_b = var_b * 100;

    //Convert RGB to XYZ
    //Observer. = 2°, illuminant = D65
    X = var_r * 0.4124 + var_g * 0.3576 + var_b * 0.1805;
    Y = var_r * 0.2126 + var_g * 0.7152 + var_b * 0.0722;
    Z = var_r * 0.0193 + var_g * 0.1192 + var_b * 0.9505;

    //cout << "X: "<<X  <<"  Y: "  <<Y <<"  Z: "<<Z << endl;


    // Convert XYZ to CIELab
    var_X = X / ref_X;          //ref_X =  95.047   Observer= 2°, Illuminant= D65
    var_Y = Y / ref_Y;          //ref_Y = 100.000
    var_Z = Z / ref_Z;          //ref_Z = 108.883
    //cout << "var_X: "<<var_X  <<"  var_Y: "  <<var_Y <<"  var_Z: "<<var_Z << endl;

    if ( var_X > 0.008856 ) {
            var_X = pow(var_X, .3333333); }
    else
            var_X = ( 7.787 * var_X) + ( 16 / 116 );
    if ( var_Y > 0.008856 ){
            var_Y = pow(var_Y, .3333333); }
    else
            var_Y = ( 7.787 * var_Y) + ( 16 / 116 );
    if  ( var_Z > 0.008856 ){
            var_Z = pow(var_Z,  .3333333); }
    else
            var_Z = ( 7.787 * var_Z) + ( 16 / 116 );


    cieL = ( 116 * var_Y ) - 16;
    cieA = 500 * ( var_X - var_Y );
    cieB = 200 * ( var_Y - var_Z );

    //cout << "L: "<<cie[0]  <<"  a: "  <<cie[1] <<"  b: "<<cie[2] << endl;
    cie[0] = cieL;
    cie[1] = cieA;
    cie[2] = cieB;
    //cout << "L: "<<cie[0]  <<"  a: "  <<cie[1] <<"  b: "<<cie[2] << endl;
    return cie;

}

Answer 1

In your rgb2lab function you create a new float[3]. This happens with every call to this function. Looking at your code, I do not see where the memory for this is freed up with a call to delete anywhere.

It is good programming practice that every new is paired with a delete. It is also more important that the delete is being called in every execution path of the program if something is created with new.

What is happening in your code is that your rgb2lab function is being called 2100 times for every line written to file. You are saying that your program craps out after about 62000 lines being written to file. In this case, the rgb2lab function is being called 130,200,000 times and is leaking memory every single time.

EDIT:

You have a couple of spots where you are calling new that you should delete, but the biggest offender is the 2100 iteration for loop that calls the rgb2lab function over and over again. Since you are using the dynamically allocated array that the function returns, just free the memory when you are done using it.

for (i = 0; i < 2100; i++)
{
    cie2 = rgb2lab (red[i], green[i], blue[i]);
    //challengeClosestVal = pow(cie1[0] - cie2[0], 2.0) + pow(cie1[1] - cie2[1], 2.0) + pow(cie1[2] - cie2[2], 2.0);
    dL = cie1[0] - cie2[0];
    dA = cie1[1] - cie2[1];
    dB = cie1[2] - cie2[2];
    c1 = sqrt(cie1[1] + cie1[2]);
    c2 = sqrt(cie2[1] + cie2[2]);
    dC = c1 - c2;
    dH = sqrt(pow(dA, 2) + pow(dB,2) - pow(dC,2));
    Kl = 2;
    K1 = .048;
    K2 = .014;
    Sl = 1;
    SC = 1 + K1*c1;
    SH = 1 + K2*c1;

    challengeClosestVal = sqrt(pow(dL/(Kl*Sl), 2) + pow(dC/(Kl*SC),2) + pow(dH/(Kl*SH), 2));


    if(challengeClosestVal < currentClosestVal){
        currentClosestIndex = i;
        currentClosestVal = challengeClosestVal;
    }
}
delete [] cie2;  // <-- Free up the memory here!

Answer 2

I don't see why you are using the new operator (except you come from a Java or C# background). You can pass results without using the new operator.

For example:

void rbg_to_lab (float r, float g, float b,
                 float& cie[3])
{
 // ...
}

By passing the cie array as a reference you can modify the caller's variable directly with allocating any new memory objects (and not having to delete the memory either.).

Another solution is to create a struct for the cie values and return the struct:

struct Cie_Values
{
  float cie_1;
  float cie_2;
  float cie_3;
};

Cie_Values rgb_to_lab(float r, float g, float b)
{
   Cie_values cie;
   // Perform conversion
   return cie;
}