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

[Reimar Döffinger]

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. A separate issue, but shouldn't flags be "static const" (or at
least const)?

> > > @@ -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...