[FFmpeg-devel] [PATCH] fix motion-test on non-MMX2 CPUs

[Diego Biurrun]

On Thu, Apr 09, 2009 at 05:29:44PM +0200, Reimar D?ffinger wrote:
> On Thu, Apr 09, 2009 at 05:16:38PM +0200, Diego Biurrun wrote:
> > On Thu, Apr 09, 2009 at 05:00:02PM +0200, Michael Niedermayer wrote:
> > > On Thu, Apr 09, 2009 at 09:22:45AM +0200, Diego Biurrun wrote:
> > > > Here is a patch to fix a sigill on CPUs without MMX2, like my trusty old
> > > > K6-III, when running motion-test.
> > > > 
> > > > If somebody has a prettier idea, I'm all ears.
> > > > 
> > > > --- libavcodec/motion-test.c	(revision 18382)
> > > > +++ libavcodec/motion-test.c	(working copy)
> > > > @@ -127,7 +128,13 @@
> > > >      AVCodecContext *ctx;
> > > >      int c;
> > > >      DSPContext cctx, mmxctx;
> > > > +#if HAVE_MMX2
> > > >      int flags[2] = { FF_MM_MMX, FF_MM_MMX2 };
> > > > +    int flags_size = 2;
> > > > +#else
> > > > +    int flags[2] = { FF_MM_MMX };
> > > > +    int flags_size = 1;
> > > > +#endif
> > > 
> > > it seems int flags[2] does noz need to be changed
> > 
> > Did you mean "noW" or "noT" here?
> 
> "not". If it's not used anyway it doesn't matter that there's a second
> entry.

OK

> A separate issue, but shouldn't flags be "static const" (or at
> least const)?

Feel free to fix :)

> > > > @@ -145,7 +152,7 @@
> > > >      ctx = avcodec_alloc_context();
> > > >      ctx->dsp_mask = FF_MM_FORCE;
> > > >      dsputil_init(&cctx, ctx);
> > > > -    for (c = 0; c < 1; c++) {
> > > > +    for (c = 0; c < flags_size; c++) {
> > > 
> > > hmm, this changes it from 1 to 2
> > 
> > This was the intention, what am I missing?
> > 
> > flags[0] is MMX, flags[1] is MMX2..
> 
> Well, so far it seems right, but your subject line claims to be a fix
> for "non-MMX CPUs", which this isn't, MMX is still always enabled it
> seems to me? So it would only fix it for non-MMX2 but MMX CPUs.
> So shouldn't it probably be
> static const int flags[3] = { 0, FF_MM_MMX, FF_MM_MMX2 };
> #if HAVE_MMX2
>     int flags_size = 3;
> #elif HAVE_MMX
>     int flags_size = 2;
> #else
>     int flags_size = 1;
> #endif
> 
> Though I admit the current code would then be comparing the C
> implementation against the C implementation which does not make that
> much sense, except for the speed values...

The file is only compiled when MMX is available.  My fix is for the
situation where MMX is available, but MMX2 is not.  The subject line is
a typo, it should be non-MMX2.

Diego