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

[Baptiste Coudurier]

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.

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