3
\$\begingroup\$

I don't have a lot of experience in C++; I'm more of a C# guy. I'm trying to convert some Matlab image processing code to C++. I would appreciate any feedback about C++ coding conventions, which data structures to use, what kills performance, and anything which is considered to be good or bad coding style for C++. I'm kind of mixing C and C++ and C# so I wanted to get some feedback.

Preprocessor.h

#pragma once
#include <stdio.h>
#include <map>
#include <algorithm>
#include <vector>
#include "ToolBoxExports.h"
#include "PreprocessorResult.h"

using namespace std;

#include <stdint.h>

typedef enum TOOLBOX_EXPORT
{
    GrRBGb = 0,
    RGrGbB,
    BGbGrR,
    GbBRGr
} color_order;

typedef class TOOLBOX_EXPORT
{
public:
    uint16_t width;
    uint16_t height;
    color_order order;
    uint16_t* raw_data;
    uint16_t bit_depth;
} bayer_raw_image;



typedef class TOOLBOX_EXPORT
{
public :
    double exposure_time;
    double analog_gain;
    std::vector<uint16_t> black_level;
} black_level_lut;

typedef class TOOLBOX_EXPORT
{
public:

    std::vector<black_level_lut*> black_level_luts;
} black_level;

class TOOLBOX_EXPORT Preprocessor
{
public:


    PreprocessorResult Function1(bayer_raw_image* rawBayerImage, black_level* blackLevelDataNative, std::vector<double> saturationLevel, double analogGain, double exposureTime);
    PreprocessorResult Function3b(bayer_raw_image* rawBayerImage, bayer_raw_image* colorCheckerImage, int sensorColorOrder, std::vector<double> saturationLevel);
    void Process(std::vector<uint16_t>& data, int width, int height, std::vector<float>& output);
    void CalculateBlackLevel(black_level* blackLevelDataNative, double analogGain, double exposureTime, double(&output)[4]);
    void SeparateChannels(uint16_t* _image, std::vector<uint16_t>& gr, std::vector<uint16_t>& r, std::vector<uint16_t>& b, std::vector<uint16_t>& gb, int width, int height, int colorOrder);
    void ScaleLscGrid(std::vector<float>& gr, std::vector<float>& r, std::vector<float>& b, std::vector<float>& gb, uint16_t height, uint16_t width, uint16_t desiredWidth, uint16_t desiredHeight, std::string method);
    void ApplyLensShadingCorrection(std::vector<float>& floatChannel_gr, std::vector<float>& floatChannel_r, std::vector<float>& floatChannel_b, std::vector<float>& floatChannel_gb, std::vector<uint16_t>& channel_gr_cc, std::vector<uint16_t>& channel_r_cc, std::vector<uint16_t>& channel_b_cc, std::vector<uint16_t>& channel_gb_cc, uint16_t channelCCWidth, uint16_t channelCCHeight);
    void CombineChannelsTo2dImage(std::vector<float>& gr, std::vector<float>& r, std::vector<float>& b, std::vector<float>& gb, int isOverride, bayer_raw_image* outputImage, std::vector<double> saturationLevel);
    int MatlabRound(double numberToRound);//needs to be in a utilities project
    void ShowImage(bayer_raw_image* colorCheckerImage, std::string title);
    void ShowImage(std::vector<float> channel, uint16_t width, uint16_t height, std::string title);     
};

Preprocessor.cpp

#include "Preprocessor.h"
#include "PreprocessorResult.h"
#include "opencv2/imgproc/imgproc.hpp"
#include "opencv2/highgui/highgui.hpp"
#include <array>
#include <algorithm>

