[FFmpeg-devel] [PATCH] Reorganized libpostproc code

[James Almer]

On 11/03/15 12:57 AM, Tucker DiNapoli wrote:
> From: Tucker DiNapoli <T.DiNapoli42 at gmail.com>

A couple comments below.

> 
> The only changes were formating and moving code.

This needs to be split into several different patches. Also, why does "moving code" end up with the 
addition of one thousand new lines?

> ---
>  libpostproc/postprocess.c          |  436 ++----------
>  libpostproc/postprocess_c.c        | 1328 ++++++++++++++++++++++++++++++++++++
>  libpostproc/postprocess_internal.h |   30 +-
>  libpostproc/postprocess_template.c |  124 ++--
>  4 files changed, 1468 insertions(+), 450 deletions(-)
>  create mode 100644 libpostproc/postprocess_c.c

If you want to properly refactor the code, it may be a good chance to move the target specific 
assembly to their own separate folders (x86, ppc, etc).
Look at the other libraries to see how it's normally done.

Admittedly outside of the scope of your qualification task, so don't worry too much for now.

[...]

> @@ -573,8 +217,13 @@ static av_always_inline void do_a_deblock_C(uint8_t *src, int step,
>  #        include "postprocess_template.c"
>  #        define TEMPLATE_PP_SSE2 1
>  #        include "postprocess_template.c"
> +#        define TEMPLATE_PP_AVX2 1
> +#        include "postprocess_template.c"
>  #    else
> -#        if HAVE_SSE2_INLINE
> +#        if HAVE_AVX2_INLINE

I'm not sure people will be happy with inline AVX2 code being introduced to the tree. Last time i 
tried with AVX it was not well received and eventually replaced with a yasm port.
I guess it's an acceptable start, considering at some point all the asm should be ported to yasm 
anyway.

Nonetheless, the addition of an avx2 codepath needs to be in a separete patch, preferably alongside 
some actual avx2 assembly (This patch is adding a bunch of "avx2" functions that contain duplicated 
mmx and sse2 code).

> +#            define TEMPLATE_PP_AVX2 1
> +#            include "postprocess_template.c"
> +#        elif HAVE_SSE2_INLINE
>  #            define TEMPLATE_PP_SSE2 1
>  #            include "postprocess_template.c"
>  #        elif HAVE_MMXEXT_INLINE
> @@ -593,10 +242,10 @@ static av_always_inline void do_a_deblock_C(uint8_t *src, int step,
>  typedef void (*pp_fn)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height,
>                        const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2);
>  
> -static inline void postProcess(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height,
> +static inline void post_process(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height,
>          const QP_STORE_T QPs[], int QPStride, int isColor, pp_mode *vm, pp_context *vc)
>  {
> -    pp_fn pp = postProcess_C;
> +    pp_fn pp = post_process_C;
>      PPContext *c= (PPContext *)vc;
>      PPMode *ppMode= (PPMode *)vm;
>      c->ppMode= *ppMode; //FIXME
> @@ -605,24 +254,27 @@ static inline void postProcess(const uint8_t src[], int srcStride, uint8_t dst[]
>  #if CONFIG_RUNTIME_CPUDETECT
>  #if ARCH_X86 && HAVE_INLINE_ASM
>          // ordered per speed fastest first
> -        if      (c->cpuCaps & AV_CPU_FLAG_SSE2)     pp = postProcess_SSE2;
> -        else if (c->cpuCaps & AV_CPU_FLAG_MMXEXT)   pp = postProcess_MMX2;
> -        else if (c->cpuCaps & AV_CPU_FLAG_3DNOW)    pp = postProcess_3DNow;
> -        else if (c->cpuCaps & AV_CPU_FLAG_MMX)      pp = postProcess_MMX;
> +        if      (c->cpuCaps & AV_CPU_FLAG_AVX2)     pp = post_process_AVX2;
> +        else if (c->cpuCaps & AV_CPU_FLAG_SSE2)     pp = post_process_SSE2;
> +        else if (c->cpuCaps & AV_CPU_FLAG_MMXEXT)   pp = post_process_MMX2;
> +        else if (c->cpuCaps & AV_CPU_FLAG_3DNOW)    pp = post_process_3DNow;
> +        else if (c->cpuCaps & AV_CPU_FLAG_MMX)      pp = post_process_MMX;
>  #elif HAVE_ALTIVEC
> -        if      (c->cpuCaps & AV_CPU_FLAG_ALTIVEC)  pp = postProcess_altivec;
> +        if      (c->cpuCaps & AV_CPU_FLAG_ALTIVEC)  pp = post_process_altivec;

Cosmetics belong in a separate patch, if anything to make the diff of the actual code changes 
smaller and easier to read.