> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Wednesday, January 15, 2020 05:55 AM
> To: FFmpeg development discussions and patches <ffmpeg-devel@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...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