PreprocessorResult Preprocessor::Function1(bayer_raw_image* rawBayerImage, black_level* blackLevelDataNative, std::vector<double> saturationLevel, double analogGain, double exposureTime)
{
    if (analogGain <= 0.0)
    {
        throw "Analog gain must be positive";
    }
    if (rawBayerImage == NULL)
    {
        throw "Missing image";
    }
    if (blackLevelDataNative == NULL)
    {
        throw "Missing BLC data";
    }
    if (saturationLevel.empty())
    {
        throw "Missing SaturationLevel data";
    }

    double blackLevelOutputTable[4];

    CalculateBlackLevel(blackLevelDataNative, analogGain, exposureTime, blackLevelOutputTable);

    // TODO: Normalization code below has multiple bugs!
    double factor0 = saturationLevel[0] / (saturationLevel[0] - blackLevelOutputTable[0]);
    double factor1 = saturationLevel[1] / (saturationLevel[1] - blackLevelOutputTable[1]);
    double factor2 = saturationLevel[2] / (saturationLevel[2] - blackLevelOutputTable[2]);
    double factor3 = saturationLevel[3] / (saturationLevel[3] - blackLevelOutputTable[3]);
    for (int i = 0; i < rawBayerImage->width* rawBayerImage->height; i++)
    {
        if (i % 4 == 0)
        {
            rawBayerImage->raw_data[i] = std::max(0.0, std::min(((rawBayerImage->raw_data[i] - blackLevelOutputTable[0]) * factor0), saturationLevel[0]));
        }
        if (i % 4 == 1)
        {
            rawBayerImage->raw_data[i] = std::max(0.0, std::min(((rawBayerImage->raw_data[i] - blackLevelOutputTable[1]) * factor1), saturationLevel[1]));
        }
        if (i % 4 == 2)
        {
            rawBayerImage->raw_data[i] = std::max(0.0, std::min(((rawBayerImage->raw_data[i] - blackLevelOutputTable[2]) * factor2), saturationLevel[2]));
        }
        if (i % 4 == 3)
        {
            rawBayerImage->raw_data[i] = std::max(0.0, std::min(((rawBayerImage->raw_data[i] - blackLevelOutputTable[3]) * factor3), saturationLevel[3]));
        }
    }

    PreprocessorResult result;
    for (int i = 0; i < 4; i++)
    {
        result.blackLevelGains[i] = blackLevelOutputTable[i];
    }
    return result;
}

PreprocessorResult Preprocessor::Function3b(bayer_raw_image* flatFieldImage, bayer_raw_image* colorCheckerImage, int sensorColorOrder, std::vector<double> saturationLevel)
{
    int sizeOfChannel = (flatFieldImage->width / 2) * (flatFieldImage->height / 2);
    std::vector<uint16_t> channel_r_ff(sizeOfChannel);
    std::vector<uint16_t> channel_gr_ff(sizeOfChannel);
    std::vector<uint16_t> channel_gb_ff(sizeOfChannel);
    std::vector<uint16_t> channel_b_ff(sizeOfChannel);

    std::vector<float> floatChannel_r(sizeOfChannel);
    std::vector<float> floatChannel_gr(sizeOfChannel);
    std::vector<float> floatChannel_gb(sizeOfChannel);
    std::vector<float> floatChannel_b(sizeOfChannel);

    SeparateChannels(flatFieldImage->raw_data, channel_gr_ff, channel_r_ff, channel_b_ff, channel_gb_ff, flatFieldImage->width, flatFieldImage->height, flatFieldImage->order);

    int channelWidth = flatFieldImage->width / 2;
    int channelHeight = flatFieldImage->height / 2;

    Process(channel_gr_ff, channelWidth, channelHeight, floatChannel_gr);
    Process(channel_r_ff, channelWidth, channelHeight, floatChannel_r);
    Process(channel_b_ff, channelWidth, channelHeight, floatChannel_b);
    Process(channel_gb_ff, channelWidth, channelHeight, floatChannel_gb);
    //there is no need to preform 
    //grid  = toolbox.bayer.ColorOrder.combineChannels(Gr, R, B, Gb, colorOrder, override); 
    // just have to switch the color order
    std::vector<uint16_t>channel_r_cc(sizeOfChannel);
    std::vector<uint16_t>channel_gr_cc(sizeOfChannel);
    std::vector<uint16_t>channel_gb_cc(sizeOfChannel);
    std::vector<uint16_t>channel_b_cc(sizeOfChannel);

    //for debug
    //ShowImage(colorCheckerImage,"before");
    SeparateChannels(colorCheckerImage->raw_data, channel_gr_cc, channel_r_cc, channel_b_cc, channel_gb_cc, colorCheckerImage->width, colorCheckerImage->height, colorCheckerImage->order);

    int channelCCWidth = colorCheckerImage->width / 2;
    int channelCCHeight = colorCheckerImage->height / 2;
    ApplyLensShadingCorrection(floatChannel_gr, floatChannel_r, floatChannel_b, floatChannel_gb, channel_gr_cc, channel_r_cc, channel_b_cc, channel_gb_cc, channelCCWidth, channelCCHeight);

    //for debug
    //ShowImage(floatChannel_gr,channelCCWidth,channelCCHeight, "gr");
    //ShowImage(floatChannel_gb,channelCCWidth,channelCCHeight, "gb");
    //ShowImage(floatChannel_b,channelCCWidth,channelCCHeight, "b");
    //ShowImage(floatChannel_r,channelCCWidth,channelCCHeight, "r");
    CombineChannelsTo2dImage(floatChannel_gr, floatChannel_r, floatChannel_b, floatChannel_gb, 1, colorCheckerImage, saturationLevel);

    //ShowImage(colorCheckerImage,"after");
    PreprocessorResult result;
    return result;
}

