3
\$\begingroup\$

What i have so far is a nice and rapid algorithm for finding the pixel dominant from bmp image. I am not sure how to make it to work.. i though it works, but the program freezes.

RGB
bitfox_get_primecolor_direct
(char *FILE_NAME)
{
    RGB primecolor;
    BYTE rgb[3];

    dword *counts = calloc(pow(256, 3), sizeof(*counts));
    dword max_count = 0;

    FILE* fp = fopen(convert(FILE_NAME), "rb");

    fseek(fp, 54, SEEK_SET);
    while (fread (rgb, sizeof(BYTE), 3, fp) != EOF)
    {
        dword idx = (((dword)rgb[0]) << 16) | (((dword)rgb[1]) << 8) | (dword)rgb[2];
        if (++counts[idx] > max_count) max_count = idx;
    }

    primecolor.R = (max_count >> 16) & 0xFF;
    primecolor.G = (max_count >> 8) & 0xFF;
    primecolor.B = max_count & 0xFF;

    free(counts);
    fclose(fp);
    return primecolor;
}

BYTE is unsigned char

RGB is struct containg BYTE R, G and B.

\$\endgroup\$
2
  • \$\begingroup\$ Not a C expert, but the sizeof char is always equal to 1 by definition, and this is interpreted by the C compiler to be one byte, by definition. However, this does not mean that the byte contains 8 bits, just a minimum of 8. So it is possible that << 16, >> 16, << 8 and >> 8 are not behaving as expected. But again, I'm no expert. See this discussion on Stack Overflow: stackoverflow.com/questions/2215445/… \$\endgroup\$ – ben rudgers Oct 23 '14 at 18:39
  • \$\begingroup\$ 1 byte in my case is 8 bits. I have been messing with bit functions and stuff on my PC from long ago. \$\endgroup\$ – Edenia Oct 23 '14 at 18:44
6
\$\begingroup\$

There are numerous problems with the code. First of all, fread never returns EOF (which is -1). The loop should end when it reads less than 3 bytes.

Second, max_count is index, not a color value. I presume you meant rgb[max_count].

Test a file pointer returned by fopen for not being NULL.

Test a return value of fseek.

Avoid magic numbers. I suppose 54 is a size of certain header. Create a constant with a meaningful name. Better yet, define a header structure and take its sizeof.

\$\endgroup\$
2
  • \$\begingroup\$ 54 is the offset of the pixel array of a bmp image. 54+ are pixels only. \$\endgroup\$ – Edenia Oct 23 '14 at 19:38
  • 1
    \$\begingroup\$ I missed the max_count issue. Good catch! \$\endgroup\$ – Edward Oct 23 '14 at 19:45
6
\$\begingroup\$

I see some things here that could help you improve your code.

Provide a full working example

This comment is really more about how to get a good code review rather than about the code itself. The function as it is posted won't compile without supplying a number of missing things, including the appropriate #includes, typedefs for BYTE and dword, a typedef for a struct RGB, the function convert(), etc.

It would be easier to provide a good review if these things were supplied.

Check your return values

The code calls fopen and then immediately does an fseek. What if either of those calls fails? What if calloc fails? The code doesn't currently check the return value, which could indicate a failure. Better would be to check the return value and do the appropriate thing -- possibly by returning early.

Check fread's return value

The code does check the return value of fread but it's the wrong one. Specifically, fread returns the number of items read, and so should be compared to the number of items that were attempted. With that said, the line currently says:

while (fread (rgb, sizeof(BYTE), 3, fp) != EOF)

It might be better to write it this way:

while (fread (rgb, sizeof(rgb), 1, fp) == 1)

The difference is that instead of indicating that you're reading three BYTEs, this says that you're reading one rgb which is really what you're doing. It also means that if the definition of rgb ever changes, you'll only have to change it once and this code will automatically get the right size for rgb.

Be careful with index arrays

We don't know how dword is defined, but we can assert that [] only operates on int index values. On a system in which dword is defined as, say, unsigned long but an int is shorter, this will cause a problem with this code. You might want to add an assert to make sure that your indexing will actually work.

Eliminate runtime calculation

The pow(256, 3) is executed at runtime every time this function is called. Since it's simply 256*256*256, one could just as easily make that a const value that is calculated at compile time, saving a little time and code.

Eliminate "magic numbers"

Things like (maxcount >> 16) & 0xFF are OK, because programmers reading this will know what that code is doing. However, when you write fseek(fp, 54, SEEK_SET) it's not at all obvious whether 54 is the correct value, or exactly what it represents. Better would be to use a named const value there such as const long HEADER_SIZE = 54;

Consider optimizing the data structure

Right now the code allocates and deallocates 16M each time the routine is called. That might not be a problem on your machine, but it's likely that it could be optimized for size and probably speed. Since this is likely to be a sparse array (that is, most values will contain 0), you might consider storing only non-zero values, perhaps in a tree structure so that the top value is always at the root of the tree. This will probably save memory and will likely also be faster since more of the data structure will be in the local cache.

\$\endgroup\$

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.