Filter: BMP Image Filtering Tool - follow-up

Question

This is a follow-up of this quesiton: Filter: BMP Image Filtering Tool

Changes:

• Added comments where necessary.
• Added checks for integer overflow.
• Added support for input and output redirection.
• Added support for more than one filter in a single invocation.
• Eliminated casts, magic numbers, and Windows typedefs for standard types.
• Changed the algorithm for blurring the image.
• Some other points raised by the reviewers.

Review Goals:

• Should the functions doing the input/output be moved to bmp.c?
• Style, potential undefined behavior, et cetera.

Code:

bmp.h:

#ifndef BMP_H
#define BMP_H

/* BMP-related data types based on Microsoft's own. */

#include <stdbool.h>
#include <stdint.h>

/* The BITMAPFILEHEADER structure contains information about the type, size,
 * and layout of a file that contains a DIB [device-independent bitmap].
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183374(VS.85).aspx.
 */
typedef struct {
    uint16_t bf_type;
    uint32_t bf_size;
    uint16_t bf_reserved1;
    uint16_t bf_reserved2;
    uint32_t bf_offbits;
} BITMAPFILEHEADER;

/* The BITMAPINFOHEADER structure contains information about the
 * dimensions and color format of a DIB [device-independent bitmap].
 * Adapted from http://msdn.microsoft.com/en-us/library/dd183376(VS.85).aspx.
 */
typedef struct {
    uint32_t bi_size;
    int32_t bi_width;
    int32_t bi_height;
    uint16_t bi_planes;
    uint16_t bi_bitcount;
    uint32_t bi_compression;
    uint32_t bi_size_image;
    int32_t bi_x_resolution_ppm;
    int32_t bi_y_resolution_ppm;
    uint32_t bi_clr_used;
    uint32_t bi_clr_important;
} BITMAPINFOHEADER;

/* The RGBTRIPLE structure describes a color consisting of relative intensities of
 * red, green, and blue. Adapted from http://msdn.microsoft.com/en-us/library/aa922590.aspx.
 */
typedef struct {
    uint8_t rgbt_blue;
    uint8_t rgbt_green;
    uint8_t rgbt_red;
} RGBTRIPLE;

bool bmp_check_header(const BITMAPFILEHEADER * restrict bf,
                      const BITMAPINFOHEADER * restrict bi);

#endif      /* BMP_H */

bmp.c:

#include "bmp.h"

#include <stdbool.h>

#define SUPPORTED_BF_TYPE           0x4d42
#define SUPPORTED_BF_OFF_BITS       54
#define SUPPORTED_BI_SIZE           40
#define SUPPORTED_BI_BIT_COUNT      24
#define SUPPORTED_BI_COMPRESSION    0

bool bmp_check_header(const BITMAPFILEHEADER * restrict bf,
                      const BITMAPINFOHEADER * restrict bi)
{
    return bf->bf_type == SUPPORTED_BF_TYPE
        && bf->bf_offbits == SUPPORTED_BF_OFF_BITS
        && bi->bi_size == SUPPORTED_BI_SIZE
        && bi->bi_bitcount == SUPPORTED_BI_BIT_COUNT
        && bi->bi_compression == SUPPORTED_BI_COMPRESSION;
}

helpers.c:

#include "helpers.h"

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

#define NEIGHBORHOOD_SIZE   9
#define SCALE               8192
#define SCALE_UP(x)         ((uint_fast32_t) ((x) * SCALE + 0.5))

static inline int min(int x, int y)
{
    return x < y ? x : y;
}

void grayscale(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    for (size_t i = 0; i < height; ++i) {
        for (size_t j = 0; j < width; ++j) {
            const int average = (image[i][j].rgbt_blue + image[i][j].rgbt_red +
                                 image[i][j].rgbt_green + 1) / 3;
            image[i][j].rgbt_red = image[i][j].rgbt_green = image[i][j].rgbt_blue =
                (uint8_t) average;
        }
    }
}

void sepia(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    for (size_t i = 0; i < height; ++i) {
        for (size_t j = 0; j < width; ++j) {
            const int sepia_red = ((SCALE_UP(0.393) * image[i][j].rgbt_red +
                                    SCALE_UP(0.769) * image[i][j].rgbt_green +
                                    SCALE_UP(0.189) * image[i][j].rgbt_blue) +
                                   SCALE / 2) / SCALE;
            const int sepia_green =
                ((SCALE_UP(0.349) * image[i][j].rgbt_red +
                  SCALE_UP(0.686) * image[i][j].rgbt_green +
                  SCALE_UP(0.168) * image[i][j].rgbt_blue) + SCALE / 2) / SCALE;
            const int sepia_blue =
                ((SCALE_UP(0.272) * image[i][j].rgbt_red +
                  SCALE_UP(0.534) * image[i][j].rgbt_green +
                  SCALE_UP(0.131) * image[i][j].rgbt_blue) + SCALE / 2) / SCALE;

            image[i][j].rgbt_red = (uint8_t) min(255, sepia_red);
            image[i][j].rgbt_blue = (uint8_t) min(255, sepia_blue);
            image[i][j].rgbt_green = (uint8_t) min(255, sepia_green);
        }
    }
}