void Preprocessor::CalculateBlackLevel(black_level* blackLevelDataNative, double analogGain, double exposureTime, double(&output)[4])
{
    const int NUMBER_OF_CHANNELS = 4;
    std::vector<double> exposureTimeLut;
    std::vector<std::array<int, NUMBER_OF_CHANNELS> > blackLevelLut;

    if (blackLevelDataNative->black_level_luts.size() == 1)
    {
        std::array<int, NUMBER_OF_CHANNELS> arr;
        for (size_t i = 0; i < NUMBER_OF_CHANNELS; i++)
        {
            arr[i] = blackLevelDataNative->black_level_luts[0]->black_level[i];
        }       
        blackLevelLut.push_back(arr);
    }
    else
    {
        double maxAnalogGain = blackLevelDataNative->black_level_luts[0]->analog_gain;
        double minAnalogGain = blackLevelDataNative->black_level_luts[0]->analog_gain;
        for (int num = 0; num < blackLevelDataNative->black_level_luts.size(); num++)
        {
            maxAnalogGain = std::max(maxAnalogGain, blackLevelDataNative->black_level_luts[num]->analog_gain);
            minAnalogGain = std::min(minAnalogGain, blackLevelDataNative->black_level_luts[num]->analog_gain);
        }
        if (analogGain >= maxAnalogGain || analogGain <= minAnalogGain)
        {
            for (int k = 0; k < blackLevelDataNative->black_level_luts.size(); k++)
            {
                //find which points you need to interpolate
                if (analogGain >= blackLevelDataNative->black_level_luts[k]->analog_gain && analogGain <= blackLevelDataNative->black_level_luts[k + 1]->analog_gain)
                {
                    std::array<int, NUMBER_OF_CHANNELS> arr;
                    for (int ch = 0; ch < NUMBER_OF_CHANNELS; ch++)
                    {
                        //y = y0 + (y1-y0)*(x-x0)/(x1-x0);
                        double y0 = blackLevelDataNative->black_level_luts[k]->black_level[ch];
                        double y1 = blackLevelDataNative->black_level_luts[k + 1]->black_level[ch];
                        double x = analogGain;
                        double x0 = blackLevelDataNative->black_level_luts[k]->analog_gain;
                        double x1 = blackLevelDataNative->black_level_luts[k + 1]->analog_gain;
                        arr[ch] = y0 + (y1 - y0)*(x - x0) / (x1 - x0);
                    }
                    blackLevelLut.push_back(arr);
                    //if the vector does not contains the exposure, add if to the vector
                    if (std::find(exposureTimeLut.begin(), exposureTimeLut.end(), blackLevelDataNative->black_level_luts[k]->exposure_time) == exposureTimeLut.end())
                    {
                        exposureTimeLut.push_back(blackLevelDataNative->black_level_luts[k]->exposure_time);
                    }
                }
            }
        }
        else //extrapolate nearest neighbor
        {
            for (int k = 0; k < blackLevelDataNative->black_level_luts.size(); k++)
            {
                if (analogGain > maxAnalogGain)
                {
                    if (blackLevelDataNative->black_level_luts[k]->analog_gain == maxAnalogGain)
                    {
                        std::array<int, NUMBER_OF_CHANNELS> arr;
                        for (int ch = 0; ch < NUMBER_OF_CHANNELS; ch++)
                        {
                            arr[ch] = blackLevelDataNative->black_level_luts[k]->black_level[ch];
                        }
                        blackLevelLut.push_back(arr);
                        //if the vector does not contains the exposure, add if to the vector
                        if (std::find(exposureTimeLut.begin(), exposureTimeLut.end(), blackLevelDataNative->black_level_luts[k]->exposure_time) == exposureTimeLut.end())
                        {
                            exposureTimeLut.push_back(blackLevelDataNative->black_level_luts[k]->exposure_time);
                        }
                    }
                }
                else //analogGain < minAnalogGain
                {
                    if (blackLevelDataNative->black_level_luts[k]->analog_gain == minAnalogGain)
                    {
                        std::array<int, NUMBER_OF_CHANNELS> arr;
                        for (int ch = 0; ch < NUMBER_OF_CHANNELS; ch++)
                        {
                            arr[ch] = arr[ch] = blackLevelDataNative->black_level_luts[k]->black_level[ch];
                        }
                        blackLevelLut.push_back(arr);
                        //if the vector does not contains the exposure, add if to the vector
                        if (std::find(exposureTimeLut.begin(), exposureTimeLut.end(), blackLevelDataNative->black_level_luts[k]->exposure_time) == exposureTimeLut.end())
                        {
                            exposureTimeLut.push_back(blackLevelDataNative->black_level_luts[k]->exposure_time);
                        }
                    }
                }

            }
        }
    }

    if (exposureTimeLut.size() == 1)
    {
        for (int i = 0; i < NUMBER_OF_CHANNELS; i++)
        {
            output[i] = blackLevelLut[0][i];
        }
        return;
    }
    else
    {

        std::vector<double>::iterator iterMax = std::max_element(exposureTimeLut.begin(), exposureTimeLut.end());
        std::vector<double>::iterator iterMin = std::min_element(exposureTimeLut.begin(), exposureTimeLut.end());

        for (int k = 0; k < exposureTimeLut.size(); k++)
        {
            if (exposureTime <= *iterMax && exposureTime >= *iterMin) //interpolate
            {
                //find which points you need to interpolate
                if (exposureTime >= exposureTimeLut[k] && exposureTime <= exposureTimeLut[k + 1])
                {
                    for (int ch = 0; ch < NUMBER_OF_CHANNELS; ch++)
                    {
                        //y = y0 + (y1-y0)*(x-x0)/(x1-x0);
                        double y0 = blackLevelLut[k][ch];
                        double y1 = blackLevelLut[k + 1][ch];
                        double x = exposureTime;
                        double x0 = exposureTimeLut[k];
                        double x1 = exposureTimeLut[k + 1];
                        output[ch] = y0 + (y1 - y0)*(x - x0) / (x1 - x0);
                    }
                    return; //break the loop
                }
            }
            else //extrapolate nearest neighbor
            {
                for (int ch = 0; ch < NUMBER_OF_CHANNELS; ch++)
                {
                    if (exposureTime > *iterMax)
                    {
                        output[ch] = blackLevelLut[exposureTimeLut.size() - 1][ch];
                    }
                    else
                    {
                        output[ch] = blackLevelLut[0][ch];
                    }
                }
                return;
            }
        }
    }
}

