Re: [AD] rotate.c hang fix

[ Thread Index | Date Index | More lists.liballeg.org/allegro-developers Archives ]


Eric Botcazou wrote:
I can't reproduce on x86 (compiled with GCC), but the x86 has a notoriously
weird FPU (i.e non IEEE-754 compliant) which may hide the bug.

Yes, this may explain it, because spr_dx/spr_dy are computed using floating numbers. Or, the bug may simply be so rarely exposed that reproducing it is very hard.

[snip]
There are three other similar non-symmetrical conditions just below, so I think the attached patch is more complete.

Sven, what do think about this?

Although I don't know any example where the bug occurs, I think I understand what the problem might be, and in this case I think your patch fixed it. Nice!

I attach another patch that does two things to rotate.c:

 (1) It improves "safety" one step more: by moving the
`if (l_bmp_x_rounded > r_bmp_x_rounded)' tests inside the loops, it guarantees that the loops never do more iterations than the width of the scanline. Although your patch already fixes the real problem, this makes the number of iterations independent of the precision of numbers, for sure. And it is easy to see that the new code does the same thing as the old.

(2) It replaces `while(){}' by `do{}while()'. This is clearly valid because the condition has already been tested. Although it's ridiculous as an optimization, I think it also serves as a clarification.

Can I commit it?

--
Sven
Index: src/rotate.c
===================================================================
RCS file: /cvsroot/alleg/allegro/src/rotate.c,v
retrieving revision 1.15
diff -c -r1.15 rotate.c
*** src/rotate.c	1 Oct 2003 10:24:06 -0000	1.15
--- src/rotate.c	4 Oct 2003 19:10:09 -0000
***************
*** 517,529 ****
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  while ((unsigned)(l_spr_x_rounded >> 16) >=
! 			 (unsigned)spr->w) {
  		     l_spr_x_rounded += spr_dx;
  		     l_bmp_x_rounded += 65536;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	    }
  	    right_edge_test = l_spr_x_rounded +
--- 517,530 ----
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  do {
  		     l_spr_x_rounded += spr_dx;
  		     l_bmp_x_rounded += 65536;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(l_spr_x_rounded >> 16) >=
! 			   (unsigned)spr->w);
! 
  	       }
  	    }
  	    right_edge_test = l_spr_x_rounded +
***************
*** 533,545 ****
  	       if (((right_edge_test < 0) && (spr_dx <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dx >= 0))) {
  		  /* This can happen. */
! 		  while ((unsigned)(right_edge_test >> 16) >=
! 			 (unsigned)spr->w) {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dx;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
--- 534,546 ----
  	       if (((right_edge_test < 0) && (spr_dx <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dx >= 0))) {
  		  /* This can happen. */
! 		  do {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dx;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(right_edge_test >> 16) >=
! 			   (unsigned)spr->w);
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
***************
*** 554,566 ****
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  while (((unsigned)l_spr_y_rounded >> 16) >=
! 			 (unsigned)spr->h) {
  		     l_spr_y_rounded += spr_dy;
  		     l_bmp_x_rounded += 65536;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	    }
  	    right_edge_test = l_spr_y_rounded +
--- 555,567 ----
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
! 		  do {
  		     l_spr_y_rounded += spr_dy;
  		     l_bmp_x_rounded += 65536;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while (((unsigned)l_spr_y_rounded >> 16) >=
! 			   (unsigned)spr->h);
  	       }
  	    }
  	    right_edge_test = l_spr_y_rounded +
***************
*** 570,582 ****
  	       if (((right_edge_test < 0) && (spr_dy <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dy >= 0))) {
  		  /* This can happen. */
! 		  while ((unsigned)(right_edge_test >> 16) >=
! 			 (unsigned)spr->h) {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dy;
! 		  }
! 		  if (l_bmp_x_rounded > r_bmp_x_rounded)
! 		     goto skip_draw;
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */
--- 571,583 ----
  	       if (((right_edge_test < 0) && (spr_dy <= 0)) ||
  		   ((right_edge_test > 0) && (spr_dy >= 0))) {
  		  /* This can happen. */
! 		  do {
  		     r_bmp_x_rounded -= 65536;
  		     right_edge_test -= spr_dy;
! 		     if (l_bmp_x_rounded > r_bmp_x_rounded)
! 			goto skip_draw;
! 		  } while ((unsigned)(right_edge_test >> 16) >=
! 			   (unsigned)spr->h);
  	       }
  	       else {
  		  /* I don't think this can happen, but I can't prove it. */


Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/