> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > Michael Niedermayer > Sent: Sunday, January 19, 2020 09:11 PM > 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 Sun, Jan 19, 2020 at 02:49:21AM +0000, Fu, Ting wrote: > > > > > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > > > Michael Niedermayer > > > Sent: Friday, January 17, 2020 05:36 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 Thu, Jan 16, 2020 at 07:27:05AM +0000, Fu, Ting wrote: > > > > > > > > > > > > > -----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. > > > > > > > > > > > > > > [...] > > > > > > -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 > > > > Hi Michael, > > > > You mean I still need to convert that code, did I get you right? > > i dont think its needed if its complex Hi Michael,
Oh, I get you now. > > > > Since NASM function will get only the address of SwsConext c ( in order to > > be > compatible with yuv2rgb_c function in parameters), not the address of c- > >redDither nor the c->dstW. I have no way to get the value of c->dstW by using > address offset. > > Do you have any suggestion for solving that problem? > > maybe the offset to redDither could be the 2nd field of the struct or the > whole > redDither block could be moved up > > but only do this if the resulting asm code is reasonable. > The goal is to eliminate some ugly wraper funcions which call the asm if the > code to avoid that is more ugly then it is not a good idea I temporarily prefer the current situation. The suggestion of moving redDither block is a good idea. I will try it after Chinese New Year holiday, if it's not that ugly will be posted in next patch. And I have sent V8 to fix all the issues mentioned before (except the wrapper function). Please take a review. Thank you, Ting Fu > > thx > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The day soldiers stop bringing you their problems is the day you have stopped > leading them. They have either lost confidence that you can help or concluded > you do not care. Either case is a failure of leadership. - Colin Powell _______________________________________________ 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".