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  maxBinValue)
        {
            maxBinValue = histogramBins[index];
        }
    }

    maxBinValue = maxBinValue == 0 ? 1 : maxBinValue;

    // Normalize to fit into the histogram UI canvas height

    for (int index = 0; index < 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



• 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.

StackExchange Code Review Q#47566, answer score: 5