void Preprocessor::SeparateChannels(uint16_t* _image, std::vector<uint16_t>& gr, std::vector<uint16_t>& r, std::vector<uint16_t>& b, std::vector<uint16_t>& gb, int width, int height, int colorOrder)
{
    //swith case the color Order
    int counter_R = 0;
    int counter_GR = 0;
    int counter_GB = 0;
    int counter_B = 0;

    switch (colorOrder)
    {

        //grbg
    case 0:
        for (int i = 0; i < height; i++)
        {
            for (int j = 0; j < width; j++)
            {
                if (i % 2 == 0 && j % 2 == 0)
                {
                    gr[counter_GR] = _image[i*width + j];
                    counter_GR++;
                }
                else if (i % 2 == 0 && j % 2 == 1)
                {
                    r[counter_R] = _image[i*width + j];
                    counter_R++;
                }
                else if (i % 2 == 1 && j % 2 == 0)
                {
                    b[counter_B] = _image[i*width + j];
                    counter_B++;
                }
                else if (i % 2 == 1 && j % 2 == 1)
                {
                    gb[counter_GB] = _image[i*width + j];
                    counter_GB++;
                }
            }
        }
        break;

        //rggb
    case 1:
        for (int i = 0; i < height; i++)
        {
            for (int j = 0; j < width; j++)
            {
                if (i % 2 == 0 && j % 2 == 0)
                {
                    r[counter_R] = _image[i*width + j];
                    counter_R++;
                }
                else if (i % 2 == 0 && j % 2 == 1)
                {
                    gr[counter_GR] = _image[i*width + j];
                    counter_GR++;
                }
                else if (i % 2 == 1 && j % 2 == 0)
                {
                    gb[counter_GB] = _image[i*width + j];
                    counter_GB++;
                }
                else if (i % 2 == 1 && j % 2 == 1)
                {
                    b[counter_B] = _image[i*width + j];
                    counter_B++;
                }
            }
        }
        break;

        //bggr
    case 2:
        for (int i = 0; i < height; i++)
        {
            for (int j = 0; j < width; j++)
            {
                if (i % 2 == 0 && j % 2 == 0)
                {
                    b[counter_B] = _image[i*width + j];
                    counter_B++;
                }
                else if (i % 2 == 0 && j % 2 == 1)
                {
                    gb[counter_GB] = _image[i*width + j];
                    counter_GB++;

                }
                else if (i % 2 == 1 && j % 2 == 0)
                {
                    gr[counter_GR] = _image[i*width + j];
                    counter_GR++;
                }
                else if (i % 2 == 1 && j % 2 == 1)
                {
                    r[counter_R] = _image[i*width + j];
                    counter_R++;
                }
            }
        }
        break;

        //gbrg
    case 3:
        for (int i = 0; i < height; i++)
        {
            for (int j = 0; j < width; j++)
            {
                if (i % 2 == 0 && j % 2 == 0)
                {
                    gb[counter_GB] = _image[i*width + j];
                    counter_GB++;
                }
                else if (i % 2 == 0 && j % 2 == 1)
                {
                    b[counter_B] = _image[i*width + j];
                    counter_B++;
                }
                else if (i % 2 == 1 && j % 2 == 0)
                {
                    r[counter_R] = _image[i*width + j];
                    counter_R++;
                }
                else if (i % 2 == 1 && j % 2 == 1)
                {
                    gr[counter_GR] = _image[i*width + j];
                    counter_GR++;
                }
            }
        }
        break;
    }
}
//function data = process(data)
//    data = medfilt2(data, [7 7], 'symmetric');
//    mask = fspecial('gaussian', [35 35], 12);
//    data = imfilter(data, mask, 'replicate', 'same');
//    maximum = max(data(:));
//    data = 1 ./ ( data/maximum );
//    data(data > 10) = 16;
//end
void Preprocessor::Process(std::vector<uint16_t>& data, int width, int height, std::vector<float>& output)
{
    //the median filter removes noises like salt & pepper// defect pixel correction
    //Gaussion filter smooths the image by blurring it - remember this is a flat field image
    cv::Mat median = cv::Mat::zeros(height, width, CV_32F);
    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            median.at<float>(i, j) = data[i*width + j];
        }
    }
    cv::medianBlur(median, median, 5);//TODO should be 7x7 kernel size  
    cv::GaussianBlur(median, median, cv::Size(35, 35), 12.0, cv::BORDER_REPLICATE);
    double min, max;
    cv::minMaxLoc(median, &min, &max);

    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            output[i*width + j] = 1.0 / (median.at<float>(i, j) / max);
            if (output[i*width + j] > 10)
            {
                output[i*width + j] = 16;
            }
        }
    }
}

