[vlc-devel] [PATCH] Added support for VDR recordings.

[Rémi Denis-Courmont]

	Hello,

Comments inline.

> diff --git a/configure.ac b/configure.ac
> index 9fc2af9..5d6cf45 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2198,6 +2198,15 @@ then
>  fi
> 
>  dnl
> +dnl  VDR recordings
> +dnl
> +AC_ARG_ENABLE(vdr,
> +  [  --enable-vdr            vdr recording access module (default
>  enabled)]) +if test "${enable_vdr}" != "no"; then
> +    VLC_ADD_PLUGIN([access_vdr])
> +fi
> +
> +dnl

I am not sure if this is really useful as you are not pulling any new 
dependency.

> +#include <vlc_charset.h>

Do you really need this one?

> +#define FPS_TEXT N_("Frames per second")

Frame rate

> +vlc_module_begin ()
> +    set_category( CAT_INPUT )
> +    set_shortname( N_("VDR") )
> +    set_help( HELP_TEXT )
> +    set_subcategory( SUBCAT_INPUT_ACCESS )
> +    set_description( N_("VDR recordings") )
> +    add_bool( "vdr-import-meta", true, NULL, \
> +        IMPORT_META_TEXT, IMPORT_META_LONGTEXT, false )

Is there a reason why someone would want to turn this off?

> +    add_integer( "vdr-caching", 5 * DEFAULT_PTS_DELAY / 1000, NULL,
> +        CACHING_TEXT, CACHING_LONGTEXT, true )

Shouldn't this piggyback on the existing file-caching parameter?

> +    set_section( N_("VDR Cut Marks"), NULL )
> +    add_bool( "vdr-import-marks", true, NULL, \
> +        IMPORT_MARKS_TEXT, IMPORT_MARKS_LONGTEXT, false )

Same question as for meta datas.

> +    add_integer( "vdr-seek-offset", 15000, NULL,
> +        SEEK_OFFSET_TEXT, SEEK_OFFSET_LONGTEXT, false )

Odd thing. Is this a hack to work around VLC's pipeline setup latency?

> +    p_sys->psz_base_path = p_access->psz_path;

Why do you need to copy this value?

> +    p_sys->fps = config_GetFloat( p_access, "vdr-fps" );

var_InheritFloat()

> +    p_sys->b_ts_format = true;
> +
> +#if defined(WIN32)
> +    /* Skip leading slash on absolute paths. */
> +    if( '/' == p_sys->psz_base_path[0] &&
> +        isalpha( (unsigned char)p_sys->psz_base_path[1] ) &&
> +        ':' == p_sys->psz_base_path[2] )
> +    {
> +        p_sys->psz_base_path++;
> +    }
> +#endif

Odd. Is this a hack for dir:// ?

> +    /* Find video files in this directory and open the first one. */
> +    if( !ScanDirectory( p_access ) ||
> +        !SwitchVideoFile( p_access, 0 ) )
> +    {
> +        Close( p_this );
> +        return VLC_EGENERIC;
> +    }
> +
> +    /* Optionally import meta data */
> +    if( var_CreateGetBool( p_access, "vdr-import-meta" ) )
> +        ImportMeta( p_access );

var_InheritBool()

> +
> +    /* Optionally import cut marks */
> +    if( var_CreateGetBool( p_access, "vdr-import-marks" ) )
> +        ImportMarks( p_access );

var_InheritBool()

> **/ +static void Close( vlc_object_t * p_this )
> +{
> +    access_t *p_access = (access_t*)p_this;
> +    access_sys_t *p_sys = p_access->p_sys;
> +
> +    for( unsigned i = 0; i < p_sys->i_file_count; ++i)
> +    {
> +        if( p_sys->files[i]->fd != -1 )
> +            close( p_sys->files[i]->fd );
> +        free( p_sys->files[i]->psz_path );

Typical file limit is 1024 per process. It seems VDR can have 1000 files per 
recording, which is quite much. Is there a reason to keep all files always 
open?

> +        else if( errno == EINTR || errno == EAGAIN )

Why EAGAIN ?

> +    /* get them all ... */
> +    while( ImportNextVideo( p_access ) )
> +        continue;

continue is surperfluous here.

> +static bool ReadLine( char *psz_line, size_t i_size, FILE *p_file )
> +{
> +    if( !fgets( psz_line, i_size, p_file ) )
> +        return false;
> +    i_size = strlen( psz_line );

You might want to use getline() here.

> +    if( i_size > 0 && psz_line[ i_size - 1 ] == '\n' )
> +        psz_line[ i_size - 1 ] = '\0';
> +    EnsureUTF8( psz_line );
> +    return true;
> +}

> +    char psz_path[ strlen( p_sys->psz_base_path ) + 10 ];
> +    sprintf( psz_path, p_sys->b_ts_format ?
> +        "%s/info" : "%s/info.vdr", p_sys->psz_base_path );

Use asprintf(). The input path can be arbitrarily long and come from untrusted 
sources.

> +    int i_seek = config_GetInt( p_access, "vdr-seek-offset" ) * p_sys->fps
>  / 1000;

var_InheritInteger()

> +    char psz_path[ strlen( p_sys->psz_base_path ) + 11 ];
> +
> +    sprintf( psz_path, p_sys->b_ts_format ?
> +        "%s/marks" : "%s/marks.vdr", p_sys->psz_base_path );

asprintf()

> +    if( sscanf( psz_line, "%u:%u:%u.%u", &h, &m, &s, &f ) >= 3 )
> +    {
> +        /* hour:min:sec.frame (frame is optional) */
> +        int64_t i_seconds = (int64_t)h * 3600 + (int64_t)m * 60 + s;
> +        return (int64_t)( i_seconds * (double)fps ) + __MAX(1, f) - 1;

f is undefined if sscanf() returns 3.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis