Filter: BMP Image Filtering Tool

Question

Filter is a C program that allows you to apply filters to BMP images.

Usage:

Usage: filter [OPTIONS] <infile> <outfile>

Review Goals:

General coding comments, style, potential undefined behavior, et cetera.

How can I eliminate the use of the non-standard packed attribute from the code?

Code:

bmp.h:

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

#include <stdint.h>

// These data types are essentially aliases for C/C++ primitive data types. 
// Adapted from http://msdn.microsoft.com/en-us/library/cc230309.aspx.
// See https://en.wikipedia.org/wiki/C_data_types#stdint.h for more on stdint.h.

typedef uint8_t BYTE;
typedef uint32_t DWORD;
typedef int32_t LONG;
typedef uint16_t WORD;

// 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 {
    WORD bfType;
    DWORD bfSize;
    WORD bfReserved1;
    WORD bfReserved2;
    DWORD bfOffBits;
} __attribute__ ((__packed__))
    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 {
    DWORD biSize;
    LONG biWidth;
    LONG biHeight;
    WORD biPlanes;
    WORD biBitCount;
    DWORD biCompression;
    DWORD biSizeImage;
    LONG biXPelsPerMeter;
    LONG biYPelsPerMeter;
    DWORD biClrUsed;
    DWORD biClrImportant;
} __attribute__ ((__packed__))
    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 {
    BYTE rgbtBlue;
    BYTE rgbtGreen;
    BYTE rgbtRed;
} __attribute__ ((__packed__))
    RGBTRIPLE;

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 <stdio.h>
#include <stdlib.h>
#include <assert.h>

#include <getopt.h>

#include "helpers.h"

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof (*(x)))

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 apply_filter(char filter, size_t height, size_t width,
                         RGBTRIPLE image[height][width])
{
    struct filter_map {
        const char filter_char;
        void (* const func)(size_t x, size_t y, RGBTRIPLE image[x][y]);
    } const filter_map[] = {
        { 'b', blur },
        { 'g', grayscale },
        { 'r', reflect },
        { 's', sepia }
    };

    for (size_t i = 0; i < ARRAY_CARDINALITY(filter_map); ++i) {
        if (filter_map[i].filter_char == filter) {
            filter_map[i].func(height, width, image);
            return;
        }
    }
}

static int write_image(const BITMAPFILEHEADER *restrict bf,
                       const BITMAPINFOHEADER *restrict bi,
                       FILE *restrict out, size_t height,
                       size_t width, const RGBTRIPLE image[height][width])
{
    // Write outfile's BITMAPFILEHEADER and BITMAPINFOHEADER. 
    if (fwrite(bf, sizeof *bf, 1, out) != 1
        || fwrite(bi, sizeof *bi, 1, out) != 1) {
        fputs("Failed to write to file.\n", stderr);
        return -1;
    }

    // Determine padding for scanlines
    const size_t padding = (4 - (width * sizeof (RGBTRIPLE)) % 4) % 4;
    const int 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) != width
            || fwrite(&pad_byte, 1, padding, out) != padding) {
            fputs("Failed to write to file.\n", stderr);
            return -1;
        }
    }

    return 0;
}

static void *read_image(BITMAPFILEHEADER *restrict bf,
                        BITMAPINFOHEADER *restrict bi, size_t *restrict height,
                        size_t *restrict width, FILE *restrict inptr)
{
    // Read infile's BITMAPFILEHEADER and BITMAPINFOHEADER.
    if (fread(bf, sizeof *bf, 1, inptr) != 1
        || fread(bi, sizeof *bi, 1, inptr) != 1) {
        fputs("Failed to read file.\n", stderr);
        return NULL;
    }

    // Ensure infile is (likely) a 24-bit uncompressed BMP 4.0
    if (bf->bfType != 0x4d42 || bf->bfOffBits != 54 || bi->biSize != 40 ||
        bi->biBitCount != 24 || bi->biCompression != 0) {
        fputs("Unsupported file format.\n", stderr);
        return NULL;
    }

    // Get image's dimensions
    *height = (size_t) abs(bi->biHeight);
    *width = (size_t) bi->biWidth;

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

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

    // Determine padding for scanlines
    const long int padding = (4 - (*width * sizeof (RGBTRIPLE)) % 4) % 4;

    // 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, inptr) != *width) {
            fputs("Failed to read file.\n", stderr);
            return NULL;
        }
        
        // Skip over padding
        fseek(inptr, padding, SEEK_CUR);
    }

    return image;
}

