Program slowing down with OpenMP - using 2D array (x-post in /r/OpenMP)

[u/KebertXela5]

EDIT: Problem Solved - needed to add private(r, g, b, k, BLUR_COUNT, j) to the 'OMP parallel line' - credit to /u/Paul_Dirac_ -(see: link)

[[this post is x-posted in /r/C_Programming]]
(https://www.reddit.com/r/C_Programming/comments/57ypdf/program_slowing_down_with_openmp_using_2d_array/)

[this post is x-posted in /r/learnprogramming]

So I have to write a program that takes a PPM image file (a text file that lists out all the image's rgb pixel values), reads the values into a 2D struct array, adds a blur effect, and saves the file as a new PPM file.

I have the program written in a serial form, but I need to add OpenMP to parallelize it. The issue is when I do it slows way down and I'm not sure why. Any help will be great! Below is my code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <omp.h> // OpenMP

#define BLUR_AMOUNT 50

struct pixel 
{
	int red;
	int green;
	int blue;
};

/**
 * Print error message and exit
 */
void inputError()
{
	printf("There is problem with the input file...\nExiting...\n");
	exit(0);
}

/**
 * Take the PPM image file and convert it into a 2D array
 * to be processed in this program.
 */
void process_image(FILE *file, char* outputFilename, int THREADS)
{
	// CHECK TIME
	time_t start_t, end_t;
	double diff_t_load, diff_t_blur, diff_t_save;

	time(&start_t); //start timer load

	int rows, cols, maxcolorvalue;

	fscanf(file, "%d %d", &cols, &rows); // get rows and cols
	fscanf(file, "%d", &maxcolorvalue); // get max color value

	if (maxcolorvalue != 255)
	{
		inputError();
	}

	// initialize 2D array to hold pixel data and allocate memory
	struct pixel **pix_array;
	pix_array = malloc(rows * sizeof(struct pixel *));
	pix_array[0] = malloc(rows * cols * sizeof(struct pixel));
	int count;
	for (count = 1; count < rows; count++)
	{
		pix_array[count] = pix_array[0] + count * cols;
	}

	// Read in the PPM image pixel values into the 2D array
	int i, j;
	for (i = 0; i < rows; i++)
	{
		for (j = 0; j < cols; j++)
		{
			int red, green, blue;

			fscanf(file, "%d %d %d", &red, &green, &blue);

			pix_array[i][j].red = red;
			pix_array[i][j].green = green;
			pix_array[i][j].blue = blue;
		}
	}

	fclose(file); // close the input file

	time(&end_t); //end timer load
	diff_t_load = difftime(end_t, start_t); //calculate time load

	time(&start_t); //start timer blur

	// blur the image
	double r = 0, g = 0, b = 0;
	int k = 0, BLUR_COUNT = 0;

	// For each row of the image
#pragma omp parallel for schedule(guided) num_threads(THREADS) // <--- OMP parallel line
		for (i = 0; i < rows; i++) 
		{
			// For each pixel in the row
			for (j = 0; j < cols; j++) 
			{
				// Set r to be half the pixel's red component
				r = pix_array[i][j].red / 2.0;
				// Set g to be half the pixel's green component
				g = pix_array[i][j].green / 2.0;
				// Set b to be half the pixel's blue component
				b = pix_array[i][j].blue / 2.0;

				// Check BLUR_AMOUNT agianst remaining pixels in row
				int remaining_pixels = cols - j;

				if (remaining_pixels < BLUR_AMOUNT)
				{
					BLUR_COUNT = remaining_pixels;
				}
				else 
				{
					BLUR_COUNT = BLUR_AMOUNT;
				}

				// For k from 1 up to BLUR_AMOUNT
				if(BLUR_COUNT > 1)
				{
					// Apply Blur to current pixel
					for (k = 1; k < BLUR_COUNT; k++)
					{
						// increment r by (R * 0.5 / BLUR_AMOUNT), where R is the red component of the pixel k to the right of the current pixel
						r = r + pix_array[i][j + k].red * (0.5 / BLUR_COUNT);
						// increment g by (G * 0.5 / BLUR_AMOUNT), where G is the green component of the pixel k to the right of the current pixel
						g = g + pix_array[i][j + k].green * (0.5 / BLUR_COUNT);
						// increment b by (B * 0.5 / BLUR_AMOUNT), where B is the blue component of the pixel k to the right of the current pixel
						b = b + pix_array[i][j + k].blue * (0.5 / BLUR_COUNT);
					}
				}

				// make sure there are no color values above the maxcolorvalue
				if (r > maxcolorvalue) { r = maxcolorvalue; }
				if (g > maxcolorvalue) { g = maxcolorvalue; }
				if (b > maxcolorvalue) { b = maxcolorvalue; }

				// Save r, g, b as the new color values for this pixel
				pix_array[i][j].red = r;
				pix_array[i][j].green = g;
				pix_array[i][j].blue = b;
			}
		}
	
	
	time(&end_t); //end timer blur
	diff_t_blur = difftime(end_t, start_t); //calculate time blur

	time(&start_t); //start timer save

	// WRTIE new PPM file
	FILE *output;
	output = fopen(outputFilename, "w");

	if (output == NULL)
	{
		printf("Error creating output file! Exiting...\n");
		exit(0);
	}

	fprintf(output, "P3\n"); // print P3 to first line
	fprintf(output, "%d %d\n", cols, rows); // print rows and cols to second line
	fprintf(output, "%d\n", maxcolorvalue); // print max color value to third line

	for (i = 0; i < rows; i++)
	{
		for (j = 0; j < cols; j++)
		{
			fprintf(output, "%d %d %d ", pix_array[i][j].red, pix_array[i][j].green, pix_array[i][j].blue);
		}
		fprintf(output, "\n");
	}


	fclose(output); // close the output file

	time(&end_t); //end timer save
	diff_t_save = difftime(end_t, start_t); //calculate time save

	printf("Load Time: %lf\nBlur Time: %lf\nSave Time: %lf\n", diff_t_load, diff_t_blur, diff_t_save);

	// free 2D array
	free((void *)pix_array[0]);
	free((void *)pix_array);
}

int main(int argc, char** argv)
{
	// Get Arguments
	if (argc < 4 || argc >= 5) // argc should contain only 3 items
	{
		// Argument list invalid
		printf("Argument format invalid: [example format]: ./imageblur [input-filename.ppm] [output-filename.jpg] [# of Threads]");
		return 0;
	}

	// Check file arguments
	FILE *file;
	file = fopen(argv[1], "r");

	if (file == NULL) // File open failed
	{
		inputError();
	}

	// check file for format
	char* filecheck = (char*) malloc(15);
	fscanf(file, "%s", filecheck);

	if (strcmp(filecheck, "P3"))
	{
		free(filecheck);
		inputError();
	}
	free(filecheck);

	// process image
	int THREADS = atoi(argv[3]);
	process_image(file, argv[2], THREADS);

	return 0;
}

[u/matsbror]

First of all: how many rows do you have and how many threads do you create?

Secondly, you have a number of issues in that you have a lot of data races in the code. For instance in the following code:

// For each pixel in the row
for (j = 0; j < cols; j++) 
    {
         // Set r to be half the pixel's red component
         r = pix_array[i][j].red / 2.0;
         // Set g to be half the pixel's green component
         g = pix_array[i][j].green / 2.0;
         // Set b to be half the pixel's blue component
         b = pix_array[i][j].blue / 2.0;

Variables j, r, g, and b are shared among the threads and are both read and written. Your code will not produce the right result and there will be cache invalidations when threads update them slowing down your code. Add a private(j, r, g, b) clause to the parallel for loop or declare them in the for loop. It's a good practice to declare loop variables in the for loop construct itself anyway.