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

[Hugo Beauzée-Luyssen]

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,


-- 
Hugo Beauzée-Luyssen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-strings-Adding-an-helper-to-convert-iso8601-duration.patch
Type: text/x-patch
Size: 3108 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20111130/d50ec2ba/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0015-tests-Running-new-strings-tests-in-the-test-suite.patch
Type: text/x-patch
Size: 3261 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20111130/d50ec2ba/attachment-0001.bin>