Hi, On Tue, Jul 31, 2012 at 3:43 PM, Diego Biurrun <[email protected]> wrote: > On Wed, Jul 25, 2012 at 08:30:19PM -0700, Ronald S. Bultje wrote: >> >> --- a/libavcodec/dct-test.c >> +++ b/libavcodec/dct-test.c >> @@ -85,7 +85,7 @@ static const struct algo fdct_tab[] = { >> { "IJG-AAN-INT", ff_fdct_ifast, SCALE_PERM }, >> { "IJG-LLM-INT", ff_jpeg_fdct_islow_8, NO_PERM }, >> >> -#if HAVE_MMX >> +#if HAVE_MMX && HAVE_INLINE_ASM >> { "MMX", ff_fdct_mmx, NO_PERM, AV_CPU_FLAG_MMX >> }, >> { "MMX2", ff_fdct_mmx2, NO_PERM, AV_CPU_FLAG_MMX2 >> }, >> { "SSE2", ff_fdct_sse2, NO_PERM, AV_CPU_FLAG_SSE2 >> }, > > Drop this controversial hunk from the patch for now, the rest is OK. > >> --- a/libavcodec/x86/dsputilenc_mmx.c >> +++ b/libavcodec/x86/dsputilenc_mmx.c >> @@ -1194,6 +1179,35 @@ void ff_dsputilenc_init_mmx(DSPContext* c, >> AVCodecContext *avctx) >> + >> +#if HAVE_YASM >> + if (mm_flags & AV_CPU_FLAG_MMX) { >> + c->hadamard8_diff[0] = ff_hadamard8_diff16_mmx; >> + c->hadamard8_diff[1] = ff_hadamard8_diff_mmx; >> + >> + if (mm_flags & AV_CPU_FLAG_MMX2) { >> + c->hadamard8_diff[0] = ff_hadamard8_diff16_mmx2; >> + c->hadamard8_diff[1] = ff_hadamard8_diff_mmx2; >> + } >> + >> + if (mm_flags & AV_CPU_FLAG_SSE2){ >> + c->sse[0] = ff_sse16_sse2; >> + >> +#if HAVE_ALIGNED_STACK >> + c->hadamard8_diff[0] = ff_hadamard8_diff16_sse2; >> + c->hadamard8_diff[1] = ff_hadamard8_diff_sse2; >> +#endif >> + } >> + >> +#if HAVE_SSSE3 && HAVE_ALIGNED_STACK >> + if (mm_flags & AV_CPU_FLAG_SSSE3) { >> + c->hadamard8_diff[0] = ff_hadamard8_diff16_ssse3; >> + c->hadamard8_diff[1] = ff_hadamard8_diff_ssse3; >> + } >> +#endif >> + } >> +#endif /* HAVE_YASM */ > > Why do you stack the ifs? It could be written more compact: > > #if HAVE_YASM > #if HAVE_SSSE3 && HAVE_ALIGNED_STACK > if (mm_flags & AV_CPU_FLAG_SSSE3) { > c->hadamard8_diff[0] = ff_hadamard8_diff16_ssse3; > c->hadamard8_diff[1] = ff_hadamard8_diff_ssse3; > } else > #endif > if (mm_flags & AV_CPU_FLAG_SSE2){ > c->sse[0] = ff_sse16_sse2; > #if HAVE_ALIGNED_STACK > c->hadamard8_diff[0] = ff_hadamard8_diff16_sse2; > c->hadamard8_diff[1] = ff_hadamard8_diff_sse2; > #endif > } else if (mm_flags & AV_CPU_FLAG_MMX2) { > c->hadamard8_diff[0] = ff_hadamard8_diff16_mmx2; > c->hadamard8_diff[1] = ff_hadamard8_diff_mmx2; > } else if (mm_flags & AV_CPU_FLAG_MMX) { > c->hadamard8_diff[0] = ff_hadamard8_diff16_mmx; > c->hadamard8_diff[1] = ff_hadamard8_diff_mmx; > } > #endif /* HAVE_YASM */
This sucks if you're debugging a particular mismatch or something along those lines and have to disable individual functions. The original concept means you can just comment out 1 line and re-run. Here, you have to re-arrange everything. Can we please leave structure of asm init functions out of this discussion? I'm leaving it exactly as it was before, so if you want to change that, it should be discussed separately and done on its own merit, not within this discussion. Ronald _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
