[MPlayer-dev-eng] FreeBSD patches review

[Dominik 'Rathann' Mierzejewski]

As promised, here's a review. BSD people, please comment.

http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-ad?rev=1.19&content-type=text/plain

Most chunks are removing large parts of configure. WTF?

Some are changing defaults (why not specify these settings in the command
line?)

This one might be useful, but it's badly done:
@@ -4670,11 +4617,11 @@
 
 
 echocheck "RTC"
-if linux ; then
+if freebsd ; then
   if test "$_rtc" = auto ; then
     cat > $TMPC << EOF
 #include <sys/ioctl.h>
-#include <linux/rtc.h>
+#include <rtc.h>
 int main(void) { return RTC_IRQP_READ; }
 EOF
     _rtc=no

This one looks promising, too, but again, done in nonportable way:
@@ -5948,7 +5898,7 @@
   CFLAGS="$CFLAGS -D_REENTRANT"
 elif bsd ; then
   # FIXME bsd needs this so maybe other OS'es
-  CFLAGS="$CFLAGS -D_THREAD_SAFE"
+  CFLAGS="$CFLAGS ${PTHREAD_CFLAGS}"
 fi
 # 64 bit file offsets?
 if test "$_largefiles" = yes || freebsd ; then


http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-ae?rev=1.12&content-type=text/plain

This one basically removes large portion of Makefile (why?).

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).

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.

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.

http://www.freebsd.org/cgi/cvsweb.cgi/~checkout~/ports/multimedia/mplayer/files/patch-libmpdemux-demux_rtp.cpp?rev=1.1&content-type=text/plain

This is already in CVS.

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.

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.

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?

To summarize: 9 patches, 3 have something worth investigating, 1 is
already applied and most of the rest looks unacceptable.

Regards,
R.

-- 
MPlayer RPMs maintainer: http://greysector.rangers.eu.org/mplayer/
"I am Grey. I stand between the candle and the star. We are Grey.
 We stand between the darkness ... and the light."
        -- Delenn in Grey Council in Babylon 5:"Babylon Squared"