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

[Tobias Güntner]

Hello,

Am 10.04.2010 22:01, schrieb Rémi Denis-Courmont:
>> +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.

Should I remove AC_ARG_ENABLE and keep only VLC_ADD_PLUGIN?

>> +#include<vlc_charset.h>
>
> Do you really need this one?

Yes (for EnsureUTF8)

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

ok

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

There was. Probably it is no longer necessary.

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

In my experience, vdr-caching needs to be larger than file-caching to 
avoid problems when switching files, i.e. there is no readahead of 
002.vdr when 001.vdr approaches EOF. Increasing the cache was the 
simplest thing to do.

>> +    add_bool( "vdr-import-marks", true, NULL, \
>> +        IMPORT_MARKS_TEXT, IMPORT_MARKS_LONGTEXT, false )
>
> Same question as for meta datas.

This was useful because VLC would resize its window every time it 
reached a new chapter. No import = no resize. Is this still an issue?

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

No, this has nothing to do with VLC. Some VDR plugins automatically 
place cut marks around commercial breaks. Unfortunately, those cut marks 
tend to be somewhat inaccurate at times. I didn't feel like rewinding 
manually every time, so I made VLC do it.

>> +    p_sys->psz_base_path = p_access->psz_path;
>
> Why do you need to copy this value?

see next oddity below

> var_Inherit...()

ok

>> +#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:// ?

This is a leftover from 1.0, similar to what file.c did back then. Is it 
still required?

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

At most one file is kept open at all times. I'm just a little paranoid.

> Why EAGAIN ?

In case VDR is still recording ...

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

I know. And I like it more than a lonely semicolon. ;)

> You might want to use getline() here.

Interesting. I only knew the C++-getline until know.

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

Does this really apply to alloca() and such? Come to think of it, there 
seems to be no way to check for out-of-memory conditions. I'll fix it.

> f is undefined if sscanf() returns 3.

Even though f was initialized to 1? Whatever, I'll rewrite it.

Regards,
Tobias Güntner