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

[Michael Niedermayer]

On Thu, Jan 16, 2020 at 07:27:05AM +0000, Fu, Ting wrote:
> 
> 
> > -----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.
> > 
> > 
> [...]
> > > -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
> > 
> Hi Michael,
> 
> The wrapper functions initiate some variables and contain one 'for cycle'. The variable initiation needs to access to the 'c->dstW', furthermore macro SWS_MAX_ FILTER_SIZE is needed. Which means extra work and much more NASM code.
> If you still prefer to do all the things in assembly, I can change from 'C->NASM' to 'call NASM function directly' in another further patch( for current patch easier to review).
> Or in my opinion, the cost in C->NASM can be ignored, and the initiation work looks clearer in C, just let it be what it is now.
> What do you think?

it probably makes no sense if its hard to convert that code

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200116/8b7d618e/attachment.sig>