1
\$\begingroup\$

I've two different types of variables: cv::Mat and array of type double[2] so I do quite lot of copying between the two types. I feel like my code isn't written well and could be improved, I'm trying to avoid lot of copying and get rid of all unuseful calculations.

I'm using fftw because it is known that it is very fast computing the fft. So making the algorithm run faster is also important for me as it will be performed in real time applications.

The code below is part of the algorithm where I'm using fftw library to perform FFT on images

//Copying my normalized image to data_in of type double[2]
for(auto j=0, k = 0;j<src.rows;j++)
        for(auto i=0;i<src.cols;i++)
        {
            data_in[k][0] = (double) src.at<float>(j,i)/A;
            data_in[k][1] = 0.0;
            k++;
        }

    //FFT of data_in and store it in double[2] fft variable
    fftw_execute( plan_data_in );

    //product of double[2] fft and cv::Mat kernel_fft and store it in double[2] fftPro
for(auto j=0, k = 0;j<src.rows;j++)
        for(auto i=0;i<src.cols;i++)
        {
            fftPro[k][0] = fft[k][0] * kernel_fft.at<cv::Vec2f>(j,i)[0] - fft[k][1] * kernel_fft.at<cv::Vec2f>(j,i)[1];
            fftPro[k][1] = fft[k][0] * kernel_fft.at<cv::Vec2f>(j,i)[1] + fft[k][1] * kernel_fft.at<cv::Vec2f>(j,i)[0];             
            k++;
        }

///////////// FFT inverse    
    // FFT inverse of double[2] fftPro ans stor in double[2] data_out
    fftw_execute( plan_data_out );

    //Normalization
    for(auto i = 0 ; i < ( kernel_fft.cols * kernel_fft.rows ) ; i++ )
        data_out[i][0] /= ( double )( kernel_fft.cols * kernel_fft.rows );

    //fft to mat
    cv::Mat ifftMat=cv::Mat::zeros(src.rows, src.cols, CV_32FC1);
    for(auto j=0, k = 0;j<kernel_fft.rows;j++)
        for(auto i=0;i<kernel_fft.cols;i++)
            ifftMat.at<float>(j,i)=data_out[k++][0];

Could you spot any part could be improved in this code?

EDIT:

I didn't share all the code, if you want it all I can share it but I wanted to be specific and not share all the code that takes time to read and understand. This is all declarations:

data_in = ( fftw_complex* )fftw_malloc( sizeof( fftw_complex ) * kernel_fft.cols * kernel_fft.rows );

fft     = ( fftw_complex* )fftw_malloc( sizeof( fftw_complex ) * kernel_fft.cols * kernel_fft.rows );

fftPro     = ( fftw_complex* )fftw_malloc( sizeof( fftw_complex ) * kernel_fft.cols * kernel_fft.rows );    

data_out    = ( fftw_complex* )fftw_malloc( sizeof( fftw_complex ) * kernel_fft.cols * kernel_fft.rows );


plan_data_in = fftw_plan_dft_1d( kernel_fft.cols * kernel_fft.rows, data_in, fft,  FFTW_FORWARD,  FFTW_ESTIMATE );

plan_data_out = fftw_plan_dft_1d( kernel_fft.cols * kernel_fft.rows, fftPro, data_out, FFTW_BACKWARD, FFTW_ESTIMATE );
\$\endgroup\$
  • \$\begingroup\$ This code doesn't provide declarations for plan_data_in and plan_data_out. Values are assigned to the data_in variable but never used. If you follow the Single Responsibility Principle then I see 3 separate functions here. plan_data_in and plan_data_out are used but never assigned values. I think the code is broken. \$\endgroup\$ – pacmaninbw Dec 1 '17 at 17:47
  • 1
    \$\begingroup\$ @pacmaninbw I just edited my question \$\endgroup\$ – Ja_cpp Dec 1 '17 at 19:12
  • 1
    \$\begingroup\$ In your normalization loop some compilers may optimize ( double )( kernel_fft.cols * kernel_fft.rows ) but not all will. You might want to create a const double and assign ( double )( kernel_fft.cols * kernel_fft.rows ) before the loop and use it in the loop. Remember to move your loop invariants out of the loop. \$\endgroup\$ – pacmaninbw Dec 2 '17 at 0:38

Your Answer

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

Browse other questions tagged or ask your own question.