static inline void swap(RGBTRIPLE *restrict lhs, RGBTRIPLE *restrict rhs)
{
    RGBTRIPLE tmp = *lhs;

    *lhs = *rhs;
    *rhs = tmp;
}

void reflect(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    for (size_t i = 0; i < height; ++i) {
        size_t start = 0;
        size_t end = width - 1;

        while (start < end) {
            swap(&image[i][start], &image[i][end]);
            --end;
            ++start;
        }
    }
}

void box_blur(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    RGBTRIPLE(*temp)[width + 2] = (errno = 0, calloc(height + 2, sizeof *temp));

    if (!temp) {
        errno ? perror("calloc()") : (void)
            fputs("Error - failed to allocate memory for the image.", stderr);
        exit(EXIT_FAILURE);
    }

    for (size_t i = 0; i < height; ++i) {
        for (size_t j = 0; j < width; ++j) {
            temp[i + 1][j + 1] = image[i][j];
        }
    }
    
    for (size_t i = 0; i < height + 2; ++i) {
        temp[i][0] = temp[i][1];                     /* Copy left edge. */
        temp[i][width + 1] = temp[i][width];         /* Copy right edge. */
    }

    for (size_t j = 0; j < width + 2; ++j) {
        temp[0][j] = temp[1][j];                     /* Copy top edge. */
        temp[height + 1][j] = temp[height][j];       /* Copy bottom edge. */
    }

    for (size_t i = 1; i < height + 1; ++i) {
        for (size_t j = 1; j < width + 1; ++j) {
            size_t blue = 0, red = 0, green = 0;

            for (size_t k = i - 1; k < i + 2; ++k) {
                for (size_t l = j - 1; l < j + 2; ++l) {
                    red += temp[k][l].rgbt_red;
                    green += temp[k][l].rgbt_green;
                    blue += temp[k][l].rgbt_blue;
                }
            }

            image[i - 1][j - 1].rgbt_red = (uint8_t) ((red + NEIGHBORHOOD_SIZE/ 2) / NEIGHBORHOOD_SIZE);
            image[i - 1][j - 1].rgbt_blue = (uint8_t) ((blue + NEIGHBORHOOD_SIZE / 2) / NEIGHBORHOOD_SIZE);
            image[i - 1][j - 1].rgbt_green = (uint8_t) ((green + NEIGHBORHOOD_SIZE / 2) / NEIGHBORHOOD_SIZE);
        }
    }
    
    free(temp);
}

void blur(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    /* We try to approximate a Gaussian blur. */
    for (size_t i = 0; i < 3; ++i) {
        box_blur(height, width, image);
    }
}

#undef NEIGHBORHOOD_SIZE  
#undef SCALE            
#undef SCALE_UP    

helpers.h:

#ifndef HELPERS_H
#define HELPERS_H

#include "bmp.h"

#include <stddef.h>

/* Convert image to grayscale. */
void grayscale(size_t height, size_t width, RGBTRIPLE image[height][width]);

/* Convert image to sepia. */
void sepia(size_t height, size_t width, RGBTRIPLE image[height][width]);

/* Reflect image horizontally. */
void reflect(size_t height, size_t width, RGBTRIPLE image[height][width]);

/* Blur image. */
void blur(size_t height, size_t width, RGBTRIPLE image[height][width]);

#endif      /* HELPERS_H */

filter.c:

#ifdef _POSIX_C_SOURCE
#undef _POSIX_C_SOURCE
#endif

#ifdef _XOPEN_SOURCE
#undef _XOPEN_SOURCE
#endif

#define _POSIX_C_SOURCE 200819L
#define _XOPEN_SOURCE   700

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

#include <unistd.h>
#include <sys/types.h>

#include <getopt.h>

#include "helpers.h"

/* ARRAY_CARDINALITY(x) calculates the number of elements in the array 'x'.
 * If 'x' is a pointer, it will trigger an assertion.
 */
#define ARRAY_CARDINALITY(x) \
        (assert((void *)&(x) == (void *)(x)), sizeof (x) / sizeof *(x))

