Better fix for [bug 829] (same surface display corruption)

[Tony Lambregts]

Lionel Ulmer wrote:

>>I took Lionel's sugestions to heart and came up with a better fix for 
>>bug 829. This has a better check for defining when the corruption could 
>>occur and should be a little faster
>>    
>>
>
>Well, I had not in mind exactly what you did.
>
>  
>
>>+        SameSurfaceOK = (!((src == iface)&&(sbase < dbuf)&&(xdst.top < xsrc.bottom)&&(xdst.left < xsrc.right)));
>>    
>>
>
>I do not really understand all the details of this check, so a comment would
>be nice, but well, I think it is unneeded (see below).
>
Well I suppose a comment would be nice. The problem with this corruption 
is that  it only occures when the following are true

1.) Both the source and destination are the same surface. -->(src == iface)
2.) The start of the source is less than the destination .  -->(sbase < 
dbuf)
3.) There is overlap.  -->(xdst.top < xsrc.bottom)&&(xdst.left < xsrc.right)

If we were copying one pixle at a time then my soloution needs on more 
check and that is:

4.) Is the top of the source the same as the top of the destination. 
(xdst.top > xsrc.top)

checking for this situation ......

Damb...! When I checked this out memcpy caused a problem,  but  memove 
does not!  # 4.) is invalid. One more time this time with comments....

>
>  
>
>>+                    if (SameSurfaceOK) {
>>+                       for (y = 0; y < dstheight; y++) {
>>+                          memcpy(dbuf, sbuf, width);
>>+                          sbuf += sdesc.u1.lPitch;
>>+                          dbuf += ddesc.u1.lPitch;
>>+                       }
>>+                    } else {
>>+                       
>>+                       sbuf += (sdesc.u1.lPitch*dstheight);
>>+                       dbuf += (ddesc.u1.lPitch*dstheight);
>>+
>>+                       for (y = 0; y < dstheight; y++) {
>>+                          sbuf -= sdesc.u1.lPitch;
>>+                          dbuf -= ddesc.u1.lPitch;
>>+                          memcpy(dbuf, sbuf, width);
>>+                       }
>>+                    }
>>+
>>    
>>
>
>The problem I can see is that you use 'memcpy' even in the SameSurfaceNotOK
>case. And this is bad as memcpy does NOT handle overlapping memory zones.
>Moreover, you only have the 'descending' copy order in that case and not
>both cases.
>
>What I had in mind in my comments to your patch was something like that :
>
> if (src != iface)
>    Do the standard memcpy code path
> else
>    if source_Y > dest_Y
>      do the ascending memmove copy
>    else
>      do the descending memmove copy
>
I had it coded kinda that way for a while.....

>
>This way you did not hurt the common case (ie Blt from two different
>surfaces). Now I agree that even in the same surface case, one could add
>some checks to see if the rectangles overlap or not, but I do not know if it
>would be worth the pain as memmove should not be THAT slower than memcpy.
>
>                     Lionel
>
>  
>
I appreciate your comments. The reason I wanted the checks is that the 
same problems can occure with flags set.  I therefore wanted to make 
sure that the fixme was only produced when those conditions were met. I 
believe that it would be unlikely for anyone to use the same surface if 
stretching is involved, so there is no fixme there....

This whole problem could be avoided if we were working with a copy of 
the source.  This may be a better solution in the long run...

By the way how did it it work out with FalloutTactics.


Tony Lambregts