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

Baptiste Coudurier baptiste.coudurier
Wed Dec 1 00:10:01 CET 2010

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

#define HAVE_SSSE3_BAK
#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 ?

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

More information about the ffmpeg-devel mailing list