On Sat, Jan 05, 2013 at 12:48:58PM -0500, Daniel Kang wrote: > On Sat, Jan 5, 2013 at 12:29 PM, Diego Biurrun <[email protected]> wrote: > > On Sat, Jan 05, 2013 at 11:01:19AM -0600, Daniel Kang wrote: > >> > >> --- a/libavfilter/x86/yadif.c > >> +++ b/libavfilter/x86/yadif.c > >> av_cold void ff_yadif_init_x86(YADIFContext *yadif) > >> { > >> int cpu_flags = av_get_cpu_flags(); > >> > >> -#if HAVE_MMXEXT_INLINE > >> +#if HAVE_YASM > >> if (cpu_flags & AV_CPU_FLAG_MMXEXT) > >> - yadif->filter_line = yadif_filter_line_mmxext; > >> -#endif > >> -#if HAVE_SSE2_INLINE > >> + yadif->filter_line = ff_yadif_filter_line_mmxext; > >> if (cpu_flags & AV_CPU_FLAG_SSE2) > >> - yadif->filter_line = yadif_filter_line_sse2; > >> -#endif > >> -#if HAVE_SSSE3_INLINE > >> + yadif->filter_line = ff_yadif_filter_line_sse2; > >> if (cpu_flags & AV_CPU_FLAG_SSSE3) > >> - yadif->filter_line = yadif_filter_line_ssse3; > >> + yadif->filter_line = ff_yadif_filter_line_ssse3; > >> #endif > > > > These could likely use HAVE_EXTERNAL_MMXEXT, etc... > > Maybe I'm missing something? > > AVX@AVX-PC /cygdrive/c/Code/libav > $ git grep HAVE_EXTERNAL > <nothing>
I confused the order, it's HAVE_MMXEXT_EXTERNAL, etc... > >> --- /dev/null > >> +++ b/libavfilter/x86/yadif_yasm.asm > >> @@ -0,0 +1,246 @@ > >> + psrlq m5, 8 > >> +%endif > >> +%if mmsize == 16 > >> + psrldq m3, 1 > >> + psrldq m4, 2 > >> +%else > >> + psrlq m3, 8 > >> + psrlq m4, 16 > >> +%endif > > > > idea (untested): > > > > %macro PSRLQ 2 > > %if mmsize == 16 > > psrlq %1, %2 * 8 > > %else > > psrlq %1, %2 > > %endif > > I don't think it's worth it for those couple of lines. I find ifdefs very ugly and prefer the macro. I'm not going to insist. > >> +%macro CHECK1 0 > >> +%endmacro > >> + > >> +%macro CHECK2 0 > >> +%endmacro > > > > These names are not terribly descriptive. > > They weren't in the original file. I have no idea what to call them. Maybe ask Loren? > >> +%macro LOAD 2 > >> + movh m%1, %2 > >> + punpcklbw m%1, m7 > >> +%endmacro > >> + > >> +%macro ABSY 1-2 > >> +%if cpuflag(ssse3) > >> + pabsw %1, %1 > >> +%else > >> + ABS1_MMXEXT %1, %2 > >> +%endif > >> +%endmacro > > > > Is this a candidate for simply extending ABS1_MMXEXT? > > No. The MMXEXT is there for a reason. I was being totally unclear, as I was referring to some changes I only have locally. Please review the following patch: http://patches.libav.org/patch/25264/ > >> +%if mmsize == 16 > >> + mova m3, m2 > >> + psrldq m3, 2 > >> +%else > >> + pshufw m3, m2, 9 > >> +%endif > > > > Didn't we have a macro for this? > > If we do, I can't find it. h264_intrapred.asm has some very similar code that likely should be extracted into a common macro. Diego _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
