patterncMinor
Image Flip algorithm in C
Viewed 0 times
algorithmimageflip
Problem
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;
}Solution
A few comments:
-
you use signed/unsigned types inconsistently. Your
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
(
can occur with mixed signed and unsigned arithmetic, so you should turn on
conversion warnings (eg
-
your naming is inconsistent. The function and the variable
"camel-case", while the other variables don't.
-
the return from
-
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
Here is a slightly simplified version of the function
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.
-
you use signed/unsigned types inconsistently. Your
width and height areunsigned, 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 thatcan 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 firstuse (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
mallocHere 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.
Code Snippets
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);
}Context
StackExchange Code Review Q#29618, answer score: 9
Revisions (0)
No revisions yet.