[FFmpeg-devel] [PATCH] yadif sse2/ssse3

[Luca Barbato]

On 12/01/2010 12:10 AM, Baptiste Coudurier wrote:
> On 11/30/2010 03:05 PM, Ronald S. Bultje wrote:
>> Hi,
>>
>> On Tue, Nov 30, 2010 at 6:00 PM, Baptiste Coudurier
>> <baptiste.coudurier at gmail.com>  wrote:
>>> On 11/30/2010 07:04 AM, Ronald S. Bultje wrote:
>>>>
>>>> Hi Baptiste,
>>>>
>>>> On Sat, Nov 20, 2010 at 9:32 PM, Baptiste Coudurier
>>>> <baptiste.coudurier at gmail.com>    wrote:
>>>>>
>>>>> Hi guys,
>>>>
>>>> Thanks for doing this. A few generic comments (didn't look at the code
>>>> itself yet, I'm sorry):
>>>>
>>>>> +#if HAVE_SSE
>>>>
>>>> I consider the HAVE_xxx namespace reserved for configure:
>>>>
>>>> bash-3.2$ grep HAVE_MMX config.h
>>>> #define HAVE_MMX 1
>>>> #define HAVE_MMX2 1
>>>> bash-3.2$ grep HAVE_SSE config.h
>>>> #define HAVE_SSE 1
>>>> bash-3.2$
>>>>
>>>> So I'd prefer if you'd use something else for the template naming (I
>>>> don't really know what, maybe peek at the template files in
>>>> libavcodec/x86/?). Same for HAVE_SSSE3 and so on.
>>>
>>> Huh ?
>>>
>>> #if HAVE_SSE2
>>> #define MMREG_WIDTH "16"
>>> #define MM "%%xmm"
>>> #define MOVQ "movdqa"
>>> #define SPREADW(a) \
>>>             "pshuflw $0, "a", "a"       \n\t"\
>>>             "punpcklwd "a", "a"         \n\t"
>>> #define PMAXW(a,b) "pmaxsw "a", "b"     \n\t"
>>> #define PMAX(a,b) \
>>>
>>> in mpegvideo_template_mmx.c
>>>
>>>>> +#define MM "%%xmm"
>>>>> +#define MOV  "movq"
>>>>> +#define MOVQ "movdqa"
>>>>> +#define MOVQU "movdqu"
>>>>> +#define STEP 8
>>>>
>>>> This is a good idea - our external asm code already does this. Maybe
>>>> (I'm not forcing, just kindly suggesting) it's a good idea to use the
>>>> same names for the same things in both internal or external asm. MM is
>>>> called m in .asm files, so you could use M##<num>. MOV is called movh
>>>> (H=half, either movd or movq), whereas MOVQ is called mova (mov full
>>>> aligned, either movq or movdqa), and MOVQU is movu (mov full
>>>> unaligned, either movq or movdqu). STEP*2 is called mmsize (16 or 8).
>>>>
>>>> +#define PSRL1(reg) "psrldq $1, "reg" \n\t"
>>>> +#define PSRL2(reg) "psrldq $2, "reg" \n\t"
>>>> +#define PSHUF(src,dst) "movdqa "dst", "src" \n\t"\
>>>> +                       "psrldq $2, "src"     \n\t"
>>>>
>>>> Could you just define the suffix here (dq vs q) instead of having full
>>>> defines for the whole thing?
>>>
>>> Please let's not start a bikeshed discussion once again.
>>> Your comments are cosmetics only, feel free to change the code once
>>> it is in
>>> svn. Thanks for understanding.
>>
>> ??? Why are you even sending patches for review if any issue raised is
>> automatically classified as cosmetic ???
>>
>> The config.h issue is not cosmetic. Please change it. You're
>> overriding config.h defines, that's bad. There's nothing cosmetic
>> about it.
> 
> What are you talking about ?
> 
> This is the way it is done in libavcodec/x86/mpegvideo_mmx.c

Thank you for pointing out, we fixed it in swscale but we overlook that one.

> #if HAVE_SSSE3
> #define HAVE_SSSE3_BAK
> #endif
> #undef HAVE_SSSE3
> #define HAVE_SSSE3 0
> 
> #undef HAVE_SSE2
> #undef HAVE_MMX2
> #define HAVE_SSE2 0
> #define HAVE_MMX2 0
> #define RENAME(a) a ## _MMX
> #define RENAMEl(a) a ## _mmx
> #include "mpegvideo_mmx_template.c"
> 
> #undef HAVE_MMX2
> #define HAVE_MMX2 1
> #undef RENAME
> #undef RENAMEl
> #define RENAME(a) a ## _MMX2
> #define RENAMEl(a) a ## _mmx2
> #include "mpegvideo_mmx_template.c"
> 
> Did you even have a look before replying to my mail ?
> 
>> I guess I won't have to actually review the code since clearly any
>> issue raised is cosmetic and I can just change it without sending
>> patches, just like you should just commit without sending patches,
>> since clearly and evidently, any issue raised is only cosmetic and can
>> be changed once committed.
> 
> Cosmetic is cosmetic, I answered the issue about config.h, WTF are you
> talking about ?

It isn't cosmetic since overriding config.h definitions might lead to
unexpected results. Sorry for not having stated that better when the
issue cropped up in swscale and for not having fixed it on all the
sources using the same pattern.

Please use COMPILE_TEMPLATE_extension instead of HAVE_extension

lu

-- 

Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero