[vlc-devel] [PATCH] Integrate the RTMPdump library to play rtmp and rtmpe streams.

[Christophe Mutricy]

On Tue, Jun 29, 10 at 19:45 +0300, Daniel Tisza wrote:
> > +#define _ACCESS_H_ 1
> >
> > That's a bit too generic macro name IMHO. Also you should probably not start
> > with an underscore, as per the C standard.
> 
> Changed to HAVE_RTMPDUMP_ACCESS_H

This time it look too much like the define created by autoconf
> Changed to HAVE_RTMPDUMP_RTMPDUMP_H

Idem

> +AC_ARG_ENABLE(rtmpdump,
> +  [  --enable-rtmpdump       RTMPDump access plugin (default disabled)])
> +if test "${enable_rtmpdump}" = "yes"; then
> +  PKG_CHECK_MODULES(RTMPDUMP, [librtmp],
> +    [
> +      VLC_ADD_PLUGIN([access_rtmpdump])
> +      VLC_ADD_LIBS([access_rtmpdump], [$RTMPDUMP_LIBS])
> +    ],
> +    [ AC_MSG_ERROR([Cannot find librtmp library...])
> +  ] )
> +fi

You could also use PKG_ENABLE_MODULES_VLC()

> diff --git a/modules/access/rtmpdump/Modules.am b/modules/access/rtmpdump/Modules.am
> @@ -0,0 +1,11 @@
> +libaccess_rtmpdump_plugin_la_SOURCES = \
> +	rtmpdump.c \
> +	access.c \
> +	$(NULL)

You also need to list the .h else they don't get distributed.

> + * Copyright (C) 2002-2010 the VideoLAN team

Are you really relying on pieces of code written in 2008.

> +#define FLASHVER_LONGTEXT N_( \
> +    "Version of the Flash plugin used to run the SWF player." \
> +    "The default is LNX 10,1,53,64" )

Missing space after full-stop

> +
> +#define HOSTNAME_TEXT N_("Name of the host to connect to")
> +#define HOSTNAME_LONGTEXT N_( \
> +    "Name of the host to connect to." )
No need of an extra string to repeat yourself. So either you expand this
one or shorten the short description (or use the same define for both)

> +#define PLAYPATH_TEXT N_("Playpath")
> +#define PLAYPATH_LONGTEXT N_( \
> +    "Playpath." \
> +    "By default no value will be sent." )

Missing space
> +#define SWFURL_TEXT N_("URL of the SWF player for the media")
> +#define SWFURL_LONGTEXT N_( \
> +    "URL of the SWF player for the media." \
> +    "By default no value will be sent." )

Idem and in a lot of other define.

> +#define SEEK_TEXT N_("Start at num seconds into the stream")
> +#define SEEK_LONGTEXT N_( \
> +    "Start at num seconds into the stream." \
> +    "Not valid for live streams." )
> 

VLC already has an option for that (--start-time)

+
> +#define STOPOFFSET_TEXT N_("Stop at num seconds into the stream")
> +#define STOPOFFSET_LONGTEXT N_( \
> +    "Stop at num seconds into the stream." )
> +
Idem


Generally, a lot of the long help start by repeating the small help
which is not that good.

A lot of the shorttext could be shorten: "URL of the SWF player" -> "SWF
player URL" and so on.


I might have read to quickly but i don't see where you parse the MRL.
Things like hostname and port should be passed in the MRL rather than an
option. It would be more user-friendly






-- 
Xtophe