On Sun, Jan 08, 2012 at 04:49:36PM +0100, Rogier Wolff wrote:
> On Thu, Jan 05, 2012 at 01:35:50PM +0100, Rogier Wolff wrote:
> > Status update.....
Found it!
----------------------------------------------------------------------
int squash_lines(float* a, float* b, float* c, float* o, int width) {
int i;
float* pointer[3];
int count[3];
float pixel[3];
volatile int *ipixel=(int*)pixel;
int p=0;
int x=0;
int mincount;
pointer[0]=a; pointer[1]=b; pointer[2]=c;
count[0]=0; count[1]=0; count[2]=0;
while (x<width) {
mincount=0x7fffffff;
for (i=0; i<3; i++) {
if (count[i]==0) {
pixel[i]=*pointer[i]++;
if (ipixel[i]<0) {
count[i]=-ipixel[i];
pixel[i]=*pointer[i]++;
} else {
count[i]=1;
}
}
if (count[i]<mincount) mincount=count[i];
}
if (mincount>1) *(int*)&o[p++]=-mincount;
o[p++]=(float)((pixel[0]+pixel[1]+pixel[1]+pixel[2])*0.0625);
for (i=0; i<3; i++) {
count[i]-=mincount;
}
x+=mincount;
}
return p;
}
----------------------------------------------------------------------
It is a pointer aliasing issue. The compiler doesn't expect ipixel[i]
to change after an assignment to pixel[i].
Adding "volatile" to the declaration of ipixel solves it.
I'm not sure if the compiler is "allowed" to make such assumptions. Or
if the code is wrong in forgetting the "volatile".
In the way these "floats" are used, it would be neater to
change the way these are declared.
union rle_element {
float pixel;
int count;
};
int squash_lines(union rle_element* a, union rle_element* b,
union rle_element* c, union rle_element * o, int width) {
int i;
union rle_element * pointer[3];
int count[3];
union rle_element pixel[3];
int p=0;
int x=0;
int mincount;
pointer[0]=a; pointer[1]=b; pointer[2]=c;
count[0]=0; count[1]=0; count[2]=0;
while (x<width) {
mincount=0x7fffffff; // XXXX Find the appropriate header for
MAXINT
for (i=0; i<3; i++) {
if (count[i]==0) {
pixel[i]=*pointer[i]++;
if (pixel[i].count < 0) {
count[i] = -pixel[i].count;
pixel[i] = *pointer[i]++;
} else {
count[i]=1;
}
}
if (count[i]<mincount) mincount=count[i];
}
if (mincount>1) o[p++].count = -mincount;
o[p++].pixel =
((pixel[0].pixel+pixel[1].pixel+pixel[1].pixel+pixel[2].pixel)*0.0625);
for (i=0; i<3; i++) {
count[i]-=mincount;
}
x+=mincount;
}
return p;
}
I tested both above versions (i.e. with the volatile and with the
rle_element union), and both work and produce correct output.
I didn't take the time to convert everything to the rle_element code.
I used:
int squash_lines(float* a, float* b, float* c, float* o, int width) {
return squash_lines2 ((union rle_element *)a, (union rle_element *)b,
(union rle_element *)c, (union rle_element *)o, width);
}
as a compatibility layer.
After reading the wikipedia entry about pointer aliasing, it seems to
imply that aliasing could occur on function arguments. But as in this
case the problem lies within a single function, this hints that the
aliasing in this case is "the compilers problem". So in principle the
code is correct, and gcc over-optimizes because it gets an aliasing
issue wrong.
Still I think rewriting the code with the union construct would be
neater.
Roger.
--
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
** Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
The plan was simple, like my brother-in-law Phil. But unlike
Phil, this plan just might work.
--
You received this message because you are subscribed to the Google Groups
"Hugin and other free panoramic software" group.
A list of frequently asked questions is available at:
http://wiki.panotools.org/Hugin_FAQ
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at http://groups.google.com/group/hugin-ptx