void Preprocessor::CombineChannelsTo2dImage(std::vector<float>& gr, std::vector<float>& r, std::vector<float>& b, std::vector<float>& gb, int isOverride, bayer_raw_image* outputImage, std::vector<double> saturationLevel)
{
    //swith case the color Order
    int counter_R = 0;
    int counter_GR = 0;
    int counter_GB = 0;
    int counter_B = 0;

    //this is the avoid redundent casting
    float saturationLevel0 = saturationLevel[0];
    float saturationLevel1 = saturationLevel[1];
    float saturationLevel2 = saturationLevel[2];
    float saturationLevel3 = saturationLevel[3];
    float tempCalc;
    //there are 2 options 4d or 2d(image)
    if (isOverride == 1)
    {
        switch (outputImage->order)
        {
            //grbg
        case 0:
            for (int i = 0; i < outputImage->height; i++)
            {
                for (int j = 0; j < outputImage->width; j++)
                {
                    if (i % 2 == 0 && j % 2 == 0)
                    {
                        tempCalc = std::min(gr[counter_GR], saturationLevel0);
                        tempCalc = MatlabRound(tempCalc);
                        tempCalc = std::max(tempCalc, 0.0f);
                        outputImage->raw_data[i*outputImage->width + j] = tempCalc;
                        counter_GR++;
                    }
                    else if (i % 2 == 0 && j % 2 == 1)
                    {
                        tempCalc = std::min(r[counter_R], saturationLevel1);
                        tempCalc = MatlabRound(tempCalc);
                        tempCalc = std::max(tempCalc, 0.0f);
                        outputImage->raw_data[i*outputImage->width + j] = tempCalc;
                        counter_R++;
                    }
                    else if (i % 2 == 1 && j % 2 == 0)
                    {
                        tempCalc = std::min(b[counter_B], saturationLevel2);
                        tempCalc = MatlabRound(tempCalc);
                        tempCalc = std::max(tempCalc, 0.0f);
                        outputImage->raw_data[i*outputImage->width + j] = tempCalc;
                        counter_B++;
                    }
                    else if (i % 2 == 1 && j % 2 == 1)
                    {
                        tempCalc = std::min(gb[counter_GB], saturationLevel3);
                        tempCalc = MatlabRound(tempCalc);
                        tempCalc = std::max(tempCalc, 0.0f);
                        outputImage->raw_data[i*outputImage->width + j] = tempCalc;
                        counter_GB++;
                    }
                }
            }
            break;
            //rggb
        case 1:
            break;

            //bggr
        case 2:
            break;
            //gbrg
        case 3:
            break;
        default:
            break;
        }
    }
    else
    {
        //need to implement the no override 
    }
}

