[Ffmpeg-devel] [PATCH] MinGW and portability

[Rich Felker]

On Tue, Mar 28, 2006 at 09:43:54PM +0100, M?ns Rullg?rd wrote:
> > Simple patch needed for MinGW: replacing keyword near.
> >
> > Index: libavcodec/jpeg_ls.c
> > ===================================================================
> > RCS file: /cvsroot/ffmpeg/ffmpeg/libavcodec/jpeg_ls.c,v
> > retrieving revision 1.5
> > diff -u -p -u -r1.5 jpeg_ls.c
> > --- libavcodec/jpeg_ls.c	2 Feb 2006 02:36:27 -0000	1.5
> > +++ libavcodec/jpeg_ls.c	28 Mar 2006 14:30:33 -0000
> > @@ -34,7 +34,7 @@ typedef struct JLSState{
> >      int T1, T2, T3;
> >      int A[367], B[367], C[365], N[367];
> >      int limit, reset, bpp, qbpp, maxval, range;
> > -    int near, twonear;
> > +    int onenear, twonear;
> 
> Rejected, "near" is not a keyword in any version of C.  Fix the compiler.

This patch was already rejected once before. The solution is to add
the equivalent of -Dnear=msvc_sucks_near to your build options, not to
invasively alter valid C code to workaround MS bugs.

> > -extern int ffm_nopts;
> > +_DL_IMPORT extern int ffm_nopts;
> 
> Ugh, yuck!!  #define extern if you must.  Changes like this are
> unacceptable.

Agree. Totally unacceptable.
Also, using the name _DL_IMPORT is illegal in a conforming C
application. This name is reserved for the implementation (__.* and
_[[:upper:]].* are reserved).

> > +#include "config.h"
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > +#ifdef HAVE_INTTYPES_H
> >  #include <inttypes.h>
> > +#endif
> > +#ifdef HAVE_STDINT_H
> > +#include <stdint.h>
> > +#endif
> 
> Both inttypes.h and stdint.h are standard C.  If you are missing them
> you should fix your environment.

Yes. This is easy to do..

> >  #ifdef __MINGW32__
> > +#if __MINGW32_MAJOR_VERSION * 1000 + __MINGW32_MINOR_VERSION > 1002
> 
> See above about ancient compilers.

Yes. This must be VERY ancient since most of the workarounds in these
patches are NOT needed in any version of mingw I've ever seen.

> >  #define fseeko(x,y,z)  fseeko64(x,y,z)
> >  #define ftello(x)      ftello64(x)
> > +#else
> > +#define fseeko(x,y,z)  fseek(x,y,z)
> > +#define ftello(x)      ftell(x)
> > +#endif

This is EXTREMELY WRONG and will break large file support on every
single platform except windows!!

> > +#if defined(WIN32) || defined(__MINGW32__)
> > +
> > +#include <stdarg.h>
> > +
> > +int snprintf(char *buf, const char *fmt, size_t size, ...) {
> > +  va_list ap;
> > +  int ret;
> > +
> > +  va_start(ap, size);
> > +  ret = vsprintf(buf, fmt, ap);
> > +  va_end(ap);
> > +  return ret;
> > +}
> > +
> > +#endif
> 
> Potential security risk.  Rejected.

More than just potential.

> Someone had put a "fix" like this in some code at work.  A case turned
> up recently where it overflowed the buffer, which wasn't "large
> enough" after all, and it took quite some time to track down the
> reason for bizarre behavior.

Agree, this is completely not acceptable. Modern mingw has snprintf as
far as I know. If not then you need to write a complete snprintf
implementation... :)

It's also possible to emulate snprintf using a lot of hacks with
dynamic allocation and transformations on the format string to prevent
overflowing, but that's very difficult and risky if you make a
mistake..

> > -if (sdl-config --version) >/dev/null 2>&1 ; then
> > -if $cc -o $TMPE `sdl-config --cflags` $TMPC `sdl-config --libs`  > /dev/null 2>&1  ; then
> > -_sdlversion=`sdl-config --version | sed 's/[^0-9]//g'`
> > +SDL_CONFIG="${cross_prefix}sdl-config"
> > +if ("${SDL_CONFIG}" --version) >/dev/null 2>&1 ; then
> > +if $cc -o $TMPE `"${SDL_CONFIG}" --cflags` $TMPC `"${SDL_CONFIG}" --libs`  > /dev/null 2>&1  ; then
> > +_sdlversion=`"${SDL_CONFIG}" --version | sed 's/[^0-9]//g'`
> >  if test "$_sdlversion" -lt 121 ; then
> >  sdl_too_old=yes
> >  else
> > @@ -1572,8 +1600,8 @@ if test "$pthreads" = "yes" ; then
> >  fi
> >  if test "$sdl" = "yes" ; then
> >    echo "CONFIG_SDL=yes" >> config.mak
> > -  echo "SDL_LIBS=`sdl-config --libs`" >> config.mak
> > -  echo "SDL_CFLAGS=`sdl-config --cflags`" >> config.mak
> > +  echo "SDL_LIBS=`"${SDL_CONFIG}" --libs`" >> config.mak
> > +  echo "SDL_CFLAGS=`"${SDL_CONFIG}" --cflags`" >> config.mak
> >  fi
> >  if test "$texi2html" = "yes"; then
> >    echo "BUILD_DOC=yes" >> config.mak
> 
> This one looks sensible, assuming SDL is normally installed like that.

Yes, I don't see anything wrong with this..

Rich