[MPlayer-dev-eng] [PATCH] libnemesi support in mplayer

[Diego Biurrun]

On Thu, Sep 06, 2007 at 02:09:31PM +0200, Luca Barbato wrote:
> Here is an initial patch to have libnemesi[1] handle rtsp instead of
> live555. (you can chose at configure which one to use)

It seems that the day has finally arrived ...

Here is my initial review, you still have some work to do :)
Nevertheless congrats on getting this up to snuff after all this time.

> --- stream/Makefile	(revision 24341)
> +++ stream/Makefile	(working copy)
> @@ -27,21 +27,27 @@
>                                      rtp.c                  \
>                                      udp.c                  \
>                                      tcp.c                  \
> +                                    stream_udp.c           \
>                                      stream_rtp.c           \
> -                                    stream_rtsp.c          \
> -                                    stream_udp.c           \
> -                                    freesdp/common.c       \
> +                                    realrtsp/asmrp.c       \
> +                                    realrtsp/real.c        \
> +                                    realrtsp/rmff.c        \
> +                                    realrtsp/sdpplin.c     \
> +                                    realrtsp/xbuffer.c     \
> +
> +ifeq ($(LIBNEMESI),no)
> +SRCS_COMMON-$(MPLAYER_NETWORK)    += stream_rtsp.c          \
> +                                     freesdp/common.c       \

indentation messup

>                                      freesdp/errorlist.c    \
>                                      freesdp/parser.c       \
>                                      librtsp/rtsp.c         \
>                                      librtsp/rtsp_rtp.c     \
>                                      librtsp/rtsp_session.c \
> -                                    realrtsp/asmrp.c       \
> -                                    realrtsp/real.c        \
> -                                    realrtsp/rmff.c        \
> -                                    realrtsp/sdpplin.c     \
> -                                    realrtsp/xbuffer.c     \
>  
> +else
> +SRCS_COMMON-$(MPLAYER_NETWORK)   += stream_nemesi.c
> +endif

Geez, all of this is ugly.  First off, nemesi should have its own
variable, that should help remove the ifeq.  Then we can add another
variable for the native RTSP stuff, that should take care of the rest.

> --- stream/stream_nemesi.c	(revision 0)
> +++ stream/stream_nemesi.c	(revision 0)
> @@ -0,0 +1,91 @@
> +/*
> + *   This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software Foundation,
> + *  Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */

This header has weird indentation and it should read MPlayer, not "this
program".

> +#include "nemesi/rtsp.h"
> +#include "../libmpdemux/demux_nemesi.h"

The .. is unneeded.

> +/*
> +#include <unistd.h>
> +#include <sys/types.h>
> +#ifndef HAVE_WINSOCK2
> +#include <netinet/in.h>
> +#include <sys/socket.h>
> +#include <arpa/inet.h>
> +#define closesocket close
> +#else
> +#include <winsock2.h>
> +#include <ws2tcpip.h>
> +#endif
> +*/

Why is this commented-out?

> +static int
> +rtsp_streaming_open (stream_t *stream, int mode, void *opts, int *file_format)

I prefer function declarations on one line.  Also, this is inconsistent.

> --- configure	(revision 24341)
> +++ configure	(working copy)
> @@ -6215,7 +6219,21 @@
>  fi
>  echores "$_qtx"
>  
> +echocheck "Nemesi Streaming Media libraries"
> +if test "$_nemesi" = auto  && test "$_network" = yes ; then
> +    if $_pkg_config --exists libnemesi ; then
> +        _ld_extra="$_ld_extra `$_pkg_config --libs libnemesi`"
> +        _def_nemesi='#define LIBNEMESI 1'
> +        _nemesi=yes
> +        _live=no
> +    else
> +        _nemesi=no
> +        _def_nemesi='#undef LIBNEMESI'
> +    fi
> +fi

This is wrong, think what happens if $_nemesi is not auto.

> @@ -6265,6 +6283,11 @@
>  fi
>  echores "$_live"
>  
> +if test "_nemesi" = no; then
> +  _no_inputmodules="nemesi $_no_inputmodules"
> +else
> +  _inputmodules="nemesi $_inputmodules"
> +fi

This should be next to the nemesi test, not at a random location.

> --- libmpdemux/Makefile	(revision 24341)
> +++ libmpdemux/Makefile	(working copy)
> @@ -52,6 +52,7 @@
>  SRCS_COMMON-$(STREAMING_LIVE555)       += demux_rtp.cpp demux_rtp_codec.cpp
> +SRCS_COMMON-$(LIBNEMESI)       	       += demux_nemesi.c

indentation messup (tabs)

> --- libmpdemux/demux_nemesi.c	(revision 0)
> +++ libmpdemux/demux_nemesi.c	(revision 0)
> @@ -0,0 +1,372 @@
> +#include "demux_nemesi.h"
> +#include "stheader.h"

Please add a proper license header to new files.

> +/*static void link_session_and_fetch_conf(Nemesi_DemuxerStreamData * ndsd, Nemesi_SessionType stype, rtsp_medium * media, rtp_buff * obuff, float * ofps)
> +{
> +}*/

Why is this commented out?

There are some loooong lines in there, please break them into more
manageable pieces.

> --- libmpdemux/demux_nemesi.h	(revision 0)
> +++ libmpdemux/demux_nemesi.h	(revision 0)
> @@ -0,0 +1,32 @@
> +#ifndef DEMUX_RTP_H
> +#define DEMUX_RTP_H

Please add a license header, same as above.

> +#ifndef STREAM_H
> +#include "stream/stream.h"
> +#endif
> +
> +#ifndef DEMUXER_H
> +#include "demuxer.h"
> +#endif

The #ifndef are unnecessary, they are already in the header files, where
they belong.

> +// Open a RTP demuxer (which was initiated either from a SDP file,
> +// or from a RTSP URL):
> +demuxer_t* demux_open_rtp(demuxer_t* demuxer);

I still prefer /* */, especially for multiline comments.  Feel free to
ignore me, though.

> +// Test whether a RTP demuxer is for a MPEG stream:
> +//int demux_is_mpeg_rtp_stream(demuxer_t* demuxer);
> +
> +// Test whether a RTP demuxer contains combined (multiplexed)
> +// audio+video (and so needs to be demuxed by higher-level code):
> +//int demux_is_multiplexed_rtp_stream(demuxer_t* demuxer);

Hmmm, commented out ...

> +#endif

Please comment the #endif.

Diego