[MPlayer-dev-eng] *** GMX Spamverdacht *** [PATCH] DJGPP support

[Reimar Döffinger]

On Tue, Mar 12, 2013 at 09:26:19PM +0100, Fabrizio Gennari wrote:
> Index: libao2/ao_wss.c
> ===================================================================
> --- libao2/ao_wss.c	(revisione 0)
> +++ libao2/ao_wss.c	(copia locale)

Don't have time for a proper review, but there's quite some
trailing whitespace in that file, and tabs and space are mixed.
Consider running uncrustify over it, the MPlayer configuration
for it is in TOOLS/mp-uncrustify-style.cfg
That applies to all new files btw.

> Index: libaf/af_format.c
> ===================================================================
> --- libaf/af_format.c	(revisione 35946)
> +++ libaf/af_format.c	(copia locale)
> @@ -35,6 +35,7 @@
>  
>  #include "libavutil/avutil.h"
>  #include "libavutil/common.h"
> +#include "libavutil/libm.h"

What is this needed for?
If only DJGPP needs it for some reason you should add a comment,
otherwise someone might remove it "because it works for me without".

> +djgpp()		{ issystem "DJGPP"; }
>  dragonfly() { issystem "DragonFly"; }

Please don't add tabs to files like configure that don't use any
currently.
I suspect some of the configure parts are simple enough that they could
be applied right away.

> -  for ld_x264 in "-lx264 $ld_pthread" "-lx264 $ld_pthread" ; do
> +  for ld_x264 in "-lx264 $ld_pthread" "-lx264 -lm $ld_pthread" ; do

Hm, do we currently really have the same thing twice?
That seems like a bug.
Might be worth spitting out and sending as a separate patch to get more
attention.

> +#ifndef __DJGPP__
>      if (codec_path)
> +#endif	
>          set_codec_path(codec_path);

That doesn't make sense to me...

