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

[Luca Barbato]

Diego Biurrun wrote:
> 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

Ok


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

Ok

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

I'll try to have the headers consistent.

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

Ok

> Why is this commented-out?

stale from the original file.

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

I'll have a round of cosmetics all over soonish

>>  
>> +echocheck "Nemesi Streaming Media libraries"
>> +if test "$_nemesi" = auto  && test "$_network" = yes ; then
>> +    if $_pkg_config --exists libnemesi ; then
...
>> +fi
> 
> This is wrong, think what happens if $_nemesi is not auto.

Got alredy some reports about that ^^;

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

it must be after the latest block touching $_nemesi

> 
> indentation messup (tabs)

Ok

> Please add a proper license header to new files.

Should be already fixed

> 
>> +/*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?

will be removed, it's an alternative implementation.

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

I'll make it suit my usual(ffmpeg) coding style

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

ok

> 
>> +// 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.

I'll fix it.

> 
>> +// 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 ...

again something from the the original file.

> 
>> +#endif
> 
> Please comment the #endif.

hmm


-- 

Luca Barbato

Gentoo/linux Gentoo/PPC
http://dev.gentoo.org/~lu_zero