[FFmpeg-devel] [PATCH] HAVE_AMD3DNOW

[Ivan Kalvachev]

On 6/10/09, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Tue, Jun 09, 2009 at 09:03:48PM -0400, Pavel Pavlov wrote:
>> > -----Original Message-----
>> > On Tue, Jun 09, 2009 at 04:05:14PM -0400, Pavel Pavlov wrote:
> [...]
>> > > See my previous thread: [BUG] : reading memory past the end of the
>> > > buffer At some point we started to get crashes (on some
>> > pc's/cameras
>> > > only) with code that worked for years and I decided to
>> > compile ffmpeg
>> > > in my spare time with icl to be able to debug the problem. It's not
>> > > importat why and what was crashing, the point is that it
>> > would never
>> > > crash if asm optimisations were disabled.
>> >
>> > > So, it would be nice to have a complete ffmpeg built with all
>> > > optimizations enabled.
>> >
>> > but thats not what your patch does ...
>> >
>>
>> Well, you may find one million of other reason like that one. Off course
>> than patch doesn't do even 1/10 to get it compiled with icl, however,
>> without it ffmpeg won't compile with icl. I'm trying to clean my changes
>> and step by step send it in, so they could be reviewed and accepted.
>> Even if one day I get tired to beg for it to included and unsibscribe
>> from the list to never come back here, everything I sent in will help
>> others who have similar goal.
>>
>>
>> Unsupported instructions:
>> c:/ffmpeg/libavcodec/x86/cavsdsp_mmx.c(437) (col. 1): error: unknown
>> opcode "pavgusb" -- __asm
>> c:/ffmpeg/libavcodec/x86/dsputil_mmx.c(2717) (col. 9): error: unknown
>> opcode "pf2id" -- __asm
>> c:/ffmpeg/libavcodec/x86/dsputil_mmx.c(2717) (col. 9): error: unknown
>> opcode "femms" -- __asm
>> c:/ffmpeg/libavcodec/x86/fft_3dn2.c(152) (col. 5): error: unknown opcode
>> "pfmul" -- __asm
>> c:/ffmpeg/libavcodec/x86/fft_3dn2.c(152) (col. 5): error: unknown opcode
>> "pfadd" -- __asm
>> c:/ffmpeg/libavcodec/x86/fft_3dn2.c(152) (col. 5): error: unknown opcode
>> "pfsub" -- __asm
>> c:/ffmpeg/libavcodec/x86/fft_3dn2.c(152) (col. 5): error: unknown opcode
>> "pswapd" -- __asm
>> c:/ffmpeg/libavcodec/x86/fft_3dn2.c(152) (col. 5): error: unknown opcode
>> "pfpnacc" -- __asm
>> c:/ffmpeg/libavcodec/h264.c(5459) (col. 32): error: unknown opcode
>> "cltd" -- __asm
>>
>> There are a few places that it rejects add (can't determin operand size
>> error: specify suffix, or something like that). Also, note that cltd has
>> nothing to do with 3dnow, it's real name is cdq.
>
> report it all to intel please
> the ffmpeg project does not accept workarounds for compiler bugs before
> they have been reported, that said we might not accept them afterwards
> either, but afterwards we at least know intels oppinion about it and
> can be sure theres no user error (wrong opcode name, ...)


Michael, please reconsider.
Do not label this as compiler workaround,
because this is not what this patch really does.

Having the ability to disable specific cpu optimization is actually good thing.
I've been thinking about way to remove all SSE asm, as it is
quite big portion of the dsp codebase, and I don't have it.
To me it is just a huge waste of space and (compile) time.

Well, indeed, this patch doesn't do what I want, but it is small step in the
right direction. And the developer may be willing to embrace much bigger
task, just in the name of doing the right thing (tm).

@Pavel,
I think by ugly Michael means, that the change could be made much smaller.
For example, when you have:

#if HAVE_SOMETHING
  if( caps&something) {
  ...
 }
#endif

it could be replaced by:
  if( HAVE_SOMETHING && caps&something ){
  ...
}

Such kind of coding trick is already used all over ffmpeg.

Also, in swscale patch, the first block is very suspicious.
Few lines above the HAVE_AMD3DNOW is undefined,
so the code probably doesn't do what you think it does.
(aka, it disabled 3dnow code unconditionally).

Actually whoever have messed with that file recently (or not so)
have made same mistake as you. In sws_rgb2rgb_init() function
HAVE_AMD_3DNOW is used as condition,
but it is left at 1 if the arch is X86. Well, obviously
nobody compiled it with all asm disabled.

It should probably be moved before the templete includes.
Also for the templete flags it may be better to rename them to
something neutral, so the original HAVE_<something> are not
messed with.
(Strange, I thought the swscale templete already does that).