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

[Baptiste Coudurier]

On 12/1/10 1:00 AM, Luca Barbato wrote:
> 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.

Yes, I know that, and I asked about it.

> 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.

:) Don't need to be sorry.

> Please use COMPILE_TEMPLATE_extension instead of HAVE_extension

Ok, thanks, I'll submit an updated patch.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org