void Preprocessor::ScaleLscGrid(std::vector<float>& gr, std::vector<float>& r, std::vector<float>& b, std::vector<float>& gb, uint16_t height, uint16_t width, uint16_t desiredWidth, uint16_t desiredHeight, std::string method = "cubic")
{
    if (height == desiredWidth && width == desiredWidth)
    {
        return;
    }

    double scaleLimit = 10.5;
    double scaleFactor = floor(scaleLimit);
    double widthTemp;
    double heightTemp;
    if ((desiredWidth / (double)width > scaleLimit) || desiredHeight / (double)height > scaleLimit)
    {
        widthTemp = (width * scaleFactor) + 1;
        heightTemp = (height * scaleFactor) + 1;
        //to do Grid...
    }
}

void Preprocessor::ApplyLensShadingCorrection(std::vector<float>& floatChannel_gr, std::vector<float>& floatChannel_r, std::vector<float>& floatChannel_b, std::vector<float>& floatChannel_gb,
    std::vector<uint16_t>& channel_gr_cc, std::vector<uint16_t>& channel_r_cc, std::vector<uint16_t>& channel_b_cc, std::vector<uint16_t>& channel_gb_cc, uint16_t channelCCWidth, uint16_t channelCCHeight)
{
    for (int i = 0; i < channelCCHeight; i++)
    {
        for (int j = 0; j < channelCCWidth; j++)
        {
            floatChannel_gr[i*channelCCWidth + j] = floatChannel_gr[i*channelCCWidth + j] * channel_gr_cc[i*channelCCWidth + j];
            floatChannel_r[i*channelCCWidth + j] = floatChannel_r[i*channelCCWidth + j] * channel_r_cc[i*channelCCWidth + j];
            floatChannel_b[i*channelCCWidth + j] = floatChannel_b[i*channelCCWidth + j] * channel_b_cc[i*channelCCWidth + j];
            floatChannel_gb[i*channelCCWidth + j] = floatChannel_gb[i*channelCCWidth + j] * channel_gb_cc[i*channelCCWidth + j];
        }
    }
}

int Preprocessor::MatlabRound(double numberToRound)
{
    return (int)floor(numberToRound + 0.5);
}

void Preprocessor::ShowImage(bayer_raw_image* colorCheckerImage, std::string title)
{
    cv::Mat image(colorCheckerImage->height, colorCheckerImage->width, CV_32F);
    for (int i = 0; i < colorCheckerImage->height; i++)
    {
        for (int j = 0; j < colorCheckerImage->width; j++)
        {
            image.at<float>(i, j) = colorCheckerImage->raw_data[i*colorCheckerImage->width + j] / 1023.0;
        }
    }
    /*cv::imshow(title, image);
    cv::waitKey(0);*/
    int sizeOfChannel = (colorCheckerImage->width / 2) * (colorCheckerImage->height / 2);
    std::vector<uint16_t> channel_r_cc(sizeOfChannel);
    std::vector<uint16_t> channel_gr_cc(sizeOfChannel);
    std::vector<uint16_t> channel_gb_cc(sizeOfChannel);
    std::vector<uint16_t> channel_b_cc(sizeOfChannel);
    //for debug

    SeparateChannels(colorCheckerImage->raw_data, channel_gr_cc, channel_r_cc, channel_b_cc, channel_gb_cc, colorCheckerImage->width, colorCheckerImage->height, colorCheckerImage->order);
    cv::Mat src = cv::Mat::zeros(colorCheckerImage->height / 2, colorCheckerImage->width / 2, CV_32FC3);
    for (int w = 0; w < colorCheckerImage->width / 2; w++)
    {
        for (int h = 0; h < colorCheckerImage->height / 2; h++)
        {
            src.at<cv::Vec3f>(h, w)[0] = channel_b_cc[h*(colorCheckerImage->width / 2) + w];
            src.at<cv::Vec3f>(h, w)[1] = (channel_gr_cc[h*(colorCheckerImage->width / 2) + w] + channel_gb_cc[h*(colorCheckerImage->width / 2) + w]) / 2.0;
            src.at<cv::Vec3f>(h, w)[2] = channel_r_cc[h*(colorCheckerImage->width / 2) + w];
        }
    }

    src = src / 1023.0;
    cv::imshow(title + " in rgb", src);
    cv::waitKey(0);
}

