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

[Tobias Güntner]

Hi!

Am 08.05.2010 22:09, schrieb Laurent Aimar:
> Useless test, psz_path cannot be NULL.
> Same for psz_access (cannot be NULL).

Then I'll remove the checks.

>> +    if( p_sys->p_meta )
>> +        vlc_meta_Delete( p_sys->p_meta );
>> +
>> +    vlc_input_title_Delete( p_sys->p_marks );
>   I think that p_marks can be NULL (in case of failure at least, as you call
> Close() in Open().

Last time I checked, vlc_input_title_Delete is a noop if called with a 
NULL pointer. vlc_meta_Delete would crash, however.

>> +        else if( errno == EINTR )
>> +        {
>> +            /* try again later */
>> +            return -1;
>   I haven't followed if this one was accepted or not by Rémi.

I don't know. We only discussed EAGAIN.

>> +static void FindSeekpoint( access_t *p_access )
>> +{
>> +    access_sys_t *p_sys = p_access->p_sys;
>   Is it possible to end up here with p_marks being NULL?

Yes, because cut marks are optional.

>   I am not sure, but maybe using bsearch() would be simpler.

Maybe ... I'll think about it.

>   I think a clean goto would reduce the duplication of code.

Actually, I think a call to OpenRelativeFile() would be even cleaner. 
But then I would need to use fread() instead of read() above, and I 
don't know if this would cause more problems. Opinions?

>> +static void UpdateVideoFileSize( access_t *p_access )
>> +{
>> +    access_sys_t *p_sys = p_access->p_sys;
>> +    struct stat st;
>> +
>> +    if( p_access->info.i_size>= p_access->info.i_pos )
>> +        return;
>   It would be better to do as the file access does it (ie checking periodically).

Why?

>> +    ssize_t read = getline( ppsz_line, pi_size, p_file );
>   It seems that getline is a GNU extension and if so, a check in configure will be
> needed.
>   Using stream_UrlNew+stream_ReadLine might be another way.

AFAIK it has been added to POSIX recently. Also, there is an 
implementation in compat/getdelim.c.

>> +    FILE *file = vlc_fopen( psz_path, b_text_mode ? "rt" : "rb" );
>   Are you sure using the text mode is needed ? If no, it would simplify things
> a bit.

>   Is it possible that some file are recorded using \r or \r\n, or it is always
> a unique \n?

I had to check the VDR source code. VDR uses binary mode only, even for 
text files, so I suppose it is reasonably safe to discard text mode 
altogether.

>> +        if( !isalpha( (unsigned char)line[0] ) || line[1] != ' ' )
>   isalpha takes an int, and so casting is useless.

isalpha accepts either EOF or a value representable as unsigned char. If 
char happens to be signed, "negative" characters might be passed to 
isalpha, entailing undefined behavior.

> Adding 'else' (here and later) would make the code easier to read.
> Using a switch() is also a possibility.

I didn't use switch because {} after case: is ugly... I'll add more 
elses instead.

>> +        if( line[0] == 'E' )
>> +        {
>> +            unsigned long i_id, i_start, i_length;
>   Using unsigned long doesn't help, either you need a 64 bits and so use int64_t
> or an unsigned will be enough.
>> +            if( sscanf( line + 2, "%lu %lu %lu",&i_id,&i_start,&i_length ) == 3 )

Is this really a better way to scanf a time_t?

> sizeof(str) instead of 50.
> snprintf is better to avoid future mistake.

OK

>   Using a local variable holding line + 2 would simplify the above code.

ACK.

>> +    free( psz_display );
>   It might be better to move it inside the if().

I think it's cleaner to keep the calls to free together.

>> +    p_marks->b_menu = true;
>   I don't think you should set b_menu. It is meant to be used when the
> title is a special title used for menu (like on DVD).

OK

>> +        sp->i_time_offset = i_frame * (int64_t)( CLOCK_FREQ / p_sys->fps );
> It would probably be better to not use a cast (for precision).

I think int64_t is more precise than float in this case. Either way, 
precision does not really matter here.

>> +    if( p_marks->i_seekpoint != 0 )
> Merging the if() will be simpler to read.

OK

>> +        *pi_offset = i_index_entry&  0xFFFFFFFFFF;
>   You probably need INT64_C() for the constant.

OK


>   I don't know the VDR format, but are you sure that you can always try to read 8 bytes
> in non TS mode? (otherwise some valid files may be rejected).

Yes. Index records are always 8 bytes in size.

-- 
Regards,
Tobias