[FFmpeg-devel] [RFC] remove lrintf fallback implementation

[Diego Biurrun]

On Sun, Oct 21, 2007 at 01:46:08AM +0200, V?ctor Paesa wrote:
> 
> M?ns Rullg?rd said:
> > "V?ctor Paesa" <wzrlpy at arsystel.com> writes:
> >
> >> --- libavutil/internal.h        (revision 10822) +++
> >> libavutil/internal.h        (working copy)
> >> @@ -278,4 +278,17 @@
> >>
> >> +#ifndef HAVE_LLRINT
> >> +#if defined(ARCH_X86)
> >> +static av_always_inline long long int llrint(double x)
> >> +{
> >> +    long long int llrintres;
> >> +    asm
> >> +    ("fistpll %0"
> >> +    : "=m" (llrintres) : "t" (x) : "st");
> >> +    return llrintres;
> >> +}
> >> +#endif /* defined(ARCH_X86) */
> >> +#endif /* HAVE_LLRINT */
> >
> > And what it's not ARCH_X86?
> 
> I cannot remember any other architecture reporting here the lack of
> llrint(), should they exist, they can also add some asm().

No architecture ever reported a lack of llrint().  And how would they,
it's not a processor feature.  The platform/OS that is known to lack
llrint() is Cygwin.  AFAIK Cygwin runs on on both i386 and AMD64.  If
your patch works fine on both then there will be no immediate breakage.

However, I think this is the wrong approach.  If you are going to
implement llrint(), do it in Cygwin, not in FFmpeg.  It is the right
place to fix the issue instead of working around it and will give you
better karma by helping many other projects, not just FFmpeg.

See this thread I started on the Cygwin mailing list (and ignore the
flamage):

http://thread.gmane.org/gmane.os.cygwin/92561

All you have to do is add llrint() to newlib, Cygwin will pick it up
from there.  Alternatively you can wait for the next gcc upgrade in
Cygwin, which will reportedly carry along llrint().

All in all I am against this patch.  We should be removing
platform-specific workarounds, not adding more of them.  If you cannot
wait for this to get fixed by the gcc upgrade, address the problem in
the right place, i.e. patch Cygwin, not FFmpeg.

Diego