[FFmpeg-devel] [PATCH V7 1/2] libswscale/x86/yuv2rgb: Change inline assembly into nasm code

[Fu, Ting]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Wednesday, January 15, 2020 05:55 AM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH V7 1/2] libswscale/x86/yuv2rgb: Change
> inline assembly into nasm code
> 
> On Fri, Jan 10, 2020 at 01:38:15AM +0800, Ting Fu wrote:
> > Signed-off-by: Ting Fu <ting.fu at intel.com>
> > ---
> > V7:
> >     Fix compile issue when user configure with --disable-mmx.
> >     Fix issue when running ./ffmpeg with --cpuflags mmx/ssse3.
> >     Adjust the SIMD verify logic in libswscale/x86/yuv2rgb.c
> >
> >  libswscale/x86/Makefile           |   1 +
> >  libswscale/x86/swscale.c          |  16 +-
> >  libswscale/x86/yuv2rgb.c          |  66 ++---
> >  libswscale/x86/yuv2rgb_template.c | 467 ++++++------------------------
> >  libswscale/x86/yuv_2_rgb.asm      | 270 +++++++++++++++++
> >  5 files changed, 405 insertions(+), 415 deletions(-)  create mode
> > 100644 libswscale/x86/yuv_2_rgb.asm
> 
> The commit message seems a bit terse
> I think it should say if the sequence of instructions is unchanged and if it was
> benchmaked. If its the same speed, when the code is run the commit message
> should say that too
> 
> the principle of this (inline -> nasm) is fine of course.
> 
Hi Michael,

Got it, will add more infos in next patch version.

> 
> >
[...]
> > -                    break;
> > -                } else
> > -                    return yuv420_bgr32_mmx;
> > -            case AV_PIX_FMT_RGB24:
> > -                return yuv420_rgb24_mmx;
> > -            case AV_PIX_FMT_BGR24:
> > -                return yuv420_bgr24_mmx;
> > -            case AV_PIX_FMT_RGB565:
> > -                return yuv420_rgb16_mmx;
> > -            case AV_PIX_FMT_RGB555:
> > -                return yuv420_rgb15_mmx;
> > +                break;
> > +            } else
> > +                return yuv420_bgr32_mmx;
> > +        case AV_PIX_FMT_RGB24:
> > +            return yuv420_rgb24_mmx;
> > +        case AV_PIX_FMT_BGR24:
> > +            return yuv420_bgr24_mmx;
> > +        case AV_PIX_FMT_RGB565:
> > +            return yuv420_rgb16_mmx;
> > +        case AV_PIX_FMT_RGB555:
> > +            return yuv420_rgb15_mmx;
> >          }
> >      }
> 
> this is a little messy to review
> it is mostly reindention
> yuv2rgb.c          |   66 +++----
> and with -w
> yuv2rgb.c          |   26 +--
> 

All reindention will be removed.

> 
> 
> [...]
> > -static inline int RENAME(yuv420_rgb16)(SwsContext *c, const uint8_t *src[],
> > -                                       int srcStride[],
> > -                                       int srcSliceY, int srcSliceH,
> > -                                       uint8_t *dst[], int dstStride[])
> > +static int RENAME(yuv420_rgb16)(SwsContext *c, const uint8_t *src[],
> > +                                               int srcStride[],
> > +                                               int srcSliceY, int srcSliceH,
> > +                                               uint8_t *dst[], int
> > +dstStride[])
> 
> maybe the removial of inline should be a seperate patch also there is the
> question why these wraper functions exist These do change from a a "free thing
> in inline asm" to a call overhead with C->NASM

I will try to call nasm directly by removing the wrapper function.

> 
> 
> >  {
> >      int y, h_size, vshift;
> > -
> >      YUV2RGB_LOOP(2)
> >
> >  #ifdef DITHER1XBPP
> > -        c->blueDither  = ff_dither8[y       & 1];
> > -        c->greenDither = ff_dither4[y       & 1];
> > -        c->redDither   = ff_dither8[(y + 1) & 1];
> > +    c->blueDither  = ff_dither8[y       & 1];
> > +    c->greenDither = ff_dither4[y       & 1];
> > +    c->redDither   = ff_dither8[(y + 1) & 1];
> 
> these changes make the patch harder to review and the resulting commit harder
> to read too (and i manually matched these up above it lookes worse in the
> actual diff

Reindention will be removed.

> 
> 
> > -#endif
> > -
> > -        YUV2RGB_INITIAL_LOAD
> > -        YUV2RGB
> > -        RGB_PACK_INTERLEAVE
> > -#ifdef DITHER1XBPP
> > -        DITHER_RGB
> >  #endif
> > -        RGB_PACK16(pb_07, 0)
> >
> > -    YUV2RGB_ENDLOOP(2)
> > -    YUV2RGB_OPERANDS
> > -    YUV2RGB_ENDFUNC
> 
> > +    RENAME(ff_yuv_420_rgb16)(index, image, pu - index, pv - index, &(c-
> >redDither), py - 2 * index);
> > +    }
> > +    return srcSliceH;
> 
> This doesnt look correctly indented

Will be changed.

Thank you,
Ting Fu
> 
> 
> thanks
> 
> 
> 
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> The smallest minority on earth is the individual. Those who deny individual rights
> cannot claim to be defenders of minorities. - Ayn Rand