[FFmpeg-devel] [PATCH] Reorganized libpostproc code

[Ronald S. Bultje]

Hi,

On Wed, Mar 11, 2015 at 2:06 AM, James Almer <jamrial at gmail.com> wrote:

> 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.


No this is not OK. No inline avx2. I spent weeks cleaning up the old crap
(in libswresample) to eventually end up with code that was 25%? faster than
the inline stuff. I don't mind cleaning up crap but let's not poop anew.
I'm happy to accept it as a qualification task completion but this code
honestly shouldn't go into the tree.

Ronald