[vlc-devel] [RFC] dcp.cpp: Creation of access-demux module for DCP (using asdcplib)

[Ilkka Ollakka]

On Mon, Jun 03, 2013 at 11:01:12AM +0200, Simona-Marinela Prodea wrote:
> The module only handles audio and video files in DCP, no subtitles yet. Is this form worthy of a patch?

Hi,

Many smaller things are worthy of a patch. Few comments inline here and there
that caught my eye.


> +libdcp_plugin_la_SOURCES = dcp.cpp
> +libdcp_plugin_la_CPPFLAGS = $(AM_CPPFLAGS) $(LIBXML2_CFLAGS)
> +libdcp_plugin_la_LIBADD = $(AM_LIBADD) $(LIBXML2_LIBS) -lasdcp
> +libvlc_LTLIBRARIES += libdcp_plugin.la
> +

This compiles libdcp_plugin unconditionally and it fails if system
doesn't have lasdcp library. Is this intented or just syntax for
checking/enabling it in configure.ac was not so clear?

> +++ b/modules/access/dcp.cpp
> +/* ASDCP headers */
> +#include "AS_DCP.h"

Should this file be included in patch too?

> +
> +using namespace ASDCP;
> +
> +#define FRAME_BUFFER_SIZE 4 * 1024 * 1024

This could maybe include some small mention where/why it is that amount

> +/* Module descriptor */
> +vlc_module_begin()
> +    set_shortname( N_( "DCP" ) )
> +    add_shortcut( "dcp" )
> +    set_description( N_( "DCP access-demux module" ) )
> +    set_capability( "access_demux", 100 ) /* score? */

setting capability to 0 means it is only used when forced (eg using
dcp://my/directory) iirc.

> +
> +static int Demux( demux_t *p_demux )

> +    return 1;

Maybe return VLC_SUCCESS ?

> +static int Control( demux_t *p_demux, int query, va_list args )
> +{
> +        case DEMUX_CAN_CONTROL_PACE:

Maybe also CAN_SEEK ?

> +/**
> + * Function which parses XML files in DCP directory in order to get video and audio files
> + * @param p_demux Demux pointer.
> + * @return Integer according to the success or not of the operation
> + */
> +
> + int parserXML ( demux_t * p_demux )
> +{

> +    return 1; /* TODO : perform checking on XML parsing */

VLC_SUCCESS ?

Any particular reason not using vlc xml_reader ability but directly
libxml2?

> +/**
> + * Function to check if a file name ends with a given suffix
> + * @param str File name
> + * @param suffix Suffix to look for
> + * @return true if the file name ends with the given suffix, fale otherwise

typo: fail


-- 
Ilkka Ollakka
Nobody knows the trouble I've been.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130603/e0435911/attachment.sig>