Wallpaper generator - skelleton code [funny litle question]

exidux · 2015-10-19 08:12:02

I hope this is a general programming sub-forum, if not then call me a fool.

A while ago i was experimenting with paterns, colors, and "filters". both with single arays or multi dimensional arrays when i actually bumped on a difference between linux and windows which i cant realy figure out. For all i know this is just something so simple its worthy of a black eye-inducing facepalm.

When compiled on Linux using GCC it produces the correct output while compiled on windows using MINGW it generates crooked, distorted results...
So could this be related to the Array being trashed trough the pointers, or is this related to some sort of file storage difference because i realy would not know ?

note ; offcourse one does not use the entropy on windows, one can quikly fill the 3 valleus with constants or rnd.

//
// PPM boiler plate code [cleaned]
// base code to generate an image and set some pixels.
//----------------------------------------------------------------------
// - reads linux entropy to get some color.
//----------------------------------------------------------------------
// - todo ; main counts to far for the initial setpixel.
//          1279 pixels dropped with a size of 1280x1024.
//
// - C code for the GCC compiler set on Linux. The entropy will not 
//   work on windows, and possibly the rgb array pointers wont either.
//
//----------------------------------------------------------------------
//

#include <stdio.h>

#define def_imgw 1280
#define def_imgh 1024
#define def_ibpp 3
#define def_irgbsize ((def_imgw * def_imgh) * def_ibpp)
#define def_iprecala (def_imgw * def_ibpp)
#define def_amrandom 3

char asc_header[17]= {"P6 1280 1024 255 "};
char buffer[def_irgbsize];    
int tempcheck = 0;

unsigned char entro[def_amrandom];

typedef struct {
		int r;
		int g;
		int b;
}triple_t;
	triple_t srgb;
	triple_t grgb;
    
void gentropy (unsigned char *array, int pos){

	FILE * loc_infile;
	
	loc_infile = fopen("/dev/urandom","r");
	fseek(loc_infile, pos, SEEK_SET);
	fread(array,sizeof(char),def_amrandom,loc_infile);
	fclose(loc_infile);
	
	printf("  gentropy ; finished reading Urandom Entropy \n");	
	printf("  gentropy ; Urandom Entropy buffer pos. 1 ; %d\n",array[1]);

}
    
int setpixel (char *array,int x, int y,int r, int g, int b){
 
	int loc_opticalc = ( (y * def_iprecala) + (x * def_ibpp));
	if (loc_opticalc > def_irgbsize) {tempcheck++; return 0;}
	
	array[loc_opticalc]      = r;    
	array[loc_opticalc + 1]  = g;   
	array[loc_opticalc + 2 ] = b;
return 0;               
}

int getpixel (char *array, int x, int y){
	
	int loc_opticalc = ( (y * def_iprecala) + (x * def_ibpp));
	if (loc_opticalc > def_irgbsize) {tempcheck++; return 0;}

	grgb.r = array[loc_opticalc];
	grgb.g = array[loc_opticalc + 1];
	grgb.b = array[loc_opticalc + 2];
return 0;
}

int filterfunc (char *array){

	int loc_x = def_imgw, loc_y = 0;
	
	do{
	for(loc_y = def_imgw; loc_y > 0; loc_y = loc_y - 3){
	getpixel(array,loc_x,loc_y);

	setpixel(array,loc_x,loc_y,srgb.r,srgb.g,srgb.b);
	}
	
	loc_x = loc_x -1;
	}while(loc_x > -1);
	
return 0;
}

int main() {  
	
	int loc_xcount = 0, loc_ycount = def_imgh;
	printf(" Main ; gentropy \n");
	gentropy(entro, 0); 
	printf(" Main ; setpixel \n");
	do{
		
		srgb.r = (entro[1] ^ loc_xcount);
		srgb.g = (entro[2] ^ loc_ycount);
		srgb.b = (entro[3] ^ loc_xcount);
		
		for(loc_xcount = def_imgw; loc_xcount--;){      
		setpixel (buffer,loc_xcount,loc_ycount,srgb.r,srgb.g,srgb.b);
		}  
	loc_ycount--;
	}while(loc_ycount > -1);
	printf("    setpixel test ; dropped pixels ; %i\n",tempcheck);
	printf(" finished plotting pixels. \n");	
	
	printf(" main ; filterfunc \n");	
	filterfunc (buffer);
	
	printf(" main ; Opening file, writing image. \n");	
	FILE *loc_outfile;
	loc_outfile = fopen("output.ppm","w");
	fwrite(asc_header, sizeof(char), sizeof(asc_header), loc_outfile);
	fwrite(buffer, sizeof(char), sizeof(buffer), loc_outfile);
	fclose(loc_outfile);
	printf(" main ; closed output file. \n end\n");

return 0; 		
}

