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 127

Summary: YUV to RGB MMX conversion code broken
Product: SDL Reporter: marchesin
Component: videoAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED DUPLICATE QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P1    
Version: HG 1.2   
Hardware: x86   
OS: Linux   

Description marchesin 2006-01-31 21:32:05 UTC
It looks like the mmx conversion code from SDL12/src/video/SDL_yuv_mmx.c is broken, at least when converting to 16bpp.

Here is the beggining of the relevant thread :
http://www.devolution.com/pipermail/sdl/2005-April/068444.html
Comment 1 Sam Lantinga 2006-02-21 15:02:41 UTC
> ld: build/.libs/SDL_yuv_mmx.o has local relocation entries in
> non-writable section (__TEXT,__text)
> /usr/bin/libtool: internal link edit command failed

> That was when linking the shared library. I think previously we just
> disabled this MMX code on the Intel Mac, so it's probably not a build
> system-specific issue, but I'm not sure.

Oh, this is beautiful.  Here's how to access those static tables in
assembly on Intel Mac:
        call    ___i686.get_pc_thunk.bx
"L00000000001$pb":
        leal    _MMX_0080w-"L00000000001$pb"(%ebx), %eax

ugh, and here's the same code for i686 Linux:
        call    .L2
.L2:
        popl    %ebx
        addl    $_GLOBAL_OFFSET_TABLE_+[.-.L2], %ebx
        movl    _MMX_0080w@GOTOFF(%ebx), %eax
        movl    %eax, -8(%ebp)
        subl    $8, %esp
        pushl   -8(%ebp)
        leal    .LC0@GOTOFF(%ebx), %eax

Nice, eh?

I'm guessing that this code just doesn't work at all for relocated
objects.  In fact, I seem to remember a bug about it...  Yep...
https://bugzilla.libsdl.org/show_bug.cgi?id=127

Doh.  We can't pass any more parameters to the assembly, right?

Oh, here's a nice tidbit from the comments in the file:
/* We don't really care about PIC - the code should be rewritten to use
   relative addressing for the static tables, so right now we take the
   COW hit on the pages this code resides. Big deal.
*/
Hah.
Comment 2 Sam Lantinga 2006-02-21 16:44:35 UTC
We could convert this file to nasm, which will support Mac Intel soon.
We'd need to convert the static data references into relocatable data references though, as described here:
http://nasm.sourceforge.net/doc/html/nasmdoc8.html#section-8.2

We could also let gcc construct the data on the stack, and reference it there instead...

In the meantime, I'm disabling the code until we get it rewritten.
Comment 3 marchesin 2006-02-21 17:07:22 UTC
nasm is definitely _not_ the right thing to do :
- we can't use it to do asm code that works on both 32 and 64 bits targets, while that's possible with gas.
- it requires an external tool and therefore is less portable than gas
- it adds even more linking tricks

Actually, the trouble with this file, which makes it necessary to use this strange parameter naming stuff is that old (2.95) gcc versions do not support enough asm parameters (10). Maybe it's time to drop gcc 2.95 support ? 

Alternatively, I think we could make those consts into an array, and hardcode the array offsets so that they end up as one asm parameter.
Comment 4 Sam Lantinga 2006-02-21 17:21:40 UTC
(In reply to comment #3)
> Alternatively, I think we could make those consts into an array, and hardcode
> the array offsets so that they end up as one asm parameter.

That seems reasonable, as long as it's documented what those offsets are referring to...
Comment 5 Sam Lantinga 2006-02-21 17:22:26 UTC
Here's some other interesting info on PIC assembly:
http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml
Comment 6 Ryan C. Gordon 2006-06-21 03:15:47 UTC
While someone will probably complain if we generally drop gcc 2.95 support, it might be worth forcibly disabling the asm on 2.95 in the configure script. These people will still get the working C fallbacks, and we can clean up the code for compilers that shipped within this decade.

--ryan.

Comment 7 Ryan C. Gordon 2007-06-02 13:58:53 UTC
Bumping a bunch of bugs to Priority 1 for consideration for the 1.2.12 release.

--ryan.

Comment 8 Ryan C. Gordon 2007-06-04 04:13:26 UTC
> Actually, the trouble with this file, which makes it necessary to use this
> strange parameter naming stuff is that old (2.95) gcc versions do not support
> enough asm parameters (10). Maybe it's time to drop gcc 2.95 support ? 

Stephane, can I get you to put together a patch for this one that uses 10 registers and just uses the C fallbacks #if (__GNUC__ < 3) ?

--ryan.

Comment 9 Ryan C. Gordon 2007-06-04 04:27:13 UTC
...actually, does Attachment #201 [details] in Bug #418 cover this issue?

--ryan.

Comment 10 marchesin 2007-06-06 14:30:20 UTC
(In reply to comment #9)
> ...actually, does Attachment #201 [details] in Bug #418 cover this issue?
> 
> --ryan.
> 

It looks like it should work (although of course it breaks gcc 2.9x, which I think people weren't prepared to do last time). I'm quite busy ATM though, so I didn't actually test it.
Comment 11 Ryan C. Gordon 2007-06-15 00:27:48 UTC
Tossing bug back to me...

--ryan.

Comment 12 Ryan C. Gordon 2007-06-15 00:36:02 UTC
Eh, tagging this as a dupe of Bug #418...we'll manage it out of there.

--ryan.



*** This bug has been marked as a duplicate of bug 418 ***