Image Flip algorithm in C

Question

This is the algorithm I am using to flip an image in-place. Though it works perfectly, I would like if any one can spot any potential problems or offer any points on improvement and optimizations.

/**
 * pixels_buffer - Pixels buffer to be operated
 * width - Image width
 * height - Image height
 * bytes_per_pixel - Number of image components, ie: 3 for rgb, 4 rgba, etc...
 **/
void flipVertically(unsigned char *pixels_buffer, const unsigned int width, const unsigned int height, const int bytes_per_pixel)
{
 const unsigned int rows = height / 2; // Iterate only half the buffer to get a full flip
 const unsigned int row_stride = width * bytes_per_pixel;
 unsigned char* temp_row = (unsigned char*)malloc(row_stride);
 int source_offset, target_offset;
 for (int rowIndex = 0; rowIndex < rows; rowIndex++)
 {
 source_offset = rowIndex * row_stride;
 target_offset = (height - rowIndex - 1) * row_stride;
 memcpy(temp_row, pixels_buffer + source_offset, row_stride);
 memcpy(pixels_buffer + source_offset, pixels_buffer + target_offset, row_stride);
 memcpy(pixels_buffer + target_offset, temp_row, row_stride);
 }
 free(temp_row);
 temp_row = NULL;
}

• Lstor: You shouldn't cast the result from malloc in C.

Answer 1

source_offset = rowIndex * row_stride;
target_offset = (height - rowIndex - 1) * row_stride;
memcpy(temp_row, pixels_buffer + source_offset, row_stride);
memcpy(pixels_buffer + source_offset, pixels_buffer + target_offset, row_stride);
memcpy(pixels_buffer + target_offset, temp_row, row_stride);

could be written:

1. with const variables
2. without any duplicated evaluation of pixels_buffer + [source/target]_offset

doing something like:

unsigned char* source = pixels_buffer + rowIndex * row_stride;
unsigned char* target = pixels_buffer + (height - rowIndex - 1) * row_stride;
memcpy(temp_row, source, row_stride);
memcpy(source, target, row_stride);
memcpy(target, temp_row, row_stride);

Also, a different way to do would be to initialize

source = pixels_buffer

and

target = pixels_buffer + (height - 1) * row_stride

and to increment/decrement them by row_stride at each iteration.

Edit: if you go for this option, you might be able to use this for the stopping condition of your loop.

Answer 2

A few comments:

• you use signed/unsigned types inconsistently. Your width and height are unsigned, while all other integers are signed. If you are using unsigned types because the values can never be negative, then the same applies to all integers in the function and, by that reasoning, they should all be unsigned (size_t is a common type used for sizes). There are arithmetic errors that can occur with mixed signed and unsigned arithmetic, so you should turn on conversion warnings (eg -Wconversion for gcc).

• your naming is inconsistent. The function and the variable rowIndex use "camel-case", while the other variables don't.

• the return from malloc should not be cast in C (C++ requires casting).

• source_offset and target_offset should be defined at the point of first use (ie. within the loop). There is no performance penalty for defining a variable within a loop in C.

• there is no check for success from malloc

Here is a slightly simplified version of the function

void flip_vertically(unsigned char *pixels, const size_t width, const size_t height, const size_t bytes_per_pixel)
{
 const size_t stride = width * bytes_per_pixel;
 unsigned char *row = malloc(stride);
 unsigned char *low = pixels;
 unsigned char *high = &pixels[(height - 1) * stride];
 for (; low < high; low += stride, high -= stride) {
 memcpy(row, low, stride);
 memcpy(low, high, stride);
 memcpy(high, row, stride);
 }
 free(row);
}

You can see that I have changed some types, shortened variable names (it is a short function so short names are appropriate) and simplified the way buffer offsets are calculated to just addition/subtraction.

Answer 3

Just a quick addition to something these already great answers stated but somewhat glossed over. Originally this question had the C++ tag in it. I'm assuming because of this line:

unsigned char* temp_row = (unsigned char*) malloc(row_stride);

that you are compiling this C code as C++ code. Don't do that. While the languages seem very similar and the compiler you are using will probably allow it, it is a bad habit to form. One of the main reasons not to do this is that you won't get certain warnings that you would normally get by compiling this as C code, meaning right now there could be certain vulnerabilities and bugs in your application.

I speak from personal experience.

Answer 4

Just some quick things:

• unsigned int width doesn't need to be const since it's passed-by-value. But that's not really significant as the compiler will optimize away the const if needed.

• Prefer size_t for types representing sizes. It's also useful for loop counters as you'll be able to loop through any size of data supported by your computer. However, you'll receive type-matching warnings if your counter and size types are not both signed or unsigned. In this case, using int (which is a signed type) will cause such warnings if the size type is unsigned.