Optimization for histogram computation algorithm in C Next is an algorithm to calculate the histogram for a given YUV420sp image.  The YUV data is given by the hardware camera as an unsigned char*.

The algorithms works correctly, but I'm looking to optimize it for speed.

void calculateHistogram(const unsigned char* yuv420sp, const int yuvWidth, const int yuvHeight, 
                        const int canvasHeight, int* histogramBins)
{
    const int BINS = 256;

    // Clear the output

    memset(histogramBins, 0, BINS * sizeof(int));

    // Get the bin values

    const unsigned int totalPixels = yuvWidth * yuvHeight;

    for (int index = 0; index &lt; totalPixels; index++)
    {
        histogramBins[yuv420sp[index]]++;
    }

    // Resolve the maximum count in bins

    int maxBinValue = 0;

    for (int index = 0; index &lt; BINS; index++)
    {
        if (histogramBins[index] &gt; maxBinValue)
        {
            maxBinValue = histogramBins[index];
        }
    }

    maxBinValue = maxBinValue == 0 ? 1 : maxBinValue;

    // Normalize to fit into the histogram UI canvas height

    for (int index = 0; index &lt; BINS; index++)
    {
        histogramBins[index] = histogramBins[index] * canvasHeight / maxBinValue;
    }
}
Style

Everything looks quite good to me.

The only thing that I find a bit disturbing is the empty line after comments : it split relevant things apart and it makes the code harder to read.

You could add a bit of documentation explaining what your code does.



Making things more simple

After the for-loop to compute the max value, you perform maxBinValue = maxBinValue == 0 ? 1 : maxBinValue;. As far as I can understand, the point is not to have a division by 0 if max is 0 which corresponds to a situation where histogramBins consist only of 0s which should happen only of totalPixels &lt;= 0 which is quite unlikely.

In any case :


I find the usage of a ternary operator a bit inappropriate here as it might as well be a simple : if (maxBinValue == 0) maxBinValue = 1;. Alternatively, if you were to define functions (or macros) for min and max, this could be : maxBinValue = max(1, maxBinValue).
You could get rid of this is a very very simple way : if you were to assign 1 to maxBinValue in the first place (before the loop), you wouldn't have to worry about it being equal to 0.


Please note that if you were to define a max function/macro, it could be useful inside the loop too as it would become : maxBinValue = max(histogramBins[index],maxBinValue).



Getting rid of magic numbers

This 256 in const int BINS = 256; definitly sounds like a value I've seen in other places. Indeed, it is not just a random setting I can update if I ever want to : it corresponds to the maximum value yuv420sp[index] can take. Its type is unsigned char and the maximum value can be retrieved from limits.h : doing const int BINS = UCHAR_MAX would probably make things easier to understand.