static int process_image(char filter, FILE *restrict inptr, FILE *restrict outptr)
{
    // Image's BITMAPFILEHEADER
    BITMAPFILEHEADER bf;

    // image's BITMAPINFOHEADER
    BITMAPINFOHEADER bi;

    size_t height = 0;
    size_t width = 0;

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

    if (!image) {
        return -1;
    }

    apply_filter(filter, height, width, image);

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

    free(image);
    return 0;
}

static char parse_args(int argc, char *argv[])
{
    // 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' },
        { NULL, 0, NULL, 0 }
    };

    // Get filter flag and check validity
    const int filter = getopt_long(argc, argv, "grsbh", long_options, NULL);

    if (filter == '?') {
        fputs("Invalid filter.\n", stderr);
        return '\0';
    }

    if (filter == -1 || filter != 'h') {
        if (argc != optind + 2) {
            fputs("Usage: filter [OPTIONS] <infile> <outfile>\n"
                  "Try filter -h for help.\n", stderr);
            return '\0';
        }
        return (char) filter;
    }
    // Ensure single flag
    if (getopt_long(argc, argv, "grsbh", long_options, NULL) != -1) {
        fputs("Only one filter allowed.\n", stderr);
        return '\0';
    }

    if (filter == 'h' && optind == argc) {
        return (char) filter;
    }

    fputs("Usage: filter [OPTIONS] <infile> <outfile>\n"
          "Try filter -h for help.\n", stderr);
    return '\0';
}

int main(int argc, char *argv[])
{
    const char filter = parse_args(argc, argv);

    if (filter == '\0') {
        return EXIT_FAILURE;
    } else if (filter == 'h') {
        help();
    }

    const char *const infile = argv[optind];
    const char *const outfile = argv[optind + 1];
    FILE *const inptr = fopen(infile, "rb");

    if (!inptr) {
        fprintf(stderr, "Could not open %s.\n", infile);
        return EXIT_FAILURE;
    }

    FILE *const outptr = fopen(outfile, "wb");

    if (!outptr) {
        fclose(inptr);
        fprintf(stderr, "Could not create %s.\n", outfile);
        return EXIT_FAILURE;
    }

    if (process_image(filter, inptr, outptr) == -1) {
        fclose(inptr);
        fclose(outptr);
        return EXIT_FAILURE;
    }

    fclose(inptr);
    fclose(outptr);
    return EXIT_SUCCESS;
}

helpers.c:

#include "helpers.h"

#include <math.h>
#include <string.h>

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].rgbtBlue + image[i][j].rgbtRed +
                                 image[i][j].rgbtGreen + 1) / 3;
            image[i][j].rgbtRed = image[i][j].rgbtGreen = image[i][j].rgbtBlue = (BYTE) 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 = round(0.393 * image[i][j].rgbtRed +
                                  0.769 * image[i][j].rgbtGreen +
                                  0.189 * image[i][j].rgbtBlue);
            const int sepia_green = round(0.349 * image[i][j].rgbtRed +
                                    0.686 * image[i][j].rgbtGreen +
                                    0.168 * image[i][j].rgbtBlue);
            const int sepia_blue = round(0.272 * image[i][j].rgbtRed +
                                   0.534 * image[i][j].rgbtGreen +
                                   0.131 * image[i][j].rgbtBlue);

            image[i][j].rgbtRed = (BYTE) min(255, sepia_red);
            image[i][j].rgbtBlue = (BYTE) min(255, sepia_blue);
            image[i][j].rgbtGreen = (BYTE) 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 blur(size_t height, size_t width, RGBTRIPLE image[height][width])
{
    RGBTRIPLE temp[height][width];

    memcpy(temp, image, sizeof temp);

    for (int i = 0; i < (int) height; ++i) {
        for (int j = 0; j < (int) width; ++j) {
            int blue = 0, red = 0, green = 0;
            double count = 0;

            for (int k = i - 1; k < i + 2; ++k) {
                /*
                 * Check for top and bottom edge. 
                 */
                if (k < 0 || k >= (int) height) {
                    continue;
                }

                for (int l = j - 1; l < j + 2; ++l) {
                    /*
                     * Check for left and right edge. 
                     */
                    if (l < 0 || l >= (int) width) {
                        continue;
                    }

                    red += temp[k][l].rgbtRed;
                    green += temp[k][l].rgbtGreen;
                    blue += temp[k][l].rgbtBlue;
                    ++count;
                }
            }

            image[i][j].rgbtRed = (BYTE) round(red / count);
            image[i][j].rgbtBlue = (BYTE) round(blue / count);
            image[i][j].rgbtGreen = (BYTE) round(green / count);
        }
    }
}

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]);