#define BMP_SCANLINE_PADDING 4
#define BF_UNPADDED_REGION_SIZE 12

struct flags {
    bool sflag;                 /* Sepia flag. */
    bool rflag;                 /* Reverse flag. */
    bool gflag;                 /* Greyscale flag. */
    bool bflag;                 /* Blur flag. */
    FILE *out_file;             /* Output to file. */
};

static void help(void)
{
    puts("Usage: filter [OPTIONS] <infile> <outfile>\n"
         "\n\tTransform your BMP images with powerful filters.\n\n"
         "Options:\n"
         "    -s, --sepia           Apply a sepia filter for a warm, vintage look.\n"
         "    -r, --reverse         Create a horizontal reflection for a mirror effect.\n"
         "    -g, --grayscale       Convert the image to classic greyscale.\n"
         "    -b, --blur            Add a soft blur to the image.\n"
         "    -h, --help            displays this message and exit.\n");
    exit(EXIT_SUCCESS);
}

static void err_msg(void)
{
    fputs("Usage: filter [OPTIONS] <infile> <outfile>\n"
          "Try filter -h for help.\n", stderr);
    exit(EXIT_FAILURE);
}

static void parse_options(const struct option *restrict long_options,
                          const char *restrict short_options,
                          struct flags *restrict opt_ptr, int argc,
                          char *const argv[])
{
    int c;

    while ((c =
            getopt_long(argc, argv, short_options, long_options, NULL)) != -1) {
        switch (c) {
            case 's':
                opt_ptr->sflag = true;
                break;
            case 'r':
                opt_ptr->rflag = true;
                break;
            case 'g':
                opt_ptr->gflag = true;
                break;
            case 'b':
                opt_ptr->bflag = true;
                break;
            case 'h':
                help();
                break;
            case 'o':
                /* We'll seek to the beginning once we've read input,
                 * in case it's the same file. 
                 */
                opt_ptr->out_file = (errno = 0, fopen(optarg, "ab"));

                if (!opt_ptr->out_file) {
                    errno ? perror(optarg) : (void)
                        fputs("Error - failed to open output file.", stderr);
                }
                break;

                /* case '?' */
            default:
                err_msg();
                break;
        }
    }
}

static void apply_filter(const struct flags *options, size_t height,
                         size_t width, RGBTRIPLE image[height][width])
{
    struct {
        bool flag;
        void (* const func)(size_t height, size_t width, RGBTRIPLE image[height][width]);
    } group[] = {
        { options->sflag, sepia },
        { options->rflag, reflect },
        { options->gflag, grayscale },
        { options->bflag, blur }, 
    };
    
    for (size_t i = 0; i < ARRAY_CARDINALITY(group); ++i) {
        if (group[i].flag) {
            group[i].func(height, width, image);
        }
    }
}

static size_t determine_padding(size_t width)
{
    /* In BMP images, each scanline (a row of pixels) must be a multiple of
     * BMP_SCANLINE_PADDING bytes in size. If the width of the image in pixels 
     * multipled by the size of each pixel (in bytes) is not a multiple of 
     * BMP_SCANLINE_PADDING, padding is added to make it so. 
     */
    return (BMP_SCANLINE_PADDING - (width * sizeof (RGBTRIPLE)) % BMP_SCANLINE_PADDING) % BMP_SCANLINE_PADDING;
}

static int write_scanlines(FILE * out_file, size_t height, size_t width,
                           const RGBTRIPLE image[][width], size_t padding)
{
    const size_t pad_byte = 0x00;

    /* Write new pixels to outfile */
    for (size_t i = 0; i < height; ++i) {
        /* Write row to outfile, with padding at the end. */
        if (fwrite(image[i], sizeof image[i][0], width, out_file) != width
            || fwrite(&pad_byte, 1, padding, out_file) != padding) {
            return -1;
        }
    }

    return 0;
}

static int write_image(const BITMAPFILEHEADER * restrict bf,
                       const BITMAPINFOHEADER * restrict bi,
                       FILE * restrict out_file, size_t height,
                       size_t width, const RGBTRIPLE image[height][width])
{
    if (out_file != stdout && (errno = 0, ftruncate(fileno(out_file), 0))) {
        errno ? perror("seek()") : 
                     (void) fputs("Error - failed to write to output file.\n", stderr);
        return -1;
    }

    if (fwrite(&bf->bf_type, sizeof bf->bf_type, 1, out_file) != 1
        || fwrite(&bf->bf_size, BF_UNPADDED_REGION_SIZE, 1, out_file) != 1
        || fwrite(bi, sizeof *bi, 1, out_file) != 1) {
        fputs("Error - failed to write to output file.\n", stderr);
        return -1;
    }

    const size_t padding = determine_padding(width);

    if (write_scanlines(out_file, height, width, image, padding) == -1) {
        fputs("Error - failed to write to output file.\n", stderr);
        return -1;
    }
    return out_file == stdout || !fclose(out_file);
}

