We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 390

Summary: [patch] Alpha blending bug in BlitRGBtoRGBPixelAlphaMMX
Product: SDL Reporter: Alex Volkov <codepro>
Component: videoAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: vapier
Version: HG 1.2   
Hardware: x86   
OS: All   

Description Alex Volkov 2007-02-04 15:49:42 UTC 
The Ashift bug in BlitRGBtoRGBPixelAlphaMMX3DNOW() has been corrected in SVN at r2914, however, the same bug is still present in BlitRGBtoRGBPixelAlphaMMX() (non-3DNOW version). My report and patch to the list went unnoticed, so I'll just put everything here.

Some brief bug history:

Original report from Gabriel Gambetta on 11/16/2006:

[...] I tracked down the alpha blending bug I referred to in my previous email
to BlitRGBtoRGBPixelAlphaMMX3DNOW(), GCC_ASMBLIT version. The bug is in
the branch that actually blends two pixels
[...]
It turns out mm5 never got the correct value. This failed :

	[...]
	"movd %1, %%mm5\n\t"
	: : "m" (amask), "m" (sf->Ashift) );

Ryan C. Gordon wrote:

"I don't think it's an alignment issue, since GCC should be smart enough 
to get it into a register even if unaligned...however, sf->AShift is 8 
bits, and ashift is 32, which is probably why you get different 
results...it probably landed in (say) %al instead of %eax, leaving junk 
in the rest of the register's bits. [...]"

Frank Mehnert wrote:

"The original code takes the _address_ of the sf->Ashift variable and passes
this address to the assembler, because the 'm' constraint is used! Therefore
the movd instruction will not only load the content of sf->Ashift into
the mm5 register but also the following bits of the SDL_Pixelformat
structure (the lower three bytes of Rmask). The original propose of using the
local ashift variable (which is 32 bits wide) as input for the assembler
statement was correct.

To force gcc to load sf->Amask into a register you could also use

-         : /* nothing */ : "m" (sf->Amask), "m" (sf->Ashift) );
+         : /* nothing */ : "r" (sf->Amask), "r" ((Uint32) sf->Ashift) );

Note the "r" constraint."

Alex Volkov (myself) wrote:

"This bug is actually my fault, and I just stumbled upon it myself a couple
days ago. I apologize for that, as I should have known better to
double-check the data type of Ashift.
IMHO, Frank's and Ryan's version of the fix is better with some minor
modifications to allow the compiler a bit more freedom.

Also another function has to be patched in a similar way -- the pure MMX (non-3DNow!) one, gcc version of BlitRGBtoRGBPixelAlphaMMX.
In light of all this I propose the following patch (also attached):"
Comment 1 Alex Volkov 2007-02-04 16:00:39 UTC 
It seems Bugzilla fails right now when attempting to create an attachment, so here is the patch:

Index: SDL_blit_A.c
===================================================================
--- SDL_blit_A.c	(revision 2952)
+++ SDL_blit_A.c	(working copy)
@@ -369,7 +369,9 @@
 	packsswb_r2r(mm6, mm3); /* 0000FFFF -> mm3 */
 	pxor_r2r(mm0, mm3); /* 0000F000 -> mm3 (~channel mask) */
 	/* get alpha channel shift */
-	movd_m2r(sf->Ashift, mm5); /* Ashift -> mm5 */
+	__asm__ __volatile__ (
+		"movd %0, %%mm5"
+		: : "rm" ((Uint32) sf->Ashift) ); /* Ashift -> mm5 */
 
 	while(height--) {
 	    DUFFS_LOOP4({
@@ -1566,7 +1568,6 @@
 	int dstskip = info->d_skip >> 2;
 	SDL_PixelFormat* sf = info->src;
 	Uint32 amask = sf->Amask;
-	Uint32 ashift = sf->Ashift;
 
 	__asm__ (
 	/* make mm6 all zeros. */
@@ -1588,7 +1589,7 @@
 	/* get alpha channel shift */
 	"movd      %1, %%mm5\n\t" /* Ashift -> mm5 */
 
-	  : /* nothing */ : "m" (amask), "m" (ashift) );
+	  : /* nothing */ : "rm" (amask), "rm" ((Uint32) sf->Ashift) );
 
 	while(height--) {
Comment 2 Ryan C. Gordon 2007-04-04 02:36:30 UTC 
Fixed in svn revision #3006 for the 1.2 branch and #3007 for the 1.3 branch.

Thanks!

--ryan.