void Preprocessor::ShowImage(std::vector<float> channel, uint16_t width, uint16_t height, std::string title)
{
    cv::Mat image(height, width, CV_32F);
    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            image.at<float>(i, j) = channel[i*width + j] / 1023.0;
        }
    }
    cv::imshow(title, image);
    cv::waitKey(0);
}
static uint16_t Clip(uint16_t data, uint16_t min, uint16_t max)
{
    return std::min(std::max(data, min), max);
}
\$\endgroup\$
  • \$\begingroup\$ Since when are "coding conventions" about what data structure to use or performance in general? Aren't they about how to capitalise names of class members, how to order constructor, getters and other methods in a class block, etc? \$\endgroup\$ – I'll add comments tomorrow Sep 3 '16 at 17:54
  • \$\begingroup\$ @Jamal is this better title? \$\endgroup\$ – Gilad Sep 3 '16 at 18:16
  • \$\begingroup\$ @I'lladdcommentstomorrow I get what you are trying to say, I care about naming convention as well, however they are not the main focus of my question, maybe I should have called it C++ best common/programming practices? \$\endgroup\$ – Gilad Sep 3 '16 at 18:19
  • \$\begingroup\$ That would be waaaay to broad. We can review specific code and you can find a lot of the best practices mentioned in answers on this site, but don't turn this into a list question. \$\endgroup\$ – Mast Sep 3 '16 at 18:29
  • \$\begingroup\$ I also suspect your code is not yet working as intended. Could you clarify this? \$\endgroup\$ – Mast Sep 3 '16 at 18:30
5
\$\begingroup\$

Preprocessor.h

using namespace std;

is always bad style in C++, but especially when you do it in a header (.h) file, because of how C++ handles #include via textual inclusion. When you using namespace std; in a header file, you're forcing that decision on every .cc file that includes your header, which can often result in changed or confusing semantics in those .cc files.

Never use using namespace... in a .h file. Prefer not to use it in a .cc file. Prefer to spell out std::vector, std::sort, et cetera, on every reference, so that your code is clear to the local (human) reader as well as to the compiler.


typedef class TOOLBOX_EXPORT
{
public:
    uint16_t width;
    uint16_t height;
    color_order order;
    uint16_t* raw_data;
    uint16_t bit_depth;
} bayer_raw_image;