static int read_scanlines(FILE * in_file, size_t height, size_t width,
                          RGBTRIPLE image[][width], size_t padding)
{
    /* Iterate over infile's scanlines */
    for (size_t i = 0; i < height; i++) {
        /* Read row into pixel array */
        if (fread(image[i], sizeof image[i][0], width, in_file) != width) {
            return -1;
        }

        /* Temporary buffer to read and discard padding. */
        uint8_t padding_buffer[BMP_SCANLINE_PADDING];

        if (fread(padding_buffer, 1, padding, in_file) != padding) {
            return -1;
        }
    }
    return 0;
}

static void *read_image(BITMAPFILEHEADER * restrict bf,
                        BITMAPINFOHEADER * restrict bi,
                        size_t *restrict height_ptr,
                        size_t *restrict width_ptr, FILE * restrict in_file)
{
    /* Read infile's BITMAPFILEHEADER and BITMAPINFOHEADER. */
    if (fread(&bf->bf_type, sizeof bf->bf_type, 1, in_file) != 1
        || fread(&bf->bf_size, BF_UNPADDED_REGION_SIZE, 1, in_file) != 1
        || fread(bi, sizeof *bi, 1, in_file) != 1) {
        fputs("Error - failed to read input file.\n", stderr);
        return NULL;
    }

    /* Ensure infile is (likely) a 24-bit uncompressed BMP 4.0 */
    if (!bmp_check_header(bf, bi)) {
        fputs("Error - unsupported file format.\n", stderr);
        return NULL;
    }
    
    /* There seems to be no need to treating the data differently. 
     * The code handles both top-down and bottom-up just fine. Or does it?
     */
#if 0    
    /* If bi_height is positive, the bitmap is a bottom-up DIB with the origin 
     * at the lower left corner. It bi_height is negative, the bitmap is a top-
     * down DIB with the origin at the upper left corner. 
     * We currenly only support images stored as top-down, so bail if the format
     * is elsewise.
     */
    if (bi->bi_height > 0) {
        fputs("Error - Bottom-up BMP image format is not yet supported.\n", stderr);
        return NULL;
    }
#endif

    /* Get image's dimensions. */
    uint32_t abs_height = bi->bi_height < 0 ? 0u - (uint32_t) bi->bi_height :
        (uint32_t) bi->bi_height;

    /* If we are on a too small a machine, there is not much hope, so bail. */
    if (abs_height > SIZE_MAX) {
        fputs
            ("Error - Image dimensions are too large for this system to process.\n",
             stderr);
        return NULL;
    }
    
    size_t height = (size_t) abs_height;
    size_t width = (size_t) bi->bi_width;

    if (!height || !width) {
        fputs("Error - corrupted BMP file: width or height is zero.\n", stderr);
        return NULL;
    }

    if (width > (SIZE_MAX - sizeof (RGBTRIPLE)) / sizeof (RGBTRIPLE)) {
        fputs("Error - image width is too large for this system to process.\n",
              stderr);
        return NULL;
    }

    /* Allocate memory for image */
    RGBTRIPLE(*image)[width] = calloc(height, sizeof *image);

    if (!image) {
        fputs("Error - not enough memory to store image.\n", stderr);
        return NULL;
    }

    const size_t padding = determine_padding(width);

    if (read_scanlines(in_file, height, width, image, padding)) {
        fputs("Error - failed to read input file.\n", stderr);
        return NULL;
    }

    *height_ptr = height;
    *width_ptr = width;
    return image;
}

static int process_image(const struct flags *restrict options,
                         FILE * restrict in_file, FILE * restrict out_file)
{
    BITMAPFILEHEADER bf;
    BITMAPINFOHEADER bi;

    size_t height = 0;
    size_t width = 0;

    void *const image = read_image(&bf, &bi, &height, &width, in_file);

    if (!image) {
        return -1;
    }

    apply_filter(options, height, width, image);

    if (write_image(&bf, &bi, out_file, height, width, image) == -1) {
        return -1;
    }

    free(image);
    return 0;
}

