2
\$\begingroup\$

I wrote a function to replace color in a bitmap image. For beginners in image processing, it could be exclusively useful, since they could use it to replace the entire gamma of a color or to replace a color with gradient/abstract lines, following the function's logic.

The main problem of the function, according to performance, is that it isn't fast enough. For larger images/replacing more then one color, this could take a minute or even more. I am sure there is a huge room for improvement of this function, therefore speed-optimized.

#include <stdio.h>
#include <stdlib.h>

#define IMAGE_WIDTH         500
#define IMAGE_OFFSET_SIZE   2
#define IMAGE_OFFSET_PIXELS 54

typedef unsigned char byte;
typedef unsigned long dword;

int main (int argc, char *argv[])
{
    dword bitmap_color_replace (char *, byte, byte, byte, byte, byte, byte);

    printf("Pixels successfully replaced: %i\n",
    bitmap_color_replace("sample.bmp", 255, 255, 255, 0, 200, 255));

    return EXIT_SUCCESS;
}

/**************************************************************************/

dword
bitmap_color_replace
(char *file_name, byte old_r, byte old_g, byte old_b, byte new_r, byte new_g, byte new_b)
{
    dword fp_height = 0;
    dword fp_size;
    FILE* fp = fopen(file_name, "r+b");
    dword i, pixels_replaced = 0;

    byte new_color[3];

    new_color[0] = new_b;
    new_color[1] = new_g;
    new_color[2] = new_r;

    fseek(fp, IMAGE_OFFSET_SIZE, SEEK_SET);
    fp_size = fgetc(fp) + fgetc(fp)*256 + fgetc(fp)*(256*256) + fgetc(fp)*(256*256*256);

    for(i = IMAGE_OFFSET_PIXELS; i < fp_size; i += sizeof(new_color))
    {
        byte current_r, current_g, current_b;

        fseek(fp, i, SEEK_SET);

        if(i == IMAGE_OFFSET_PIXELS + (IMAGE_WIDTH * (fp_height+1))) // skip padding
        {
            fp_height++;
            i += IMAGE_WIDTH % 4;
            fseek(fp, IMAGE_WIDTH % 4, SEEK_CUR);
        }

        current_b = fgetc(fp);
        current_g = fgetc(fp);
        current_r = fgetc(fp);

        if(current_b == old_b && current_g == old_g && current_r == old_r)
        {
            fseek(fp, i, SEEK_SET);
            fwrite(new_color, sizeof(new_color), 1, fp);
            pixels_replaced++;
        }
    }

    fclose(fp);
    return pixels_replaced;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Have you considered loading the whole file or at least chunks of it in memory, doing the replacement there and then saving the results, all in as few I/O operations as possible? \$\endgroup\$ – user29120 Nov 13 '14 at 16:23
  • \$\begingroup\$ I did.. but I also need it directly. \$\endgroup\$ – Edenia Nov 13 '14 at 17:09
  • \$\begingroup\$ And have you actually ran some benchmarks to see if the I/O isn't your bottleneck? Because if it is, then you're not in luck. \$\endgroup\$ – user29120 Nov 13 '14 at 17:33
  • 1
    \$\begingroup\$ Perhaps benchmarking was not the most appropriate term. Profiling would have probably been better? \$\endgroup\$ – user29120 Nov 13 '14 at 17:43
  • 1
    \$\begingroup\$ Why is it securer? fwrite and fgetc are both buffered so I don't see the difference. You could use 'mmap' instead of file io if you care about memory usage. I think the main thing is that you didn't list all your requirements in the question. You only stated that you wanted your code to run faster. I'm still not clear on what you are "securing" against. Are multiple processes operating on these files? Are you afraid of running out of memory? \$\endgroup\$ – JS1 Nov 15 '14 at 21:26
2
\$\begingroup\$

I see some ways in which this code might be improved.

Make sure printf format specifiers match the data

In the sample main, the format specifier used is "%i" but bitmap_color_replace returns a dword which is an alias for unsigned long, so the correct format specifier would actually be "%lu" for that data.

Omit return statement in main

This line

return EXIT_SUCCESS;

is not needed with modern C compilers. Getting to the end of main implicitly means that the program has succeeded, so the compiler will include code to do that return if you simply omit it.

Rearrange the code to reduce the need for declarations

A function declaration is only needed by the compiler if it hasn't already encountered the function itself. With that said, if the bitmap_color_replace function is moved to the top of the file, above main, the function declaration within main can be omitted, making main a single printf.

Omit unused variables

Because argc and argv are unused, you could use the alternative form of main:

int main ()

Avoid hardcoding values

The IMAGE_WIDTH, IMAGE_OFFSET_PIXELS are not valid for most images. Rather than hardcode those, you could simply read them from the file header in the same way that fp_size is read.

Avoid uneccessary fseek calls

There are a lot of fseek calls within the current routine which are not necessary because the program moves linearly through the data anyway. Eliminating them would likely speed up the program.

Calculate pad size just once

The padding size is the same for every row so you can calulate it based on the image width and the size of each color.

Process only the pixel data

In some version of BMP files, there is data after the pixel data, so the current code, which continues to the end of the physical file, would overwrite that data. Better would be to calculate just the size of the pixel data which is obtained from the height and width of the image.

Don't overwrite the input file

Create a new file instead of overwriting the input file. This helps the user as well as preventing file corruption if something goes wrong during the processing of the file. It may also speed the program.

Process larger chunks at a time in memory

Instead of reading the colors byte at a time, it would likely speed things up considerably by processing entire rows at a time, or even the entire file as a memory image. I/O is often the bottleneck for programs like this, so minimizing it can result in considerable time savings.

Note the correct data order

BMP files contain pixel data as R,G,B in that order. It's important to get the order right to correctly match colors.

One possibility

Here's a version of the code with some, but not all of these ideas incorporated:

// these are absolute offsets to the two most important 
// items in the BMP file 
#define IMAGE_PIXEL_START   10
#define IMAGE_WIDTH         18

typedef unsigned char byte;
typedef unsigned long dword;

dword
bitmap_color_replace
(char *file_name, byte old_r, byte old_g, byte old_b, byte new_r, byte new_g, byte new_b)
{
    FILE* fp = fopen(file_name, "r+b");
    dword pixels_replaced = 0;

    byte new_color[3];

    new_color[0] = new_r;
    new_color[1] = new_g;
    new_color[2] = new_b;

    byte old_color[3];

    fseek(fp, IMAGE_PIXEL_START, SEEK_SET);
    dword fp_pixelstart = fgetc(fp) + fgetc(fp)*256 + fgetc(fp)*(256*256) + fgetc(fp)*(256*256*256);
    fseek(fp, IMAGE_WIDTH, SEEK_SET);
    dword fp_width = fgetc(fp) + fgetc(fp)*256 + fgetc(fp)*(256*256) + fgetc(fp)*(256*256*256);
    dword fp_height = fgetc(fp) + fgetc(fp)*256 + fgetc(fp)*(256*256) + fgetc(fp)*(256*256*256);
    // rows are padded to a multiple of 4 bytes
    dword padsize = (4 - ( (sizeof(new_color) * fp_width) & 0x3 )) & 0x3;

    fseek(fp, fp_pixelstart, SEEK_SET);
    for (int row = fp_height; row; --row) 
    {
        for (int col = fp_width; col; --col) 
        {
            fread(old_color, sizeof(old_color), 1, fp);
            if(    old_color[0] == old_r 
                && old_color[1] == old_g 
                && old_color[2] == old_b)
            {
                fseek(fp, -sizeof(old_color), SEEK_CUR);
                fwrite(new_color, sizeof(new_color), 1, fp);
                pixels_replaced++;
            }
        }
        if (padsize)
        {
            fseek(fp, padsize, SEEK_CUR);
        }
    }
    fclose(fp);
    return pixels_replaced;
}
|improve this answer|||||
\$\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.