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).
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
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
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
Lionel Ulmer lionel.ulmer@free.fr writes:
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
Do you actually need memmove in all these cases? It seems to me that it would only be needed if source_Y == dest_Y.
On Fri, Jul 05, 2002 at 12:50:07PM -0700, Alexandre Julliard wrote:
Lionel Ulmer lionel.ulmer@free.fr writes:
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
Do you actually need memmove in all these cases? It seems to me that it would only be needed if source_Y == dest_Y.
Yeah, you're right. So the thing should be :
if ((not on the same surface) OR (src_Y > dst_Y) OR ((src_Y == dst_Y) AND ((src_rect_Right <= dst_rect_Left) OR (dst_rect_Right <= src_rect_Left)))) ascending memcpy path else if src_Y == dst_Y memmove path else descending memcpy path
And the code should even be understandable :-)
Lionel
-
Alexandre Julliard -
Lionel Ulmer -
Tony Lambregts