>      /* Check codecs.conf. */
>      if (!codecs_file || !parse_codec_cfg(codecs_file)) {
> +#ifndef __DJGPP__        
>          char *conf_path = get_path("codecs.conf");
> +#else
> +		char *conf_path = get_path("codecs.cfg");
> +#endif        
>          if (!parse_codec_cfg(conf_path)) {
> +#ifndef __DJGPP__             
>              if (!parse_codec_cfg(MPLAYER_CONFDIR "/codecs.conf")) {
> +#else
> +		if (!parse_codec_cfg(MPLAYER_CONFDIR "/codecs.cfg")) {
> +#endif		
>                  if (!parse_codec_cfg(NULL)) {
>                      free(conf_path);
>                      return 0;
>                  }
> +#ifndef __DJGPP__    				
>                  mp_msg(MSGT_CPLAYER, MSGL_V, "Using built-in default codecs.conf.\n");
> +#else
> +				mp_msg(MSGT_CPLAYER, MSGL_V, "Using built-in default codecs.cfg.\n");
> +#endif            

something along the lines of
#define CODECS_CONF "codecs.conf"
and then e.g.
mp_msg(MSGT_CPLAYER, MSGL_V, "Using built-in default " CODECS_CONF ".\n");
etc. would be nicer IMHO.

> +#ifndef __DJGPP__   
>       videobuffer = memalign(8, VIDEOBUFFER_SIZE + MP_INPUT_BUFFER_PADDING_SIZE);
> +#else
> +	 videobuffer = memalign(VIDEOBUFFER_SIZE + MP_INPUT_BUFFER_PADDING_SIZE, 8);
> +#endif     	 

Doing it this way is a lot of code and risky (e.g. when another memalign
called is added this ifdef will almost certainly be forgotten, breaking
DJGPP).
Either changing it by some "global" define (though that might interact
badly with your FFmpeg patches) or replacing the memalign by e.g.
av_malloc where easily possible (will also require replacing the
corresponding free() ) might be alternative options.
Also, why #ifndef everywhere instead of the simpler #ifdef and swapping
the if/else parts?

> Index: loader/ldt_keeper.c
> ===================================================================
> --- loader/ldt_keeper.c	(revisione 35946)
> +++ loader/ldt_keeper.c	(copia locale)
> @@ -35,6 +35,10 @@
>  #include "osdep/mmap_anon.h"
>  #include "mp_msg.h"
>  #include "help_mp.h"
> +#ifdef __DJGPP__
> +#include <sys/nearptr.h>
> +#include <dpmi.h>
> +#endif
>  #ifdef __linux__
>  #include <asm/unistd.h>
>  #include <asm/ldt.h>
> @@ -116,7 +120,11 @@
>  
>  void Setup_FS_Segment(void)
>  {
> +#ifndef  __DJGPP__   
>      unsigned int ldt_desc = LDT_SEL(fs_ldt);
> +#else
> +    unsigned int ldt_desc = fs_ldt;
> +#endif

You can avoid that ifdef by defining LDT_SEL in the
one with the headers I think.

> --- osdep/timer-djgpp.c	(revisione 0)
> +++ osdep/timer-djgpp.c	(copia locale)
[...]
> +int usec_sleep (int usec_delay);
> +unsigned int GetTimer (void);
> +unsigned int GetTimerMS (void);
> +float GetRelativeTime (void);
> +void InitTimer (void);

#include "timer.h"
instead of these.

> +void * mmap(void *addr, size_t len, int prot, int flags, int fildes, off_t off)
> +{
> +
> +    void  * ret;
> +    int pos;
> +
> +	/*printf("Mapping addr %X, len %X\n", (unsigned) addr, len);*/
> +    if (addr)
> +    {	
> +		if(flags & MAP_FIXED)
> +			ret = addr;
> +  		else 
> +			return MAP_FAILED;
> +   }
> +   else
> +   {
> +    	len = (len + 0xFFF) & 0xFFFFF000;
> +		ret = memalign(len, 0x1000);
> +    	if(!ret) 
> +			return MAP_FAILED;
> +   }
> +
> +   if(!(flags & MAP_ANON) && (fildes != -1))
> +   {
> +        if((pos = lseek(fildes, off, SEEK_SET)) == -1)
> +        {
> +			if(!(flags & MAP_FIXED))
> +				free(ret);
> +            return MAP_FAILED;
> +        }
> +
> +        _read(fildes, ret, len);
> +        lseek(fildes, pos, SEEK_SET );
> +    }
> +	
> +	/*printf("Mapped to %X\n", (unsigned)ret);*/
> +	return ret;
> +}

This does not at all look like a working mmap implementation.
If the platform doesn't support mmap you should compile
MPlayer without mmap support.

> Index: vidix/sysdep/pci_djgpp.c
> ===================================================================
> --- vidix/sysdep/pci_djgpp.c	(revisione 0)
> +++ vidix/sysdep/pci_djgpp.c	(copia locale)
> @@ -0,0 +1,9 @@
> +static __inline__ int enable_os_io(void)
> +{
> +  return 0;
> +}
> +
> +static __inline__ int disable_os_io(void)

inline should be a regular keyword, please don't add __ if it's not
necessary.

> Index: libvo/gtf.c
> ===================================================================
> --- libvo/gtf.c	(revisione 35946)
> +++ libvo/gtf.c	(copia locale)
> @@ -36,6 +36,10 @@
>  #define DEBUG_PRINTF(a,b)
>  #endif
>  
> +#ifdef __DJGPP__
> +#include "libavutil/libm.h"
> +#endif

#ifdef should be unnecessary, but a comment explaining the reason is
appropriate.

> --- stream/network.h	(revisione 35946)
> +++ stream/network.h	(copia locale)
> @@ -28,12 +28,14 @@
>  #include <sys/types.h>
>  
>  #include "config.h"
> +#ifdef CONFIG_NETWORKING
>  #if !HAVE_WINSOCK2_H
>  #include <netdb.h>
>  #include <netinet/in.h>
>  #include <sys/socket.h>
>  #include <arpa/inet.h>
>  #endif
> +#endif

If CONFIG_NETWORKING is not set, I believe this header
is not supposed to be included.
So I think it would be better to fix the places where
it is included even if networking is disabled.
Which seems to be stream/stream.c and I can't see
any reason why it is included from there.
At least on Linux just removing it doesn't break anything.