Your style here is highly unusual. First of all, you should be aware that in C++, a class is just like a struct in most languages, except that class members (and bases) are private by default instead of public by default. So, instead of writing class Foo { public: ..., it is often more readable to write struct Foo { ....

Also be aware that in C++, unlike in C, you can refer to any type Foo directly; you don't have to qualify the type's name with struct Foo, enum Foo, etc. This means that the C practice of typedef struct _Foo { ... } Foo; is frowned upon in C++.

Lastly, notice that your struct members will be laid out in order; so you could eliminate some padding bytes by shuffling them differently. Result:

struct TOOLBOX_EXPORT bayer_raw_image {
    uint16_t width;
    uint16_t height;
    uint16_t* raw_data;
    uint16_t bit_depth;
    color_order order;
};

I strongly suspect that class Preprocessor should actually be namespace Preprocessor; a class with no data members is highly suspicious. Remember that in C++, unlike Java, it's perfectly fine and normal to have "free functions" existing outside of any class.


Preprocessor.cpp

for (int i = 0; i < channelCCHeight; i++)
{
    for (int j = 0; j < channelCCWidth; j++)
    {
        floatChannel_gr[i*channelCCWidth + j] = floatChannel_gr[i*channelCCWidth + j] * channel_gr_cc[i*channelCCWidth + j];
        floatChannel_r[i*channelCCWidth + j] = floatChannel_r[i*channelCCWidth + j] * channel_r_cc[i*channelCCWidth + j];
        floatChannel_b[i*channelCCWidth + j] = floatChannel_b[i*channelCCWidth + j] * channel_b_cc[i*channelCCWidth + j];
        floatChannel_gb[i*channelCCWidth + j] = floatChannel_gb[i*channelCCWidth + j] * channel_gb_cc[i*channelCCWidth + j];
    }
}

You should know that in basically all curly-brace languages, x = x * y can be rewritten as x *= y.

In C++, operators can be overloaded; including the ++ prefix and postfix operators. The prefix version means "increment my value and return me"; the postfix version means "copy me, increment my value, and return the copy." Therefore, in general prefix ++ will perform fewer operations and be more optimizable than postfix ++ (and the same goes for --). It certainly won't matter for primitive types like int, but if you get in the habit of writing ++it in general, then you'll never have to stop and think about this issue ever again. Result:

for (int i = 0; i < channelCCHeight; ++i) {
    for (int j = 0; j < channelCCWidth; ++j) {
        floatChannel_gr[i*channelCCWidth + j] *= channel_gr_cc[i*channelCCWidth + j];
        floatChannel_r[i*channelCCWidth + j] *= channel_r_cc[i*channelCCWidth + j];
        floatChannel_b[i*channelCCWidth + j] *= channel_b_cc[i*channelCCWidth + j];
        floatChannel_gb[i*channelCCWidth + j] *= channel_gb_cc[i*channelCCWidth + j];
    }
}

No different in the code generated, but significantly easier on the eyes, and there's several fewer places for typos to lurk. You could make it even simpler, of course:

for (int i = 0; i < channelCCHeight * channelCCWidth; ++i) {
    floatChannel_gr[i] *= channel_gr_cc[i];
    floatChannel_r[i] *= channel_r_cc[i];
    floatChannel_b[i] *= channel_b_cc[i];
    floatChannel_gb[i] *= channel_gb_cc[i];
}

Again, this shouldn't result in any better codegen (for any halfway decent compiler); but your human reader will thank you.


void Preprocessor::ShowImage(std::vector<float> channel, uint16_t width, uint16_t height, std::string title)
{
    cv::Mat image(height, width, CV_32F);
    for (int i = 0; i < height; i++)
    {
        for (int j = 0; j < width; j++)
        {
            image.at<float>(i, j) = channel[i*width + j] / 1023.0;
        }
    }
    cv::imshow(title, image);
    cv::waitKey(0);
}

The expression channel[i*width + j] / 1023.0 struck me as odd. For one thing, 1023.0 is a double; if you meant to do the arithmetic in single-precision, you should have said 1023.0f or simply 1023 (which would be an int, which would promote to double).

For another thing, I think that if you're trying to convert an integer in the range [0,1023] to a float in the range [0,1], that expression isn't what you want; for the same reason that if you were trying to convert a float in [0,1] to an integer in [0,1023], simply multiplying by 1023 wouldn't be what you want. I would think you'd want some rounding and +0.5'ing in the latter case, and therefore also in the former. I'm not sure, though.

Lastly but most importantly in terms of your growing C++ intuition: See that vector<float> parameter? You're passing it by value, which means "by copy" — similar to how when you pass an int to a C# function, it gets a copy of the original value, rather than a reference to the original itself. Making a copy of an int is cheap, but copying a whole vector (or a whole string) is expensive. You should explicitly pass large arguments by reference in C++, and then also mark them const to indicate that you promise not to modify them.

Passing uint16_t instead of int buys you nothing — except of course grief the first time someone accidentally passes -1 and it's silently converted to 65535!

Result:

void Preprocessor::ShowImage(const std::vector<float>& channel, int width, int height, const std::string& title)
{
    cv::Mat image(height, width, CV_32F);
    for (int i = 0; i < height; ++i) {
        for (int j = 0; j < width; ++j {
            image.at<float>(i, j) = channel[i*width + j] / 1023;
        }
    }
    cv::imshow(title, image);
    cv::waitKey(0);
}

Your function CombineChannelsTo2dImage is on its way to becoming an unmaintainable mess. Rather than write out every case by hand, you should look for a way to factor out the "differences" between the cases into small functions; then assign one of those functions to a local variable f and write the "samenesses" just once, in terms of f.


Your function Function1 (terrible name, btw) spends much too much time validating its argument values. Instead of validating arguments at runtime, you should strive to make them validate at compile-time, by using C++'s rich type system.

For example, anytime you start writing a check of the form

if (rawBayerImage == NULL)
{
    throw "Missing image";
}

that should be a sign that you're using the wrong type for rawBayerImage. Instead of taking a bayer_raw_image * (which can be null), prefer to take a bayer_raw_image& (which cannot be null). In other words, a C++ reference is very similar to a non-nullable pointer. (And if you need actual non-nullable pointers, check out the GSL's not_null template.)

Also, remember to const-qualify your pointers and references to indicate that you're not modifying their pointees; and remember that saturationLevel should be passed by const reference, not by value.

Hope this helps!

\$\endgroup\$
  • \$\begingroup\$ One of the best code review I have had this past year, thank you very much I learned a lot from it. \$\endgroup\$ – Gilad Sep 10 '16 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.