On 11/03/15 12:57 AM, Tucker DiNapoli wrote:
> From: Tucker DiNapoli <t.dinapol...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
