[FFmpeg-devel] [PATCH 1/3] lavf/srtdec: simplify start/end computation.

[Reimar Döffinger]

On 21 Oct 2012, at 10:49, Clément Bœsch <ubitux at gmail.com> wrote:
> On Sun, Oct 21, 2012 at 10:17:29AM +0200, Nicolas George wrote:
>> Le decadi 30 vendémiaire, an CCXXI, Clément Bœsch a écrit :
>>> Also fix potential overflow (CID733778)
>>> ---
>>> libavformat/srtdec.c | 17 +++++++----------
>>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/libavformat/srtdec.c b/libavformat/srtdec.c
>>> index a66ced3..056165e 100644
>>> --- a/libavformat/srtdec.c
>>> +++ b/libavformat/srtdec.c
>>> @@ -53,19 +53,16 @@ static int srt_read_header(AVFormatContext *s)
>>> 
>>> static int64_t get_pts(const char *buf, int *duration)
>>> {
>>> -    int i, hour, min, sec, hsec;
>>> -    int he, me, se, mse;
>>> +    int i;
>>> 
>>>     for (i=0; i<2; i++) {
>>> -        int64_t start, end;
>>> +        int hh1, mm1, ss1, ms1;
>>> +        int hh2, mm2, ss2, ms2;
>>>         if (sscanf(buf, "%d:%2d:%2d%*1[,.]%3d --> %d:%2d:%2d%*1[,.]%3d",
>>> -                   &hour, &min, &sec, &hsec, &he, &me, &se, &mse) == 8) {
>>> -            min += 60*hour;
>>> -            sec += 60*min;
>>> -            start = sec*1000+hsec;
>>> -            me += 60*he;
>>> -            se += 60*me;
>>> -            end = se*1000+mse;
>>> +                   &hh1, &mm1, &ss1, &ms1,
>>> +                   &hh2, &mm2, &ss2, &ms2) == 8) {
>>> +            int64_t start = (hh1*3600LL + mm1*60LL + ss1) * 1000LL + ms1;
>>> +            int64_t end   = (hh2*3600LL + mm2*60LL + ss2) * 1000LL + ms2;
>>>             *duration = end - start;
>>>             return start;
>>>         }
>> 
>> It looks better, thanks.
>> 
>> I do not like the type mismatch: there is no guarantee that long long is the
>> same as int64_t.
> 
> I thought the long long type was guaranteed to be at least 64 bits by the
> c99 standard…
> 
>>                  Maybe you could write:
>> 
>> hh1 * (int64_t)3600 + ...
>> 
> 
> looks ugly :(

Also semantically it would make more sense to cast hh1...
It's a matter of taste, so I won't tell anyone what to do, and I think LL is fine in principle.
But one way that sometimes looks nicer (though more verbose and possibly more obfuscated) is like this:
int64_t start = hh1;
start *= 60;
start += mm1;
start *= 60;
start += ss1;
start *= 1000;
start += ms1;

Also it might make sense to split it out into a separate function and properly validate it, e.g. checking that mm1 is neither < 0 or >= 60.