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

[Hugo Beauzée-Luyssen]

2011/12/6 Rémi Denis-Courmont <remi at remlab.net>:
> Le mardi 6 décembre 2011 14:11:10 Hugo Beauzée-Luyssen, vous avez écrit :
>> 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,
>>
>> Hi,
>>
>> Could somebody review these patches?
>
>
> +    if ( *psz_duration++ != 'P' )
> +        return -1;
>
> With *three* independent operators (dereference, increment and inequality) on
> psz_duration in a single expression, you really need more explicit parenthesis
> IMHO.
>

Okey, I thought that was ok but I will fix it.

> Also, the test case name is very poor IMHO.
>

It is. Will add some.

Regards,

-- 
Hugo Beauzée-Luyssen