I was thinking that the problem would be this when on windows while using MINGW, but then... why ?

int setpixel (char *array,int x, int y,int r, int g, int b){
 
	int loc_opticalc = ( (y * def_iprecala) + (x * def_ibpp));
	if (loc_opticalc > def_irgbsize) {tempcheck++; return 0;}
	
	array[loc_opticalc]      = r;    
	array[loc_opticalc + 1]  = g;   
	array[loc_opticalc + 2 ] = b;
return 0;               
}

Last edited by exidux (2015-10-19 08:28:09)

mpan · 2015-10-19 12:10:09

Your code is barely formatted, scaring people off from even trying to analyze it; and in the first lines of the actual code there are global variables — which are, to make it worse, used for communication between functions — causing me to make ask myself if it's even worth digging further into the code. Lines 26–37 along with the uses of these variables are enough to consider the program too error–prone for attempts to fix it in other place.

In lines 18–23 you're defining macro names in lowercase. You're asking for trouble: preprocessor does not respect compile time scopes, about which it has actually no knowledge. Macros are not constants! This is only a primitive text substitution, so naming them in the same way you name other elements of the problem will lead to errors. And, believe me, when you encounter one, the error message — if any — will be very cryptic and unhelpful. A common convention is to use all–uppercase names. So, for example, DEF_IMGW instead of def_imgw. This way you can also drop hungarian–like notation with "def_" prefix and use those 4 characters for making macro names more descriptive. Since in many languages all–uppercase–with–underscores form is used for constants, you're also making your constants immedietely recognizable to any coder.

In line 25 you're giving an explicit size of the array. There is no need to do so until you actually need an array of specific size (rarely outside microcontroller world). The initializer already provides size automagically. Setting size verbosely in such case is also making this declaration prone to errors when the code is modified or simply C syntax is misunderstood by the author… which has happened to you in this case. You're declaring an array of 17 bytes and then try to put a c–string of 18 characters in it. C silently truncates data and you're ending with a malformed data. This is not a c–string as it misses the terminating characters '\0'. And this means that any character–oriented function using asc_header will encounter out–of–bounds access and, in the best case, cause a SIGSEGV. In the same line you're also using an initializer format that is a peculiar exception to the C syntax. In all and every other case what you have written there would be an initializer for an array and at the first glance this looks like an initializer for an array of char pointers. The exception made for this sole case causes it to actually be a char* initializer and do what you want it to do, but in reality you're basing your code here on merely a history–motivated accident. The proper declaration for this should be at least:

char asc_header[] = "P6 1280 1024 255 ";

Since this is used as a constant, const would also be nice. Making this symbol internal to the compilation unit is not a bad idea too (actually the same goes to all symbols except main):

static char const asc_header[] = "P6 1280 1024 255 ";

You're later using sizeof operator on the array to obtain its length, but since you're gaining no actual advantage over just calling strlen, you may as well declare it as char const*:

static char const* const asc_header = "P6 1280 1024 255 ";

Seriously, a cost of dynamically calculating the length using strlen compared to using a constant expression is unimportant when used with fwrite to a file. The I/O part is much, much heavier. And any decent, modern compiler may actually eliminate the call to strlen easily:

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

static char const*  const testMessage = "blah blah blah";

void test(void) {
    printf("%zu", strlen(testMessage));
}

gcc-5.2.0 with -O2:

0000000000000000 <test>:
   0:	be 0e 00 00 00       	mov    $0xe,%esi
   5:	bf 00 00 00 00       	mov    $0x0,%edi
			6: R_X86_64_32	.rodata.str1.1
   a:	31 c0                	xor    %eax,%eax
   c:	e9 00 00 00 00       	jmpq   11 <test+0x11>
			d: R_X86_64_PC32	printf-0x4

