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

[Ronald S. Bultje]

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.

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.

Ronald