[FFmpeg-devel] [PATCH 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 Carl
> Eugen Hoyos
> Sent: Tuesday, December 3, 2019 04:23 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] libswscale/x86/yuv2rgb: Change inline
> assembly into nasm code
> 
> Am Di., 3. Dez. 2019 um 04:53 Uhr schrieb Fu, Ting <ting.fu at intel.com>:
> >
> >
> >
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Carl Eugen Hoyos
> > > Sent: Monday, December 2, 2019 05:49 PM
> > > To: FFmpeg development discussions and patches
> > > <ffmpeg-devel at ffmpeg.org>
> > > Subject: Re: [FFmpeg-devel] [PATCH 1/2] libswscale/x86/yuv2rgb:
> > > Change inline assembly into nasm code
> > >
> > > Am Mo., 2. Dez. 2019 um 04:17 Uhr schrieb Fu, Ting <ting.fu at intel.com>:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf
> > > > > Of Michael Niedermayer
> > > > > Sent: Friday, November 29, 2019 05:33 AM
> > > > > To: FFmpeg development discussions and patches
> > > > > <ffmpeg-devel at ffmpeg.org>
> > > > > Subject: Re: [FFmpeg-devel] [PATCH 1/2] libswscale/x86/yuv2rgb:
> > > > > Change inline assembly into nasm code
> > > > >
> > > > > On Thu, Nov 28, 2019 at 02:07:07PM +0800, Ting Fu wrote:
> > > > > > Signed-off-by: Ting Fu <ting.fu at intel.com>
> > > > > > ---
> > > > > >  libswscale/x86/Makefile           |   1 +
> > > > > >  libswscale/x86/swscale.c          |  16 +-
> > > > > >  libswscale/x86/yuv2rgb.c          |  81 ++----
> > > > > >  libswscale/x86/yuv2rgb_template.c | 441 ++++++------------------------
> > > > > >  libswscale/x86/yuv_2_rgb.asm      | 270 ++++++++++++++++++
> > > > > >  5 files changed, 394 insertions(+), 415 deletions(-)  create
> > > > > > mode
> > > > > > 100644 libswscale/x86/yuv_2_rgb.asm
> > > > >
> > > > > This changes the output, i presume that is unintentional
> > > > >
> > > > > ./ffmpeg -cpuflags 0 -i matrixbench_mpeg2.mpg -t 1 -vf
> > > > > format=yuv420p,format=rgb565le -an -f framecrc -
> > > > >
> > > > > 0,          0,          0,        1,   829440, 0x1bd78b86
> > > > > 0,          1,          1,        1,   829440, 0x85910b33
> > > > > ...
> > > > > vs.
> > > > > 0,          0,          0,        1,   829440, 0x31f4a2bd
> > > > > 0,          1,          1,        1,   829440, 0xf0c66218
> > > > > ...
> > > > >
> > > > >
> > > >
> > > > Hi Michael,
> > > >
> > > > This unexpected change is because of the missing verify of current
> > > > SIMD
> > > support.
> > > > So, when cpuflag=0, ffmpeg used mmx code to compute as default.
> > > > I added if (EXTERNAL_XXX(cpu_flags)) to verify the SIMD in
> > > libswscale/x86/yuv2rgb.c.
> > >
> > > Could the patch be split to make this change easier to understand?
> >
> > Hi Carl,
> >
> > I didn’t come across any good idea to separate the PATCH.
> > Since the [PATCH 1/2] is consisted of mmx code for
> yuv2rgb24/bgr24/rgb32/bgr32/rgb15/rgb16 and they're all come from former
> inline assembly.
> > Should it be separated into something like PATCH 1: mmx
> > yuv2rgb24/bgr24 PATCH 2: mmx yuv2rgb32/bgr32 PATCH 3: mmx
> > yuv2rgb15/rgb16 Or adding more comments in nasm file would be more
> > helpful?
> >
> > Can you show me if there is any better solution? I cannot be more grateful to
> it.
> 
> I didn't want to imply that I know of a better way, just that your answer to
> Michael's question made me wonder if adding EXTERNAL_XXX() in a separate
> patch would fix his concern.

Hi Carl,

The EXTERNAL_XXX() is added to fix the bug of compute under "-cpuflags 0" condition.
Without which the ffmpeg would use mmx code as default to compute unscale yuv2rgb part.
So, in my opinion it is inconvenient to put it in a separate part. 
Plus, I have added more comments in the nasm file in PATCH V3. Hope it helps.

Thank you so much for your review.
Ting Fu

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request at ffmpeg.org
> with subject "unsubscribe".