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
typedef for a
struct RGB, the function
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.
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
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
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"
(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.