[vlc-devel] [Patch] Adding an helper to convert iso8601 durations to seconds

[Hugo Beauzée-Luyssen]

On Wed, Nov 30, 2011 at 2:08 PM, Tobias Güntner <fatbull at web.de> wrote:
> Am 30.11.2011 11:05, schrieb Hugo Beauzée-Luyssen:
>> +/*
>> +  Decodes a duration as defined by ISO 8601
>> +  http://en.wikipedia.org/wiki/ISO_8601#Durations
>> +  @param str A null-terminated string to convert
>
> Perhaps a short example of a valid input string would be useful, so
> readers can see at first glance what kind of data this function expects,
> without looking it up.
>

Yep I'm going to write some tests in a this afternoon or tomorrow, but
I could add a link to the format description and an exemple in the
comments.

>> +  @return: The duration in seconds. 0 if an error occured.
>
> This function usually does not return 0 if the input is invalid.
>

Yep i'm going to change the return type as a signed int 64, so -1 is an error.

>> + */
>> +uint64_t str_iso8601_duration_to_seconds( const char *psz_duration )
>> +{
>> +    bool        timeDesignatorReached = false;
>> +    double      mul = 0;
>
> I think an integer should be fine.
>

Well in the exemple file I've got
(http://www-itec.uni-klu.ac.at/ftp/datasets/mmsys12/Selected_VLC_Plugin_MPDs/BigBuckBunny_15s_DASH_VLC_Compatible_4-Representations_720p.mpd)
there are some decimal values, so I try to reduce the loss of
precision when converting by using doubles.

>> +    mtime_t     res = 0;
>
> Shouldn't this type and the return type be the same?
>

I was using mtime_t at first, it seems I forgot to change this one, my bad.

Since the line below is a direct pointer dereferencement without any
prior check, this was intended, but a check could be added.

>> +
>> +    if ( *psz_duration++ != 'P' )
>> +        return 0;
>> +    for ( const char* nptr = psz_duration; *psz_duration; ++psz_duration )
>> +    {
>> +        switch( *psz_duration )
>> +        {
>> +            case 'M':
>> +            {
>
> I find this a little confusing because letters normally follow numbers
> in the input, yet you somehow parse letters first. ;)
>

That is absolutely right...

>> +                //M can mean month or minutes, if the 'T' flag has been reached.
>> +                //We don't handle months though.
>> +                if ( timeDesignatorReached == true )
>> +                    mul = 60.0;
>> +                break ;
>> +            }
>> +            case 'Y':
>> +            case 'W':
>> +                break ; //Don't handle this duration.
>
> Maybe return an error here?
>

I don't really know... warning should be mandatory, an error may be to
agressive...

>> +            case 'D':
>> +                mul = 86400.0;
>> +                break ;
>> +            case 'T':
>> +                timeDesignatorReached = true;
>> +                break ;
>> +            case 'H':
>> +                mul = 3600.0;
>> +                break ;
>> +            case 'S':
>> +                mul = 1.0;
>> +                break ;
>> +            default:
>> +                continue ;
>> +        }
>> +        //It means we went trough some digits. So we must compute a number
>> +        if ( nptr != psz_duration )
>> +        {
>> +            double tmp = strtold( nptr, NULL );
>
> I think you can achieve a slightly cleaner design if you utilize the
> second parameter of that function. The strtoX functions store the
> address of the first invalid character in there, so all you have to do
> is basically:
>

Yep since I'm doing it in the wrong way, using the second parameter
makes a lot of sense now :D

> 1. Call strtoul(psz_duration, &endptr, 10)
> 2. Check the character at *endptr
> 3. Repeat with psz_duration = endptr + 1
>
>> +            res += (uint64_t)(tmp * mul);
>> +            nptr = psz_duration;
>> +        }
>> +        nptr++;
>> +    }
>> +    return res;
>> +}
>
> Regards,
> Tobias
>

Thanks for the review!
Regards,

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel



-- 
Hugo Beauzée-Luyssen