makefile:

CFLAGS  := -std=c17
CFLAGS  += -no-pie
# CFLAGS    += -g3
# CFLAGS    += -ggdb
CFLAGS  += -Wall
CFLAGS  += -Wextra
CFLAGS  += -Warray-bounds
CFLAGS  += -Wconversion
CFLAGS  += -Wmissing-braces
CFLAGS  += -Wno-parentheses
CFLAGS  += -Wno-format-truncation
CFLAGS  += -Wpedantic
CFLAGS  += -Wstrict-prototypes
CFLAGS  += -Wwrite-strings
CFLAGS  += -Winline
CFLAGS  += -s
CFLAGS  += -O2
CFLAGS  += -D_FORTIFY_SOURCE=2

BINDIR  := bin
BIN     := $(BINDIR)/filter
SRCS    := $(wildcard src/*.c)
OBJS    := $(patsubst src/%.c, obj/%.o, $(SRCS))
LDLIBS  := -lm

all: $(BIN)

$(BIN): $(OBJS)
    $(CC) -o $@ $^ $(CFLAGS) $(LDLIBS)

obj/%.o: src/%.c
    $(CC) $(CFLAGS) -c $< -o $@ $(LDLIBS)

clean:
    $(RM) -rf $(OBJS) 

fclean:
    $(RM) -rf $(BIN) 

.PHONY: clean all fclean
.DELETE_ON_ERROR:

Or you could instead clone this github repository: Filter

Toby Speight · Accepted Answer · 2023-10-29 14:23:15Z

typedef uint8_t BYTE;
typedef uint32_t DWORD;
typedef int32_t LONG;
typedef uint16_t WORD;

These definitions just serve to obscure the code (perhaps they are written as a crutch for someone more familiar with a different language?). Do your (C programmer) readers a favour and use the <stdint.h> types directly. It's a lot easier if we can understand the data structure without having to refer back to these definitions.

Or perhaps they were used just so we could copy the structure definitions from the other language? In which case a sensible option might be to use macros, which we could #undef at the end of the header so they don't infect the implementation files.

Please don't use all-caps for type names - that makes them look like macros, which they are not. We use all-caps for macros so that we can quickly see that they are text substitutions rather than real C code.

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof (*(x)))

It's good to use plenty of parens when writing macros, because we don't know what kind of expression the argument might be nor do we know the context in which it's expanded. So the parens around x are needed, and those around the entire expression. But we don't need the ones that enclose *(x):

#define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof *(x))

Do take care when using this macro that there's no guard against misusing it with a pointer argument. The equivalent that's used in the Linux kernel has an ugly (but effective) guard to ensure it's only applied to arrays, but in this code, we're relying on programmer care.

     const size_t padding = (4 - (width * sizeof (RGBTRIPLE)) % 4) % 4;

There's no explanation of the magic number 4 here. If it's a requirement of the file format, we should have at least a comment for this.

*height = (size_t) abs(bi->biHeight);
*width = (size_t) bi->biWidth;

It's not clear why height may be the wrong sign, but not width - again, needs a comment to explain the vagaries of the file format.

Since biWidth is a LONG, which is a int32_t, conversion to int may truncate the value on systems with 16-bit int. That's a standard problem in using the fixed-width types in C. ~~I suggest using labs(), since long must be at least 32 bits.~~ The best approach here is probably to write a simple abs32() of our own such as:

uint32_t abs32(int32_t x) { return x < 0 ? 0u - (uint32_t)x : (uint32_t)x; }

We could be more robust about trusting the values of width and height, and not only because calloc() is implementation-defined when either is zero. A more defensive approach to malformed input is a good thing generally; for example, we should be checking the width to ensure it won't overflow when multiplied by sizeof (RGBTRIPLE).

        fseek(inptr, padding, SEEK_CUR);

Why are we ignoring the return value here?

It's probably better to use fread() and discard the input, so that we can parse images from non-seekable sources such as pipes. That would make the program much more flexible.

// Ensure single flag
if (getopt_long(argc, argv, "grsbh", long_options, NULL) != -1) {
    fputs("Only one filter allowed.\n", stderr);
    return '\0';
}

We have completely lost the advantage of using a single program if it can only perform one operation - it would be simpler to have separate programs (built from shared functions) that each perform one transformation.

All through this code, I had assumed that the whole point of this being a single program was that we could perform several transformations without having to decode and re-encode the image between each, so it's a disappointment to get here and discover that we've squandered that advantage.

obj/%.o: src/%.c
    $(CC) $(CFLAGS) -c $< -o $@ $(LDLIBS)

You're working against Make here by insisting on writing the object files to a subdirectory. There's no need to write our own rule, if we work with Make, allowing it to put the objects alongside the sources, or using VPATH so that they end up in the working directory.

The same is true of the executable program: with the object files in the working directory, we should just be able to write its dependency on the object files and let Make do the rest.

And neither of those rules has been correctly copied from Make's built-in rules anyway; if you're really going to insist on that layout, at least do it right:

$(BIN): $(OBJS)
    $(LINK.o) $^ $(LOADLIBES) $(LDLIBS) -o $@

obj/%.o: src/%.c
    $(COMPILE.c) $(OUTPUT_OPTION) $<

We're completely missing the dependencies of object files on any of the headers. I recommend you make that automatic: add CFLAGS += -MM (or related option) and then use include to reference the resultant .d files in the Makefile.

Good point about "We have completely lost the advantage of using a single program if it can only perform one operation". — chux
– chux, Commented Oct 28, 2023 at 11:47
Concerning abs(), labs(): what is needed is uint32_t abs32(int32_t) or a simple ?: or if() code. — chux
– chux, Commented Oct 28, 2023 at 11:51
Possibly, or you could look at the contortions Linux kernel uses. Or you can keep it as it is (simple and clear) and just take care how it's used. I'd probably keep it simple in this application. In other words, that was just an observation, rather than a definite defect. :-) — Toby Speight
– Toby Speight, Commented Oct 28, 2023 at 16:25
Ok yes, overloading would be useful to define your own uabs (or absu? Naming is hard) that returns std::make_unsigned<T> for uabs(T). If that's what you meant to talk about, not std::abs itself, then agreed. — Peter Cordes
– Peter Cordes, Commented Oct 29, 2023 at 14:46
BYTE, DWORD and the like are how Microsoft defines the BMP format. These names are all throughout the Windows API (or at least they used to be back when I did Windows programming, it’s been a long time). — Cris Luengo
– Cris Luengo, Commented Oct 29, 2023 at 16:33

user555045 · Accepted Answer · 2023-10-27 20:09:40Z

How can I eliminate the use of the non-standard packed attribute from the code?

RGBTRIPLE and BITMAPINFOHEADER don't need that attribute. There's already naturally no padding, and your program won't violate their alignment requirements either: RGBTRIPLE has a natural alignment of 1 anyway, and BITMAPINFOHEADER is never used in a context that would force its alignment to be unnatural (this would be different if you memory-mapped the file, but since you fread the data it doesn't matter whether the data is aligned in the file).

BITMAPFILEHEADER is small enough that you could reasonably read it field-by-field, in which case it doesn't need to be packed. Reading things field-by-field is error-prone in general, but with so few fields it's not that bad..

for (int i = 0; i < (int) height; ++i) {
    for (int j = 0; j < (int) width; ++j) {

I understand why you did this, but I'd like to offer an alternative: handle the edges and corners separately. Then you don't have to deal with any negative indices, and i and j don't need to be int. As a bonus you can also rid of the nasty divide-by-variable (actual run-time division, not multiply-by-reciprocal), since that way you would always know the count. It would cost more code though.

-b, --blur Add a soft blur to the image

It's a single iteration of 3x3 box blur, that's fine but "soft blur" is IMO more suggestive of being a Gaussian blur or something sufficiently close (for example iterated box blurs can approximate a Gaussian blur). A single box blur tends to come out looking somewhat "blocky" (but you'd probably have to zoom in to notice).

There are ways to implement a k-by-k box blur without paying a k² cost for it, but for 3-by-3 that's not really necessary.

potential undefined behavior

I cannot say that this will be an exhaustive list of possibilities..

*height = (size_t) abs(bi->biHeight);

If int is 32-bit and 2's complement and biHeight has a value of −2147483648, then taking its absolute value is UB. This dumb situation is brought to you by abs needlessly returning a signed integer. You could use llabs to work around this.. or just reject that height (a BMP with that height is asking for trouble anyway).

calloc(*height, *width * sizeof (RGBTRIPLE))

If size_t is a 32-bit type and *width has some absurd (but possible) value, then this multiplication can wrap, the calloc can then succeed though it allocates much less than *height * *width * 3 bytes, so afterwards some out-of-bounds accesses can happen.

The nice thing about Gaussian blur is that like box blur, it can be implemented as independent horizontal and vertical passes. — Toby Speight
– Toby Speight, Commented Oct 29, 2023 at 15:10
”handle the edges and corners separately.” This is important. The conditional inside the loop makes the filtering very slow. The division is peanuts compared to the conditional. It is usually significantly faster to copy the input image into a larger array, fill the padding region by extrapolating, and then looping without conditionals. But of course handling edge pixels separately is fastest. This requires lots more code though. — Cris Luengo
– Cris Luengo, Commented Oct 29, 2023 at 16:24
@CrisLuengo How do you suggest I perform the extrapolating part? Do I simply initialize the border pixels to 0, 0, 0? — Madagascar
– Madagascar, Commented Oct 29, 2023 at 21:35
@Harith The mist common border extension method is mirroring, filling img[-1,x] = img[1,x], img[-2,x] = img[2,x], etc (with the negative indices being the pixels in the padding region. But you can also replicate the border value, or any of many other options. Filling with zeros means the convolution result near the edge will be lower than expected, it is not the best choice for blur filters. — Cris Luengo
– Cris Luengo, Commented Oct 29, 2023 at 22:21
@Harith btw I would not explicitly create the out-of-bounds pixels. When implementing the blur for the edges, you can do it there implicitly: in the place where you would use an out-of-bounds pixel, you instead use a different pixel that does exist (eg mirrored or replicated) — user555045
– user555045, Commented Oct 29, 2023 at 22:29

vnp · Accepted Answer · 2023-10-27 21:36:35Z

7

Error messages, like

  fprintf(stderr, "Could not open %s.\n", infile);
  fprintf(stderr, "Could not create %s.\n", outfile);

are not informative. Use perror.

Applying multiple filters (e.g. blur and grayscale) requires multiple invocations. Consider supporting more than one filter.
The program cannot be used in a pipeline. Consider supporting input and output redirection.
It doesn't feel right that help is treated as a filter.
bf->bfType, bi->biHeight, bi->biWidth would be interpreted wrong on a big endian platforms. In other words, do not fread an entire structure in one go. Read it byte by byte, and reconstruct ints and shorts as you read them. This also eliminates the need for __attribute__ ((__packed__)).

While we are here, 0x4d42 is a magic number. Avoid them. Consider
```
  #define SUPPORTED_BF_TYPE 0x4d42
```
When allocating an image with
```
  RGBTRIPLE(*image)[*width] = calloc(*height, *width * sizeof (RGBTRIPLE));
```
make up your mind. Do you want VLA, or do you want a heap allocation? In fact, I am surprised that it compiles at all, and I darenot to say what it compiles to.

In blur you do RGBTRIPLE temp[height][width];. Why not follow the same pattern?
More functions please. Every time you feel compelled to put a comment like
```
  // Iterate over infile's scanlines
```
you admit that you fail to express your intent clearly in the code. Give the commented block a good name, extract it into a function, and get rid of comment.

Along the same line, a comment in
```
  // Image's BITMAPFILEHEADER
  BITMAPFILEHEADER bf;
```
just adds noise. Do not overcomment.
size_t is likely wider than int, Casting it as in i < (int) height is dangerous.

answered Oct 27, 2023 at 21:36

vnp

58.7k4 gold badges55 silver badges144 bronze badges

1

\$\begingroup\$ "are not informative. Use perror" --> C does not specify that errno is set due to a failed fopen(). If might be more informative to use perror(). \$\endgroup\$

chux
– chux

2023-10-28 05:19:01 +00:00
Commented Oct 28, 2023 at 5:19
2

\$\begingroup\$ Windows, Linux, MacOS, OpenBSD, and AIX all document fopen as setting errno. Statistically, perror seems more likely to add value than not. \$\endgroup\$

aghast
– aghast

2023-10-28 06:40:43 +00:00
Commented Oct 28, 2023 at 6:40
1

\$\begingroup\$ @aghast To set errno as part of fopen() may be common, yet it is a compiler choice, not an OS one. IAC, coding to the C spec is a good consideration. - as well as common practice. \$\endgroup\$

chux
– chux

2023-10-28 10:48:47 +00:00
Commented Oct 28, 2023 at 10:48
2

\$\begingroup\$ While I usually agree with relying only on C's guarantees, this feature is so widely available (POSIX also extends the C definition this way) and so useful it's worth taking advantage of. Maybe it's worth writing a small report_file_failure() function that uses perror() when errno is known to be set by the function and falls back to fprintf() otherwise. Or, if we have the discipline to reset errno to zero before calling fopen(), we could fall back if it hasn't been assigned a non-zero value. \$\endgroup\$

Toby Speight
– Toby Speight

2023-10-28 12:45:43 +00:00
Commented Oct 28, 2023 at 12:45
1

\$\begingroup\$ In blur you do RGBTRIPLE temp[height][width];. Why not follow the same pattern? Not possible to return a variable with automatic-storage duration. \$\endgroup\$

Madagascar
– Madagascar

2023-10-28 16:53:30 +00:00
Commented Oct 28, 2023 at 16:53

| Show 5 more comments

chux · Accepted Answer · 2023-10-29 21:07:37Z

Round with integer math

Round with integer math and avoid expensive double math.

// const int sepia_red = round(0.393 * image[i][j].rgbtRed +
//     0.769 * image[i][j].rgbtGreen + 0.189 * image[i][j].rgbtBlue);
const int sepia_red = ((393 * image[i][j].rgbtRed +
    769 * image[i][j].rgbtGreen + 189 * image[i][j].rgbtBlue) + 500)/1000;

On a 16-bit or wider machine:

#include <stdint.h>.
const int_least32_t sepia_red = ((INT32_C(393) * image[i][j].rgbtRed + ...

We can even go faster by scaling 0.393, 0.769, and 0.189 up a ways so the final division is a power-of-2.

#define SCALE 8192
#define SCALE_UP(x) ((uint_fast32_t) ((x)*SCALE + 0.5))
const int sepia_red = ((SCALE_UP(0.393) * image[i][j].rgbtRed +
    SCALE_UP(0.769) * image[i][j].rgbtGreen +
    SCALE_UP(0.189) * image[i][j].rgbtBlue) + SCALE/2)/SCALE;
#undef SCALE
#undef SCALE_UP

Some pedantic thoughts

*height = (size_t) abs(bi->biHeight); is amiss for a couple reasons :

bi->biHeight is a 32-bit signed integer. A negative value has a special meaning and attempting |x| loses that info. TBD code to handle that.

Rather than assume size_t is at least 32-bit and int is at least 32-bit (the abs() function), consider more portable code.

uint32_t absHeight = bi->biHeight < 0 ? 0u - (uint32_t) bi->biHeight : (uint32_t) bi->biHeight;

// If we are on a too small a small machine, there is not much hope, so bail.
assert(absHeight <= SIZE_MAX);

size_t height = (size_t) height;

[Edit] Packing Avoidance

Use static_assert(sizeof(BITMAPFILEHEADER)==14, "Packing issue"); and like code for other BMP types to make certain something unusual did not happen.
If above fails for BITMAPFILEHEADE without packing as it may give OP trouble in padding between WORD bfType and DWORD bfSize;. The remaining struct members look safe (no likely padding). Consider reading/writing the BITMAPFILEHEADER in 2 steps: First a 2-byte read of .bfType and then a remaining read of 12 bytes starting at .bfSize.

[Edit 2] VLA usage

Variable Logic Array (VLA) usage comes in 2 flavors: 1) defining an array and 2) defining a type.

C standard VLA support beings with C99, optional in C11, C17. With C23, looks like it will remain optional for #1, but required for #2.

A particular use, below, merits review.

RGBTRIPLE(*image)[*width] = calloc(*height, *width * sizeof (RGBTRIPLE));
if (!image) {

Above is a VLA type #2, not an array #1 which could be

// Array example
RGBTRIPLE image[*width][*height];
memset(image, 0, sizeof image);

Weaknesses of *width * sizeof (RGBTRIPLE) includes extreme potential overrun. Could use following to detect:

if (*width > (SIZE_MAX-3)/sizeof (RGBTRIPLE)) {
  error code ...
}
// now do calloc

Sizing by the object, rather than type is easier to code right, review and maintain.

RGBTRIPLE(*image)[*width] = calloc(*height, sizeof image[0]);
if (!image) {

A singular weakness with read_image() is the use of *height and *width throughout. Consider instead using

static void *read_image(BITMAPFILEHEADER *restrict bf, BITMAPINFOHEADER *restrict bi,
    size_t *restrict ptrheight, size_t *restrict ptrwidth, FILE *restrict inptr) {
    ...
    size_t height = (size_t) abs(bi->biHeight);
    size_t width = (size_t) bi->biWidth;
    ...
    // Lots of code with height, width
    ...
    *ptrheight = height;
    *ptrwidth = wdith;
    return image;
}

This will help programmers with

RGBTRIPLE(*image)[*width] = calloc(*height, *width * sizeof (RGBTRIPLE));

which looks questionable versus

RGBTRIPLE (*image)[width] = calloc(height, width * sizeof (RGBTRIPLE));
// or even better
RGBTRIPLE (*image)[width] = calloc(height, sizeof image[0]);

is easier on the eyes.

Given RGBTRIPLE temp[height][width]; in blur(), consider an allocation instead to handle large bitmaps.

@Harith Done. VLA usage, even as a type, is still novel in some circles and so deserves simplification. I'd avoid VLA arrays for bitmaps as they can be very large. Using VLA for a typing is fine and IMHO, preferable for 2D objects. — chux
– chux, Commented Oct 28, 2023 at 17:19
Ooh, I like the idea to use a power of 2 for the division in colour conversion. If I ever have to write one of those by hand again, I'll make sure to remember that! — Toby Speight
– Toby Speight, Commented Oct 29, 2023 at 15:05
“expensive double math”? In my experience, a series of double multiplications takes the same amount of time as a series of int multiplications. You’re just adding an expensive division. — Cris Luengo
– Cris Luengo, Commented Oct 29, 2023 at 16:47
@CrisLuengo Perhaps - and a reasonable concern. Such code in a deep loop deserves profiling as this is the 3% of the time Is premature optimization really the root of all evil? that is not evil. In the cases where FP is slower, it is often magnitudes slower than integer math, whereas the reverse is a far more modest difference. Recall that in 2023, the majority of processors are those ubiquitous embedded ones (billions per year) where FPU are less common. Of course a BMP app is more likely found in more complex processors. — chux
– chux, Commented Oct 29, 2023 at 21:21
@CrisLuengo: On CPUs with fast integer multiply, division by a compile time constant isn't very expensive. (Why does GCC use multiplication by a strange number in implementing integer division?). That's still cheaper than converting from integer to double and back.But as this answer suggests, a power-of-2 scale factor like 8192 is worth using if possible. Especially if you're hoping for this to auto-vectorize (where narrow integer multiply-accumulate could be a significant speed boost vs. float or especially double). — Peter Cordes
– Peter Cordes, Commented Oct 30, 2023 at 19:28

Stack Exchange Network

Filter: BMP Image Filtering Tool

4 Answers 4

You must log in to answer this question.

Linked

Hot Network Questions

Filter: BMP Image Filtering Tool

4 Answers 4

You must log in to answer this question.

Linked

Related

Hot Network Questions