[MPlayer-dev-eng] possible bugs in vf_decimate filter

[Trent Piepho]

On Tue, 17 Oct 2006, Michael Niedermayer wrote:
> > >                 "decl %%ecx \n\t"
> > >
> > > id use a "cmp %%"REG_S", ...  here, some cpus have a dissike for inc/dec as
> > > inc/dec just change part of the flags which creates a dependancy to the
> > > previous flag value
> >
> > The code is also using ecx without listing it as a clobbered register.  It
>
> :/

I've already written a patch that re-writes the asm code to not use
hardcoded registers or undeclared clobbers.  I wondered if there was a
reason for it (other than 'movsl', etc.), because I see it a lot in
mplayer/lavc (not much elsewhere).

> > Using mmx in asm statements anywhere near floating point is very dangerous.
> > For example, this code will miscompile and print "nan" if you use -O3 (so
> > that mmxfunc() is inlined).  There is clearly not a missing emms here.  The
> > problem is that gcc leaves 'f' in a floating pointer register across the
> > inlined call to mmxfunc(), and the mmx code clears the fp regiters.

> > void mmxfunc(void) { asm("emms" : : : "st"); }
> > int main(void){

>
> hmm what about adding the float registers to the clobber list? if that

It's already there in the list.  Doesn't work.  Seems the only way to get
gcc 4.0.1 to understand that the fp stack is clobbered is to have a
non-inlined function call, or use the MMX instrinsic _mm_empty().

> fails then the simplest solution for vf_decimate.c seems to me to replace
> the float by an int (fixedpoint, dunno what its range needs to be ...)

I already fixed it.  I just calculate the integer 't' value in the filter
config() function, which is more efficient anyway.  That way the mmx code
and the emms are contained in a non-inlined call-tree that has no floating
point what-so-ever, which seems to the be only safe way to use MMX asm.

I wonder if there are any other bugs in mplayer like this.

> the alternatives are to put the mmx registers on the clobber list (breaks gcc
> 2.95 IIRC) or maybe some hack with volatile float  ...

Yes, you need gcc 3.1+ to put mmx registers on the clobber list.  It
doesn't do any good in this case, gcc still thinks the fp stack is
preserved across the mmx code.