[patch] Alpha blending bug in BlitRGBtoRGBPixelAlphaMMX

[SDLBugzilla]

This bug report was migrated from our old Bugzilla tracker.

Reported in version: HG 1.2
Reported for operating system, platform: All, x86

Comments on the original bug report:

On 2007-02-04 15:49:42 +0000, Alex Volkov wrote:

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):"

On 2007-02-04 16:00:39 +0000, Alex Volkov wrote:

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--) {

On 2007-04-04 02:36:30 +0000, Ryan C. Gordon wrote:

Fixed in svn revision # 3006 for the 1.2 branch and # 3007 for the 1.3 branch.

Thanks!

--ryan.