On Fri, Feb 15, 2013 at 1:53 PM, Diego Biurrun <[email protected]> wrote:
>
> On Fri, Feb 15, 2013 at 12:53:44AM -0500, Daniel Kang wrote:
> > On Thu, Feb 14, 2013 at 7:59 AM, Diego Biurrun <[email protected]> wrote:
> > > On Wed, Feb 13, 2013 at 05:53:36PM -0500, Daniel Kang wrote:
> > >>
> > >> --- a/libavcodec/x86/cavsdsp.c
> > >> +++ b/libavcodec/x86/cavsdsp.c
> > >> @@ -475,12 +481,18 @@ CAVS_MC(put_, 8, 3dnow)
> > >> CAVS_MC(put_, 16,3dnow)
> > >> CAVS_MC(avg_, 8, 3dnow)
> > >> CAVS_MC(avg_, 16,3dnow)
> > >> +#endif /* HAVE_AMD3DNOW_INLINE */
> > >>
> > >> static av_cold void ff_cavsdsp_init_3dnow(CAVSDSPContext *c,
> > >> AVCodecContext *avctx)
> > >> {
> > >> +#if HAVE_YASM
> > >> + c->put_cavs_qpel_pixels_tab[0][0] =
> > >> ff_put_cavs_qpel16_mc00_mmxext;
> > >> + c->put_cavs_qpel_pixels_tab[1][0] =
> > >> ff_put_cavs_qpel16_mc00_mmxext;
> > >> +#endif
> > >>
> > >> +#if HAVE_INLINE_ASM
> > >> #define dspfunc(PFX, IDX, NUM) \
> > >> - c->PFX ## _pixels_tab[IDX][ 0] = ff_ ## PFX ## NUM ##
> > >> _mc00_mmxext; \
> > >> c->PFX ## _pixels_tab[IDX][ 2] = ff_ ## PFX ## NUM ##
> > >> _mc20_3dnow; \
> > >> c->PFX ## _pixels_tab[IDX][ 4] = ff_ ## PFX ## NUM ##
> > >> _mc01_3dnow; \
> > >
> > > mmxext functions in the 3dnow init function?
> >
> > Yes this is correct. Does not contain any mmxext specific instructions.
>
> That's not my definition of "correct". It is not wrongly placed, just
> wrongly named then. Please fix the name.
What do you suggest?
> > >> --- a/libavcodec/x86/dsputil_mmx.c
> > >> +++ b/libavcodec/x86/dsputil_mmx.c
> > >> @@ -128,26 +136,45 @@ void
> > >> ff_put_no_rnd_pixels8_y2_exact_mmxext(uint8_t *block,
> > >> void ff_put_pixels8_mmxext(uint8_t *block, const uint8_t *pixels,
> > >> ptrdiff_t line_size, int h);
> > >> static void ff_put_pixels16_mmxext(uint8_t *block, const uint8_t
> > >> *pixels,
> > >> - int line_size, int h)
> > >> + ptrdiff_t line_size, int h)
> > >
> > > Is there a reason not to do this separately, i.e. right away?
> >
> > No.
>
> So let's go ahead and change it separately. :)
Sure
> > >> --- a/libavcodec/x86/dsputil_rnd_template.c
> > >> +++ b/libavcodec/x86/dsputil_rnd_template.c
> > >> @@ -25,570 +25,28 @@
> > >>
> > >> //FIXME optimize
> > >> -static void DEF(put, pixels16_y2)(uint8_t *block, const uint8_t
> > >> *pixels, ptrdiff_t line_size, int h){
> > >> - DEF(put, pixels8_y2)(block , pixels , line_size, h);
> > >> - DEF(put, pixels8_y2)(block+8, pixels+8, line_size, h);
> > >> +static void DEF(ff_put, pixels16_y2)(uint8_t *block, const uint8_t
> > >> *pixels, ptrdiff_t line_size, int h){
> > >> + DEF(ff_put, pixels8_y2)(block , pixels , line_size, h);
> > >> + DEF(ff_put, pixels8_y2)(block+8, pixels+8, line_size, h);
> > >> }
> > >
> > > Is the FIXME comment still valid in some way?
> >
> > Yes and no. There are mmxext versions of the same thing and they're
> > faster anyway.
>
> So it's cruft more than anything else, please delete it.
Done
> > >> + lea r1, [r1+r2*2]
> > >> + lea r0, [r0+r2*2]
> > >> + sub r3d, 4
> > >> + jne .loop
> > >> + RET
> > >
> > > Weird placement of .loop; I suggest aligning it with the rest.
> > > Probably it is handled inconsistently throughout...
> >
> > All my code I've written has the loop in that placement. It's very
> > possible it's inconsistent across files.
>
> I suggest no idiosyncratic formatting for jump instructions and/or
> maintaining the style of the file.
The majority of the code in that file already has loops in that
format. The exceptions are the two functions at the bottom (the sse2
ones).
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
