[MPlayer-dev-eng] FreeBSD patches review

[Reimar Döffinger]

Hi,
> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-cpudetect.h?rev=1.1&content-type=text/plain
> 
> This looks useful, but should be made portable (i.e. BSD-specific).

> --- cpudetect.h.orig	Sun Apr  6 01:28:52 2003
> +++ cpudetect.h	Sun Apr  6 01:29:06 2003
> @@ -5,6 +5,8 @@
>  #define CPUTYPE_I486	4
>  #define CPUTYPE_I586	5
>  #define CPUTYPE_I686    6
> +#define CPUTYPE_I686_7	7
> +#define CPUTYPE_I686_8	8
>  
>  typedef struct cpucaps_s {
>  	int cpuType;

Huh? What sense does it make to define these as they aren't used anyway?

> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-libao2-ao_arts.c?rev=1.1&content-type=text/plain
> 
> This one has already been discussed here, AFAIR. It won't be applied, the
> bug is supposedly in arts.

> --- libao2/ao_arts.c.orig	Fri Dec 27 14:35:07 2002
> +++ libao2/ao_arts.c	Mon Mar  3 17:10:38 2003
> @@ -102,8 +102,14 @@
>  
>  static void uninit()
>  {
> -	arts_close_stream(stream);
> +	if (stream != NULL) {
> +		arts_close_stream(stream);
> +		stream = NULL;
> +        }
> +/* XXX - we get "MPlayer interrupted by signal 11 in module:
> +   exit_player" unless commented :(
>  	arts_free();
> +*/ 
>  }
>  
>  static int play(void* data,int len,int flags)

That check (stream != NULL) should be unneccassary, as init already 
fails in that case and uninit won't be called, probably a leftover from 
debugging. It's funny that the arts sample code does it the same way...

> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-libdha-Makefile?rev=1.2&content-type=text/plain
> 
> All it does is change the libdha version string in lib filename. Probably
> some FreeBSD naming convention. Doesn't look important.

I guess we don't care about names around here ;-)

> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-libmpdvdkit2-dvd_reader.c?rev=1.1&content-type=text/plain
> 
> Looks completely unnecessary.

> --- libmpdvdkit2/dvd_reader.c.orig	Sat Aug  9 16:12:35 2003
> +++ libmpdvdkit2/dvd_reader.c	Fri Oct  3 18:16:43 2003
> @@ -239,6 +239,11 @@
>     Darwin  /dev/rdisk0,  it needs to be the raw device
>     BSD/OS  /dev/sr0c (if not mounted) or /dev/rsr0c ('c' any letter will do) */
>  static char *bsd_block2char( const char *path )
> +#if defined(__FreeBSD__)
> +{
> +    return (char *) strdup( path );
> +}
> +#else
>  {
>      char *new_path;
>  
> @@ -253,6 +258,7 @@
>  
>      return new_path;
>  }
> +#endif /* __FreeBSD__ */
>  #endif
>  
>  dvd_reader_t *DVDOpen( const char *path )

I don't know. Some lines above it says: FreeBSD /dev/(r)(a)cd0c (a is 
for atapi), recomended to _not_ use r
Those block2char functions look quite dubious to me anyway... can anyone 
explain why they are needed?

> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-libvo-vo_md5.c?rev=1.1&content-type=text/plain
> 
> Makes vo_md5 use md5 instead of md5sum binary, but this should be done in
> a portable way.

As I stated in another mail, this has been done in CVS (it tries both, 
md5sum first).

> http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-vidix-drivers-Makefile?rev=1.2&content-type=text/plain
> 
> Adds X11 includes to CFLAGS for *all* VIDIX drivers. Why is it necessary
> on BSD? VIDIX drivers are independent of X, aren't they?

>  RADEON_LIBS=-L../../libdha -ldha -lm $(X_LIB)
> -RADEON_CFLAGS=$(OPTFLAGS) -fPIC -I. -I..
> +RADEON_CFLAGS=$(OPTFLAGS) $(X11_INC) -fPIC -I. -I..

well, Radeon and Rage128 seem to link against X libs, so it _might_ make 
sense there, but I don't know.

Greetings,
Reimar Döffinger