mov $0xe,%esi is all that has left from both testMessage variable and a call to strlen. Even without an optimisation, the difference would be neglible — like 10% in synthetic tests, much less in reality. Until you do profiling and detect that this is an issue for your program, this is really not an issue to care about or sacrifice program clarity for it.

But the bottom line is… if you use fputs, the whole topic of strlen versus sizeof would not came up .

In line 26 you're creating a huge object as a variable. Don't do this. Use dynamic memory allocation for that purpose! Until you have some specific low–level optimization in mind global variables are not the place for that. Typically they are put in data or bss segment of executable (depending on if they're initialized or not) and either eat up executable size or occupy memory thorugh whole process lifetime. The heap is where large objects go.

In line 43 you open a binary file in text mode. Under Linux there is no difference between both modes, but this is not a panplatformic truth. Under Windows, for example, the difference exists. The same goes to line 117, where you write binary data in text mode, basically damaging it in the process.

All through your code, you're using printf to output unformatted strings. For this purpose you have unformatted output functions, like puts (whch will also add a newline to the message) or fputs. Leave printf family for the actual formattng. And if you want to output plain string using printf, do it this way:

printf("%s", "blah blah");

(bizarre, of course, but this is just a result of using wrong function for the task).

In calls to fread and fwrite you use sizeof on char. You should realize that sizeof(x) means "the number of chars occupied by x", so basically you're asking "how many chars a char occupies". Like the answer coule ever be different than 1. char is itself the unit of size in C. And even you care about size of char that much, you completly ignore the fact that whole gentropy function is tightly bound to a particular global variable, uses its hardcoded size and will never work if used with anything else. Despite it looks like it could accept a pointer to arbitrary buffer, the only one it may accept is the global entro array. And actually quite similar accusation can be made towards setpixel and getpixel.

Why are you performing a fseek on "/dev/urandom" in line 44? And, which is another curiosity, why are you performing a fseek to an arbitrary position on a file opened in text mode, while only results of ftell make any sense in such case?

Why are setpixel, getpixel and filterfunc returning a value if the only value they may return is 0 and the value is never checked anyway? In libraries one may encounter functions that have a return value reserved for future use for not–yet–known error codes, but this is not a common case for non–library programs and in your code there is nothing that produces an error code right now.

filterfunc iterates over the image in wrong order. I mean: from the perspective of program logic there is no difference, but from performance point of view there is a huge one. Data is ordered first in rows, then in columns. To maintain good reference locality the loops should keep to this convention, if possible. And here, definetly, nothing prevents you from doing so. You're wreaking havoc on the cache here and you were caring about a difference between sizeof vs strlen? Also: why are the loops using so strange order of operations? Why doing this from the end of the rows and columns instead of the straightforward way, why do…while instead of for and why so verbose expressions instead of preincrementation etc.?

Use char for textual data only. For binary be explicit on what do you want: signed char or usnigned char. The sign of char depends on a platform. Most systems use signed, but for example Microsoft — for some reason — decided that this particular integral type should be unsigned by default and so Windows used unsigned char for char (yet all other integral types are signed).

I have already said about the global variables, I'll not repeat myself — let's just assume you'll eradicate this abomination in the next version.

Also… it seems like I have too much time .

Last edited by mpan (2015-10-19 20:14:03)

exidux · 2015-10-20 08:44:38

First of all, thank you. I have saved your post to a file on disk to read trough it again at later times when i get back to this silly program.

This was written while i was attempting to read and understand some obfuscated image based source codes which might explain some of the oddities that probably seeped in because of that, while the def_ and loc_ notations where a way for me to make a normal source more readable to myself though... which might do the opposite for others. However i do easily admit that my knowledge about C has severely degraded during the last few years... which also explains a few of the other oddities.

Also, the return values where set in place to indicate the if blocks in possible future additions, extra checks. Your post was very helpful, i hope i can put it to use some time in life.

Last edited by exidux (2015-10-20 09:14:10)

Arch Linux

#1 2015-10-19 08:12:02

Wallpaper generator - skelleton code [funny litle question]

#2 2015-10-19 12:10:09

Re: Wallpaper generator - skelleton code [funny litle question]

#3 2015-10-20 08:44:38

Re: Wallpaper generator - skelleton code [funny litle question]

Board footer