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.
> >> --- 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. :)
> >> --- 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.
> >> + 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.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