int main(int argc, char *argv[])
{
    /* Sanity check. POSIX requires the invoking process to pass a non-NULL 
     * argv[0].
     */
    if (!argv) {
        fputs("A NULL argv[0] was passed through an exec system call.\n",
              stderr);
        return EXIT_FAILURE;
    }

    /* Define allowable filters */
    static const struct option long_options[] = {
        { "grayscale", no_argument, NULL, 'g' },
        { "reverse", no_argument, NULL, 'r' },
        { "sepia", no_argument, NULL, 's' },
        { "blur", no_argument, NULL, 'b' },
        { "help", no_argument, NULL, 'h' },
        { "output", required_argument, NULL, 'o' },
        { NULL, 0, NULL, 0 }
    };

    FILE *in_file = stdin;
    struct flags options = { false, false, false, false, stdout };
    int result = EXIT_SUCCESS;

    parse_options(long_options, "grsbho:", &options, argc, argv);

    if ((optind + 1) == argc) {
        in_file = (errno = 0, fopen(argv[optind], "rb"));

        if (!in_file) {
            errno ? perror(argv[optind]) : (void)
                fputs("Error - failed to open input file.", stderr);
            return EXIT_FAILURE;
        }
    } else if (optind > argc) {
        err_msg();
    }

    if (process_image(&options, in_file, options.out_file) == -1) {
        result = EXIT_FAILURE;
    }

    if (in_file != stdin) {
        fclose(in_file);
    }

    return result;
}

Answer 1

• Should the functions doing the input/output be moved to bmp.c?

I recommend a different approach.

Think of all these image processing code as a set of routines in a library, maybe called hbmp.

In order to use them, and not conflict with user code, the file names, functions, your new types, defines, ... should be prefixed with hbmp_ and the top level routines prototyped in hbmp.h. Form various .c files like hbmp_init.c, hbmp_io.c, hbmp_filter.c to place the related code. Common file names like filter.c are sure to collide. Corral your name space.

The file with main() might be quite small.

The larger point being: plan to use many of these routines in larger code sets.

---

• Style, potential undefined behavior, et cetera.

#define with non-zero

Consider

#ifndef BMP_H
#define BMP_H

If other code uses #if BMP_H, that is true when BMP_H is defined and non-zero. It is false when BMP_H is not defined. It is also false when * Should the functions doing the input/output be moved to bmp.c? Style, potential undefined behavior, et cetera.

BMP_H is defined and zero or unvalued. Better to create code guards that differ in testing when using #if as well as #ifdef/#indef.

Recommend instead:

#ifndef BMP_H
#define BMP_H 1

Portability

I would hope write_image(), read_image(), apply_filter(), (and most of filter.c) could exist in .c file that does not need non-C standard includes like <unistd.h>, <sys/types.h>, <getopt.h>. Save those includes for the main() and option processing .c file.

int size

I have not found a dependency on int size as 32-bits. I suspect code would work well with int as 16, 32, 64, ... bits. Good!

Bug: wrong test

// if (!argv) { fputs("A NULL argv[0] was passed through an exec system call.\n", ...
if (!argv[0]) { fputs("A NULL argv[0] was passed through an exec system call.\n",...

Curious, do you really need this check, something about parse_options()?

Bubble up some fatal errors or exit()

Well done: I see some deep errors calling exit() and others returns an error code. I might disagree with some choices, yet I find your selection mostly good. What is best is fairly subjective.

Mixed types?

int c; ... c = getopt_long(...) looks wrong.
Hmmm, I see it is right

Style: Interesting use of , operator

in_file = (errno = 0, fopen(argv[optind], "rb"));

I understand why it is there - to tightly bound the errno = 0; with the following errno function.

Auto format?

It looks like code, though neat, is not using an auto-formatter.

Consider improving productively and auto-format.

Round ties to even?

Should you want a round ties to even rather than the current round ties down:

// int average = (image[i][j].rgbt_blue + image[i][j].rgbt_red +
//                              image[i][j].rgbt_green + 1) / 3;
unsigned average = image[i][j].rgbt_blue + image[i][j].rgbt_red +
                             image[i][j].rgbt_green;
average = (average + (average & 1u) + 1u)/3u;

size_t subtraction

I often get a little concerned with code like size_t end = width - 1; and wonder what would happen with width == 0 and then end === SIZE_MAX.

Of course, such values are pathological, yet sometimes we can simply handle that edge case too.

    size_t start = 0;
    // size_t end = width - 1;
    size_t end = width;

    while (start < end) {
        --end; // add
        swap(&image[i][start], &image[i][end]);
        // --end;
        ++start;
    }

Downside: On odd wide pics, we do one more loop per for().

I see that if (!height || !width) { would have caught that. Yet retain the higher level thought: be careful about subtracting size_t.