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

[Hugo Beauzée-Luyssen]

On Wed, Nov 30, 2011 at 4:41 PM, Hugo Beauzée-Luyssen
<beauze.h at gmail.com> wrote:
> On Wed, Nov 30, 2011 at 2:45 PM, Hugo Beauzée-Luyssen
> <beauze.h at gmail.com> wrote:
>> 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,
>>
>
> Here is a new version, along with some unit tests.
>
> Regards,
>
>

Ping for review!

Regards,

-- 
Hugo Beauzée-Luyssen