I just spent the last half hour trying to figure out why SDL_BlitSurface was suddenly giving me only the leftmost 64 pixels of result data on a surface that’s 96 pixels wide.? I managed to track it down to the most recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some MMX fixes from Patrick Baggett.”
The above information should make it immediately obvious what the error is if you look at a before-and-after diff of the routine…
Mason
!((uintptr_t) src & 7) && !(srcskip & 7) &&
!((uintptr_t) dst & 7) && !(dstskip & 7)) {
The alignment requirement on the pointer should be removed as noted in the
patch notes, but not the srcskip/dstskip. It should read:
if(SDL_HasMMX() && !(srcskip & 7) && !(dstskip & 7)) {
Next time, why not just post this information instead of “The above
information should make it immediately obvious what the error is if you look
at a before-and-after diff of the routine…”?
PatrickOn Fri, Oct 28, 2011 at 12:09 AM, Mason Wheeler wrote:
I just spent the last half hour trying to figure out why SDL_BlitSurface
was suddenly giving me only the leftmost 64 pixels of result data on a
surface that’s 96 pixels wide. I managed to track it down to the most
recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some
MMX fixes from Patrick Baggett.”The above information should make it immediately obvious what the error is
if you look at a before-and-after diff of the routine…Mason
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
…because just in case there’s another bug, saying something like that might lead someone to find it.? Which you did; that’s not the problem I found at all! 
Again, I was trying to do a blit from one 8-bit surface to another.? The blit rectangle was 96 pixels wide.? The leftmost 64 pixels got filled in correctly, but the rightmost 32 pixels remained unchanged.________________________________
From: baggett.patrick@gmail.com (Patrick Baggett)
Subject: Re: [SDL] Error in SDL_memcpyMMX
The alignment requirement on the pointer should be removed as noted in the patch notes, but not the srcskip/dstskip. It should read:
if(SDL_HasMMX() && !(srcskip & 7) && !(dstskip & 7)) {
Next time, why not just post this information instead of"The above information should make it immediately obvious what the error is if you look at a before-and-after diff of the routine…"?
Patrick
On Fri, Oct 28, 2011 at 12:09 AM, Mason Wheeler <@Mason_Wheeler> wrote:
I just spent the last half hour trying to figure out why SDL_BlitSurface was suddenly giving me only the leftmost 64 pixels of result data on a surface that’s 96 pixels wide.? I managed to track it down to the most recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some MMX fixes from Patrick Baggett.”
The above information should make it immediately obvious what the error is if you look at a before-and-after diff of the routine…
Mason
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
…then what was the problem? I agree reviewing the patch is useful, but now
we’ve swapped emails and nobody knows exactly what you’re thinking. :\On Fri, Oct 28, 2011 at 7:25 AM, Mason Wheeler wrote:
…because just in case there’s another bug, saying something like that
might lead someone to find it. Which you did; that’s not the problem I
found at all!Again, I was trying to do a blit from one 8-bit surface to another. The
blit rectangle was 96 pixels wide. The leftmost 64 pixels got filled in
correctly, but the rightmost 32 pixels remained unchanged.
From: Patrick Baggett <@Patrick_Baggett>
**Subject: Re: [SDL] Error in SDL_memcpyMMX
- if (SDL_HasMMX() &&
!((uintptr_t) src & 7) && !(srcskip & 7) &&!((uintptr_t) dst & 7) && !(dstskip & 7)) {
- if (SDL_HasMMX()) {\
The alignment requirement on the pointer should be removed as noted in the
patch notes, but not the srcskip/dstskip. It should read:if(SDL_HasMMX() && !(srcskip & 7) && !(dstskip & 7)) {
Next time, why not just post this information instead of “The above
information should make it immediately obvious what the error is if you look
at a before-and-after diff of the routine…”?Patrick
On Fri, Oct 28, 2011 at 12:09 AM, Mason Wheeler wrote:
I just spent the last half hour trying to figure out why SDL_BlitSurface
was suddenly giving me only the leftmost 64 pixels of result data on a
surface that’s 96 pixels wide. I managed to track it down to the most
recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some
MMX fixes from Patrick Baggett.”The above information should make it immediately obvious what the error is
if you look at a before-and-after diff of the routine…Mason
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
The problem is the cleanup at the end.? The MMX blit works in multiples of 64 bits, and then at the end there’s a standard SDL_Memcpy to get whatever’s left over, except it doesn’t.
??? for(i= len / 64; i– {
???
?? ? ??? d64[0] = s64[0];
? ?? ??? d64[1] = s64[1];
?? ? ??? d64[2] = s64[2];
??? ? ?? d64[3] = s64[3];
??? ?? ? d64[4] = s64[4];
??? ? ?? d64[5] = s64[5];
??? ?? ? d64[6] = s64[6];
??? ??? d64[7] = s64[7];
???
??? ??? d64 += 8;
?? ? ??? s64 += 8;
+??? src += 64;
+??? dst += 64;???
??? }
?
??? if (len & 63)
??? SDL_memcpy(dst, src, len & 63);________________________________
From: baggett.patrick@gmail.com (Patrick Baggett)
To: Mason Wheeler <@Mason_Wheeler>; SDL Development List
Sent: Friday, October 28, 2011 6:14 AM
Subject: Re: [SDL] Error in SDL_memcpyMMX
…then what was the problem? I agree reviewing the patch is useful, but now we’ve swapped emails and nobody knows exactly what you’re thinking. :\
On Fri, Oct 28, 2011 at 7:25 AM, Mason Wheeler <@Mason_Wheeler> wrote:
…because just in case there’s another bug, saying something like that might lead someone to find it.? Which you did; that’s not the problem I found at all!
Again, I was trying to do a blit from one 8-bit surface to another.? The blit rectangle was 96 pixels wide.? The leftmost 64 pixels got filled in correctly, but the rightmost 32 pixels remained unchanged.
From: Patrick Baggett <baggett.patrick at gmail.com>
Subject: Re: [SDL] Error in SDL_memcpyMMX
- ? ?if (SDL_HasMMX() &&
- ? ? ? ?!((uintptr_t) src & 7) && !(srcskip & 7) &&
- ? ? ? ?!((uintptr_t) dst & 7) && !(dstskip & 7)) {
- ? ?if (SDL_HasMMX()) {\
The alignment requirement on the pointer should be removed as noted in the patch notes, but not the srcskip/dstskip. It should read:
if(SDL_HasMMX() && !(srcskip & 7) && !(dstskip & 7)) {
Next time, why not just post this information instead of"The above information should make it immediately obvious what the error is if you look at a before-and-after diff of the routine…"?
Patrick
On Fri, Oct 28, 2011 at 12:09 AM, Mason Wheeler <@Mason_Wheeler> wrote:
I just spent the last half hour trying to figure out why SDL_BlitSurface was suddenly giving me only the leftmost 64 pixels of result data on a surface that’s 96 pixels wide.? I managed to track it down to the most recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some MMX fixes from Patrick Baggett.”
The above information should make it immediately obvious what the error is if you look at a before-and-after diff of the routine…
Mason
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
Good catch. ;)On Fri, Oct 28, 2011 at 8:19 AM, Mason Wheeler wrote:
The problem is the cleanup at the end. The MMX blit works in multiples of
64 bits, and then at the end there’s a standard SDL_Memcpy to get whatever’s
left over, except it doesn’t.for(i= len / 64; i--;) { d64[0] = s64[0]; d64[1] = s64[1]; d64[2] = s64[2]; d64[3] = s64[3]; d64[4] = s64[4]; d64[5] = s64[5]; d64[6] = s64[6]; d64[7] = s64[7]; d64 += 8; s64 += 8;
src += 64;dst += 64;}
if (len & 63)
SDL_memcpy(dst, src, len & 63);
From: Patrick Baggett <@Patrick_Baggett>
To: Mason Wheeler ; SDL Development List <
sdl at lists.libsdl.org>
Sent: Friday, October 28, 2011 6:14 AMSubject: Re: [SDL] Error in SDL_memcpyMMX
…then what was the problem? I agree reviewing the patch is useful, but
now we’ve swapped emails and nobody knows exactly what you’re thinking. :\On Fri, Oct 28, 2011 at 7:25 AM, Mason Wheeler wrote:
…because just in case there’s another bug, saying something like that
might lead someone to find it. Which you did; that’s not the problem I
found at all!Again, I was trying to do a blit from one 8-bit surface to another. The
blit rectangle was 96 pixels wide. The leftmost 64 pixels got filled in
correctly, but the rightmost 32 pixels remained unchanged.
From: Patrick Baggett <@Patrick_Baggett>
**Subject: Re: [SDL] Error in SDL_memcpyMMX
- if (SDL_HasMMX() &&
!((uintptr_t) src & 7) && !(srcskip & 7) &&!((uintptr_t) dst & 7) && !(dstskip & 7)) {
- if (SDL_HasMMX()) {\
The alignment requirement on the pointer should be removed as noted in the
patch notes, but not the srcskip/dstskip. It should read:if(SDL_HasMMX() && !(srcskip & 7) && !(dstskip & 7)) {
Next time, why not just post this information instead of “The above
information should make it immediately obvious what the error is if you look
at a before-and-after diff of the routine…”?Patrick
On Fri, Oct 28, 2011 at 12:09 AM, Mason Wheeler wrote:
I just spent the last half hour trying to figure out why SDL_BlitSurface
was suddenly giving me only the leftmost 64 pixels of result data on a
surface that’s 96 pixels wide. I managed to track it down to the most
recent change to SDL_blit_copy.c, in function SDL_memcpyMMX, tagged as “Some
MMX fixes from Patrick Baggett.”The above information should make it immediately obvious what the error is
if you look at a before-and-after diff of the routine…Mason
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
Good catch.
Both of your fixes are now in revision control; can you two sanity-check
them for me?
Thanks,
–ryan.
What about a little unit test to sanity check it?
I know the pygame unit tests would have caught this if they were run against
it, but unfortunately it doesn’t work against SDL 1.3.
cheers,On Sat, Oct 29, 2011 at 7:16 AM, Ryan C. Gordon wrote:
Good catch.
Both of your fixes are now in revision control; can you two sanity-check
them for me?Thanks,
–ryan._____________**
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/**listinfo.cgi/sdl-libsdl.orghttp://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
Good idea. If someone sends me some UT code (or the exact API
specification for the functions we want to test), I’ll integrated it
into the test automation framework.
–AndreasOn 10/29/11 12:45 AM, Ren? Dudfield wrote:
What about a little unit test to sanity check it?
I know the pygame unit tests would have caught this if they were run
against it, but unfortunately it doesn’t work against SDL 1.3.cheers,
On Sat, Oct 29, 2011 at 7:16 AM, Ryan C. Gordon <icculus at icculus.org <mailto:icculus at icculus.org>> wrote:
Good catch. ;) Both of your fixes are now in revision control; can you two sanity-check them for me? Thanks, --ryan. _______________________________________________ SDL mailing list SDL at lists.libsdl.org <mailto:SDL at lists.libsdl.org> http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org